Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

Issue 31960043: State server backup scripts (Closed)

Can't Edit
Can't Publish+Mail
Start Review
6 years, 7 months ago by wallyworld
6 years, 7 months ago
mp+196473, rog


State 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -0 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A cmd/plugins/juju-backup/juju-backup View 1 2 3 1 chunk +108 lines, -0 lines 8 comments Download


Total messages: 7
Please take a look.
6 years, 7 months ago (2013-11-25 08:10:46 UTC) #1
Please take a look.
6 years, 7 months ago (2013-11-25 11:42:30 UTC) #2
Please take a look.
6 years, 7 months ago (2013-11-28 08:20:16 UTC) #3
I've tweaked the script so it restarts mongo if the backup fails.
6 years, 7 months ago (2013-11-28 08:21:01 UTC) #4
I think this will be really useful, thanks. Quite a few comments, but I think ...
6 years, 7 months ago (2013-11-28 12:39:57 UTC) #5
Please take a look. https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/juju-backup File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/31960043/diff/40001/cmd/plugins/juju-backup/juju-backup#newcode3 cmd/plugins/juju-backup/juju-backup:3: DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" ...
6 years, 7 months ago (2013-11-29 06:14:36 UTC) #6
6 years, 7 months ago (2013-11-29 10:44:09 UTC) #7
Message was sent while issue was closed.
Thanks for making those changes. I have a few more comments.

File cmd/plugins/juju-backup/juju-backup (right):

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.

File cmd/plugins/juju-backup/run-juju-backup.sh (right):

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
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
> > 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'

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).

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
> 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

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.

File cmd/plugins/juju-backup/juju-backup (right):

cmd/plugins/juju-backup/juju-backup:4: shopt -s extglob
I think the the contents of this function
should be indented as per usual.

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.

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.

cmd/plugins/juju-backup/juju-backup:23: exit 1;

cmd/plugins/juju-backup/juju-backup:49: DUMP_MONGO='mongodump --dbpath
How about setting


and using it throughout, rather than repeating the /var/lib/juju
directory name everywhere?

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.

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.


copyfiles() {
    for i in "$@"; do
        dest=$(dirname "./$i")
        mkdir -p "$dest"
        execute "Copying $i" cp -r "$i" "$dest"

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

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.

	$(declare -f 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b