various: Support multiple NICs with the same MAC

various: Support multiple NICs with the same MAC

Fixes bug #1307513, allowing more than one network
interface with the same MAC address, and introduces
the concept of virtual and physical NIC.
This is needed because you can have more than one
network interface with the same MAC address on the
same machine (but it has to be on different networks):
one physical and multiple virtual NICs. Changes were
needed mostly in state to use a different primary
key for networkinterfaces collection. Also added a
unique index on "interfacename" and "networkname",
improved tests.

Fixed the MAAS provider to configure networks properly,
so that you can have more than one VLAN virtual NIC
on the same physical NIC.

Introduced environs/network package and defined an
Id type to make it apparent that network ids are
provider-specific (like instance.Id). Also moved
environs.NetworkInfo to network.Info.

In a follow-up, machine addresses and networks will
be integrated, so we can link a network interface's
name and MAC address to its network (IP) address.
Finally, networks and machine interfaces will be
visible in juju status.

https://code.launchpad.net/~dimitern/juju-core/400-lp-1307513-multiple-nics-same-mac/+merge/216453

(do not edit description out of merge proposal)

[dimitern, 2014-04-18 14:18:42]

Please take a look.

[fwereade, 2014-04-18 14:41:17]

mostly looking good, but (1) IsVirtual on network still seems ill-formed to me;
and (2) are we explicitly ignoring compatibility with earlier 1.19? these new
fields in state worry me a little, the people who want to work with networks
will likely be upgrading from there...

https://codereview.appspot.com/89260044/diff/1/environs/broker.go
File environs/broker.go (right):

https://codereview.appspot.com/89260044/diff/1/environs/broker.go#newcode51
environs/broker.go:51: ProviderId string
We could make this clearer if we had a type for it, like instance.Id -- as it is
I think ProviderId is not much of a win over NetworkId. But `NetworkId
NetworkId` should be less confusing.

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

https://codereview.appspot.com/89260044/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:331: IsVirtual bool
I'm less certain about this one. If it's possible for virtual/physical
interfaces to be mixed, this is meaningless; if it's not possible, it's not
necessary on the interface definition. Right?

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

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode570
state/machine.go:570: var doc networkInterfaceDoc
hey, I'm feeling more strongly about this now: please assert that the machine is
Dead in this method, just so that if (when) someone tries to use it from
elsewhere they get something pointing out the race with adding interfaces.

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode1077
state/machine.go:1077: panic("unknown error while adding network interface")
This is on the state server, let's not panic even if it should never happen.

[dimitern, 2014-04-18 15:03:04]

On 2014/04/18 14:41:17, fwereade wrote:
> mostly looking good, but (1) IsVirtual on network still seems ill-formed to
me;
> and (2) are we explicitly ignoring compatibility with earlier 1.19? these new
> fields in state worry me a little, the people who want to work with networks
> will likely be upgrading from there...
> 

1) I'll drop IsVirtual from networks and keep it on interfaces only.
2) 1.19.0 is a dev release which was out recently, so I think we can get away
with saying "if you used networks, please redeploy your environment when
upgrading to 1.19.1" - I'll add a note to the bug and ping sinzui.

[dimitern, 2014-04-18 15:37:26]

Please take a look.

https://codereview.appspot.com/89260044/diff/1/environs/broker.go
File environs/broker.go (right):

https://codereview.appspot.com/89260044/diff/1/environs/broker.go#newcode51
environs/broker.go:51: ProviderId string
On 2014/04/18 14:41:18, fwereade wrote:
> We could make this clearer if we had a type for it, like instance.Id -- as it
is
> I think ProviderId is not much of a win over NetworkId. But `NetworkId
> NetworkId` should be less confusing.

I wanted to make the code use consistent naming - NetworkId is only used here. I
like the idea of type NetworkId string and then perhaps use ProviderId NetworkId
as a compromise?

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

https://codereview.appspot.com/89260044/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:331: IsVirtual bool
On 2014/04/18 14:41:18, fwereade wrote:
> I'm less certain about this one. If it's possible for virtual/physical
> interfaces to be mixed, this is meaningless; if it's not possible, it's not
> necessary on the interface definition. Right?

Frankly, I don't know whether it's possible to mix physical and virtual
interfaces on the same network, perhaps it is. But the actual flag makes more
sense for NICs, rather than networks.

I'll drop it from networks and leave it only on interfaces.

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

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode570
state/machine.go:570: var doc networkInterfaceDoc
On 2014/04/18 14:41:18, fwereade wrote:
> hey, I'm feeling more strongly about this now: please assert that the machine
is
> Dead in this method, just so that if (when) someone tries to use it from
> elsewhere they get something pointing out the race with adding interfaces.

