state: Add machine networks and interfaces

state: Add machine networks and interfaces

Added a couple of entities (and related methods)  in state:
 * Network - holds a name, CIDR and VLAN tag; used to 
   describe actual configured networks that machines
   need to be on.
 * NetworkInterface - linked to a machine id and network
   name, holds the network interface name to MAC address
   mapping in state (MAC address as key for uniqueness);
   machines support adding one or more NICs.

Renamed machine.Network() and st.networks to RequestedNetworks()
and st.requestedNetworks (in code only, schema remains the same).
This helps avoid confusion between the requested networks
(to include/exclude on a machine, coming from the service; they
are now called "requested networks"), and actually configured
networks on a running machine (just "networks").

For now networks are immutable, can't be removed and NICs can be
added to them only if their machine is not provisioned yet.
NICs are only removed when the machine itself is removed.

Provisioner API change: renamed Networks() to RequestedNetworks()
which is fine, because Networks() was added recently and not
released yet.

We'll use the added machine NICs to show enabled networks
in juju status later on, and they will be set at StartInstance()
time inside the provisioner. Internally, MaaS provider changes
will be needed to get NIC-to-MAC mapping for an acquired node
and the list of networks for it.

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

Requires: https://code.launchpad.net/~dimitern/juju-core/380-provisioner-honors-networks/+merge/213647

(do not edit description out of merge proposal)

[dimitern, 2014-04-02 09:55:29]

Please take a look.

[dimitern, 2014-04-02 10:12:45]

Please take a look.

[fwereade, 2014-04-02 10:18:45]

Couple of tweaks please.

https://codereview.appspot.com/83070047/diff/20001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/20001/state/machine.go#newcode915
state/machine.go:915: Update: bson.D{{"$set", bson.D{{"networkinterfaces",
ifaces}}}},
I get really nervous when we use the same struct in the interface and in the
actual data storage -- it's just too easy for changes to break stuff.

And... can we put it on a separate doc please? It's just the usual
read/write/watch usage patterns issue.

[dimitern, 2014-04-03 15:14:05]

Please take a look.

https://codereview.appspot.com/83070047/diff/20001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/20001/state/machine.go#newcode915
state/machine.go:915: Update: bson.D{{"$set", bson.D{{"networkinterfaces",
ifaces}}}},
On 2014/04/02 10:18:45, fwereade wrote:
> I get really nervous when we use the same struct in the interface and in the
> actual data storage -- it's just too easy for changes to break stuff.
> 
> And... can we put it on a separate doc please? It's just the usual
> read/write/watch usage patterns issue.

Changed as discussed - separate docs for networks and NICs, linked to a machine.

[fwereade, 2014-04-03 16:37:08]

Some naming quibbles; and quite a lot of freakouts wrt races. Most can be
eliminated by dropping Remove methods, I think.

https://codereview.appspot.com/83070047/diff/40001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/juju/conn_test.go#newcode406
juju/conn_test.go:406: include, exclude, err := machine.LinkedNetworks()
Can we call this RequestedNetworks or something? "Linked" doesn't fit my brain
here.

