Add output for ensure-availability command.

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
maintaining machines: "0"
adding machines: "1", "2", "3", "4", "5", "6"

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

(do not edit description out of merge proposal)

[domas, 2014-05-30 14:12:51]

Please take a look.

[domas, 2014-05-30 14:20:16]

Please take a look.

[domas, 2014-05-30 14:34:32]

On 2014/05/30 14:12:51, domas wrote:
> Please take a look.

This is a rewrite of the proposal https://codereview.appspot.com/92390050/
submitted earlier.

[fwereade, 2014-06-02 09:50:04]

Definitely do the formatters, but you might want to hold off on hitting the API
too hard until nate's had a chance to comment. Ping me if anything makes little
sense.

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

https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailabilit...
cmd/juju/ensureavailability.go:91: }
I feel like this ought to be controlled by formatters with a cmd.Out -- so you
can use a default "--format simple" to get output like the above, and json/yaml
for the use of machines. See https://codereview.appspot.com/92610043/ for a bit
more discussion around this topic.

https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailabilit...
cmd/juju/ensureavailability.go:97: ln := len(machines)
I'd prefer "count" to "ln".

https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailabilit...
cmd/juju/ensureavailability.go:104: result += fmt.Sprintf("\"%s\"", machineId)
%q?

https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailabilit...
cmd/juju/ensureavailability.go:107: }
strings.Join()?

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

