state: Add networks doc for services

state: Add networks doc for services

Adds a new document to state for describing which networks
units in a service should be associated with, and which
networks it should not be associated with.

I want some unit tests just for the networks file I think,
what should they look like? Most state testing is at a
higher level so I was struggling to find good examples.

https://code.launchpad.net/~gz/juju-core/networks_state_doc/+merge/211565

(do not edit description out of merge proposal)

[gz, 2014-03-18 15:56:44]

Please take a look.

[fwereade, 2014-03-18 16:02:58]

looks solid to me, modulo a spot more testing (eg handling notFound from a
pre-networks service, making sure we can recreate a service (I think we already
do have a test for that actually, but still), etc)

https://codereview.appspot.com/77270046/diff/1/state/networks.go
File state/networks.go (right):

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode15
state/networks.go:15: // networksDoc represents the networks a service should be
associated with
serviceNetworksDoc maybe, just to be explicit

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode37
state/networks.go:37: func setNetworksOp(st *State, id string, includeNetworks,
excludeNetworks []string) txn.Op {
I'd leave this out while they're still immutable

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode55
state/networks.go:55: err = nil
d

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode60
state/networks.go:60: // XXX: need to explictly copy these slices to be safe?
not really necessary afaics, no other references to them will live on

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode67
state/networks.go:67: func writeNetworks(st *State, id string, includeNetworks,
excludeNetworks []string) error {
again this isn't really needed for now

https://codereview.appspot.com/77270046/diff/1/state/open.go
File state/open.go (right):

https://codereview.appspot.com/77270046/diff/1/state/open.go#newcode258
state/open.go:258: networks:       db.C("networks"),
serviceNetworks, again, I think

[gz, 2014-03-18 17:48:59]

Please take a look.

https://codereview.appspot.com/77270046/diff/1/state/networks.go
File state/networks.go (right):

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode15
state/networks.go:15: // networksDoc represents the networks a service should be
associated with
On 2014/03/18 16:02:59, fwereade wrote:
> serviceNetworksDoc maybe, just to be explicit

Done.

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode37
state/networks.go:37: func setNetworksOp(st *State, id string, includeNetworks,
excludeNetworks []string) txn.Op {
On 2014/03/18 16:02:59, fwereade wrote:
> I'd leave this out while they're still immutable

Done.

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode55
state/networks.go:55: err = nil
On 2014/03/18 16:02:59, fwereade wrote:
> d

Done.

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode60
state/networks.go:60: // XXX: need to explictly copy these slices to be safe?
On 2014/03/18 16:02:59, fwereade wrote:
> not really necessary afaics, no other references to them will live on

Removed comment.

https://codereview.appspot.com/77270046/diff/1/state/networks.go#newcode67
state/networks.go:67: func writeNetworks(st *State, id string, includeNetworks,
excludeNetworks []string) error {
On 2014/03/18 16:02:59, fwereade wrote:
> again this isn't really needed for now

Removed as well for now.

https://codereview.appspot.com/77270046/diff/1/state/open.go
File state/open.go (right):

https://codereview.appspot.com/77270046/diff/1/state/open.go#newcode258
state/open.go:258: networks:       db.C("networks"),
On 2014/03/18 16:02:59, fwereade wrote:
> serviceNetworks, again, I think

I've left the attribute on State as networks (we can freely change this at any
point, right?) but renamed the db part to "servicenetworks" as that's more
annoying to modify in future. Does that work for you?

[fwereade, 2014-03-19 08:59:30]

WRT testing, the conn_test.go file has its own ConnSuite that gives (or at least
allows) direct access to the collections -- you can use that to do things like
delete the network doc once you've created a service, to make sure we can handle
services created without them. The StateSuite in state_test embeds this
ConnSuite so it shouldn't require too much jiggery-pokery

https://codereview.appspot.com/77270046/diff/1/state/open.go
File state/open.go (right):

https://codereview.appspot.com/77270046/diff/1/state/open.go#newcode258
state/open.go:258: networks:       db.C("networks"),
On 2014/03/18 17:48:59, gz wrote:
> On 2014/03/18 16:02:59, fwereade wrote:
> > serviceNetworks, again, I think
> 
> I've left the attribute on State as networks (we can freely change this at any
> point, right?) but renamed the db part to "servicenetworks" as that's more
> annoying to modify in future. Does that work for you?

IMO it'd be nicer to make the names match, because machineNetworks will come
pretty soon... and I think I'd also prefer to see interCaps in the collection
name (even if we have had a tendency not to in the past).

[gz, 2014-03-21 12:14:02]

Please take a look.

[dimitern, 2014-03-21 14:21:56]

Looks almost ready, just a few more things.

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

https://codereview.appspot.com/77270046/diff/40001/state/networks.go#newcode13
state/networks.go:13: // serviceNetworksDoc represents the network restrictions
for a service
Since we'll be using this for services, machines and possibly units, let's call
it networksDoc and make it non-service-specific.

Also, I'd prefer IncludedNetworks []string and ExcludedNetworks []string. No
need for *[]string.

Finally, I'd mention that the document ID field is the globalKey of a service or
machine.

https://codereview.appspot.com/77270046/diff/40001/state/networks.go#newcode35
state/networks.go:35: // While networks are immutable, there is no
setServiceNetworksOp function
No need to mention this for now - I'd just drop it.

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

https://codereview.appspot.com/77270046/diff/40001/state/open.go#newcode157
state/open.go:157: createServiceNetworksOp(st, environGlobalKey, []string{},
[]string{}),
This is wrong I think - why are we creating a networks doc linked to the
environment?

https://codereview.appspot.com/77270046/diff/40001/state/open.go#newcode258
state/open.go:258: serviceNetworks: db.C("servicenetworks"),
s/serviceNetworks/networks/ etc.

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

https://codereview.appspot.com/77270046/diff/40001/state/service.go#newcode825
state/service.go:825: // Networks returns the networks a service is associated
with
s/with/with./

For the sake of clarity, I'd name the two return arguments as includedNetworks
[]string, excludedNetworks []string.

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

https://codereview.appspot.com/77270046/diff/40001/state/service_test.go#newc...
state/service_test.go:1433: func (s *ServiceSuite) TestNetworks(c *gc.C) {
Please add a test to check we can set networks at service creation and we can
read them back.

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

https://codereview.appspot.com/77270046/diff/40001/state/state.go#newcode913
state/state.go:913: func (st *State) AddService(name, ownerTag string, ch
*Charm) (service *Service, err error) {
You'll need to add includedNetworks, excludedNetworks []string to the arguments
I think... (how else would we set them?)

https://codereview.appspot.com/77270046/diff/40001/state/state.go#newcode957
state/state.go:957: createServiceNetworksOp(st, svc.globalKey(), []string{},
[]string{}),
...and set them here.

[gz, 2014-03-21 15:46:00]

Responses to some queries.

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

https://codereview.appspot.com/77270046/diff/40001/state/networks.go#newcode13
state/networks.go:13: // serviceNetworksDoc represents the network restrictions
for a service
On 2014/03/21 14:21:56, dimitern wrote:
> Since we'll be using this for services, machines and possibly units, let's
call
> it networksDoc and make it non-service-specific.

What exactly does "make it non-service-specific" entail? I didn't understand
that from the log between you and William.

> Also, I'd prefer IncludedNetworks []string and ExcludedNetworks []string. No
> need for *[]string.

Copied from what tags in contraints uses... Can omit the pointer if mongo
doesn't care.

> Finally, I'd mention that the document ID field is the globalKey of a service
or
> machine.

In the comment here? Ok.

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

https://codereview.appspot.com/77270046/diff/40001/state/open.go#newcode157
state/open.go:157: createServiceNetworksOp(st, environGlobalKey, []string{},
[]string{}),
On 2014/03/21 14:21:56, dimitern wrote:
> This is wrong I think - why are we creating a networks doc linked to the
> environment?

Do networks make any sense outside the context of an environment? You'll need to
explain state bits to me here I think.

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

https://codereview.appspot.com/77270046/diff/40001/state/service_test.go#newc...
state/service_test.go:1433: func (s *ServiceSuite) TestNetworks(c *gc.C) {
On 2014/03/21 14:21:56, dimitern wrote:
> Please add a test to check we can set networks at service creation and we can
> read them back.

That is deliberately not included in this mp, but I am missing some test
coverage for the functionality that is here.

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

https://codereview.appspot.com/77270046/diff/40001/state/state.go#newcode913
state/state.go:913: func (st *State) AddService(name, ownerTag string, ch
*Charm) (service *Service, err error) {
On 2014/03/21 14:21:56, dimitern wrote:
> You'll need to add includedNetworks, excludedNetworks []string to the
arguments
> I think... (how else would we set them?)

That's deliberately ommited from this MP to avoid changing all the AddService
callers, untill we actually glue the other bits together.

https://codereview.appspot.com/77270046/diff/40001/state/state.go#newcode957
state/state.go:957: createServiceNetworksOp(st, svc.globalKey(), []string{},
[]string{}),
On 2014/03/21 14:21:56, dimitern wrote:
> ...and set them here.

Indeed, when needed, will be from function args.

[dimitern, 2014-03-21 16:11:31]

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

https://codereview.appspot.com/77270046/diff/40001/state/open.go#newcode157
state/open.go:157: createServiceNetworksOp(st, environGlobalKey, []string{},
[]string{}),
On 2014/03/21 15:46:00, gz wrote:
> On 2014/03/21 14:21:56, dimitern wrote:
> > This is wrong I think - why are we creating a networks doc linked to the
> > environment?
> 
> Do networks make any sense outside the context of an environment? You'll need
to
> explain state bits to me here I think.

As discussed on mumble, just for the record here: this actually adds a networks
doc linked to the environment global key, which we don't want - only services
and machines will have networks attached.

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

https://codereview.appspot.com/77270046/diff/40001/state/service_test.go#newc...
state/service_test.go:1433: func (s *ServiceSuite) TestNetworks(c *gc.C) {
On 2014/03/21 15:46:00, gz wrote:
> On 2014/03/21 14:21:56, dimitern wrote:
> > Please add a test to check we can set networks at service creation and we
can
> > read them back.
> 
> That is deliberately not included in this mp, but I am missing some test
> coverage for the functionality that is here.

I'm fine with adding this in a follow-up.

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

https://codereview.appspot.com/77270046/diff/40001/state/state.go#newcode913
state/state.go:913: func (st *State) AddService(name, ownerTag string, ch
*Charm) (service *Service, err error) {
On 2014/03/21 15:46:00, gz wrote:
> On 2014/03/21 14:21:56, dimitern wrote:
> > You'll need to add includedNetworks, excludedNetworks []string to the
> arguments
> > I think... (how else would we set them?)
> 
> That's deliberately ommited from this MP to avoid changing all the AddService
> callers, untill we actually glue the other bits together.

Fair enough (I still need it for my branch, but follow-up's OK).

[gz, 2014-03-21 16:31:05]

Please take a look.

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

https://codereview.appspot.com/77270046/diff/40001/state/networks.go#newcode13
state/networks.go:13: // serviceNetworksDoc represents the network restrictions
for a service
On 2014/03/21 14:21:56, dimitern wrote:
> Since we'll be using this for services, machines and possibly units, let's
call
> it networksDoc and make it non-service-specific.
> 
> Also, I'd prefer IncludedNetworks []string and ExcludedNetworks []string. No
> need for *[]string.
> 
> Finally, I'd mention that the document ID field is the globalKey of a service
or
> machine.

Done.

https://codereview.appspot.com/77270046/diff/40001/state/networks.go#newcode35
state/networks.go:35: // While networks are immutable, there is no
setServiceNetworksOp function
On 2014/03/21 14:21:56, dimitern wrote:
> No need to mention this for now - I'd just drop it.

Keeping, just because I'll forget why otherwise, even if it's obvious to
everyone else. :)

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

https://codereview.appspot.com/77270046/diff/40001/state/open.go#newcode258
state/open.go:258: serviceNetworks: db.C("servicenetworks"),
On 2014/03/21 14:21:56, dimitern wrote:
> s/serviceNetworks/networks/ etc.

This is the one rename I didn't straight revert - bare networks is kind of a bad
name, I've gone with 'linkednetworks' - these are names specifically associated
with a machine or service, not a juju-level modelling of cloud networks, which
we may also want later.

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

https://codereview.appspot.com/77270046/diff/40001/state/service.go#newcode825
state/service.go:825: // Networks returns the networks a service is associated
with
On 2014/03/21 14:21:56, dimitern wrote:
> s/with/with./
> 
> For the sake of clarity, I'd name the two return arguments as includedNetworks
> []string, excludedNetworks []string.

Done.

[dimitern, 2014-03-22 12:56:08]

LGTM, thanks.

I'd suggest a live test where a 1.16 environment gets upgraded to 1.18 (current
version, faked) and reading the networks of and old (1.16-created) service does
not cause issues.

[fwereade, 2014-03-24 09:04:23]

Please add unit tests for the no-network-doc behaviour, even if you do run a
live test and it works. I supplied pointers earlier in this chain, but please
come and talk to me if they're insufficient.