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

Issue 94330043: Corrected restore tu support current juju code

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 8 months ago by hduran
Modified:
5 years, 8 months ago
Reviewers:
vladislav.klyachin, mp+218998, jameinel
Visibility:
Public.

Description

Corrected restore tu support current juju code Current restore plugin, takes a backup which contains a dump of the mongodb for the backed up machine and several configuration files, it bootstraps a new machine and: Deletes the fresh db Loads the dump Places the files instead of the fresh machine ones Restarts services. Since replicaset was introduced the db load was not working properly. A replicaset initiate with a fresh config was added to solve it. https://code.launchpad.net/~hduran-8/juju-core/upgrade_restore_for_replicaset/+merge/218998 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 11

Patch Set 2 : Corrected restore tu support current juju code #

Total comments: 8

Patch Set 3 : Corrected restore tu support current juju code #

Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -25 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/plugins/juju-backup/juju-backup View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M cmd/plugins/juju-restore/restore.go View 1 2 5 chunks +84 lines, -25 lines 0 comments Download

Messages

Total messages: 5
hduran
Please take a look.
5 years, 8 months ago (2014-05-09 14:29:29 UTC) #1
vladislav.klyachin
LGTM with some notices https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go File cmd/plugins/juju-restore/restore.go (right): https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode107 cmd/plugins/juju-restore/restore.go:107: set -x I consider -e ...
5 years, 8 months ago (2014-05-09 15:33:49 UTC) #2
hduran
Please take a look. https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go File cmd/plugins/juju-restore/restore.go (right): https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode120 cmd/plugins/juju-restore/restore.go:120: mkdir /tmp/oldbackup/inits On 2014/05/09 15:33:49, ...
5 years, 8 months ago (2014-05-09 18:01:03 UTC) #3
jameinel
I have a couple of suggestions, if you're fine with them, then the rest LGTM. ...
5 years, 8 months ago (2014-05-12 12:59:53 UTC) #4
hduran
5 years, 8 months ago (2014-05-12 16:10:25 UTC) #5
Please take a look.

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
cmd/plugins/juju-restore/restore.go:107: set -x
On 2014/05/12 12:59:53, jameinel wrote:
> I'd rather see us use || true syntax than let failing things be missed by the
> script.
> Also, we should be setting -u (treat unknown environment variables as an error
> rather than silently passing "").
> 
> for -e, I'm guessing we are worried that we might set something up but fail to
> cleanup afterwards?

Done.

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
cmd/plugins/juju-restore/restore.go:118: rm -r /var/log/juju
On 2014/05/12 12:59:53, jameinel wrote:
> it looks like you mixed tabs and spaces here, its probably better to be
> consistent.

Done.

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
cmd/plugins/juju-restore/restore.go:142: for i in $(seq 1 50)
On 2014/05/12 12:59:53, jameinel wrote:
> this change seems odd to me, is it just to make testing faster?

This was a bug, I made the change to actually produce a failure and forgot to
remove it. Tx for spotting it.

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
cmd/plugins/juju-restore/restore.go:316: paddr, err :=
conn.State.Client().PrivateAddress("0")
On 2014/05/12 12:59:53, jameinel wrote:
> I really think we should be grabbing the Client() as a variable and using it
> repeatedly, rather than re-instantiating it for each call.

Done.
Sign in to reply to this message.

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