Merge lp:~wallyworld/juju-core/retryprovisioning-command into lp:~go-bot/juju-core/trunk

[Ian Booth]

Add resolve-provisioning-error command

A new command is added: resolve-provisioning-error
The command is used to enabe the provisioner to
retry provisioning a machine if the previous
attempt failed.

https://codereview.appspot.com/80410044/

Add resolve-provisioning-error command

A new command is added: resolve-provisioning-error
The command is used to enabe the provisioner to
retry provisioning a machine if the previous
attempt failed.

https://codereview.appspot.com/80410044/

[Ian Booth (wallyworld)]

Reviewers: mp+212781_code.launchpad.net,

Message:
Please take a look.

Description:
Add resolve-provisioning-error command

A new command is added: resolve-provisioning-error
The command is used to enabe the provisioner to
retry provisioning a machine if the previous
attempt failed.

https://code.launchpad.net/~wallyworld/juju-core/retryprovisioning-command/+merge/212781

(do not edit description out of merge proposal)

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

Affected files (+143, -5 lines):
   A [revision details]
   M cmd/juju/main.go
   M cmd/juju/main_test.go
   A cmd/juju/resolveprovisioningerror.go
   A cmd/juju/resolveprovisioningerror_test.go
   M state/apiserver/common/setstatus.go
   M state/apiserver/common/setstatus_test.go

[Wayne Witzel III (wwitzel3)]

LGTM with some comments that are more questions than suggestions.

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go
File cmd/juju/resolveprovisioningerror.go (right):

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go#newcode1
cmd/juju/resolveprovisioningerror.go:1: // Copyright 2012, 2013
Canonical Ltd.
Don't know if this even matters, but it is 2014.

https://codereview.appspot.com/80410044/diff/1/state/apiserver/common/setstatus.go
File state/apiserver/common/setstatus.go (right):