Done.

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode1077
state/machine.go:1077: panic("unknown error while adding network interface")
On 2014/04/18 14:41:18, fwereade wrote:
> This is on the state server, let's not panic even if it should never happen.

I'll log an error instead.

[fwereade, 2014-04-18 16:02:53]

Thanks, LGTM with trivials.

https://codereview.appspot.com/89260044/diff/1/environs/broker.go
File environs/broker.go (right):

https://codereview.appspot.com/89260044/diff/1/environs/broker.go#newcode51
environs/broker.go:51: ProviderId string
On 2014/04/18 15:37:26, dimitern wrote:
> On 2014/04/18 14:41:18, fwereade wrote:
> > We could make this clearer if we had a type for it, like instance.Id -- as
it
> is
> > I think ProviderId is not much of a win over NetworkId. But `NetworkId
> > NetworkId` should be less confusing.
> 
> I wanted to make the code use consistent naming - NetworkId is only used here.
I
> like the idea of type NetworkId string and then perhaps use ProviderId
NetworkId
> as a compromise?

Yeah, that works for me. Thanks.

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

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode570
state/machine.go:570: var doc networkInterfaceDoc
On 2014/04/18 15:37:27, dimitern wrote:
> On 2014/04/18 14:41:18, fwereade wrote:
> > hey, I'm feeling more strongly about this now: please assert that the
machine
> is
> > Dead in this method, just so that if (when) someone tries to use it from
> > elsewhere they get something pointing out the race with adding interfaces.
> 
> Done.

Sorry, I expressed myself poorly. The op is nice, and could/should happily stay
(other invariants *should* make it unnecessary, but +1 to defence in depth), but
what I'm really looking for is a quick check against m.doc.Life so we can abort
with an error before even running the txn.

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

https://codereview.appspot.com/89260044/diff/10021/state/api/params/internal....
state/api/params/internal.go:320: ProviderId instance.NetworkId
hmm, instance.NetworkId doesn't feel quite right -- I'd say environs.NetworkId,
with the expectation that soon enough we'll want an environs/network package and
we can just call it network.Id.

Or if you want to start the package with a single type, and start gradually
migrating stuff over to it as you go in future CLs, that would also work for me.

https://codereview.appspot.com/89260044/diff/10021/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/89260044/diff/10021/state/machine.go#newcode1063
state/machine.go:1063: if err = m.st.networkInterfaces.FindId(doc.Id).One(nil);
err == nil {
.One(&doc) perhaps? I suspect you're grabbing the data anyway, even if you're
not storing it, so you may as well return a doc that definitely completely
matches what's in the db.

I know it doesn't make a difference *now* but it feels like an encouragement to
subtle bugs.

[dimitern, 2014-04-18 16:44:34]

Please take a look.

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

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode570
state/machine.go:570: var doc networkInterfaceDoc
On 2014/04/18 16:02:54, fwereade wrote:
> On 2014/04/18 15:37:27, dimitern wrote:
> > On 2014/04/18 14:41:18, fwereade wrote:
> > > hey, I'm feeling more strongly about this now: please assert that the
> machine
> > is
> > > Dead in this method, just so that if (when) someone tries to use it from
> > > elsewhere they get something pointing out the race with adding interfaces.
> > 
> > Done.
> 
> Sorry, I expressed myself poorly. The op is nice, and could/should happily
stay
> (other invariants *should* make it unnecessary, but +1 to defence in depth),
but
> what I'm really looking for is a quick check against m.doc.Life so we can
abort
> with an error before even running the txn.

As discussed, I'm adding a check for m.doc.Life == Dead at the beginning.

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

https://codereview.appspot.com/89260044/diff/10021/state/api/params/internal....
state/api/params/internal.go:320: ProviderId instance.NetworkId
On 2014/04/18 16:02:54, fwereade wrote:
> hmm, instance.NetworkId doesn't feel quite right -- I'd say
environs.NetworkId,
> with the expectation that soon enough we'll want an environs/network package
and
> we can just call it network.Id.
> 
> Or if you want to start the package with a single type, and start gradually
> migrating stuff over to it as you go in future CLs, that would also work for
me.

I tried adding it to environs, but there are import loops. I like the idea of
having environs/network package, so I added that and moved environs.NetworkInfo
there.

https://codereview.appspot.com/89260044/diff/10021/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/89260044/diff/10021/state/machine.go#newcode1063
state/machine.go:1063: if err = m.st.networkInterfaces.FindId(doc.Id).One(nil);
err == nil {
On 2014/04/18 16:02:54, fwereade wrote:
> .One(&doc) perhaps? I suspect you're grabbing the data anyway, even if you're
> not storing it, so you may as well return a doc that definitely completely
> matches what's in the db.
> 
> I know it doesn't make a difference *now* but it feels like an encouragement
to
> subtle bugs.

Done.