https://codereview.appspot.com/83070047/diff/40001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/addmachine.go#newcod...
state/addmachine.go:425: createLinkedNetworksOp(st, machineGlobalKey(mdoc.Id),
can we have a TODO and bug for the lack of sanity-checking here? ideally we'd be
checking names against actual networks.

https://codereview.appspot.com/83070047/diff/40001/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/cleanup.go#newcode140
state/cleanup.go:140: // depend upon the supplied machine, and removes the
machine from state. It's
whoops, please s/removes the machine from state/sets the machine to Dead/

https://codereview.appspot.com/83070047/diff/40001/state/cleanup.go#newcode157
state/cleanup.go:157: if err :=
st.cleanupLinkedNetworks(machineGlobalKey(machineId)); err != nil {
Sorry I missed this before, but I don't see the point of this code. All it does
is leave an extant machine doc without linked networks -- not fatal, sure, but
not very useful either.

We should just run the appropriate ops when we remove a service/machine, and
leave cleanup out of it.

https://codereview.appspot.com/83070047/diff/40001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode890
state/machine.go:890: }
Possibly this is something more like a NetworkSpecification?

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode893
state/machine.go:893: func (m *Machine) MachineNetworks() ([]*MachineNetwork,
error) {
and I think this is more like just Networks()

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode939
state/machine.go:939: Name:        name,
InterfaceName?

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode952
state/machine.go:952: }}
I think we need an assert that the machine is alive. IIRC machines can't
fast-forward from alive to removed, so that should be sufficient to maintain
integrity in the face of remove-all-interfaces ops called at machine-Remove
time.

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go#newc...
state/machine_test.go:394: net, err := st.AddMachineNetwork(networkName, cidr,
vlanTag)
I don't think this is really a MachineNetwork, is it? it feels like it's the
heart of the actual network model in juju and deserves the name Network...

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go#newc...
state/machine_test.go:396: iface, err := machine.AddNetworkInterface(mac,
ifaceName, networkName)
wondering if this should be net.AddMachine? just a thought, open to
counterarguments

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go
File state/machinenetworks.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go#n...
state/machinenetworks.go:23: // can be a part of.
yeah, I think this is a straight-up Network.

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go#n...
state/machinenetworks.go:30: // VLANTag needs to be between 1 and 4094 for VLANs
and 0 for
We were saying 4095 at one point. Is this a typo or a correction? :)

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go#n...
state/machinenetworks.go:77: err = m.st.runTransaction(ops)
This is racy -- there's nothing stopping us adding more interfaces in between
the Find and the runTransaction. I'd rather have it impossible to remove
networks (at lest in this CL) than have their removal risk db integrity.

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks_test.go
File state/machinenetworks_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks_test...
state/machinenetworks_test.go:33: // Add an interface and verify we can't remove
the network.
We'd need a TransactionHooks test to really verify this, and it'd fail.

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go
File state/networkinterfaces.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:41: func (ni *NetworkInterface) Name() string {
InterfaceName for pedantic completeness?

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:69: func removeNetworkInterfacesOps(st *State,
machineId string) []txn.Op {
This is racy in isolation -- if we move it into the machine's Remove method,
though, I think we're good. (with the check-machine-alive assert in
AddNetworkInterface, anyway)

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:82: }
check iter.Err()

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces_te...
File state/networkinterfaces_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces_te...
state/networkinterfaces_test.go:32: err := s.iface.Remove()
I think again that actually I'd rather not allow removal if we can't get it
right. I have a nasty feeling that we need interfaces to have lifecycles -- ie
we Destroy() them, then the MA cleans them up and sets them to Dead.

This presupposes dynamic networking, but that's coming soon.


