Add output for ensure-availability command.

Add output for ensure-availability command.

The ensure-availability command will notify the user, how many state servers were added to the environment.

$ juju ensure-availability -n 7
Added 6 state servers.

https://code.launchpad.net/~tasdomas/juju-core/ensure-availability-more-output/+merge/220306

(do not edit description out of merge proposal)

[domas, 2014-05-20 16:28:43]

Please take a look.

[mattyw, 2014-05-20 16:32:23]

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

https://codereview.appspot.com/92390050/diff/1/cmd/juju/ensureavailability.go...
cmd/juju/ensureavailability.go:83: ctx.Infof("Added %d state servers.",
availabilityResult.NewCount)
Maybe we should add a test for this?

[domas, 2014-05-20 16:51:48]

Please take a look.

[axw, 2014-05-21 01:46:54]

https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
File cmd/juju/ensureavailability.go (right):

https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
cmd/juju/ensureavailability.go:83: ctx.Infof("Added %d state servers.",
availabilityResult.NewCount)
Plural form is wrong if only one is added. I think it would also be nice to see
the Promote, Demote and Remove results.

[domas, 2014-05-21 07:35:06]

On 2014/05/21 01:46:54, axw wrote:
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> File cmd/juju/ensureavailability.go (right):
> 
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> cmd/juju/ensureavailability.go:83: ctx.Infof("Added %d state servers.",
> availabilityResult.NewCount)
> Plural form is wrong if only one is added. I think it would also be nice to
see
> the Promote, Demote and Remove results.
I agree about the plural form - a fix is on its way.

Not so sure about displaying promote, demote and remove results - are they
really relevant from a
juju user's standpoint?

[domas, 2014-05-21 07:42:52]

Please take a look.

[axw, 2014-05-21 07:44:44]

On 2014/05/21 07:35:06, domas wrote:
> On 2014/05/21 01:46:54, axw wrote:
> >
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> > File cmd/juju/ensureavailability.go (right):
> > 
> >
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> > cmd/juju/ensureavailability.go:83: ctx.Infof("Added %d state servers.",
> > availabilityResult.NewCount)
> > Plural form is wrong if only one is added. I think it would also be nice to
> see
> > the Promote, Demote and Remove results.
> I agree about the plural form - a fix is on its way.
> 
> Not so sure about displaying promote, demote and remove results - are they
> really relevant from a
> juju user's standpoint?

Fair point, I suppose it is a bit low-level. I was thinking in terms of
developers. As you were :)

[fwereade, 2014-05-21 07:45:09]

On 2014/05/21 07:35:06, domas wrote:
> On 2014/05/21 01:46:54, axw wrote:
> >
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> > File cmd/juju/ensureavailability.go (right):
> > 
> >
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> > cmd/juju/ensureavailability.go:83: ctx.Infof("Added %d state servers.",
> > availabilityResult.NewCount)
> > Plural form is wrong if only one is added. I think it would also be nice to
> see
> > the Promote, Demote and Remove results.
> I agree about the plural form - a fix is on its way.
> 
> Not so sure about displaying promote, demote and remove results - are they
> really relevant from a
> juju user's standpoint?

I think they are -- knowing that N have been added is somewhat interesting, but
if I'm responsible for HA of my environment I think I'd appreciate knowing that
(say):

 * machines 0 and 1 are still healthy and were not touched
 * machine 2 looked like it was broken, and was demoted
 * machine 7 was added to replace machine 2

[domas, 2014-05-21 17:25:07]

Please take a look.

[domas, 2014-05-21 17:27:10]

On 2014/05/21 07:45:09, fwereade wrote:
> On 2014/05/21 07:35:06, domas wrote:
> > On 2014/05/21 01:46:54, axw wrote:
> > >
> >
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> > > File cmd/juju/ensureavailability.go (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/92390050/diff/20001/cmd/juju/ensureavailabilit...
> > > cmd/juju/ensureavailability.go:83: ctx.Infof("Added %d state servers.",
> > > availabilityResult.NewCount)
> > > Plural form is wrong if only one is added. I think it would also be nice
to
> > see
> > > the Promote, Demote and Remove results.
> > I agree about the plural form - a fix is on its way.
> > 
> > Not so sure about displaying promote, demote and remove results - are they
> > really relevant from a
> > juju user's standpoint?
> 
> I think they are -- knowing that N have been added is somewhat interesting,
but
> if I'm responsible for HA of my environment I think I'd appreciate knowing
that
> (say):
> 
>  * machines 0 and 1 are still healthy and were not touched
>  * machine 2 looked like it was broken, and was demoted
>  * machine 7 was added to replace machine 2

Discussed this with Nate Finch - verbose mode will display the servers that were
promoted/demoted/removed/maintained.

Non-verbose mode will only display the number of servers added and the list of
servers being removed.

[fwereade, 2014-05-26 13:27:34]

Couple more comments. I should chat to nate.

https://codereview.appspot.com/92390050/diff/60001/cmd/juju/ensureavailabilit...
File cmd/juju/ensureavailability.go (right):

https://codereview.appspot.com/92390050/diff/60001/cmd/juju/ensureavailabilit...
cmd/juju/ensureavailability.go:85: ctx.Infof("Added %d state server.",
availabilityResult.NewCount)
We've generally been consistent about lower-case in messages.

https://codereview.appspot.com/92390050/diff/60001/cmd/juju/ensureavailabilit...
cmd/juju/ensureavailability.go:115: result += fmt.Sprintf("\"%s\"", machine.Tag)
Tags are for the API, not for the UI. There should be a func in the names
package for getting ids back from tags.

https://codereview.appspot.com/92390050/diff/60001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/92390050/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:504: func (st *State) EnsureAvailability(numStateServers
int, cons constraints.Value, series string) (params.EnsureAvailabilityResult,
error) {
I'm not very keen on this (although I see someone recently did it with
params.StateServingInfo -- Nate, shouldn't that be fixed as well?). It makes it
way to easy to change the API by "just" adding/removing a field that's used by
the state package.

And this is talking in terms of tags, anyway. While there's been some leakage of
the tag vocabulary into state, it's not really a good thing, and only used when
there's some uncertainty about the kind of entity we're dealing with. Please
either use a result struct appropriate to the problem (*including* the new
machines created) or, I think better, leave this method alone and just figure
out what changed in the course of the call by diffing two
what-are-the-state-servers calls.

https://codereview.appspot.com/92390050/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:637: }
This doesn't tell us which machines are the added state servers, which is IMO
the main thing people want to know when they call ensure-availability.