Juju status supports containers

Juju status supports containers

juju status shows containers nested against their parent machine.
The machine id less than function needed to be updated to support
container ids. A drive by additional test for adding containers
to an injected machine is also included.

https://code.launchpad.net/~wallyworld/juju-core/container-status/+merge/169096

(do not edit description out of merge proposal)

[wallyworld, 2013-06-13 02:59:54]

Please take a look.

[thumper, 2013-06-13 03:28:00]

https://codereview.appspot.com/10252044/diff/1/cmd/juju/status.go
File cmd/juju/status.go (left):

https://codereview.appspot.com/10252044/diff/1/cmd/juju/status.go#oldcode131
cmd/juju/status.go:131: func (ctxt *statusContext) processMachines()
map[string]machineStatus {
I don't suppose I can convince you to rename "ctxt" to "context" ?

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

https://codereview.appspot.com/10252044/diff/1/cmd/juju/status.go#newcode170
cmd/juju/status.go:170: break
Why is this break needed?

https://codereview.appspot.com/10252044/diff/1/state/container.go
File state/container.go (right):

https://codereview.appspot.com/10252044/diff/1/state/container.go#newcode102
state/container.go:102: func NestingLevel(machineId string) int {
Where are the tests for this and TopParentId?

[wallyworld, 2013-06-13 05:50:09]

Please take a look.

https://codereview.appspot.com/10252044/diff/1/cmd/juju/status.go
File cmd/juju/status.go (left):

https://codereview.appspot.com/10252044/diff/1/cmd/juju/status.go#oldcode131
cmd/juju/status.go:131: func (ctxt *statusContext) processMachines()
map[string]machineStatus {
On 2013/06/13 03:28:00, thumper wrote:
> I don't suppose I can convince you to rename "ctxt" to "context" ?

Done.

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

https://codereview.appspot.com/10252044/diff/1/cmd/juju/status.go#newcode170
cmd/juju/status.go:170: break
On 2013/06/13 03:28:00, thumper wrote:
> Why is this break needed?

It exists the loop and hence the function and steps back up a level in the
recursive call chain (to continue processing the machines at the previous
nesting level).

https://codereview.appspot.com/10252044/diff/1/state/container.go
File state/container.go (right):

https://codereview.appspot.com/10252044/diff/1/state/container.go#newcode102
state/container.go:102: func NestingLevel(machineId string) int {
On 2013/06/13 03:28:00, thumper wrote:
> Where are the tests for this and TopParentId?

They ran away but I hunted them down and found them.

[fwereade, 2013-06-13 14:48:31]

LGTM modulo quibbling about MachineIdLessThan

https://codereview.appspot.com/10252044/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10252044/diff/6001/state/state.go#newcode390
state/state.go:390: func MachineIdLessThan(id1, id2 string) bool {
Can we keep this unexported and test it via export_test please?

...and consider that this'll be called rather a lot when we're sorting a big
list of machines. Might it be worth sorting machines by using an ancillary
[][]struct{kind string; index int} for the actual compares, in a sort of DSU
style?

Maybe it doesn't pay off early enough, I don't have actual performance numbers;
but it's a thought.

[wallyworld, 2013-06-14 00:25:09]

Please take a look.

https://codereview.appspot.com/10252044/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10252044/diff/6001/state/state.go#newcode390
state/state.go:390: func MachineIdLessThan(id1, id2 string) bool {
On 2013/06/13 14:48:31, fwereade wrote:
> Can we keep this unexported and test it via export_test please?
> 
> ...and consider that this'll be called rather a lot when we're sorting a big
> list of machines. Might it be worth sorting machines by using an ancillary
> [][]struct{kind string; index int} for the actual compares, in a sort of DSU
> style?
> 
> Maybe it doesn't pay off early enough, I don't have actual performance
numbers;
> but it's a thought.

I've put a short circuit in the implementation for when both ids are ints (which
will be the common case). We only do the hard bit if there's at least one
container id.

[thumper, 2013-06-14 02:00:44]

On 2013/06/14 00:25:09, wallyworld wrote:
> Please take a look.
> 
> https://codereview.appspot.com/10252044/diff/6001/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/10252044/diff/6001/state/state.go#newcode390
> state/state.go:390: func MachineIdLessThan(id1, id2 string) bool {
> On 2013/06/13 14:48:31, fwereade wrote:
> > Can we keep this unexported and test it via export_test please?
> > 
> > ...and consider that this'll be called rather a lot when we're sorting a big
> > list of machines. Might it be worth sorting machines by using an ancillary
> > [][]struct{kind string; index int} for the actual compares, in a sort of DSU
> > style?
> > 
> > Maybe it doesn't pay off early enough, I don't have actual performance
> numbers;
> > but it's a thought.
> 
> I've put a short circuit in the implementation for when both ids are ints
(which
> will be the common case). We only do the hard bit if there's at least one
> container id.

LGTM now \o/..

remember you need to resubmit against trunk and approve.

[jameinel, 2013-06-16 08:28:24]

A small tweak about how you express mod 2, but otherwise
LGTM

https://codereview.appspot.com/10252044/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10252044/diff/6001/state/state.go#newcode407
state/state.go:407: return m1 < m2
You have an int, and as near as I can tell, go does support the '%' operator:
http://golang.org/ref/spec

So this could be either:

if x % 2 == 1

or since it works in binary

if x & 1 == 1

I don't really care, but using math.Mod(float()) seems very strange here.

[wallyworld, 2013-06-16 11:13:18]

On 2013/06/16 08:28:24, jameinel wrote:
> A small tweak about how you express mod 2, but otherwise
> LGTM
> 
> https://codereview.appspot.com/10252044/diff/6001/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/10252044/diff/6001/state/state.go#newcode407
> state/state.go:407: return m1 < m2
> You have an int, and as near as I can tell, go does support the '%' operator:
> http://golang.org/ref/spec
> 
> So this could be either:
> 
> if x % 2 == 1
> 
> or since it works in binary
> 
> if x & 1 == 1
> 
> I don't really care, but using math.Mod(float()) seems very strange here.

Thanks for that. I didn't think there was a mod operator. Will fix.