state: Add Machine.Networks() and create them

state: Add Machine.Networks() and create them

This adds a new method to state.Machine: Networks(),
which returns any associated networks to include or
exclude for the machine.

Added a couple of extra fields to state.MachineTemplate:
IncludeNetworks and ExcludeNetworks. Each time a new
machine or container is created, if these are populated,
they will be also saved in state for the machine, as a
linked networksDoc.

A few drive-by fixes for naming consistency, including
changing Included/ExcludedNetworks across the codebase
to Include/ExcludeNetworks to better illustrate the
intent (i.e. we're saying what networks should be
included or excluded, not how the reality is yet - we
can do that post-provisioning only).

Added tests to ensure when the new networkDocs are
missing for a service/machine .Networks() still works
and returns nil error, as suggested in reviews.

https://code.launchpad.net/~dimitern/juju-core/360-state-machine-networks/+merge/212418

(do not edit description out of merge proposal)

[dimitern, 2014-03-24 13:47:48]

Please take a look.

[fwereade, 2014-03-24 14:01:55]

Needs tests for upgraded environments, otherwise looking good. Otherwise ust
trivials.

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

https://codereview.appspot.com/79250044/diff/1/state/machine.go#newcode884
state/machine.go:884: // Networks returns the list of networks the machine is on
*should be* on -- please be ultra-explicit that this is provisioning input, not
a reflection of objective reality.

https://codereview.appspot.com/79250044/diff/1/state/networks.go
File state/networks.go (left):

