apiserver: StatusSetter common mixin

apiserver: StatusSetter common mixin

Extracted SetStatus handling from Machiner and created
a StatusSetter mixin in apiserver/common, using the
same model as for Remover and PasswordChanger. Also
refactored the Machiner facade to use it.

As a drive-by fix changed several test packages to
import gocheck aliased as "gc".

This is the first of several steps to extract common
API code from the machiner, so it can be used by the
upcoming Uniter facade.

https://code.launchpad.net/~dimitern/juju-core/088-api-statussetter/+merge/178529

(do not edit description out of merge proposal)

[dimitern, 2013-08-05 10:33:49]

Please take a look.

[rog, 2013-08-05 11:25:25]

LGTM with backward compatibility preserved (and please explicitly check that you
can upgrade from 1.12 to tip)

https://codereview.appspot.com/12443044/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/12443044/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:67: Entities []SetEntityStatus
please add (deprecated) Machines here for backward compatibility.

the MachinesSetStatus and MachineSetStatus types can go.

[gz, 2013-08-05 13:15:53]

LGTM. I don't fully comprehend the upgrade/deprecation story for the api, but
trust the comments about removal in 1.14 make sense.

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
File state/apiserver/common/setstatus.go (right):

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
state/apiserver/common/setstatus.go:19: // state.State implements StatusSetter
to provide wats for us to
wats?

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
File state/apiserver/common/setstatus_test.go (right):

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
state/apiserver/common/setstatus_test.go:22: func (*statusSetterSuite)
TestSetStatus(c *gc.C) {
I don't really like that this is one giant test with a whole bunch of conditions
in it, but it does at least seem reasonably comprehensive.

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
state/apiserver/common/setstatus_test.go:86: type fakeStatusSetterState struct {
I'd generally put the fakes above the tests using them in the file.

[dimitern, 2013-08-05 13:25:44]

Please take a look.

https://codereview.appspot.com/12443044/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/12443044/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:67: Entities []SetEntityStatus
On 2013/08/05 11:25:26, rog wrote:
> please add (deprecated) Machines here for backward compatibility.
> 
> the MachinesSetStatus and MachineSetStatus types can go.

Done, added DEPRECATED(v1.14) comment, and a test to make sure it passes with
just Machines.

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
File state/apiserver/common/setstatus.go (right):

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
state/apiserver/common/setstatus.go:19: // state.State implements StatusSetter
to provide wats for us to
On 2013/08/05 13:15:53, gz wrote:
> wats?

Done.

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
File state/apiserver/common/setstatus_test.go (right):

https://codereview.appspot.com/12443044/diff/1/state/apiserver/common/setstat...
state/apiserver/common/setstatus_test.go:86: type fakeStatusSetterState struct {
On 2013/08/05 13:15:53, gz wrote:
> I'd generally put the fakes above the tests using them in the file.

It's like that in other common_test files, but I'll change them all if you
prefer.

[jameinel, 2013-08-08 17:45:01]

Changing an API has 2 compatibility directions. New clients will make a call on
an old server, and old clients will place a call on a New server.

Generally, the easiest way is to create a new API so that old clients call the
old API on the new server, and when new clients call the new api they notice it
is missing, and fall back to the old api.

I don't know if SetStatus failing is fatal to the Machine agent, but I think it
might be.

I think this is a good point at which we want to raise the discussion about how
much effort we want to put into maintaining API compatibility, and what is sane
to do.

https://codereview.appspot.com/12443044/diff/9001/state/apiserver/common/sets...
File state/apiserver/common/setstatus.go (right):

https://codereview.appspot.com/12443044/diff/9001/state/apiserver/common/sets...
state/apiserver/common/setstatus.go:48: if len(args.Entities) == 0 {
Can we add a log message here to indicate that the deprecated functionality is
being called?

This might also help in the upgrade case.

There is also a slight concern that clients will upgrade to 1.14 before the core
server will upgrade to 1.14. (In my testing of upgrade, clients always upgraded
before machine-0 because of how the graceful shutdown and restart worked).

If the result is just that the machine agents restart a couple times while
waiting for machine-0 to restart, that is probably ok. As long as they don't get
into a "restart indefinitely on a condition that will never resolve".