ensure-availability command

ensure-availability command

We introduce a new ensure-availability juju subcommand,
which calls the new EnsureAvailability client API. This
is the CLI for ensuring high availability of Juju
state servers.

Note: I have not exposed the ensure-availability command yet,
as there is still work to be done in the agents to
properly support HA state servers.

https://code.launchpad.net/~axwalk/juju-core/cmdapi-ensureavailability/+merge/213190

(do not edit description out of merge proposal)

[axw, 2014-03-28 03:51:09]

Please take a look.

[rog, 2014-03-28 07:54:23]

LGTM modulo the below, thanks!

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

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode19
cmd/juju/ensureha.go:19: // If specified, use this series, else use the
environment default-series
use this series for newly started instances ?

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode50
cmd/juju/ensureha.go:50: Name:    "ensure-ha",
I'm not sure about "ha" in lower case - it looks too much like a word. I'm
thinking that either we make it into a proper word ("availability") or we
capitalise the ha - ensure-HA, thus making it clearer that it's an acronym.

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode58
cmd/juju/ensureha.go:58: f.IntVar(&c.NumStateServers, "n", 1, "number of state
servers to make available")
Thought for the future - it would be good to be able to run "juju ensure-HA"
without any arguments at all, for when we just want to maintain the current
availability. Perhaps rather than defaulting to 1, we should default to -1 and
complain if it's not specified. That way we leave that path open.

https://codereview.appspot.com/81700043/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/81700043/diff/1/state/api/client.go#newcode680
state/api/client.go:680: } else if err := result.Error; err != nil {
There's no point in having an Error in the result as well as an error return.

https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:649: Error *Error
d

https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:990: if err :=
c.api.state.EnsureAvailability(args.NumStateServers, args.Constraints, series);
err != nil {
err := c.api.state.EnsureAvailability(args.NumStateServers, args.Constraints,
series)
return params.EnsureAvailabilityResult{}, err

[axw, 2014-03-28 10:03:41]

Please take a look.

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

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode19
cmd/juju/ensureha.go:19: // If specified, use this series, else use the
environment default-series
On 2014/03/28 07:54:23, rog wrote:
> use this series for newly started instances ?

Done.

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode50
cmd/juju/ensureha.go:50: Name:    "ensure-ha",
On 2014/03/28 07:54:23, rog wrote:
> I'm not sure about "ha" in lower case - it looks too much like a word. I'm
> thinking that either we make it into a proper word ("availability") or we
> capitalise the ha - ensure-HA, thus making it clearer that it's an acronym.

Yeah, it didn't look quite right to me, I was following what was in the card.
I've changed it to ensure-availability to be consistent with the API.

If others disagree, it can be changed easily enough later -- this command is not
yet exposed.

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode58
cmd/juju/ensureha.go:58: f.IntVar(&c.NumStateServers, "n", 1, "number of state
servers to make available")
On 2014/03/28 07:54:23, rog wrote:
> Thought for the future - it would be good to be able to run "juju ensure-HA"
> without any arguments at all, for when we just want to maintain the current
> availability. Perhaps rather than defaulting to 1, we should default to -1 and
> complain if it's not specified. That way we leave that path open.

Good point, will disallow for now.

https://codereview.appspot.com/81700043/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/81700043/diff/1/state/api/client.go#newcode680
state/api/client.go:680: } else if err := result.Error; err != nil {
On 2014/03/28 07:54:23, rog wrote:
> There's no point in having an Error in the result as well as an error return.

Done.

https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:649: Error *Error
On 2014/03/28 07:54:23, rog wrote:
> d

Done.

https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:990: if err :=
c.api.state.EnsureAvailability(args.NumStateServers, args.Constraints, series);
err != nil {
On 2014/03/28 07:54:23, rog wrote:
> err := c.api.state.EnsureAvailability(args.NumStateServers, args.Constraints,
> series)
> return params.EnsureAvailabilityResult{}, err

Done, except the result is no longer necessary so I've dropped it.

[rog, 2014-03-28 10:09:59]

On 2014/03/28 10:03:41, axw wrote:
> Please take a look.
> 
> https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go
> File cmd/juju/ensureha.go (right):
> 
> https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode19
> cmd/juju/ensureha.go:19: // If specified, use this series, else use the
> environment default-series
> On 2014/03/28 07:54:23, rog wrote:
> > use this series for newly started instances ?
> 
> Done.
> 
> https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode50
> cmd/juju/ensureha.go:50: Name:    "ensure-ha",
> On 2014/03/28 07:54:23, rog wrote:
> > I'm not sure about "ha" in lower case - it looks too much like a word. I'm
> > thinking that either we make it into a proper word ("availability") or we
> > capitalise the ha - ensure-HA, thus making it clearer that it's an acronym.
> 
> Yeah, it didn't look quite right to me, I was following what was in the card.
> I've changed it to ensure-availability to be consistent with the API.
> 
> If others disagree, it can be changed easily enough later -- this command is
not
> yet exposed.
> 
> https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode58
> cmd/juju/ensureha.go:58: f.IntVar(&c.NumStateServers, "n", 1, "number of state
> servers to make available")
> On 2014/03/28 07:54:23, rog wrote:
> > Thought for the future - it would be good to be able to run "juju ensure-HA"
> > without any arguments at all, for when we just want to maintain the current
> > availability. Perhaps rather than defaulting to 1, we should default to -1
and
> > complain if it's not specified. That way we leave that path open.
> 
> Good point, will disallow for now.
> 
> https://codereview.appspot.com/81700043/diff/1/state/api/client.go
> File state/api/client.go (right):
> 
> https://codereview.appspot.com/81700043/diff/1/state/api/client.go#newcode680
> state/api/client.go:680: } else if err := result.Error; err != nil {
> On 2014/03/28 07:54:23, rog wrote:
> > There's no point in having an Error in the result as well as an error
return.
> 
> Done.
> 
> https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go
> File state/api/params/params.go (right):
> 
>
https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go#new...
> state/api/params/params.go:649: Error *Error
> On 2014/03/28 07:54:23, rog wrote:
> > d
> 
> Done.
> 
>
https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client.go
> File state/apiserver/client/client.go (right):
> 
>
https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client....
> state/apiserver/client/client.go:990: if err :=
> c.api.state.EnsureAvailability(args.NumStateServers, args.Constraints,
series);
> err != nil {
> On 2014/03/28 07:54:23, rog wrote:
> > err := c.api.state.EnsureAvailability(args.NumStateServers,
args.Constraints,
> > series)
> > return params.EnsureAvailabilityResult{}, err
> 
> Done, except the result is no longer necessary so I've dropped it.

LGTM, thanks!

[axw, 2014-03-28 13:37:45]

Please take a look.