Merge lp://staging/~axwalk/juju-core/lp1167441-report-instance-state into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+178222_code.launchpad.net,

Message:
Please take a look.

Description:
Add Instance.State method, use in juju status

Add State() method to Instance interface, and
implementations for azure, ec2, openstack and lxc.
Local and MAAS always return "".

https://code.launchpad.net/~axwalk/juju-core/lp1167441-report-instance-state/+merge/178222

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/status.go
   M container/lxc/instance.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M environs/azure/instance.go
   M environs/azure/instance_test.go
   M environs/dummy/environs.go
   M environs/ec2/ec2.go
   M environs/ec2/local_test.go
   M environs/local/instance.go
   M environs/maas/instance.go
   M environs/openstack/local_test.go
   M environs/openstack/provider.go
   M environs/polling_test.go
   M instance/instance.go

LGTM, this is great. I'd love to see a followup for the local provider,
that should be quick and easy.

https://codereview.appspot.com/12333043/diff/1/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/container/lxc/instance.go#newcode30
container/lxc/instance.go:30: return string(state)
I see this, and it looks good, but I'm a bit confused as to why it
doesn't work with the local provider. (Well, I can see why, but I can't
understand the motivation -- can anyone think of a drawback to putting
the lxc.Instances into the local.Instances so that we can access this
method? I can understand differences in address handling, but I don't
think the same forces apply here.)

https://codereview.appspot.com/12333043/diff/1/environs/local/instance.go
File environs/local/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/environs/local/instance.go#newcode27
environs/local/instance.go:27: return ""
I'd like to do what I suggested earlier and expose the lxc instance
state here. But please do it in a followup, and run it by thumper first,
because if there's a reason not to do it he'll know what it is.

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go
File environs/maas/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go#newcode26
environs/maas/instance.go:26: // TODO determine MAAS instance state
Add a bug in LP and note the number here please; and ping bigjools, or
someone else who knows maas, to find out what'll be needed to implement
it, and note that in the bug.

https://codereview.appspot.com/12333043/diff/1/environs/polling_test.go
File environs/polling_test.go (left):

https://codereview.appspot.com/12333043/diff/1/environs/polling_test.go#oldcode63
environs/polling_test.go:63: func (*dnsNameFakeInstance) Ports(string)
([]instance.Port, error) { return nil, nil }
Complete side note: would it be useful for dnsNameFakeInstance to just
embed an instance.Instance, and thus panic on use of unwanted methods?
Feels a bit saner than just returning nonsense without error...

https://codereview.appspot.com/12333043/

https://codereview.appspot.com/12333043/diff/1/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/container/lxc/instance.go#newcode30
container/lxc/instance.go:30: return string(state)
On 2013/08/02 17:30:32, fwereade wrote:
> I see this, and it looks good, but I'm a bit confused as to why it
doesn't work
> with the local provider. (Well, I can see why, but I can't understand
the
> motivation -- can anyone think of a drawback to putting the
lxc.Instances into
> the local.Instances so that we can access this method? I can
understand
> differences in address handling, but I don't think the same forces
apply here.)

Are you referring to the lxc.Instance being converted to local.Instance?
I don't understand the reason behind this. I think I'll have to have a
chat with thumper after IOM to understand this better.

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go
File environs/maas/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go#newcode26
environs/maas/instance.go:26: // TODO determine MAAS instance state
On 2013/08/02 17:30:32, fwereade wrote:
> Add a bug in LP and note the number here please; and ping bigjools, or
someone
> else who knows maas, to find out what'll be needed to implement it,
and note
> that in the bug.

I had a chat with bigjools on IRC about this. He tells me that the MAAS
server does not track the status of nodes after they're allocated, and
so any node that juju knows about will always show up as "allocated".

The TODO should probably be replaced with a note explaining that status
isn't available. Sound reasonable?

https://codereview.appspot.com/12333043/diff/1/environs/polling_test.go
File environs/polling_test.go (left):

