Issue 31960043: State server backup scripts (Closed)
|
Created:
11 years, 3 months ago by wallyworld Modified:
11 years, 3 months ago Reviewers:
mp+196473, rog Visibility:
Public. |
DescriptionState server backup scripts
This branch provides a juju plugin to backup
critical files from the state server (machine 0).
The files are tarred up and copied to the local
machine used to invoke the backup. i
The plugin itself is written as two bash scripts.
One script run on the state server and collects
and tars the files. The other is used as the
launcher which responds to the "juju backup"
command and which copies the tarball of the
state server when the backup is finished.
I'm proposing against trunk. This will need to
land in 1.16. If we think the scripts have
ongoing value once HA comes along, I can also
land in trunk.
https://code.launchpad.net/~wallyworld/juju-core/juju-backup-script/+merge/196473
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : State server backup scripts #Patch Set 3 : State server backup scripts #
Total comments: 30
Patch Set 4 : State server backup scripts #
Total comments: 8
MessagesTotal messages: 7
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
I've tweaked the script so it restarts mongo if the backup fails.
Sign in to reply to this message.
I think this will be really useful, thanks. Quite a few comments, but I think the general approach seems good. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:3: DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" why not just: DIR=$(dirname $0) ? But to be honest, I'm not too keen on the fact that we can't just have all the other shell script embedded in this one. Having one shell script need to find another one always feels a little bit fragile, and in this case I don't think it's actually necessary. How about something like this? remote_cmd() { everything that is currently inside juju-backup.sh } juju ssh 0 sudo -n bash -c "$(declare -f remote_cmd)" Then we can avoid all the sudo calls inside the backup shell script and we can still write the script naturally without worrying about nested quoting issues, and there's no need to have the remote script separately. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:4: juju ssh 0 bash < $DIR/run-juju-backup.sh or: if juju ssh 0 bash < $DIR/run-juju-backup.sh then echo "Copying tarball to `pwd`..." juju scp 0:~/juju-backup/juju-backup.tar.gz . fi ? https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:7: juju scp 0:~/juju-backup/juju-backup.tar.gz .; s/;// https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... File cmd/plugins/juju-backup/run-juju-backup.sh (right): https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:11: ERR=$( { eval $CMD; } 2>&1 ) The multiple levels of evaluation going on here (sometimes 3 levels deep) makes me a little nervous. How about keeping the command as a simple list of arguments and just running them directly. Something like this, perhaps? # usage: execute message cmd [arg...] # Execute the given command with the given arguments # and exit with an error on failure. The first argument # describes the command. execute() { echo -n "$1"... shift if ! ERR=$("$@"); then echo FAILED echo '--------------------------------------------------------------' echo "Command failed: $CMD" echo "Error: $ERR" exit 1 fi echo SUCCESS } https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:17: echo "------------------------------------------------------------" If we're going to have dividers, can I suggest that we have only one divider at a time. Starting with a divider to signify that something significant is about to be printed seems like a reasonable approach to me. Putting another one at the end seems a bit like overkill. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:28: echo "**************************************************************" As above. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:33: next_step "Performing Juju backup of critical files" We seem to be using single and double quoting fairly arbitrarily here. I'd prefer it if we were consistent, using double quotes only when we need to have something expanded inside the quotes. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:36: execute 'sudo -n rm -rf juju-backup-previous' "Removing existing backup archive" We've got lots and lots of these "sudo -n" occurrences. Why not just run the whole script as root? It might be polite to chown the juju-backup directory to ubuntu after creating it too. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:44: export LC_ALL export LC_ALL=C is perhaps more usual. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:51: execute 'sudo -n bash -c "($DUMP_MONGO && $EXPORT_ENVCONFIG) || (start juju-db; exit 1)"' "Backing up mongo" This seems a bit more complex than necessary. How about something like this instead? trap 'execute start juju-db' 0 # ensure that we restart juju-db even if the backup fails execute mongodump... execute mongoexport... trap - 0 https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:57: execute 'mkdir upstart' "Making upstart backup directory" Perhaps name this "etcinit" rather than "upstart" because that's more reminiscent of the original of the files in it? In general, I wonder if we could make a part of the backup directory a little reflection of the filesystem layout it comes from. For instance: root/etc/init/... root/home/ubuntu/.ssh/authorized_keys etc That makes it easier for the restorer to know exactly where to put things without needing special logic for each thing, and we can tweak the backup procedure to add more items without needing to tweak the restore script. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:78: execute 'sudo -n cp /var/log/juju/+(all-|)machine+(s|\-0).log logs' "Copying machine log files" how about: cp /var/log/juju/all-machines.log /var/log/juju/machine-0.log logs ? then we could lose the extglob extension. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:82: execute 'sudo -n tar -zvcf juju-backup.tar.gz *' "Performing tar" I personally prefer archives to contain a single directory. Also I'm not sure we need the verbose output (we're not going to see it by default anyway), so perhaps: cd .. tar -czf juju-backup.tgz juju-backup One other thought: it might be nice to associate the current date with the backup, but perhaps that's better done on the client side. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:87: echo I'd delete this, and one of the echos above, but YMMV.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:3: DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" On 2013/11/28 12:39:57, rog wrote: > why not just: > DIR=$(dirname $0) > > ? > > But to be honest, I'm not too keen on the fact that we can't just have all the > other shell script embedded in this one. Having one shell script need to find > another one always feels a little bit fragile, and in this case I don't think > it's actually necessary. > > How about something like this? > > remote_cmd() { > everything that is currently inside juju-backup.sh > } > > juju ssh 0 sudo -n bash -c "$(declare -f remote_cmd)" > > Then we can avoid all the sudo calls inside the backup shell script > and we can still write the script naturally without worrying > about nested quoting issues, and there's no need to have > the remote script separately. I had to tweak things because sudo -n doesn't like to pass the function properly but it now passes a function to execute to the remote machine https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:4: juju ssh 0 bash < $DIR/run-juju-backup.sh On 2013/11/28 12:39:57, rog wrote: > or: > > if juju ssh 0 bash < $DIR/run-juju-backup.sh > then > echo "Copying tarball to `pwd`..." > juju scp 0:~/juju-backup/juju-backup.tar.gz . > fi > > ? I find my way using $? easier to read https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:7: juju scp 0:~/juju-backup/juju-backup.tar.gz .; On 2013/11/28 12:39:57, rog wrote: > s/;// Done. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... File cmd/plugins/juju-backup/run-juju-backup.sh (right): https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:11: ERR=$( { eval $CMD; } 2>&1 ) On 2013/11/28 12:39:57, rog wrote: > The multiple levels of evaluation going on here (sometimes 3 levels deep) makes > me a little nervous. > > How about keeping the command as a simple list of arguments > and just running them directly. > > Something like this, perhaps? > > # usage: execute message cmd [arg...] > # Execute the given command with the given arguments > # and exit with an error on failure. The first argument > # describes the command. > execute() { > echo -n "$1"... > shift > if ! ERR=$("$@"); then > echo FAILED > echo '--------------------------------------------------------------' > echo "Command failed: $CMD" > echo "Error: $ERR" > exit 1 > fi > echo SUCCESS > } I changed the function to take the args as suggested. I kept the $? check and the code to capture stderr while leaving stdout alone. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:17: echo "------------------------------------------------------------" On 2013/11/28 12:39:57, rog wrote: > If we're going to have dividers, can I suggest that we have only one divider at > a time. Starting with a divider to signify that something significant is about > to be printed seems like a reasonable approach to me. Putting another one at the > end seems a bit like overkill. I find them a lot easier to read since otherwise the header info merges into the subsequent content. The output actually looks pretty good https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:33: next_step "Performing Juju backup of critical files" On 2013/11/28 12:39:57, rog wrote: > We seem to be using single and double quoting fairly arbitrarily here. I'd > prefer it if we were consistent, using double quotes only when we need to have > something expanded inside the quotes. Done. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:36: execute 'sudo -n rm -rf juju-backup-previous' "Removing existing backup archive" On 2013/11/28 12:39:57, rog wrote: > We've got lots and lots of these "sudo -n" occurrences. > Why not just run the whole script as root? > It might be polite to chown the juju-backup directory > to ubuntu after creating it too. Done. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:44: export LC_ALL On 2013/11/28 12:39:57, rog wrote: > export LC_ALL=C > > is perhaps more usual. Done. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:51: execute 'sudo -n bash -c "($DUMP_MONGO && $EXPORT_ENVCONFIG) || (start juju-db; exit 1)"' "Backing up mongo" On 2013/11/28 12:39:57, rog wrote: > This seems a bit more complex than necessary. > > How about something like this instead? > > > trap 'execute start juju-db' 0 # ensure that we restart juju-db even if the > backup fails > execute mongodump... > execute mongoexport... > trap - 0 I could only get trap to work in a stand alone script. I don't think the construct I had to too bad - it's just using std && and || constructs https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:57: execute 'mkdir upstart' "Making upstart backup directory" On 2013/11/28 12:39:57, rog wrote: > Perhaps name this "etcinit" rather than "upstart" because that's more > reminiscent of the original of the files in it? > > In general, I wonder if we could make a part of the backup directory > a little reflection of the filesystem layout it comes from. > > For instance: > > root/etc/init/... > root/home/ubuntu/.ssh/authorized_keys > > etc > > That makes it easier for the restorer to know exactly where to put things > without needing special logic for each thing, and we can tweak the backup > procedure to add more items without needing to tweak the restore script. Done. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:78: execute 'sudo -n cp /var/log/juju/+(all-|)machine+(s|\-0).log logs' "Copying machine log files" On 2013/11/28 12:39:57, rog wrote: > how about: > cp /var/log/juju/all-machines.log /var/log/juju/machine-0.log logs > ? > > then we could lose the extglob extension. I'd rather keep the extglob stuff so we can more easily include other log files as needed https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:82: execute 'sudo -n tar -zvcf juju-backup.tar.gz *' "Performing tar" On 2013/11/28 12:39:57, rog wrote: > I personally prefer archives to contain a single directory. > Also I'm not sure we need the verbose output (we're not going > to see it by default anyway), so perhaps: > > cd .. > tar -czf juju-backup.tgz juju-backup > > One other thought: it might be nice to associate the current date > with the backup, but perhaps that's better done on the client side. Done. I added the time/date to the scp command. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:87: echo On 2013/11/28 12:39:57, rog wrote: > I'd delete this, and one of the echos above, but YMMV. I added the extra echos because it's easier to read
Sign in to reply to this message.
Message was sent while issue was closed.
Thanks for making those changes. I have a few more comments. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:4: juju ssh 0 bash < $DIR/run-juju-backup.sh On 2013/11/29 06:14:36, wallyworld wrote: > On 2013/11/28 12:39:57, rog wrote: > > or: > > > > if juju ssh 0 bash < $DIR/run-juju-backup.sh > > then > > echo "Copying tarball to `pwd`..." > > juju scp 0:~/juju-backup/juju-backup.tar.gz . > > fi > > > > ? > > I find my way using $? easier to read Another possibility you might wish to consider, which is also commonly used: juju ssh 0 bash < $DIR/run-juju-backup.sh && { echo "Copying tarball to `pwd`..." cp 0:~/juju-backup/juju-backup.tar.gz . } Personally I'm not that keen on the $? test as it introduces at least three additional concepts (the test operator, and $? variable and the fact that 0 means success) where just the usual status testing mechanism works fine. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... File cmd/plugins/juju-backup/run-juju-backup.sh (right): https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:51: execute 'sudo -n bash -c "($DUMP_MONGO && $EXPORT_ENVCONFIG) || (start juju-db; exit 1)"' "Backing up mongo" On 2013/11/29 06:14:36, wallyworld wrote: > On 2013/11/28 12:39:57, rog wrote: > > This seems a bit more complex than necessary. > > > > How about something like this instead? > > > > > > trap 'execute start juju-db' 0 # ensure that we restart juju-db even if the > > backup fails > > execute mongodump... > > execute mongoexport... > > trap - 0 > > I could only get trap to work in a stand alone script. I don't think the > construct I had to too bad - it's just using std && and || constructs It seems to work fine for me - I just tried it, e.g. $ juju ssh 0 'trap "echo gone" 0' gone $ I think it's a cleaner solution than introducing another bash -c into the mix, and less fragile too (we don't need any extra levels of evaluation here). https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/ru... cmd/plugins/juju-backup/run-juju-backup.sh:78: execute 'sudo -n cp /var/log/juju/+(all-|)machine+(s|\-0).log logs' "Copying machine log files" On 2013/11/29 06:14:36, wallyworld wrote: > On 2013/11/28 12:39:57, rog wrote: > > how about: > > cp /var/log/juju/all-machines.log /var/log/juju/machine-0.log logs > > ? > > > > then we could lose the extglob extension. > > I'd rather keep the extglob stuff so we can more easily include other log files > as needed I'd prefer to avoid using obscure (well, I had never seen it :-]) bash extensions until we actually need them. If we were to use some kind of regexp, I'd prefer to use ls and grep, as that's a syntax that everyone is familiar with. As it is, the above pattern will match more than we'd like (e.g. /var/log/juju/all-all-all-machines-0s-0sss-0.log ) Mentioning the files directly seems easier to understand and still not hard to extend if we want to include other log files. https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:4: shopt -s extglob I think the the contents of this function should be indented as per usual. https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:14: CMD="sudo -n $@" $@ has no effect unless it's inside double quotes inside an argument to a command. See comment below for an alternative to this. https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:16: ERR=$( { eval $CMD; } 2>&1 ) The point about my earlier suggestion was so that we could remove this extra level of evaluation. The eval and the curly braces seem a bit like cargo-culting to me (both are unnecessary). ERR=$("$@" 2>&1} should be sufficient. https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:23: exit 1; s/;// https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:49: DUMP_MONGO='mongodump --dbpath /var/lib/juju/db' How about setting LIBJUJU=/var/lib/juju and using it throughout, rather than repeating the /var/lib/juju directory name everywhere? https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:59: ETCINIT=etc/init I think it might be better to have all the copied files in a single directory (root?), so that we can unarchive with a single command, rather than having to know the set of archived directories. https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:61: execute 'Copying mongo upstart script' cp /etc/init/juju-db.conf $ETCINIT Just a thought - rather than having separate cp's for each file, we could just have a big list of files, which might turn out easier to maintain. e.g. copyfiles() { for i in "$@"; do dest=$(dirname "./$i") mkdir -p "$dest" execute "Copying $i" cp -r "$i" "$dest" done } copyfiles \ /etc/init/juju-db.conf \ /etc/init/jujud-machine-*.conf \ /var/lib/juju/agents/machine* \ /var/lib/juju/tools \ ~/.ssh/authorized_keys \ /etc/rsyslog.d/*juju.conf \ ... etc https://codereview.appspot.com/31960043/diff/60001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:100: juju ssh 0 "$REMOTE_SCRIPT; typeset -fx remote_cmd; remote_cmd" I don't believe the typeset statement is necessary here - remote_cmd doesn't need to be exported. Also, to make things a bit simpler above, why not run the whole script inside a single sudo? It does mean we have to quote it, but that's not too hard. REMOTE_SCRIPT=" $(declare -f remote_cmd) remote_cmd " QUOTED_SCRIPT="'$(echo "$REMOTE_SCRIPT" | sed "s/'/'\"'\"'/g")'" juju ssh 0 "sudo -n bash -c $QUOTED_SCRIPT" Then there's a single point of failure for sudo and no need to call it in the execute function,
Sign in to reply to this message.
|