https://codereview.appspot.com/99670044/diff/20001/state/api/params/params.go...
state/api/params/params.go:738: type EnsureAvailabilityResult struct {
Surely, this is not an anythingResult? It's a StateServerChange, or something,
isn't it?

While you're at it, would you change the name of EnsureAvailability please?
That's a verb, not a noun; I'm drawing a bit of a blank on a better name at the
moment, but things like StateServerRequest and StateServersSpec are floating
around my head.

Regardless, I'd expect us to be accepting/returning arrays of same; and the
structures in play to look something like:

type StateServerChange struct { ... }
type StateServerChangeResult struct {
    Result *StateServerChangeResult
    Error  *Error
}
type StateServerChangeResults struct {
    Results []StateServerChangeResult
}

https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:1085: func
ensureAvailabilityResultFromSSI(SSIBefore, SSIAfter *state.StateServerInfo)
params.EnsureAvailabilityResult {
it's jarring to have vars with uppercase at the beginning. ssiBefore, ssiAfter?

https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:1128: return result
I feel we could probably do this more easily with some set.Strings~s?

https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:1132: func (c *Client) EnsureAvailability(args
params.EnsureAvailability) (params.EnsureAvailabilityResult, error) {
Grar. Nate, have we yet released a stable with this API? It ought to be both
accepting and returning bulk args.

(note: I'm easy on how we indicate "the current environment", I'd prefer a
special magic string that can't be confused for an environ tag, but I think we
still want to specify it)

https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:2388: var ensureAvailabilityResultTests =
[]ensureAvailabilityResultTest{
If these are used in just one method, please put them inside rather than making
them global. Given the opportunity, people can and will abuse even these
unassuming test0data sorts of globals :(.

https://codereview.appspot.com/99670044/diff/20001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/99670044/diff/20001/state/state.go#newcode385
state/state.go:385: func MachineIdLessThan(id1, id2 string) bool {
If you're exporting this, I think it would probably live most happily in the
names package.

[domas, 2014-06-02 17:07:01]

Please take a look.

[domas, 2014-06-02 17:12:29]

Thanks for taking the time to review this.
>
https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailabilit...
> cmd/juju/ensureavailability.go:91: }
> I feel like this ought to be controlled by formatters with a cmd.Out -- so you
> can use a default "--format simple" to get output like the above, and
json/yaml
> for the use of machines. See https://codereview.appspot.com/92610043/ for a
bit
> more discussion around this topic.
Added support for formatters.

>
https://codereview.appspot.com/99670044/diff/20001/state/api/params/params.go...
> state/api/params/params.go:738: type EnsureAvailabilityResult struct {
> Surely, this is not an anythingResult? It's a StateServerChange, or something,
> isn't it?
> 
> While you're at it, would you change the name of EnsureAvailability please?
> That's a verb, not a noun; I'm drawing a bit of a blank on a better name at
the
> moment, but things like StateServerRequest and StateServersSpec are floating
> around my head.
> 
> Regardless, I'd expect us to be accepting/returning arrays of same; and the
> structures in play to look something like:
> 
> type StateServerChange struct { ... }
> type StateServerChangeResult struct {
>     Result *StateServerChangeResult
>     Error  *Error
> }
> type StateServerChangeResults struct {
>     Results []StateServerChangeResult
> }
I'm not really sure how this would fit with the kind of results the ensure
availability call is returning
(i.e. separate lists of maintained, removed, added machines). As for the naming
- I tried to copy the patterns
I saw in the params.go file.

>
https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
> state/apiserver/client/client.go:1128: return result
> I feel we could probably do this more easily with some set.Strings~s?
Thanks! I managed to miss the string set implemented in juju-core/utils.

>
https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/cli...
> state/apiserver/client/client.go:1132: func (c *Client)
EnsureAvailability(args
> params.EnsureAvailability) (params.EnsureAvailabilityResult, error) {
> Grar. Nate, have we yet released a stable with this API? It ought to be both
> accepting and returning bulk args.
> 
> (note: I'm easy on how we indicate "the current environment", I'd prefer a
> special magic string that can't be confused for an environ tag, but I think we
> still want to specify it)
I need to take a closer look a bulk apis (I'm not very familiar with them at the
moment). In any case - can we consider landing this as-is and doing the move to
bulk apis as a separate work item?

As for all the smaller items in the comments - I think I've addressed them all.
Once again, thanks!

[rog, 2014-06-03 07:55:50]

This looks fine for what it's trying to do, thanks, but I think we should get
state.EnsureAvailability to report what it's done directly, rather than
inferring it by looking at the resulting state servers. And one more suggestion
below.

https://codereview.appspot.com/99670044/diff/20001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/99670044/diff/20001/state/state.go#newcode380
state/state.go:380: // MachineIdLessThan returns true if id1 < id2, false
otherwise.
Go-conventional phrasing for this would be:

// MachineIdLessThan reports whether id1 is less than id2.

https://codereview.appspot.com/99670044/diff/20001/state/state.go#newcode385
state/state.go:385: func MachineIdLessThan(id1, id2 string) bool {
On 2014/06/02 09:50:03, fwereade wrote:
> If you're exporting this, I think it would probably live most happily in the
> names package.

+1

BTW, it seems to me that this is somewhat overkill for what we're actually using
it for, which is to sort state server machines which are never containerised in
fact.

That said, since this is written, we might as well keep it - then
again, I wonder if we should just use exactly the same logic that yaml uses
to sort map keys - that way we'll definitely be consistent with the
output of status, for example. See goyaml.keyList.Less for that
logic:

func less(a, b string) bool {
	ar, br := []rune(a), []rune(b)
	for i := 0; i < len(ar) && i < len(br); i++ {
		if ar[i] == br[i] {
			continue
		}
		al := unicode.IsLetter(ar[i])
		bl := unicode.IsLetter(br[i])
		if al && bl {
			return ar[i] < br[i]
		}
		if al || bl {
			return bl
		}
		var ai, bi int
		var an, bn int64
		for ai = i; ai < len(ar) && unicode.IsDigit(ar[ai]); ai++ {
			an = an*10 + int64(ar[ai]-'0')
		}
		for bi = i; bi < len(br) && unicode.IsDigit(br[bi]); bi++ {
			bn = bn*10 + int64(br[bi]-'0')
		}
		if an != bn {
			return an < bn
		}
		if ai != bi {
			return ai < bi
		}
		return ar[i] < br[i]
	}
	return len(ar) < len(br)
}

If we export the above function (probably from utils) then we'll have
a nice general routine that can be used for all kinds of ids
and tags and all the machine-id-specific parsing behaviour here 
becomes unnecessary.

https://codereview.appspot.com/99670044/diff/20001/state/state.go#newcode397
state/state.go:397: nrParts1 := len(idParts1)
From a performance point of view, there's no point in caching slice length, and
I actually suspect the code would read better with the more direct len(idPartsX)
rather than the extra temp vars.

https://codereview.appspot.com/99670044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/99670044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:1092: ssiBefore, err :=
c.api.state.StateServerInfo()
I don't think this is quite right. state.EnsureAvailability was restructured
some time ago so that it would be easy for that command to return information on
what it has done. Specifically, there's a struct in there,
"ensureAvailabilityIntent" which has *almost* all the information you'll need.
The only thing missing is the ids of the new state servers, but that should be
easily added.

Then you can easily produce output that reflects everything that
EnsureAvailability is doing, including making machines voting, removing
state server status, etc - all of which is very useful for a user to be
able to find out.

[domas, 2014-06-03 08:30:34]

On 2014/06/03 07:55:50, rog wrote:
> This looks fine for what it's trying to do, thanks, but I think we should get
> state.EnsureAvailability to report what it's done directly, rather than
> inferring it by looking at the resulting state servers. And one more
suggestion
> below.
Me and Matthew had implemented it this way initially, but were advised to pursue
this approach instead.

> https://codereview.appspot.com/99670044/diff/20001/state/state.go#newcode380
> state/state.go:380: // MachineIdLessThan returns true if id1 < id2, false
> otherwise.
I've reverted changes to that - once I rewrote the change detection to use
utils/set, I no longer needed
machineIdLessThan to be public.

>
https://codereview.appspot.com/99670044/diff/40001/state/apiserver/client/cli...
> state/apiserver/client/client.go:1092: ssiBefore, err :=
> c.api.state.StateServerInfo()
> I don't think this is quite right. state.EnsureAvailability was restructured
> some time ago so that it would be easy for that command to return information
on
> what it has done. Specifically, there's a struct in there,
> "ensureAvailabilityIntent" which has *almost* all the information you'll need.
> The only thing missing is the ids of the new state servers, but that should be
> easily added.
> 
> Then you can easily produce output that reflects everything that
> EnsureAvailability is doing, including making machines voting, removing
> state server status, etc - all of which is very useful for a user to be
> able to find out.
This was the initial implementation (it had it's problems, but it followed the
logic you outlined): https://codereview.appspot.com/92390050/

[rog, 2014-06-03 11:06:25]

On 3 June 2014 09:30,  <domas.monkus@canonical.com> wrote:
> On 2014/06/03 07:55:50, rog wrote:
>>
>> This looks fine for what it's trying to do, thanks, but I think we
>
> should get
>>
>> state.EnsureAvailability to report what it's done directly, rather
>
> than
>>
>> inferring it by looking at the resulting state servers. And one more
>
> suggestion
>>
>> below.
>
> Me and Matthew had implemented it this way initially, but were advised
> to pursue this approach instead.

From the review, it looks to me as if the main thing was not that
we couldn't use results from the EnsureAvailability call, but just
that we don't use the results type *directly* from the EnsureAvailability
call in the API.

But I did miss this:

: 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.

I still think this is not a great approach. For one thing, it doesn't
actually say what that EnsureAvailability call did - it just finds
out what has happened  recently. So if two people do an EnsureAvailability
call at the same moment, the results may be confusing and
hard to understand, because they may pertain to the logic
run twice.

Another reason I'm not keen on it is that it second-guesses the
EnsureAvailability logic - it means that the API server
has to know everything that the underlying call
might have done, inferring it from before-and-after state.

We have the info, but we then throw it away and try to reconstruct it.

FWIW the initial implementation looked fine to me. With
respect to the push-back, the params package
is specifically about types that are used in the API, so you *know*
if you're changing a type in state/api/params that you will be changing
API call parameters. And the state.EnsureAvailability method is just
there in the service of the api server, so adding an extra layer
seems to me unnecessary and counterproductive. I appreciate
that others have different views on this though.