https://codereview.appspot.com/12333043/diff/1/environs/polling_test.go#oldcode63
environs/polling_test.go:63: func (*dnsNameFakeInstance) Ports(string)
([]instance.Port, error) { return nil, nil }
On 2013/08/02 17:30:32, fwereade wrote:
> Complete side note: would it be useful for dnsNameFakeInstance to just
embed an
> instance.Instance, and thus panic on use of unwanted methods? Feels a
bit saner
> than just returning nonsense without error...

SGTM, done

https://codereview.appspot.com/12333043/

LGTM with a couple of suggestions.

https://codereview.appspot.com/12333043/diff/1/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12333043/diff/1/container/lxc/lxc_test.go#newcode130
container/lxc/lxc_test.go:130: c.Assert(instance.State(), gc.Equals,
string(golxc.StateUnknown))
This is a bit confusing.. should it be "stopped" or something instead?

https://codereview.appspot.com/12333043/diff/1/environs/azure/instance_test.go
File environs/azure/instance_test.go (right):

https://codereview.appspot.com/12333043/diff/1/environs/azure/instance_test.go#newcode28
environs/azure/instance_test.go:28: func (*InstanceSuite) TestId(c *C) {
Thanks for fixing this. Maybe while you're at it, make it instanceState
instead - no need to be exported.

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go
File environs/maas/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/environs/maas/instance.go#newcode26
environs/maas/instance.go:26: // TODO determine MAAS instance state
On 2013/08/05 07:09:06, axw1 wrote:
> On 2013/08/02 17:30:32, fwereade wrote:
> > Add a bug in LP and note the number here please; and ping bigjools,
or someone
> > else who knows maas, to find out what'll be needed to implement it,
and note
> > that in the bug.

> I had a chat with bigjools on IRC about this. He tells me that the
MAAS server
> does not track the status of nodes after they're allocated, and so any
node that
> juju knows about will always show up as "allocated".

> The TODO should probably be replaced with a note explaining that
status isn't
> available. Sound reasonable?

+1

https://codereview.appspot.com/12333043/

Is it possible to call the function "Instance.Status()"? We have a bit
of an overload of State things, and it would be one case less.

I think the reasoning might be that the status command outputs 'state:
pending' but avoiding the word State() in an interface seems useful to
me.

https://codereview.appspot.com/12333043/

+1 to Status, good call.

On 8 August 2013 18:27, John A Meinel <email address hidden> wrote:
> Is it possible to call the function "Instance.Status()"? We have a bit
> of an overload of State things, and it would be one case less.
>
> I think the reasoning might be that the status command outputs 'state:
> pending' but avoiding the word State() in an interface seems useful to
> me.
>
>
> https://codereview.appspot.com/12333043/
>
> --
> https://code.launchpad.net/~axwalk/juju-core/lp1167441-report-instance-state/+merge/178222
> Your team juju hackers is requested to review the proposed merge of lp:~axwalk/juju-core/lp1167441-report-instance-state into lp:juju-core.

Another small thing, if you can mention "bug #1167441" somewhere in your
proposal, it makes it obvious that there is a bug thread on Launchpad
that can be read to find more context.

It is true that the LP merge proposal does give the link to related
bugs, but it is a helpful reminder when reading the proposal (CL) that
there is more context available.

https://codereview.appspot.com/12333043/

On 2013/08/08 17:27:01, jameinel wrote:
> Is it possible to call the function "Instance.Status()"? We have a bit
of an
> overload of State things, and it would be one case less.

> I think the reasoning might be that the status command outputs 'state:
pending'
> but avoiding the word State() in an interface seems useful to me.

Hah, I had it as Status originally, and changed it to State to match
"instance-state". I'll change it back.

https://codereview.appspot.com/12333043/

Please take a look.

https://codereview.appspot.com/12333043/

The attempt to merge lp:~axwalk/juju-core/lp1167441-report-instance-state into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.667s
ok launchpad.net/juju-core/agent/tools 26.411s
ok launchpad.net/juju-core/bzr 6.795s
ok launchpad.net/juju-core/cert 2.645s
ok launchpad.net/juju-core/charm 0.518s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.019s
ok launchpad.net/juju-core/cmd 0.248s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 105.948s
ok launchpad.net/juju-core/cmd/jujud 51.162s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 0.859s
ok launchpad.net/juju-core/constraints 0.024s
ok launchpad.net/juju-core/container/lxc 0.433s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.308s
ok launchpad.net/juju-core/environs 1.305s
? launchpad.net/juju-core/environs/all [no test files]
ok launchpad.net/juju-core/environs/azure 4.000s
ok launchpad.net/juju-core/environs/cloudinit 0.443s
ok launchpad.net/juju-core/environs/config 0.745s
ok launchpad.net/juju-core/environs/dummy 16.007s
2013-08-09 02:50:55 WARNING juju.environs.config config.go:429 unknown config field "future"
2013-08-09 02:50:55 WARNING juju.environs.config config.go:429 unknown config field "future"
2013-08-09 02:50:55 WARNING juju.environs.config config.go:429 unknown config field "future"

----------------------------------------------------------------------
FAIL: local_test.go:306: localServerSuite.TestInstanceStatus

[LOG] 68.93735 INFO juju environs/testing: uploading FAKE tools 1.13.1-precise-amd64
[LOG] 68.95787 INFO juju environs/ec2: opening environment "sample"
[LOG] 73.78884 INFO juju environs/ec2: bootstrapping environment "sample"
[LOG] 73.78886 INFO juju environs: reading tools with major version 1
[LOG] 73.78888 DEBUG juju.agent.tools reading v1.* tools
[LOG] 73.78932 INFO juju environs: falling back to public bucket
[LOG] 73.78933 DEBUG juju.agent.tools reading v1.* tools
[LOG] 73.78992 DEBUG juju.agent.tools found 1.13.1-precise-amd64
[LOG] 73.78998 INFO juju environs: filtering tools by series: precise
[LOG] 73.79000 INFO juju environs: picked newest version: 1.13.1
[LOG] 73.82766 WARNING juju.environs.imagemetadata cannot load index "test:"/"streams/v1/index.sjson": cannot find URL "test:/streams/v1/index.sjson" not found
[LOG] 73.82778 DEBUG juju.environs.imagemetadata candidate matches for products ["com.ubuntu.cloud:server:12.04:amd64"] are [0xc20021da00]
[LOG] 73.99677 DEBUG juju environs/ec2: ec2 user data; 7410 bytes
[LOG] 74.00157 INFO juju environs/ec2: started instance "i-3"
[LOG] 74.00251 DEBUG juju waiting for DNS name(s) of state server instances [i-3]
[LOG] 74.00499 INFO juju environs: reading tools with major version 1
[LOG] 74.00500 DEBUG juju.agent.tools reading v1.* tools
[LOG] 74.00549 INFO juju environs: falling back to public bucket
[LOG] 74.00550 DEBUG juju.agent.tools reading v1.* tools
[L...

Bot needs an updated goamz. I take it that it's not using dependencies.tsv yet...

https://codereview.appspot.com/12333043/diff/1/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/12333043/diff/1/container/lxc/instance.go#newcode30
container/lxc/instance.go:30: return string(state)
On 2013/08/05 07:09:06, axw1 wrote:
> On 2013/08/02 17:30:32, fwereade wrote:
> > I see this, and it looks good, but I'm a bit confused as to why it
doesn't
> work
> > with the local provider. (Well, I can see why, but I can't
understand the
> > motivation -- can anyone think of a drawback to putting the
lxc.Instances into
> > the local.Instances so that we can access this method? I can
understand
> > differences in address handling, but I don't think the same forces
apply
> here.)

> Are you referring to the lxc.Instance being converted to
local.Instance? I don't
> understand the reason behind this. I think I'll have to have a chat
with thumper
> after IOM to understand this better.

I think the comment is more to have local.Instance use lxc.Instance not
the other way around. I've not looked at the differences yet, so for
now would prefer to keep them separate.

Perhaps, we have local.Instance be composed of an lxc.Instance so it can
defer to the lxc commands and just override what is different. Take
this with a grain of salt as I haven't looked at the code right now.

https://codereview.appspot.com/12333043/