https://codereview.appspot.com/80410044/diff/1/state/apiserver/common/setstatus.go#newcode89
state/apiserver/common/setstatus.go:89: if len(newData) > 0 &&
existingStatus != params.StatusError {
No where else in this package do we use fmt for the errors. For
consistency should we consider creating a new error type and returning
that? It just stands out like a sore thumb to me.

https://codereview.appspot.com/80410044/

[John A Meinel (jameinel)]

We'll want to change this to Retry instead of Resolve and the command to
"retry-provisioning" from "resolve-provisioning-error", but otherwise
LGTM.

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go
File cmd/juju/resolveprovisioningerror.go (right):

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go#newcode23
cmd/juju/resolveprovisioningerror.go:23: Name:
"resolve-provisioning-error",
We should call this "retry-provisioning"

https://codereview.appspot.com/80410044/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/80410044/diff/1/state/apiserver/common/setstatus.go
File state/apiserver/common/setstatus.go (right):

https://codereview.appspot.com/80410044/diff/1/state/apiserver/common/setstatus.go#newcode90
state/apiserver/common/setstatus.go:90: return fmt.Errorf("machine %q is
not in an error state", tag)
This error seems odd to me. Shouldn't it be something more like:
"non-error status has unexpected associated data" ?

https://codereview.appspot.com/80410044/

[William Reade (fwereade)]

I'll add to the chorus: LGTM

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go
File cmd/juju/resolveprovisioningerror.go (right):

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go#newcode23
cmd/juju/resolveprovisioningerror.go:23: Name:
"resolve-provisioning-error",
retry-provisioning, as discussed

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go#newcode25
cmd/juju/resolveprovisioningerror.go:25: Purpose: "marks provisioning
errors resolved",
"retries provisioning for a failed machine"?

https://codereview.appspot.com/80410044/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go
File cmd/juju/resolveprovisioningerror.go (right):

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go#newcode1
cmd/juju/resolveprovisioningerror.go:1: // Copyright 2012, 2013
Canonical Ltd.
On 2014/03/26 11:29:16, wwitzel3 wrote:
> Don't know if this even matters, but it is 2014.

Done.

https://codereview.appspot.com/80410044/diff/1/cmd/juju/resolveprovisioningerror.go#newcode23
cmd/juju/resolveprovisioningerror.go:23: Name:
"resolve-provisioning-error",
On 2014/03/26 11:55:12, jameinel wrote:
> We should call this "retry-provisioning"

Done.

https://codereview.appspot.com/80410044/diff/1/state/apiserver/common/setstatus.go
File state/apiserver/common/setstatus.go (right):

https://codereview.appspot.com/80410044/diff/1/state/apiserver/common/setstatus.go#newcode89
state/apiserver/common/setstatus.go:89: if len(newData) > 0 &&
existingStatus != params.StatusError {
On 2014/03/26 11:29:16, wwitzel3 wrote:
> No where else in this package do we use fmt for the errors. For
consistency
> should we consider creating a new error type and returning that? It
just stands
> out like a sore thumb to me.

See comment below - I'm maintaining consistency with entity.SetStatus()
- see setEntityStatus() earlier in this file.

https://codereview.appspot.com/80410044/diff/1/state/apiserver/common/setstatus.go#newcode90
state/apiserver/common/setstatus.go:90: return fmt.Errorf("machine %q is
not in an error state", tag)
On 2014/03/26 11:55:23, rog wrote:
> This error seems odd to me. Shouldn't it be something more like:
"non-error
> status has unexpected associated data" ?

This error message is consistent with the error returned under the same
circumstances when setting status on a machine/unit - see
setEntityStatus() earlier in this file. I'd rather keep it as it for
consistency.

https://codereview.appspot.com/80410044/

[Go Bot (go-bot)]

The attempt to merge lp:~wallyworld/juju-core/retryprovisioning-command into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.013s
ok launchpad.net/juju-core/agent 1.138s
ok launchpad.net/juju-core/agent/mongo 0.561s
ok launchpad.net/juju-core/agent/tools 0.209s
ok launchpad.net/juju-core/bzr 5.381s
ok launchpad.net/juju-core/cert 2.972s
ok launchpad.net/juju-core/charm 0.504s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.028s
ok launchpad.net/juju-core/cloudinit/sshinit 0.790s
ok launchpad.net/juju-core/cmd 0.158s
ok launchpad.net/juju-core/cmd/charm-admin 0.745s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 201.498s
ok launchpad.net/juju-core/cmd/jujud 63.681s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.949s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.183s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.024s
ok launchpad.net/juju-core/container 0.051s
ok launchpad.net/juju-core/container/factory 0.050s
ok launchpad.net/juju-core/container/kvm 0.211s
ok launchpad.net/juju-core/container/kvm/mock 0.043s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.326s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.260s
ok launchpad.net/juju-core/environs 2.427s
ok launchpad.net/juju-core/environs/bootstrap 10.287s
ok launchpad.net/juju-core/environs/cloudinit 0.539s
ok launchpad.net/juju-core/environs/config 2.015s
ok launchpad.net/juju-core/environs/configstore 0.030s
ok launchpad.net/juju-core/environs/filestorage 0.027s
ok launchpad.net/juju-core/environs/httpstorage 0.670s
ok launchpad.net/juju-core/environs/imagemetadata 0.453s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.044s
ok launchpad.net/juju-core/environs/jujutest 0.203s
ok launchpad.net/juju-core/environs/manual 10.205s
ok launchpad.net/juju-core/environs/simplestreams 0.280s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.943s
ok launchpad.net/juju-core/environs/storage 0.884s
ok launchpad.net/juju-core/environs/sync 45.726s
ok launchpad.net/juju-core/environs/testing 0.153s
ok launchpad.net/juju-core/environs/tools 4.643s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.011s
ok launchpad.net/juju-core/instance 0.017s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 18.094s
ok launchpad.net/juju-core/juju/arch...

=== modified file 'cmd/juju/main.go'
--- cmd/juju/main.go	2014-03-26 05:44:41 +0000
+++ cmd/juju/main.go	2014-03-26 13:26:23 +0000
@@ -98,6 +98,7 @@
 	jujucmd.Register(wrap(&ResolvedCommand{}))
 	jujucmd.Register(wrap(&DebugLogCommand{sshCmd: &SSHCommand{}}))
 	jujucmd.Register(wrap(&DebugHooksCommand{}))
+	jujucmd.Register(wrap(&RetryProvisioningCommand{}))
 
 	// Configuration commands.
 	jujucmd.Register(wrap(&InitCommand{}))

=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go	2014-03-26 05:44:41 +0000
+++ cmd/juju/main_test.go	2014-03-26 13:26:23 +0000
@@ -239,6 +239,7 @@
 	"remove-service",  // alias for destroy-service
 	"remove-unit",     // alias for destroy-unit
 	"resolved",
+	"retry-provisioning",
 	"run",
 	"scp",
 	"set",

=== added file 'cmd/juju/retryprovisioning.go'
--- cmd/juju/retryprovisioning.go	1970-01-01 00:00:00 +0000
+++ cmd/juju/retryprovisioning.go	2014-03-26 13:26:23 +0000
@@ -0,0 +1,59 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package main
+
+import (
+	"fmt"
+
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/juju"
+	"launchpad.net/juju-core/names"
+)
+
+// RetryProvisioningCommand updates machines' error status to tell
+// the provisoner that it should try to re-provision the machine.
+type RetryProvisioningCommand struct {
+	cmd.EnvCommandBase
+	Machines []string
+}
+
+func (c *RetryProvisioningCommand) Info() *cmd.Info {
+	return &cmd.Info{
+		Name:    "retry-provisioning",
+		Args:    "<machine> [...]",
+		Purpose: "retries provisioning for failed machines",
+	}
+}
+
+func (c *RetryProvisioningCommand) Init(args []string) error {
+	if len(args) == 0 {
+		return fmt.Errorf("no machine specified")
+	}
+	c.Machines = make([]string, len(args))
+	for i, arg := range args {
+		if !names.IsMachine(arg) {
+			return fmt.Errorf("invalid machine %q", arg)
+		}
+		c.Machines[i] = names.MachineTag(arg)
+	}
+	return nil
+}
+
+func (c *RetryProvisioningCommand) Run(context *cmd.Context) error {
+	client, err := juju.NewAPIClientFromName(c.EnvName)
+	if err != nil {
+		return err
+	}
+	defer client.Close()
+	results, err := client.RetryProvisioning(c.Machines...)
+	if err != nil {
+		return err
+	}
+	for i, result := range results {
+		if result.Error != nil {
+			fmt.Fprintf(context.Stderr, "cannot retry provisioning %q: %v\n", c.Machines[i], result.Error)
+		}
+	}
+	return nil
+}

=== added file 'cmd/juju/retryprovisioning_test.go'
--- cmd/juju/retryprovisioning_test.go	1970-01-01 00:00:00 +0000
+++ cmd/juju/retryprovisioning_test.go	2014-03-26 13:26:23 +0000
@@ -0,0 +1,82 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package main
+
+import (
+	"strings"
+
+	jc "github.com/juju/testing/checkers"
+	gc "launchpad.net/gocheck"
+
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/testing"
+)
+
+type retryProvisioningSuite struct {
+	jujutesting.JujuConnSuite
+}
+
+var _ = gc.Suite(&retryProvisioningSuite{})
+
+var resolvedMachineTests = []struct {
+	args   []string
+	err    string
+	stdErr string
+}{
+	{
+		err: `no machine specified`,
+	}, {
+		args: []string{"jeremy-fisher"},
+		err:  `invalid machine "jeremy-fisher"`,
+	}, {
+		args:   []string{"42"},
+		stdErr: `cannot retry provisioning "machine-42": machine 42 not found`,
+	}, {
+		args:   []string{"1"},
+		stdErr: `cannot retry provisioning "machine-1": machine "machine-1" is not in an error state`,
+	}, {
+		args: []string{"0"},
+	}, {
+		args:   []string{"0", "1"},
+		stdErr: `cannot retry provisioning "machine-1": machine "machine-1" is not in an error state`,
+	},
+}
+
+func (s *retryProvisioningSuite) TestResolved(c *gc.C) {
+	m, err := s.State.AddOneMachine(state.MachineTemplate{
+		Series: "quantal",
+		Jobs:   []state.MachineJob{state.JobManageEnviron},
+	})
+	c.Assert(err, gc.IsNil)
+	err = m.SetStatus(params.StatusError, "broken", nil)
+	c.Assert(err, gc.IsNil)
+	_, err = s.State.AddOneMachine(state.MachineTemplate{
+		Series: "quantal",
+		Jobs:   []state.MachineJob{state.JobHostUnits},
+	})
+	c.Assert(err, gc.IsNil)
+
+	for i, t := range resolvedMachineTests {
+		c.Logf("test %d: %v", i, t.args)
+		context, err := testing.RunCommand(c, &RetryProvisioningCommand{}, t.args)
+		if t.err != "" {
+			c.Check(err, gc.ErrorMatches, t.err)
+			continue
+		} else {
+			c.Check(err, gc.IsNil)
+		}
+		output := testing.Stderr(context)
+		stripped := strings.Replace(output, "\n", "", -1)
+		c.Check(stripped, gc.Equals, t.stdErr)
+		if t.args[0] == "0" {
+			status, info, data, err := m.Status()
+			c.Check(err, gc.IsNil)
+			c.Check(status, gc.Equals, params.StatusError)
+			c.Check(info, gc.Equals, "broken")
+			c.Check(data["transient"], jc.IsTrue)
+		}
+	}
+}

=== modified file 'state/api/client.go'
--- state/api/client.go	2014-03-26 09:49:29 +0000
+++ state/api/client.go	2014-03-26 13:26:23 +0000
@@ -138,18 +138,17 @@
 	return c.call("Resolved", p, nil)
 }
 
-// ResolveProvisioningError updates the provisioning status of a machine allowing the
+// RetryProvisioning updates the provisioning status of a machine allowing the
 // provisioner to retry.
-func (c *Client) ResolveProvisioningError(machine string) error {
-	p := params.Entities{
-		Entities: []params.Entity{{Tag: machine}},
+func (c *Client) RetryProvisioning(machines ...string) ([]params.ErrorResult, error) {
+	p := params.Entities{}
+	p.Entities = make([]params.Entity, len(machines))
+	for i, machine := range machines {
+		p.Entities[i] = params.Entity{Tag: machine}
 	}
 	var results params.ErrorResults
-	err := c.st.Call("Client", "", "ResolveProvisioningError", p, &results)
-	if err != nil {
-		return err
-	}
-	return results.OneError()
+	err := c.st.Call("Client", "", "RetryProvisioning", p, &results)
+	return results.Results, err
 }
 
 // PublicAddress returns the public address of the specified

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-03-26 09:49:29 +0000
+++ state/apiserver/client/client.go	2014-03-26 13:26:23 +0000
@@ -966,8 +966,8 @@
 	return charm.Quote(fmt.Sprintf("%s-%d-%s", name, revision, uuid)), nil
 }
 
-// ResolveProvisioningError marks a provisioning error as transient on the machines.
-func (c *Client) ResolveProvisioningError(p params.Entities) (params.ErrorResults, error) {
+// RetryProvisioning marks a provisioning error as transient on the machines.
+func (c *Client) RetryProvisioning(p params.Entities) (params.ErrorResults, error) {
 	entityStatus := make([]params.EntityStatus, len(p.Entities))
 	for i, entity := range p.Entities {
 		entityStatus[i] = params.EntityStatus{Tag: entity.Tag, Data: params.StatusData{"transient": true}}

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-03-26 09:49:29 +0000
+++ state/apiserver/client/client_test.go	2014-03-26 13:26:23 +0000
@@ -2068,12 +2068,12 @@
 	return strings.TrimPrefix(bundleURL.RequestURI(), "/dummyenv/private/")
 }
 
-func (s *clientSuite) TestResolveProvisioningError(c *gc.C) {
+func (s *clientSuite) TestRetryProvisioning(c *gc.C) {
 	machine, err := s.State.AddMachine("quantal", state.JobHostUnits)
 	c.Assert(err, gc.IsNil)
 	err = machine.SetStatus(params.StatusError, "error", nil)
 	c.Assert(err, gc.IsNil)
-	err = s.APIState.Client().ResolveProvisioningError(machine.Tag())
+	_, err = s.APIState.Client().RetryProvisioning(machine.Tag())
 	c.Assert(err, gc.IsNil)
 
 	status, info, data, err := machine.Status()

=== modified file 'state/apiserver/common/setstatus.go'
--- state/apiserver/common/setstatus.go	2014-03-26 00:24:30 +0000
+++ state/apiserver/common/setstatus.go	2014-03-26 13:26:23 +0000
@@ -4,6 +4,8 @@
 package common
 
 import (
+	"fmt"
+
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
 )
@@ -84,6 +86,9 @@
 	if !ok {
 		return NotSupportedError(tag, "updating status")
 	}
+	if len(newData) > 0 && existingStatus != params.StatusError {
+		return fmt.Errorf("machine %q is not in an error state", tag)
+	}
 	return entity.SetStatus(existingStatus, existingInfo, newData)
 }
 

=== modified file 'state/apiserver/common/setstatus_test.go'
--- state/apiserver/common/setstatus_test.go	2014-03-26 00:24:30 +0000
+++ state/apiserver/common/setstatus_test.go	2014-03-26 13:26:23 +0000
@@ -122,16 +122,17 @@
 	st := &fakeState{
 		entities: map[string]entityWithError{
 			"x0": &fakeStatusSetter{status: params.StatusPending, info: "blah", err: fmt.Errorf("x0 fails")},
-			"x1": &fakeStatusSetter{status: params.StatusStarted, info: "foo", data: params.StatusData{"foo": "blah"}},
+			"x1": &fakeStatusSetter{status: params.StatusError, info: "foo", data: params.StatusData{"foo": "blah"}},
 			"x2": &fakeStatusSetter{status: params.StatusError, info: "some info"},
 			"x3": &fakeStatusSetter{fetchError: "x3 error"},
-			"x4": &fakeStatusSetter{status: params.StatusStopped, info: ""},
+			"x4": &fakeStatusSetter{status: params.StatusStarted},
+			"x5": &fakeStatusSetter{status: params.StatusStopped, info: ""},
 		},
 	}
 	getCanModify := func() (common.AuthFunc, error) {
 		return func(tag string) bool {
 			switch tag {
-			case "x0", "x1", "x2", "x3":
+			case "x0", "x1", "x2", "x3", "x4":
 				return true
 			}
 			return false
@@ -145,7 +146,8 @@
 			{Tag: "x2", Data: params.StatusData{"foo": "bar"}},
 			{Tag: "x3", Data: params.StatusData{"foo": "bar"}},
 			{Tag: "x4", Data: params.StatusData{"foo": "bar"}},
-			{Tag: "x5", Data: nil},
+			{Tag: "x5", Data: params.StatusData{"foo": "bar"}},
+			{Tag: "x6", Data: nil},
 		},
 	}
 	result, err := s.UpdateStatus(args)
@@ -156,6 +158,7 @@
 			{nil},
 			{nil},
 			{&params.Error{Message: "x3 error"}},
+			{&params.Error{Message: `machine "x4" is not in an error state`}},
 			{apiservertesting.ErrUnauthorized},
 			{apiservertesting.ErrUnauthorized},
 		},
@@ -163,7 +166,7 @@
 	get := func(tag string) *fakeStatusSetter {
 		return st.entities[tag].(*fakeStatusSetter)
 	}
-	c.Assert(get("x1").status, gc.Equals, params.StatusStarted)
+	c.Assert(get("x1").status, gc.Equals, params.StatusError)
 	c.Assert(get("x1").info, gc.Equals, "foo")
 	c.Assert(get("x1").data, gc.DeepEquals, params.StatusData{"foo": "blah"})
 	c.Assert(get("x2").status, gc.Equals, params.StatusError)