(related thought: given that we're stuck doing this at cloudinit time right now,
we should ideally only allow an AddNetworkInterface op at SetProvisioned time --
that's the only time it'll actually reflect reality. might be hard, depending on
how the provisioner works, haven't got that far yet)

https://codereview.appspot.com/83070047/diff/40001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/open.go#newcode195
state/open.go:195: {"networkinterfaces", []string{"machineid"}},
Thank you for thinking of this. I award you a large number of internet points.

https://codereview.appspot.com/83070047/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/state.go#newcode1009
state/state.go:1009: // AddMachineNetwork creates a new machine network with the
given name, CIDR and VLAN tag.q
s/q//

https://codereview.appspot.com/83070047/diff/40001/state/state.go#newcode1010
state/state.go:1010: func (st *State) AddMachineNetwork(name, cidr string,
vlanTag int) (m *MachineNetwork, err error) {
AddNetwork, I think.

https://codereview.appspot.com/83070047/diff/40001/state/state.go#newcode1043
state/state.go:1043: func (st *State) MachineNetwork(name string)
(*MachineNetwork, error) {
Network.

[dimitern, 2014-04-04 09:14:43]

Please take a look.

https://codereview.appspot.com/83070047/diff/40001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/juju/conn_test.go#newcode406
juju/conn_test.go:406: include, exclude, err := machine.LinkedNetworks()
On 2014/04/03 16:37:09, fwereade wrote:
> Can we call this RequestedNetworks or something? "Linked" doesn't fit my brain
> here.

Done.

https://codereview.appspot.com/83070047/diff/40001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/addmachine.go#newcod...
state/addmachine.go:425: createLinkedNetworksOp(st, machineGlobalKey(mdoc.Id),
On 2014/04/03 16:37:09, fwereade wrote:
> can we have a TODO and bug for the lack of sanity-checking here? ideally we'd
be
> checking names against actual networks.

Done.

https://codereview.appspot.com/83070047/diff/40001/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/cleanup.go#newcode140
state/cleanup.go:140: // depend upon the supplied machine, and removes the
machine from state. It's
On 2014/04/03 16:37:09, fwereade wrote:
> whoops, please s/removes the machine from state/sets the machine to Dead/

Done.

https://codereview.appspot.com/83070047/diff/40001/state/cleanup.go#newcode157
state/cleanup.go:157: if err :=
st.cleanupLinkedNetworks(machineGlobalKey(machineId)); err != nil {
On 2014/04/03 16:37:09, fwereade wrote:
> Sorry I missed this before, but I don't see the point of this code. All it
does
> is leave an extant machine doc without linked networks -- not fatal, sure, but
> not very useful either.
> 
> We should just run the appropriate ops when we remove a service/machine, and
> leave cleanup out of it.

Removed.

https://codereview.appspot.com/83070047/diff/40001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode890
state/machine.go:890: }
On 2014/04/03 16:37:09, fwereade wrote:
> Possibly this is something more like a NetworkSpecification?

Do you mean rename the method or something else? I thought you suggested to
rename it to RequestedNetworks earlier.

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode893
state/machine.go:893: func (m *Machine) MachineNetworks() ([]*MachineNetwork,
error) {
On 2014/04/03 16:37:09, fwereade wrote:
> and I think this is more like just Networks()

So, let's call this Networks() and the former RequestedNetworks() or even maybe
ServiceNetworks()?

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode939
state/machine.go:939: Name:        name,
On 2014/04/03 16:37:09, fwereade wrote:
> InterfaceName?

Done.

https://codereview.appspot.com/83070047/diff/40001/state/machine.go#newcode952
state/machine.go:952: }}
On 2014/04/03 16:37:09, fwereade wrote:
> I think we need an assert that the machine is alive. IIRC machines can't
> fast-forward from alive to removed, so that should be sufficient to maintain
> integrity in the face of remove-all-interfaces ops called at machine-Remove
> time.

Done.

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go#newc...
state/machine_test.go:394: net, err := st.AddMachineNetwork(networkName, cidr,
vlanTag)
On 2014/04/03 16:37:09, fwereade wrote:
> I don't think this is really a MachineNetwork, is it? it feels like it's the
> heart of the actual network model in juju and deserves the name Network...

Done, although arguably this specific definition of network concerns only
machines (i.e. NICs and the networks they're on are a property of a machine).

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go#newc...
state/machine_test.go:396: iface, err := machine.AddNetworkInterface(mac,
ifaceName, networkName)
On 2014/04/03 16:37:09, fwereade wrote:
> wondering if this should be net.AddMachine? just a thought, open to
> counterarguments

That's a fair point, I'm not sure. Networks (as of now) are created when you
need to add a NIC to a machine, but they stay past the lifetime of the initial
machine. On another hand, the machine comes first in the sequence of things
created in state (although this might change in the future).

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go
File state/machinenetworks.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go#n...
state/machinenetworks.go:23: // can be a part of.
On 2014/04/03 16:37:09, fwereade wrote:
> yeah, I think this is a straight-up Network.

Done.

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go#n...
state/machinenetworks.go:30: // VLANTag needs to be between 1 and 4094 for VLANs
and 0 for
On 2014/04/03 16:37:09, fwereade wrote:
> We were saying 4095 at one point. Is this a typo or a correction? :)

I was saying 4096, but I checked later and 4094 is actual value quoted in MAAS
docs.

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks.go#n...
state/machinenetworks.go:77: err = m.st.runTransaction(ops)
On 2014/04/03 16:37:09, fwereade wrote:
> This is racy -- there's nothing stopping us adding more interfaces in between
> the Find and the runTransaction. I'd rather have it impossible to remove
> networks (at lest in this CL) than have their removal risk db integrity.

I wasn't sure we need to remove them at all (for now) and despite trying
couldn't do it in a single []txn.Op list. You can't just do {C:
m.st.networkInterfaces.Name, Assert: D{{"networkname", m.doc.Name}}} in mgo - it
complains Id: is missing.

Remove() dropped.

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks_test.go
File state/machinenetworks_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machinenetworks_test...
state/machinenetworks_test.go:33: // Add an interface and verify we can't remove
the network.
On 2014/04/03 16:37:09, fwereade wrote:
> We'd need a TransactionHooks test to really verify this, and it'd fail.

Remove()-d. :)

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go
File state/networkinterfaces.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:41: func (ni *NetworkInterface) Name() string {
On 2014/04/03 16:37:09, fwereade wrote:
> InterfaceName for pedantic completeness?

Done.

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:69: func removeNetworkInterfacesOps(st *State,
machineId string) []txn.Op {
On 2014/04/03 16:37:09, fwereade wrote:
> This is racy in isolation -- if we move it into the machine's Remove method,
> though, I think we're good. (with the check-machine-alive assert in
> AddNetworkInterface, anyway)

I don't quite follow - removeNetworkInterfacesOps() is called in
machine.Remove() and run with the other []txn.Op already. Adding an assert on
machine.Life is only needed to keep dying or dead machines from having NICs
added (which can't happen right now as NICs are immutable and only changed after
provisioning), but if NICs can change at run-time (as can
HardwareCharacteristics btw), that sounds exactly like a job for the Networker
worker we'll have after MVP.  

As with MachineNetworks, I think dropping Remove() entirely here, for this CL is
perhaps what we need.

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:82: }
On 2014/04/03 16:37:09, fwereade wrote:
> check iter.Err()

Done.

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces_te...
File state/networkinterfaces_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces_te...
state/networkinterfaces_test.go:32: err := s.iface.Remove()
On 2014/04/03 16:37:09, fwereade wrote:
> I think again that actually I'd rather not allow removal if we can't get it
> right. I have a nasty feeling that we need interfaces to have lifecycles -- ie
> we Destroy() them, then the MA cleans them up and sets them to Dead.
> 
> This presupposes dynamic networking, but that's coming soon.
> 
> 
> (related thought: given that we're stuck doing this at cloudinit time right
now,
> we should ideally only allow an AddNetworkInterface op at SetProvisioned time
--
> that's the only time it'll actually reflect reality. might be hard, depending
on
> how the provisioner works, haven't got that far yet)

See previous comment; dropped Remove(). If you're suggesting to assert on
machine.is-provisioned == false in AddNetworkInterface(), that's an excellent
idea. Done!

https://codereview.appspot.com/83070047/diff/40001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/open.go#newcode195
state/open.go:195: {"networkinterfaces", []string{"machineid"}},
On 2014/04/03 16:37:09, fwereade wrote:
> Thank you for thinking of this. I award you a large number of internet points.

(..and the crowd grows wild!... :)) Cheers, state/ is daunting place to be, so
better do it right.

https://codereview.appspot.com/83070047/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/state.go#newcode1009
state/state.go:1009: // AddMachineNetwork creates a new machine network with the
given name, CIDR and VLAN tag.q
On 2014/04/03 16:37:09, fwereade wrote:
> s/q//

Done.

https://codereview.appspot.com/83070047/diff/40001/state/state.go#newcode1010
state/state.go:1010: func (st *State) AddMachineNetwork(name, cidr string,
vlanTag int) (m *MachineNetwork, err error) {
On 2014/04/03 16:37:09, fwereade wrote:
> AddNetwork, I think.

Done.

https://codereview.appspot.com/83070047/diff/40001/state/state.go#newcode1043
state/state.go:1043: func (st *State) MachineNetwork(name string)
(*MachineNetwork, error) {
On 2014/04/03 16:37:09, fwereade wrote:
> Network.

Done.

[fwereade, 2014-04-04 09:56:22]

LGTM with relatively trivial fixes:

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go#newc...
state/machine_test.go:394: net, err := st.AddMachineNetwork(networkName, cidr,
vlanTag)
On 2014/04/04 09:14:43, dimitern wrote:
> On 2014/04/03 16:37:09, fwereade wrote:
> > I don't think this is really a MachineNetwork, is it? it feels like it's the
> > heart of the actual network model in juju and deserves the name Network...
> 
> Done, although arguably this specific definition of network concerns only
> machines (i.e. NICs and the networks they're on are a property of a machine).

I'm pretty sure netName/cidr/vlanTag defines a network -- and
ifceName/MAC/machine/network defines an interface. Am I ludicrously wrong about
something?

https://codereview.appspot.com/83070047/diff/40001/state/machine_test.go#newc...
state/machine_test.go:396: iface, err := machine.AddNetworkInterface(mac,
ifaceName, networkName)
On 2014/04/04 09:14:43, dimitern wrote:
> On 2014/04/03 16:37:09, fwereade wrote:
> > wondering if this should be net.AddMachine? just a thought, open to
> > counterarguments
> 
> That's a fair point, I'm not sure. Networks (as of now) are created when you
> need to add a NIC to a machine, but they stay past the lifetime of the initial
> machine. On another hand, the machine comes first in the sequence of things
> created in state (although this might change in the future).

I don't think we need to get this perfect now, because (yay!) this bit of state
at least is properly isolated.

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go
File state/networkinterfaces.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:69: func removeNetworkInterfacesOps(st *State,
machineId string) []txn.Op {
On 2014/04/04 09:14:43, dimitern wrote:
> On 2014/04/03 16:37:09, fwereade wrote:
> > This is racy in isolation -- if we move it into the machine's Remove method,
> > though, I think we're good. (with the check-machine-alive assert in
> > AddNetworkInterface, anyway)
> 
> I don't quite follow - removeNetworkInterfacesOps() is called in
> machine.Remove() and run with the other []txn.Op already. Adding an assert on
> machine.Life is only needed to keep dying or dead machines from having NICs
> added (which can't happen right now as NICs are immutable and only changed
after
> provisioning), but if NICs can change at run-time (as can
> HardwareCharacteristics btw), that sounds exactly like a job for the Networker
> worker we'll have after MVP.  

I'd quite like that assert-alive-on-add to be in place anyway, because it'll be
way too easy to forget when we make this dynamic.

And this method in particular will be too easy to just call when we add Remove
-- but if we move it onto Machine and include a dead-check at the start we at
least put a little roadblock in the way of thoughtless later refactoring.

> As with MachineNetworks, I think dropping Remove() entirely here, for this CL
is
> perhaps what we need.

+1 to that. But please make this func a Machine method, and add the alive
assert.

https://codereview.appspot.com/83070047/diff/60001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:425: // TODO(dimitern) Once we can add networks
independently
and a bug please :)

https://codereview.appspot.com/83070047/diff/60001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode567
state/machine.go:567: // Remove sets the machine to Dead. It will fail if the
machine is not
Nah, this one removes it from state (and requires that it's already dead before
it's called).

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode897
state/machine.go:897: func (m *Machine) Networks() ([]*Network, error) {
Mildly forward-looking quibble, doesn't need to be done now: do we need this? I
presume it's being done for use in status; but should we not perhaps actually be
showing the machine's addresses *annotated* with what network that address is
on? ie we will want to combine the info from NetworkInterfaces with that from
Addresses and show that, and have a separate top-level section in status for
Networks?

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode941
state/machine.go:941: aliveAndNotProvisioned := append(isAliveDoc,
bson.D{{"nonce", ""}}...)
I think this is fine, assuming we have the provisioner pass the interfaces into
SetProvisioned, and inside the APi server call AddNetworkInterface before
SetProvisioned?

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode976
state/machine.go:976: return nil, fmt.Errorf("machine already provisioned")
maybe "machine already provisioned: dynamic network interfaces not currently
supported"?

https://codereview.appspot.com/83070047/diff/60001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/machine_test.go#newc...
state/machine_test.go:474: c.Assert(ifaces, gc.HasLen, 0)
I'd kinda prefer that this return NotFound, but I can't really see how to do it.

https://codereview.appspot.com/83070047/diff/60001/state/machine_test.go#newc...
state/machine_test.go:477: func (s *MachineSuite)
TestAddNetworkInterfaceErrors(c *gc.C) {
Thanks for splitting this.

[dimitern, 2014-04-04 11:12:18]

Please take a look.

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go
File state/networkinterfaces.go (right):

https://codereview.appspot.com/83070047/diff/40001/state/networkinterfaces.go...
state/networkinterfaces.go:69: func removeNetworkInterfacesOps(st *State,
machineId string) []txn.Op {
On 2014/04/04 09:56:22, fwereade wrote:
> On 2014/04/04 09:14:43, dimitern wrote:
> > On 2014/04/03 16:37:09, fwereade wrote:
> > > This is racy in isolation -- if we move it into the machine's Remove
method,
> > > though, I think we're good. (with the check-machine-alive assert in
> > > AddNetworkInterface, anyway)
> > 
> > I don't quite follow - removeNetworkInterfacesOps() is called in
> > machine.Remove() and run with the other []txn.Op already. Adding an assert
on
> > machine.Life is only needed to keep dying or dead machines from having NICs
> > added (which can't happen right now as NICs are immutable and only changed
> after
> > provisioning), but if NICs can change at run-time (as can
> > HardwareCharacteristics btw), that sounds exactly like a job for the
Networker
> > worker we'll have after MVP.  
> 
> I'd quite like that assert-alive-on-add to be in place anyway, because it'll
be
> way too easy to forget when we make this dynamic.
> 
> And this method in particular will be too easy to just call when we add Remove
> -- but if we move it onto Machine and include a dead-check at the start we at
> least put a little roadblock in the way of thoughtless later refactoring.
> 
> > As with MachineNetworks, I think dropping Remove() entirely here, for this
CL
> is
> > perhaps what we need.
> 
> +1 to that. But please make this func a Machine method, and add the alive
> assert.

Alive assert is already in place, moving this to a machine method.

https://codereview.appspot.com/83070047/diff/60001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:425: // TODO(dimitern) Once we can add networks
independently
On 2014/04/04 09:56:23, fwereade wrote:
> and a bug please :)

Done.

https://codereview.appspot.com/83070047/diff/60001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode567
state/machine.go:567: // Remove sets the machine to Dead. It will fail if the
machine is not
On 2014/04/04 09:56:23, fwereade wrote:
> Nah, this one removes it from state (and requires that it's already dead
before
> it's called).

Done.

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode897
state/machine.go:897: func (m *Machine) Networks() ([]*Network, error) {
On 2014/04/04 09:56:23, fwereade wrote:
> Mildly forward-looking quibble, doesn't need to be done now: do we need this?
I
> presume it's being done for use in status; but should we not perhaps actually
be
> showing the machine's addresses *annotated* with what network that address is
> on? ie we will want to combine the info from NetworkInterfaces with that from
> Addresses and show that, and have a separate top-level section in status for
> Networks?

For now we can't really link IP addresses to NICs, but at least we can show on
what networks the machine is on. To have address + network we need the worker to
update the address once it's set I think.

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode941
state/machine.go:941: aliveAndNotProvisioned := append(isAliveDoc,
bson.D{{"nonce", ""}}...)
On 2014/04/04 09:56:23, fwereade wrote:
> I think this is fine, assuming we have the provisioner pass the interfaces
into
> SetProvisioned, and inside the APi server call AddNetworkInterface before
> SetProvisioned?

I was thinking of having AddNetworkInterface on the Provisioner API later and
call it as needed before calling SetProvisioned (and changing machine state to
error if it happens, like we do after StartInstance errors).

Is there a particular reason you want NICs passed to SetProvisioned?

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode976
state/machine.go:976: return nil, fmt.Errorf("machine already provisioned")
On 2014/04/04 09:56:23, fwereade wrote:
> maybe "machine already provisioned: dynamic network interfaces not currently
> supported"?

Done.

https://codereview.appspot.com/83070047/diff/60001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/machine_test.go#newc...
state/machine_test.go:474: c.Assert(ifaces, gc.HasLen, 0)
On 2014/04/04 09:56:23, fwereade wrote:
> I'd kinda prefer that this return NotFound, but I can't really see how to do
it.

I think this should be enough for now, and once we can we'll change the tests.

https://codereview.appspot.com/83070047/diff/60001/state/machine_test.go#newc...
state/machine_test.go:477: func (s *MachineSuite)
TestAddNetworkInterfaceErrors(c *gc.C) {
On 2014/04/04 09:56:23, fwereade wrote:
> Thanks for splitting this.

Yeah, it got a bit too long.

[dimitern, 2014-04-04 11:22:38]

Please take a look.

[fwereade, 2014-04-04 12:10:01]

LGTM, one last quibble

https://codereview.appspot.com/83070047/diff/60001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode941
state/machine.go:941: aliveAndNotProvisioned := append(isAliveDoc,
bson.D{{"nonce", ""}}...)
On 2014/04/04 11:12:19, dimitern wrote:
> On 2014/04/04 09:56:23, fwereade wrote:
> > I think this is fine, assuming we have the provisioner pass the interfaces
> into
> > SetProvisioned, and inside the APi server call AddNetworkInterface before
> > SetProvisioned?
> 
> I was thinking of having AddNetworkInterface on the Provisioner API later and
> call it as needed before calling SetProvisioned (and changing machine state to
> error if it happens, like we do after StartInstance errors).
> 
> Is there a particular reason you want NICs passed to SetProvisioned?

(1) chatty APIs suck (2) the quicker we can get all the instance info away from
the provisioner and into the api server the better.

...in fact ...I'm almost tempted to say that this functionality should be of a
transaction with the SetProvisioned method in state *anyway* -- consider what
happens if a machine has a NIC set up but then the process fails before
SetProvisioned.

That can be a minor refactoring when you hook up the provisioner, though.

https://codereview.appspot.com/83070047/diff/80001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/80001/state/machine.go#newcode567
state/machine.go:567: func (m *Machine) removeNetworkInterfacesOps() ([]txn.Op,
error) {
just add a quick pedantic check that the machine really is Dead in here too

[dimitern, 2014-04-04 13:10:12]

Please take a look.

https://codereview.appspot.com/83070047/diff/60001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/60001/state/machine.go#newcode941
state/machine.go:941: aliveAndNotProvisioned := append(isAliveDoc,
bson.D{{"nonce", ""}}...)
On 2014/04/04 12:10:03, fwereade wrote:
> On 2014/04/04 11:12:19, dimitern wrote:
> > On 2014/04/04 09:56:23, fwereade wrote:
> > > I think this is fine, assuming we have the provisioner pass the interfaces
> > into
> > > SetProvisioned, and inside the APi server call AddNetworkInterface before
> > > SetProvisioned?
> > 
> > I was thinking of having AddNetworkInterface on the Provisioner API later
and
> > call it as needed before calling SetProvisioned (and changing machine state
to
> > error if it happens, like we do after StartInstance errors).
> > 
> > Is there a particular reason you want NICs passed to SetProvisioned?
> 
> (1) chatty APIs suck (2) the quicker we can get all the instance info away
from
> the provisioner and into the api server the better.
> 
> ...in fact ...I'm almost tempted to say that this functionality should be of a
> transaction with the SetProvisioned method in state *anyway* -- consider what
> happens if a machine has a NIC set up but then the process fails before
> SetProvisioned.
> 
> That can be a minor refactoring when you hook up the provisioner, though.

Yes, I agree provisioning should be atomic, but it's not right now because of
StartInstance and SetProvisioned getting called in sequence (and adding a couple
of extra API calls will make it only a bit worse). I'm in favor of fixing it in
general, but this is out of scope for now.

https://codereview.appspot.com/83070047/diff/80001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/83070047/diff/80001/state/machine.go#newcode567
state/machine.go:567: func (m *Machine) removeNetworkInterfacesOps() ([]txn.Op,
error) {
On 2014/04/04 12:10:03, fwereade wrote:
> just add a quick pedantic check that the machine really is Dead in here too

Done inside machine.Remove().