Merge lp:~hduran-8/juju-core/upgrade_restore_for_replicaset into lp:~go-bot/juju-core/trunk

[Horacio Durán]

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://codereview.appspot.com/94330043/

R=jameinel, klyachin

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://codereview.appspot.com/94330043/

[Horacio Durán (hduran-8)]

Reviewers: mp+218998_code.launchpad.net,

Message:
Please take a look.

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)

Please review this at https://codereview.appspot.com/94330043/

Affected files (+89, -25 lines):
   A [revision details]
   M cmd/plugins/juju-restore/restore.go

[Vladislav Klyachin (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 as good practice option
If you want to ignore result code of command, use:
command || true

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
Use mkdir -p to get ok result code if directory exists and to create
several directories
Although, in this place, I would remove old backup before moving the new
one.
I am nor sure that /tmp is a good place for backups. Ofhen /tmp is
located on ramdisk and it will consume the memory. I would use
/var/lib/juju.backup.
The second 'mv' has invalid indent (spaces vs tabs).

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode131
cmd/plugins/juju-restore/restore.go:131: fi
IMHO export is not needed here and before

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode137
cmd/plugins/juju-restore/restore.go:137: mongo --ssl -u admin -p
{{.Creds.OldPassword | printf "%q" }} localhost:37017/admin --eval "$1"
I would use 'shquote' instead of 'printf "%q"' here and further, because
'string' is better than "string" , because bash processes '/' and '$'
inside of "string".

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode146
cmd/plugins/juju-restore/restore.go:146: for i in $(seq 1 50)
Shortcut for $(seq 1 50): {1..50}

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode348
cmd/plugins/juju-restore/restore.go:348: OldPassword string
OldPassword is confusing, why not use AdminPassword

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode387
cmd/plugins/juju-restore/restore.go:387: if !ok || password == "" {
Go style is oldPassword rather than olspassword
typo: oldpassword == ""

https://codereview.appspot.com/94330043/

[Horacio Durán (hduran-8)]

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, vladislav.klyachin wrote:
> Use mkdir -p to get ok result code if directory exists and to create
several
> directories
> Although, in this place, I would remove old backup before moving the
new one.
> I am nor sure that /tmp is a good place for backups. Ofhen /tmp is
located on
> ramdisk and it will consume the memory. I would use
/var/lib/juju.backup.
> The second 'mv' has invalid indent (spaces vs tabs).

Done.

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode146
cmd/plugins/juju-restore/restore.go:146: for i in $(seq 1 50)
On 2014/05/09 15:33:49, vladislav.klyachin wrote:
> Shortcut for $(seq 1 50): {1..50}

Noted although I will try to keep the other way since it seems more
likely that less bash savvy people will understand it.

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode348
cmd/plugins/juju-restore/restore.go:348: OldPassword string
On 2014/05/09 15:33:49, vladislav.klyachin wrote:
> OldPassword is confusing, why not use AdminPassword
Because this is referred to as old password everywhere, since it is the
old mongo access password (used for admin)

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode387
cmd/plugins/juju-restore/restore.go:387: if !ok || password == "" {
On 2014/05/09 15:33:49, vladislav.klyachin wrote:
> Go style is oldPassword rather than olspassword
> typo: oldpassword == ""

Done.

https://codereview.appspot.com/94330043/

[John A Meinel (jameinel)]

I have a couple of suggestions, if you're fine with them, then the rest
LGTM. (if you can justify why we want to get rid of 'set -e' that can be
ok, but I do think we want -u for sure, and I'd really like -e unless
there is a strong reason not to.)

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

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode107
cmd/plugins/juju-restore/restore.go:107: set -x
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?

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode118
cmd/plugins/juju-restore/restore.go:118: rm -r /var/log/juju
it looks like you mixed tabs and spaces here, its probably better to be
consistent.

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode142
cmd/plugins/juju-restore/restore.go:142: for i in $(seq 1 50)
this change seems odd to me, is it just to make testing faster?

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

https://codereview.appspot.com/94330043/

[Horacio Durán (hduran-8)]

Please take a look.

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

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode107
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/restore.go#newcode118
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/restore.go#newcode142
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/restore.go#newcode316
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.

https://codereview.appspot.com/94330043/

=== modified file 'cmd/plugins/juju-backup/juju-backup'
--- cmd/plugins/juju-backup/juju-backup	2014-04-23 15:10:58 +0000
+++ cmd/plugins/juju-backup/juju-backup	2014-05-12 16:40:42 +0000
@@ -85,6 +85,9 @@
 		$LIBJUJU/agents/machine-* \
 		$LIBJUJU/tools \
 		$LIBJUJU/server.pem \
+		$LIBJUJU/system-identity \
+		$LIBJUJU/nonce.txt \
+		$LIBJUJU/shared-secret \
 		~/.ssh/authorized_keys \
 		/etc/rsyslog.d/*juju.conf \
 		/var/log/juju/all-machines.log \

=== modified file 'cmd/plugins/juju-restore/restore.go'
--- cmd/plugins/juju-restore/restore.go	2014-05-09 13:24:50 +0000
+++ cmd/plugins/juju-restore/restore.go	2014-05-12 16:40:42 +0000
@@ -103,42 +103,89 @@
 }
 
 var updateBootstrapMachineTemplate = mustParseTemplate(`
-	set -e -x
+	set -exu
+
+	export LC_ALL=C
 	tar xzf juju-backup.tgz
 	test -d juju-backup
-
+	apt-get --option=Dpkg::Options::=--force-confold --option=Dpkg::options::=--force-unsafe-io --assume-yes --quiet install mongodb-clients
+	
 	initctl stop jujud-machine-0
 
 	initctl stop juju-db
-	rm -r /var/lib/juju /var/log/juju
+	rm -r /var/lib/juju
+	rm -r /var/log/juju
+
 	tar -C / -xvp -f juju-backup/root.tar
 	mkdir -p /var/lib/juju/db
-	export LC_ALL=C
-	mongorestore --drop --dbpath /var/lib/juju/db juju-backup/dump
+
+	# Prefer jujud-mongodb binaries if available 
+	export MONGORESTORE=mongorestore
+	if [ -f /usr/lib/juju/bin/mongorestore ]; then
+		export MONGORESTORE=/usr/lib/juju/bin/mongorestore;
+	fi	
+	$MONGORESTORE --drop --dbpath /var/lib/juju/db juju-backup/dump
+
 	initctl start juju-db
 
+	mongoAdminEval() {
+		mongo --ssl -u admin -p {{.Creds.OldPassword | shquote}} localhost:37017/admin --eval "$1"
+	}
+
+
 	mongoEval() {
 		mongo --ssl -u {{.Creds.Tag}} -p {{.Creds.Password | shquote}} localhost:37017/juju --eval "$1"
 	}
+
 	# wait for mongo to come up after starting the juju-db upstart service.
 	for i in $(seq 1 100)
 	do
 		mongoEval ' ' && break
 		sleep 5
 	done
+
 	mongoEval '
 		db = db.getSiblingDB("juju")
-		db.machines.update({_id: "0"}, {$set: {instanceid: '{{.NewInstanceId | printf "%q" | shquote}}' } })
-		db.instanceData.update({_id: "0"}, {$set: {instanceid: '{{.NewInstanceId | printf "%q"| shquote}}' } })
-	'
+		db.machines.update({_id: "0"}, {$set: {instanceid: {{.NewInstanceId | printf "%q" }} } })
+		db.instanceData.update({_id: "0"}, {$set: {instanceid: {{.NewInstanceId | printf "%q" }} } })
+	'
+	
+	# Create a new replicaSet conf and re initiate it
+	mongoAdminEval '
+		conf = { "_id" : "juju", "version" : 1, "members" : [ { "_id" : 1, "host" : "{{ .PrivateAddress | printf "%s:37017" }}" , "tags" : { "juju-machine-id" : "0" } }]}
+		rs.initiate(conf)
+	'
+
+	# Give time to replset to initiate
+	for i in $(seq 1 20)
+	do
+		mongoEval ' ' && break
+		sleep 5
+	done
+
+	initctl stop juju-db
+
+	# Update the agent.conf for machine-0 with the new addresses
+	cd /var/lib/juju/agents
+	sed -i.old -r -e "/^(stateaddresses):/{
+		n
+		s/- .*(:[0-9]+)/- {{.Address}}\1/
+	}" -e "/^(apiaddresses):/{
+		n
+		s/- .*(:[0-9]+)/- {{.PrivateAddress}}\1/
+	}"  machine-0/agent.conf
+
+	initctl start juju-db
 	initctl start jujud-machine-0
 `)
 
-func updateBootstrapMachineScript(instanceId instance.Id, creds credentials) string {
+func updateBootstrapMachineScript(instanceId instance.Id, creds credentials, addr, paddr string) string {
 	return execTemplate(updateBootstrapMachineTemplate, struct {
-		NewInstanceId instance.Id
-		Creds         credentials
-	}{instanceId, creds})
+		NewInstanceId  instance.Id
+		Creds          credentials
+		Address        string
+		PrivateAddress string
+	}{instanceId, creds, addr, paddr})
 }
 
 func (c *restoreCommand) Run(ctx *cmd.Context) error {
@@ -261,11 +308,16 @@
 }
 
 func restoreBootstrapMachine(conn *juju.APIConn, backupFile string, creds credentials) (newInstId instance.Id, addr string, err error) {
-	addr, err = conn.State.Client().PublicAddress("0")
+	client := conn.State.Client()
+	addr, err = client.PublicAddress("0")
 	if err != nil {
 		return "", "", fmt.Errorf("cannot get public address of bootstrap machine: %v", err)
 	}
-	status, err := conn.State.Client().Status(nil)
+	paddr, err := client.PrivateAddress("0")
+	if err != nil {
+		return "", "", fmt.Errorf("cannot get private address of bootstrap machine: %v", err)
+	}
+	status, err := client.Status(nil)
 	if err != nil {
 		return "", "", fmt.Errorf("cannot get environment status: %v", err)
 	}
@@ -280,15 +332,16 @@
 		return "", "", fmt.Errorf("cannot copy backup file to bootstrap instance: %v", err)
 	}
 	progress("updating bootstrap machine")
-	if err := runViaSsh(addr, updateBootstrapMachineScript(newInstId, creds)); err != nil {
+	if err := runViaSsh(addr, updateBootstrapMachineScript(newInstId, creds, addr, paddr)); err != nil {
 		return "", "", fmt.Errorf("update script failed: %v", err)
 	}
 	return newInstId, addr, nil
 }
 
 type credentials struct {
-	Tag      string
-	Password string
+	Tag         string
+	Password    string
+	OldPassword string
 }
 
 func extractCreds(backupFile string) (credentials, error) {
@@ -326,9 +379,15 @@
 	if !ok || password == "" {
 		return credentials{}, fmt.Errorf("agent password not found in configuration")
 	}
+	oldPassword, ok := m["oldpassword"].(string)
+	if !ok || oldPassword == "" {
+		return credentials{}, fmt.Errorf("agent old password not found in configuration")
+	}
+
 	return credentials{
-		Tag:      "machine-0",
-		Password: password,
+		Tag:         "machine-0",
+		Password:    password,
+		OldPassword: oldPassword,
 	}, nil
 }
 
@@ -356,14 +415,14 @@
 		s/- .*(:[0-9]+)/- {{.Address}}\1/
 	}" $agent/agent.conf
 
-    # If we're processing a unit agent's directly
-    # and it has some relations, reset
-    # the stored version of all of them to
-    # ensure that any relation hooks will
-    # fire.
+	# If we're processing a unit agent's directly
+	# and it has some relations, reset
+	# the stored version of all of them to
+	# ensure that any relation hooks will
+	# fire.
 	if [[ $agent = unit-* ]]
 	then
-	find $agent/state/relations -type f -exec sed -i -r 's/change-version: [0-9]+$/change-version: 0/' {} \;
+		find $agent/state/relations -type f -exec sed -i -r 's/change-version: [0-9]+$/change-version: 0/' {} \;
 	fi
 	initctl start jujud-$agent
 done