api/provisioner: Allow adding networks and NICs

api/provisioner: Allow adding networks and NICs

This adds two new Provisioner API calls:
 * AddNetwork (exposed as st.AddNetworks bulk call on
   the client-side provisioner API)
 * AddNetworkInterface (exposed in the client-side API
   as machine.AddNetworkInterfaces bulk call)

These will be used to set the networks/interfaces that
will be configured by MAAS at provisioning time.

Next, we'll make the necessary changes to StartInstance()
in general (and in provider/maas specifically) to return
what networks will the machine start with.

https://code.launchpad.net/~dimitern/juju-core/382-api-provisioner-machine-nics/+merge/214272

Requires: https://code.launchpad.net/~dimitern/juju-core/381-state-machine-nics/+merge/213796

(do not edit description out of merge proposal)

[dimitern, 2014-04-04 14:58:11]

Please take a look.

[gz, 2014-04-04 15:40:02]

LGTM. I am a little panicked around future-proofing though, I think these api
structs will want to look a little different, or maybe quite a bit different,
when we add other clouds and better networking support. We seem to be reserving
some pretty generic API names 'AddNetworks" and will probably want a number of
revisions. We may get to several -Ex suffixes or something.

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

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:398: NetworkName   string
This will probably be a non-name id for clouds other than MAAS. Shouldn't matter
apart from naming.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:85: if interfaces[i].MachineTag != m.tag {
Why the conditional?

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:359: c.Assert(net.Name(), gc.Equals,
expectParams.Name)
Can use c.Check for these two after the err Assert.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:412: // Ensure only the first error is
reported.
This feels like a seperate testcase really, the current test is > one screen
high which is a bit of a personal bugbear of mine. Sharing setup steps is not
that hard.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:828: c.Assert(net.Name(),
gc.Equals, "vlan42")
Can use c.Check, and below.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:839: // Test it fails with a
non-environment-manager.
This feels like a seperate testcase.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:850: func (s
*withoutStateServerSuite) TestAddNetworkInterface(c *gc.C) {
Giant complicated test again...

[dimitern, 2014-04-04 16:34:41]

Please take a look.

https://codereview.appspot.com/84570043/diff/1/[revision%20details]
File [revision details] (right):

https://codereview.appspot.com/84570043/diff/1/[revision%20details]#newcode1
[revision details]:1: Old revision:
dimiter.naydenov@canonical.com-20140404130803-ud1wt0tvilokd5vx
> LGTM. I am a little panicked around future-proofing though, I
> think these api structs will want to look a little different,
> or maybe quite a bit different, when we add other clouds and 
> better networking support. We seem to be reserving some
> pretty generic API names 'AddNetworks" and will probably want
> a number of revisions. We may get to several -Ex suffixes or
> something.

Structs can evolve and we can phase-out API calls and add new ones with extended
functionality. This is a general problem with the API w/o versioning.

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

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:398: NetworkName   string
On 2014/04/04 15:40:02, gz wrote:
> This will probably be a non-name id for clouds other than MAAS. Shouldn't
matter
> apart from naming.

We can add fields as needed later (and "name" can also be a UUID if needed :)

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:85: if interfaces[i].MachineTag != m.tag {
On 2014/04/04 15:40:02, gz wrote:
> Why the conditional?

To make sure we can only add NICs to the current machine (in case it's empty or
different). There are tests for that in state/api/provisioner.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:359: c.Assert(net.Name(), gc.Equals,
expectParams.Name)
On 2014/04/04 15:40:02, gz wrote:
> Can use c.Check for these two after the err Assert.

Done.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:412: // Ensure only the first error is
reported.
On 2014/04/04 15:40:02, gz wrote:
> This feels like a seperate testcase really, the current test is > one screen
> high which is a bit of a personal bugbear of mine. Sharing setup steps is not
> that hard.

Moved to a separate test case.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:828: c.Assert(net.Name(),
gc.Equals, "vlan42")
On 2014/04/04 15:40:02, gz wrote:
> Can use c.Check, and below.

Done.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:839: // Test it fails with a
non-environment-manager.
On 2014/04/04 15:40:02, gz wrote:
> This feels like a seperate testcase.

Done.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:850: func (s
*withoutStateServerSuite) TestAddNetworkInterface(c *gc.C) {
On 2014/04/04 15:40:02, gz wrote:
> Giant complicated test again...

I did some more refactoring to shorten the test and reuse common bits. There are
a lot of cases to test and the setup is more or less the same.

[fwereade, 2014-04-04 17:54:23]

I'm a bit disappointed that we're making this so chatty, when ISTM that we could
pack all this into SetProvisioned, but... well, deadlines are tight and internal
APIs are much easier to change in future. Let it stand, I guess :).