Update add machine command to create containers

Update add machine command to create containers

The add machine command accepts additional command line
arguments to specify a container to create. The container
can be on a new machine or an existing instance. Container
constraints are also supported eg create a container with 
2G memory. The state is updated with the new machine document
but the machine watcher ignores the machine since there's no
code to provision containers yet.

examples:

juju add-machine /lxc
juju add-machine --constraints mem=4G /lxc
juju add-machine 1/lxc

https://code.launchpad.net/~wallyworld/juju-core/add-machine-container/+merge/167195

(do not edit description out of merge proposal)

[wallyworld, 2013-06-04 03:36:13]

Please take a look.

[mue, 2013-06-04 08:05:18]

Most parts look good, only provisioner needs a different solution which I'm
currently working on.

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

https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode186
state/state.go:186: // AddMachine adds a new machine configured to run the
supplied jobs on the
AddMachineWithConstraints

[mue, 2013-06-04 11:20:04]

On 2013/06/04 08:05:18, mue wrote:
> Most parts look good, only provisioner needs a different solution which I'm
> currently working on.
> 
> https://codereview.appspot.com/9886045/diff/1/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode186
> state/state.go:186: // AddMachine adds a new machine configured to run the
> supplied jobs on the
> AddMachineWithConstraints

I forgot the LGTM.

[fwereade, 2013-06-04 13:12:09]

Several concerns here, I'm afraid. Let's discuss live.

