Merge lp://staging/~tasdomas/juju-core/ensure-availability-output into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+221549_code.launchpad.net,

Message:
Please take a look.

Description:
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-output/+merge/221549

(do not edit description out of merge proposal)

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

Affected files (+290, -43 lines):
   A [revision details]
   M cmd/juju/ensureavailability.go
   M cmd/juju/ensureavailability_test.go
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go
   M state/apiserver/client/export_test.go
   M state/export_test.go
   M state/state.go

Please take a look.

https://codereview.appspot.com/99670044/

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/ensureavailability.go
File cmd/juju/ensureavailability.go (right):

https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailability.go#newcode91
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/ensureavailability.go#newcode97
cmd/juju/ensureavailability.go:97: ln := len(machines)
I'd prefer "count" to "ln".

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

https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailability.go#newcode107
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#newcode738
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/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/99670044/diff/20001/state/apiserver/client/client.go#newcode1085
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/client.go#newcode1128
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/client.go#newcode1132
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.

(...

Please take a look.

https://codereview.appspot.com/99670044/

Thanks for taking the time to review this.

https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailability.go#newcode91
> 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#newcode738
> 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/client.go#newcode1128
> 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/client.go#newcode1132
> 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!

https://codereview.appspot.com/99670044/

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/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/99670044/diff/40001/state/apiserver/client/client.go#newcode1092
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, b...

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/client.go#newcode1092
> 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/

https://codereview.appspot.com/99670044/

On 3 June 2014 09:30, <email address hidden> 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.