https://codereview.appspot.com/79250044/diff/1/state/networks.go#oldcode17
state/networks.go:17: NetworksToExclude []string
Can we make the marshalling explicit here? And maybe a bit more compact? Every
document will have those field names spelled out in full, and that's just a
kinda wanton waste of space -- if we decide the benefits of readability win out
(I'm not sold on that in this case) we should make sure they marshall
consistently (ie "networks-to-include" -- but still, I think context makes
"include" clear enough).

[dimitern, 2014-03-24 16:20:40]

Please take a look.

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

https://codereview.appspot.com/79250044/diff/1/state/machine.go#newcode884
state/machine.go:884: // Networks returns the list of networks the machine is on
On 2014/03/24 14:01:55, fwereade wrote:
> *should be* on -- please be ultra-explicit that this is provisioning input,
not
> a reflection of objective reality.

Done.

https://codereview.appspot.com/79250044/diff/1/state/networks.go
File state/networks.go (left):

https://codereview.appspot.com/79250044/diff/1/state/networks.go#oldcode17
state/networks.go:17: NetworksToExclude []string
On 2014/03/24 14:01:55, fwereade wrote:
> Can we make the marshalling explicit here? And maybe a bit more compact? Every
> document will have those field names spelled out in full, and that's just a
> kinda wanton waste of space -- if we decide the benefits of readability win
out
> (I'm not sold on that in this case) we should make sure they marshall
> consistently (ie "networks-to-include" -- but still, I think context makes
> "include" clear enough).

Done.

[gz, 2014-03-24 16:35:01]

LGTM, naming quibble.

https://codereview.appspot.com/79250044/diff/1/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/79250044/diff/1/state/addmachine.go#newcode62
state/addmachine.go:62: ExcludedNetworks []string
Naming quibble, I've been thinking of this as future/active rather than past
tense. What we have in state is instructions to the provisioner about what it
should do with networks when bringing up machines, not what current reality on
the machine actually is.

I think generally s/((In|Ex)clude)d/$1/g generally makes sense, there are only a
few UI level concerns where we're actually talking about a machine/service that
does already exist and should, therefore, have had those network operations done
already.

[fwereade, 2014-03-24 16:35:43]

Fix the error from readNetworks, and I think we're good to go.

https://codereview.appspot.com/79250044/diff/20001/state/compat_test.go
File state/compat_test.go (right):

https://codereview.appspot.com/79250044/diff/20001/state/compat_test.go#newco...
state/compat_test.go:81: // compatibility.
This sort of comment makes me happy. Thanks :).

https://codereview.appspot.com/79250044/diff/20001/state/compat_test.go#newco...
state/compat_test.go:92: c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
I don't think we should be reporting an error here -- we should be faking up the
impression that everything is ok (it is -- we really should just treat them as
having been added without networking). And we should do similarly for services.

https://codereview.appspot.com/79250044/diff/20001/state/compat_test.go#newco...
state/compat_test.go:111: c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
ditto

https://codereview.appspot.com/79250044/diff/20001/state/networks.go
File state/networks.go (right):

https://codereview.appspot.com/79250044/diff/20001/state/networks.go#newcode49
state/networks.go:49: err = errors.NotFoundf("linked networks")
I think you should just set `err = nil` here, and thus fix the behaviour for
both services and machines. Just needs a test for that behaviour on service as
well.

[fwereade, 2014-03-24 16:39:25]

On 2014/03/24 16:35:01, gz wrote:
> LGTM, naming quibble.
> 
> https://codereview.appspot.com/79250044/diff/1/state/addmachine.go
> File state/addmachine.go (right):
> 
> https://codereview.appspot.com/79250044/diff/1/state/addmachine.go#newcode62
> state/addmachine.go:62: ExcludedNetworks []string
> Naming quibble, I've been thinking of this as future/active rather than past
> tense. What we have in state is instructions to the provisioner about what it
> should do with networks when bringing up machines, not what current reality on
> the machine actually is.
> 
> I think generally s/((In|Ex)clude)d/$1/g generally makes sense, there are only
a
> few UI level concerns where we're actually talking about a machine/service
that
> does already exist and should, therefore, have had those network operations
done
> already.

We want a different struct, and probably a different collection, for the
networks-in-reality info. It's most conceptually similar to the
HardwareCharacteristics, which is how we record what we got when we asked for
the machine constraints; we hide constraints and show HCs, just as we should
hide requested-networks and show actual-networks.

In the service case we should still be showing the request, though: that'll show
what networks we can guarantee all that service's units will be on.

...*assuming* we have sanity-checking code that prevents unit placement that
can't satisfy the networking concerns. Do we have a card for that?

[fwereade, 2014-03-24 16:40:24]

On 2014/03/24 16:39:25, fwereade wrote:
> On 2014/03/24 16:35:01, gz wrote:
> > LGTM, naming quibble.
> > 
> > https://codereview.appspot.com/79250044/diff/1/state/addmachine.go
> > File state/addmachine.go (right):
> > 
> > https://codereview.appspot.com/79250044/diff/1/state/addmachine.go#newcode62
> > state/addmachine.go:62: ExcludedNetworks []string
> > Naming quibble, I've been thinking of this as future/active rather than past
> > tense. What we have in state is instructions to the provisioner about what
it
> > should do with networks when bringing up machines, not what current reality
on
> > the machine actually is.
> > 
> > I think generally s/((In|Ex)clude)d/$1/g generally makes sense, there are
only
> a
> > few UI level concerns where we're actually talking about a machine/service
> that
> > does already exist and should, therefore, have had those network operations
> done
> > already.
> 
> We want a different struct, and probably a different collection, for the
> networks-in-reality info. It's most conceptually similar to the
> HardwareCharacteristics, which is how we record what we got when we asked for
> the machine constraints; we hide constraints and show HCs, just as we should
> hide requested-networks and show actual-networks.
> 
> In the service case we should still be showing the request, though: that'll
show
> what networks we can guarantee all that service's units will be on.
> 
> ...*assuming* we have sanity-checking code that prevents unit placement that
> can't satisfy the networking concerns. Do we have a card for that?

(sorry, this comment has an implied context of "what we show the user in status
and why", because I was thinking about that as well)

[dimitern, 2014-03-25 09:22:21]

Please take a look.

https://codereview.appspot.com/79250044/diff/1/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/79250044/diff/1/state/addmachine.go#newcode62
state/addmachine.go:62: ExcludedNetworks []string
On 2014/03/24 16:35:02, gz wrote:
> Naming quibble, I've been thinking of this as future/active rather than past
> tense. What we have in state is instructions to the provisioner about what it
> should do with networks when bringing up machines, not what current reality on
> the machine actually is.
> 
> I think generally s/((In|Ex)clude)d/$1/g generally makes sense, there are only
a
> few UI level concerns where we're actually talking about a machine/service
that
> does already exist and should, therefore, have had those network operations
done
> already.

s/((In|Ex)clude)d/$1/g Done.

https://codereview.appspot.com/79250044/diff/20001/state/compat_test.go
File state/compat_test.go (right):

https://codereview.appspot.com/79250044/diff/20001/state/compat_test.go#newco...
state/compat_test.go:92: c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
On 2014/03/24 16:35:43, fwereade wrote:
> I don't think we should be reporting an error here -- we should be faking up
the
> impression that everything is ok (it is -- we really should just treat them as
> having been added without networking). And we should do similarly for
services.

As discussed online, I'm changing readNetworks (and the tests) to return nil
when there's no record in state.

https://codereview.appspot.com/79250044/diff/20001/state/compat_test.go#newco...
state/compat_test.go:111: c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
On 2014/03/24 16:35:43, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/79250044/diff/20001/state/networks.go
File state/networks.go (right):

https://codereview.appspot.com/79250044/diff/20001/state/networks.go#newcode49
state/networks.go:49: err = errors.NotFoundf("linked networks")
On 2014/03/24 16:35:43, fwereade wrote:
> I think you should just set `err = nil` here, and thus fix the behaviour for
> both services and machines. Just needs a test for that behaviour on service as
> well.

Error ignored and added a comment. Tests for both machines and services are
already added in compat_test.go

[fwereade, 2014-03-25 09:26:21]

LGTM, thanks.

[dimitern, 2014-03-25 09:34:10]

Please take a look.