https://codereview.appspot.com/9886045/diff/1/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/9886045/diff/1/constraints/constraints.go#newc...
constraints/constraints.go:79: func (v Value) IsCompatible(v0 Value) bool {
Not very keen here. Logic is fine, but use case is shaky, and having this method
exist encourages the use case.

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

https://codereview.appspot.com/9886045/diff/1/state/machine.go#newcode61
state/machine.go:61: NumChildren   int
Not sure about putting refcounts directly in docs -- it just increases potential
contention on that document, I think, for relatively few benefits.

https://codereview.appspot.com/9886045/diff/1/state/machine.go#newcode100
state/machine.go:100: func (m *Machine) NumChildren() int {
ContainerCount?

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

https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode196
state/state.go:196: // new machine instance is created.
Hmmmmmm. I'm worried that this overlaps really badly with assignment policy. If
I've got an empty machine lying around, I don't think I want to create a *new*
machine just to put a new container on, do I?

Regardless, I don't believe there's enough difference between adding a machine
and adding a container to justify this new method. Can we do something like
specify "location" in AMWC?

https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode206
state/state.go:206: if !containerCons.IsCompatible(machineCons) {
Strongly -1 on doing this here. We've got a perfectly good mechanism for this in
the pipeline, which will fail in exactly the same way that existing provisioning
will if series isn't available, or constraints can't be matched, or whatever.
Having this code exist is IMO misleading: it implies the existence of
information and functionality we don't currently have. I'm all for doing smart
checks early *once we can*, but doing bad checks early and hoping we'll be able
to improve them later will just prejudice whatever we end up doing later.

https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode212
state/state.go:212: // machine first (if required) and delete it if there's an
error.
Er, yes it does. You'll need to refactor the machine-creation logic so you can
collect the parent-creation ops, including the child count (this may be a good
reason to take it out of the machine doc...), and append the child-creation ops.
The fact that we have non-transactional crack in this area already is not an
excuse for adding more.

https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode270
state/state.go:270: mdoc.Id = fmt.Sprintf("%s/%s/%d", parentId,
mdoc.ContainerType, p.NumChildren())
Can't use NumChildren, it can (will be able to) go down as well as up. We'll
need a new sequence for the given parent/kind string, surely?

https://codereview.appspot.com/9886045/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/9886045/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:231: if m.ContainerType() != "" {
I think it's more correct to fix st.WatchMachines() to ignore non-int ids.

[fwereade, 2013-06-04 13:13:06]

On 2013/06/04 13:12:09, fwereade wrote:
> Several concerns here, I'm afraid. Let's discuss live.
> 
> https://codereview.appspot.com/9886045/diff/1/constraints/constraints.go
> File constraints/constraints.go (right):
> 
>
https://codereview.appspot.com/9886045/diff/1/constraints/constraints.go#newc...
> constraints/constraints.go:79: func (v Value) IsCompatible(v0 Value) bool {
> Not very keen here. Logic is fine, but use case is shaky, and having this
method
> exist encourages the use case.
> 
> https://codereview.appspot.com/9886045/diff/1/state/machine.go
> File state/machine.go (right):
> 
> https://codereview.appspot.com/9886045/diff/1/state/machine.go#newcode61
> state/machine.go:61: NumChildren   int
> Not sure about putting refcounts directly in docs -- it just increases
potential
> contention on that document, I think, for relatively few benefits.
> 
> https://codereview.appspot.com/9886045/diff/1/state/machine.go#newcode100
> state/machine.go:100: func (m *Machine) NumChildren() int {
> ContainerCount?
> 
> https://codereview.appspot.com/9886045/diff/1/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode196
> state/state.go:196: // new machine instance is created.
> Hmmmmmm. I'm worried that this overlaps really badly with assignment policy.
If
> I've got an empty machine lying around, I don't think I want to create a *new*
> machine just to put a new container on, do I?
> 
> Regardless, I don't believe there's enough difference between adding a machine
> and adding a container to justify this new method. Can we do something like
> specify "location" in AMWC?
> 
> https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode206
> state/state.go:206: if !containerCons.IsCompatible(machineCons) {
> Strongly -1 on doing this here. We've got a perfectly good mechanism for this
in
> the pipeline, which will fail in exactly the same way that existing
provisioning
> will if series isn't available, or constraints can't be matched, or whatever.
> Having this code exist is IMO misleading: it implies the existence of
> information and functionality we don't currently have. I'm all for doing smart
> checks early *once we can*, but doing bad checks early and hoping we'll be
able
> to improve them later will just prejudice whatever we end up doing later.
> 
> https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode212
> state/state.go:212: // machine first (if required) and delete it if there's an
> error.
> Er, yes it does. You'll need to refactor the machine-creation logic so you can
> collect the parent-creation ops, including the child count (this may be a good
> reason to take it out of the machine doc...), and append the child-creation
ops.
> The fact that we have non-transactional crack in this area already is not an
> excuse for adding more.
> 
> https://codereview.appspot.com/9886045/diff/1/state/state.go#newcode270
> state/state.go:270: mdoc.Id = fmt.Sprintf("%s/%s/%d", parentId,
> mdoc.ContainerType, p.NumChildren())
> Can't use NumChildren, it can (will be able to) go down as well as up. We'll
> need a new sequence for the given parent/kind string, surely?
> 
>
https://codereview.appspot.com/9886045/diff/1/worker/provisioner/provisioner.go
> File worker/provisioner/provisioner.go (right):
> 
>
https://codereview.appspot.com/9886045/diff/1/worker/provisioner/provisioner....
> worker/provisioner/provisioner.go:231: if m.ContainerType() != "" {
> I think it's more correct to fix st.WatchMachines() to ignore non-int ids.

(just for explicitness' sake, NOT LGTM as is)

[fwereade, 2013-06-04 13:59:22]

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

https://codereview.appspot.com/9886045/diff/1/state/machine.go#newcode61
state/machine.go:61: NumChildren   int
On 2013/06/04 13:12:10, fwereade wrote:
> Not sure about putting refcounts directly in docs -- it just increases
potential
> contention on that document, I think, for relatively few benefits.

Also, FWIW, you'll need asserts for 0 refcount when advancing machine lifecycle.

[thumper, 2013-06-05 02:36:27]

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

https://codereview.appspot.com/9886045/diff/1/state/machine.go#newcode61
state/machine.go:61: NumChildren   int
On 2013/06/04 13:12:10, fwereade wrote:
> Not sure about putting refcounts directly in docs -- it just increases
potential
> contention on that document, I think, for relatively few benefits.

Why?  We have Principals in the doc, so why not the number of children?

Also, if we are going to bother having a count here, why not actually have a
[]string that refers to the machine ids of the children as was one of the
original plans?

[fwereade, 2013-06-06 16:48:17]

Sorry for the vacillation re refcount/list: I think I'm convinced that a list is
better (it's surely more convenient, and it should always be tractably small).

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

https://codereview.appspot.com/9886045/diff/1/state/machine.go#newcode61
state/machine.go:61: NumChildren   int
On 2013/06/05 02:36:27, thumper wrote:
> On 2013/06/04 13:12:10, fwereade wrote:
> > Not sure about putting refcounts directly in docs -- it just increases
> potential
> > contention on that document, I think, for relatively few benefits.
> 
> Why?  We have Principals in the doc, so why not the number of children?
> 
> Also, if we are going to bother having a count here, why not actually have a
> []string that refers to the machine ids of the children as was one of the
> original plans?

I'm not convinced Principals was implemented with proper forethought, but we're
stuck with it until we can handle a schema change.

Ian, I am on reflection comfortable with (something like) the containerInfo doc
you showed me yesterday; but I think the fields you should be considering are
something like:

    Id string `bson:_id` // parent machine global key
    Children []string    // full child container ids

...with an option on Sequence map[string]int, so long as you're careful about
concurrent changes not being able to dupe sequence numbers. I think this'll be
very amenable to simple watching, very much like the state.UnitsWatcher
implementations.

FWIW I'd still prefer, in general, not to use TxnRevno unless we have to -- I
feel like we really ought to be able to add two different containers at the
"same time" without aborting any transactions -- but I'm not feeling overly
dogmatic about that, just likely to be a bit persistently whiny. I'll try to
keep a lid on it.

[wallyworld, 2013-06-07 07:34:21]

*** Submitted:

Update add machine command to create containers

The add machine command accepts additional command line
arguments to specify a container to create. The container
can be on a new machine or an existing instance. Container
constraints are also supported eg create a container with 
2G memory. The state is updated with the new machine document
but the machine watcher ignores the machine since there's no
code to provision containers yet.

examples:

juju add-machine /lxc
juju add-machine --constraints mem=4G /lxc
juju add-machine 1/lxc

R=mue, fwereade, thumper
CC=
https://codereview.appspot.com/9886045

[fwereade, 2013-06-07 16:24:47]

A test was failing in cmd/juju, so dimitern reverted this. I have a few more
comments; hopefully we'll manage to have a chat on monday or tues if it's
unclear.

https://codereview.appspot.com/9886045/diff/11002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9886045/diff/11002/state/machine.go#newcode61
state/machine.go:61: ContainerType string
ContainerType is part of the id, isn't it?

https://codereview.appspot.com/9886045/diff/11002/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/9886045/diff/11002/state/machine_test.go#newco...
state/machine_test.go:31: func (s *MachineSuite) ContainerDefaults(c *C) {
TestContainerDefaults, otherwise it won't get run.

https://codereview.appspot.com/9886045/diff/11002/state/machine_test.go#newco...
state/machine_test.go:105: _, err = state.MachineContainers(s.State,
s.machine.Id())
Can we maybe maintain the domain-object style within state, for now, just
consistency's sake? Once we isolate state behind the API we can make bulk-style
changes as indicated, but for now Machine.Containers() seems appropriate at this
level.

https://codereview.appspot.com/9886045/diff/11002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9886045/diff/11002/state/state.go#newcode198
state/state.go:198: func (st *State) AddContainerWithConstraints(
I still think that AddMachine vs AddContainer is an unhelpful distinction -- you
could surely pass a `location string` into AddMachine for the same effect? I'd
prefer not to distinguish between the two at the state level unless there's a
really good reason in favour of doing so.

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go
File state/watcher.go (left):

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go#oldcode69
state/watcher.go:69: func (st *State) WatchMachines() *LifecycleWatcher {
WatchEnvironMachines, just to be explicit, perhaps? They're all machines, but
the "environment machines" are the ones that we expect to have provisioned via
the environment?

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go#newcode187
state/watcher.go:187: ids = w.filter(ids)
Just a thought -- how about filtering before merging, so there's less work to do
in merge?

[fwereade, 2013-06-07 16:28:16]

On 2013/06/07 16:24:47, fwereade wrote:
> A test was failing in cmd/juju, so dimitern reverted this. I have a few more
> comments; hopefully we'll manage to have a chat on monday or tues if it's
> unclear.

That said, none of the above should be considered blockers so long as you ponder
them and either address them or have a good reason not to, so this essentially
LGTM if we don't get to talk.

[wallyworld, 2013-06-11 21:11:31]

*** Submitted:

Update add machine command to create containers

The add machine command accepts additional command line
arguments to specify a container to create. The container
can be on a new machine or an existing instance. Container
constraints are also supported eg create a container with 
2G memory. The state is updated with the new machine document
but the machine watcher ignores the machine since there's no
code to provision containers yet.

examples:

juju add-machine /lxc
juju add-machine --constraints mem=4G /lxc
juju add-machine 1/lxc

R=
CC=
https://codereview.appspot.com/9886045

https://codereview.appspot.com/9886045/diff/11002/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/9886045/diff/11002/state/machine_test.go#newco...
state/machine_test.go:31: func (s *MachineSuite) ContainerDefaults(c *C) {
On 2013/06/07 16:24:48, fwereade wrote:
> TestContainerDefaults, otherwise it won't get run.

Done.

https://codereview.appspot.com/9886045/diff/11002/state/machine_test.go#newco...
state/machine_test.go:105: _, err = state.MachineContainers(s.State,
s.machine.Id())
On 2013/06/07 16:24:48, fwereade wrote:
> Can we maybe maintain the domain-object style within state, for now, just
> consistency's sake? Once we isolate state behind the API we can make
bulk-style
> changes as indicated, but for now Machine.Containers() seems appropriate at
this
> level.

Done.

https://codereview.appspot.com/9886045/diff/11002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9886045/diff/11002/state/state.go#newcode198
state/state.go:198: func (st *State) AddContainerWithConstraints(
On 2013/06/07 16:24:48, fwereade wrote:
> I still think that AddMachine vs AddContainer is an unhelpful distinction --
you
> could surely pass a `location string` into AddMachine for the same effect? I'd
> prefer not to distinguish between the two at the state level unless there's a
> really good reason in favour of doing so.

The 3 related functions:
AddMachine
AddMachineWithConstraints
AddContainerWithConstraints

were provided since Go doesn't have default parameter values, and essentially
provide a semantic wrapper around state.addMachine(). I collapsed the
...WithConstraints variants down to one function taking the (now) exported
AddMachineParams struct as an argument.

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go
File state/watcher.go (left):

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go#oldcode69
state/watcher.go:69: func (st *State) WatchMachines() *LifecycleWatcher {
On 2013/06/07 16:24:48, fwereade wrote:
> WatchEnvironMachines, just to be explicit, perhaps? They're all machines, but
> the "environment machines" are the ones that we expect to have provisioned via
> the environment?

Done.

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/9886045/diff/11002/state/watcher.go#newcode187
state/watcher.go:187: ids = w.filter(ids)
On 2013/06/07 16:24:48, fwereade wrote:
> Just a thought -- how about filtering before merging, so there's less work to
do
> in merge?

If I do that the initial ids do not get filtered. So easiest just to filter
after the merge to ensure everything is picked up.