Add container support to force-machine

Add container support to force-machine

The deploy --force-machine option now supports containers,
either deploying to existing containers or creating new
ones on nominated machines.

Example syntax:

 juju deploy mysql --force-machine 23
 juju deploy mysql --force-machine 24/lxc/3
 juju deploy mysql --force-machine 25/lxc

The last example creates a new lxc container on 
machine 25.

The add-machine syntax has been tweaked to remove the 
leading "/" from the container arg.

https://code.launchpad.net/~wallyworld/juju-core/force-machine-containers/+merge/173836

Requires: https://code.launchpad.net/~wallyworld/juju-core/clean-policy-container/+merge/173633

(do not edit description out of merge proposal)

[wallyworld, 2013-07-09 22:44:51]

Please take a look.

[fwereade, 2013-07-10 09:15:52]

On 2013/07/09 22:44:51, wallyworld wrote:
> Please take a look.

Diff is very dirty, but two preliminary comments:

  * the placement syntax we use for machine creation here should match what we
use in add-machine
  * I think it's more important to make add-unit respect force-machine than it
is to roll machine creation into the deploy variant

[wallyworld, 2013-07-10 12:18:27]

Please take a look.

[wallyworld, 2013-07-11 00:08:01]

Please take a look.

[wallyworld, 2013-07-11 00:31:47]

Please take a look.

[fwereade, 2013-07-11 04:10:25]

I'm not sure about the new args to AddUnits. Am I missing something?

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/addmachine_test.g...
cmd/juju/addmachine_test.go:76: err := runAddMachine(c, fmt.Sprintf("%s",
ctype))
This should not work. The problem AIUI is in the implicit creation of host
machines when explicitly asked for a container. I don't think we want to be
solving the placement problem here...

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/deploy.go#newcode111
cmd/juju/deploy.go:111: c.ForceMachineId =
strings.Join(specParts[:len(specParts)-1], "/")
I don't think this is a sensible split. The ForceMachineId arg can now either
mean "put it on this machine" or "put it on a different machine", depending on
an additional arg which just opens up a whole bunch more impossible/insane
requests.

What's the advantage over checking the string for best-guess validity and
passing it straight down to conn.AddUnits for handling therein?

https://codereview.appspot.com/10777044/diff/13001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/10777044/diff/13001/juju/conn.go#newcode329
juju/conn.go:329: }
Yeah, I don't think we lose anything by deferring the unpack to this point, do
we? And we avoid threading meaningless args right though the codebase...

[wallyworld, 2013-07-11 06:41:02]

Please take a look.

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/addmachine_test.g...
cmd/juju/addmachine_test.go:76: err := runAddMachine(c, fmt.Sprintf("%s",
ctype))
On 2013/07/11 04:10:26, fwereade wrote:
> This should not work. The problem AIUI is in the implicit creation of host
> machines when explicitly asked for a container. I don't think we want to be
> solving the placement problem here...

OK, so we are saying we want people to have to do this in 2 steps?
1. create the machine and do a juju status to get the id
2. create the container on the new machine using the id

There's a fair bit of logic in the various addMachine methods in the state
package to support the implicit machine creation, which was done a fair while
ago when the container work first started. I'd like to defer any work to revert
this logic to a separate branch.

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/10777044/diff/13001/cmd/juju/deploy.go#newcode111
cmd/juju/deploy.go:111: c.ForceMachineId =
strings.Join(specParts[:len(specParts)-1], "/")
On 2013/07/11 04:10:26, fwereade wrote:
> I don't think this is a sensible split. The ForceMachineId arg can now either
> mean "put it on this machine" or "put it on a different machine", depending on
> an additional arg which just opens up a whole bunch more impossible/insane
> requests.
> 
> What's the advantage over checking the string for best-guess validity and
> passing it straight down to conn.AddUnits for handling therein?

I've done as suggested above. The reason for doing it as separate parameters was
because it would be easier for the GUI. But the GUI can just combine the machine
id and container into one parameter and pass that down.

[dimitern, 2013-07-11 21:34:15]

A few suggestions. Looks good in general.

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:34: Doc:     "Machines are created in a clean state and
ready to have units deployed.",
a few examples (like in the CL description, but with explanations on each one's
effect) will be very useful to the users. you can even pull Doc in a const
outside.

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go#newcode64
cmd/juju/deploy.go:64: f.StringVar(&c.ForceMachineSpec, "force-machine", "",
"Machine/container to deploy initial unit, bypasses constraints")
s/Machine/machine/ (since all the other are first-lower anyway)

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go#newcode96
cmd/juju/deploy.go:96: return errors.New("cannot use --num-units with
--force-machine")
technically --num-units=1 --force-machine=... will still work, so this is a bit
misleading..

maybe the default for NumUnits should be 0 and get set to 1 if 0 (where you
check for --force-machine=="" and --num-units==1) - or even -1 as an invalid
value - i don't see checks for negative values below

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go#newcode99
cmd/juju/deploy.go:99: return fmt.Errorf("invalid force machine parameter %q",
c.ForceMachineSpec)
s/force machine/--force-machine/

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go#newcode138
cmd/juju/deploy.go:138: if numUnits == 1 && c.ForceMachineSpec == "" {
see above

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy_test.go#ne...
cmd/juju/deploy_test.go:207: ms, err := s.State.AllMachines()
s/ms/machines/ please

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy_test.go#ne...
cmd/juju/deploy_test.go:219: ms, err := s.State.AllMachines()
ditto

https://codereview.appspot.com/10777044/diff/20001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/10777044/diff/20001/juju/conn.go#newcode196
juju/conn.go:196: // Use string for deploy-to machine to avoid ambiguity around
machine 0.
move this line (and rephrase it probably a bit) after the next one, so the doc
comment begins properly: // ForceMachineSpec ...

https://codereview.appspot.com/10777044/diff/20001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/10777044/diff/20001/juju/conn_test.go#newcode553
juju/conn_test.go:553: // The newly created container will use the constraints.
blank line before this one

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

https://codereview.appspot.com/10777044/diff/20001/state/state.go#newcode72
state/state.go:72: const containerNameSnippet = "(/[a-z]+)"
i'd really like if you use this in the line above, so we'll have to update only
one const if the name pattern needs to change.

https://codereview.appspot.com/10777044/diff/20001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10777044/diff/20001/state/unit.go#newcode137
state/unit.go:137: // Series returns the series.
s/series./deployed charm's series./ ?

[wallyworld, 2013-07-12 03:44:53]

Please take a look.

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:34: Doc:     "Machines are created in a clean state and
ready to have units deployed.",
On 2013/07/11 21:34:15, dimitern wrote:
> a few examples (like in the CL description, but with explanations on each
one's
> effect) will be very useful to the users. you can even pull Doc in a const
> outside.

I don't know if we are going to keep this long term, and there is still debate
over the syntax to be used so I'd rather leave it as is for now.

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go#newcode64
cmd/juju/deploy.go:64: f.StringVar(&c.ForceMachineSpec, "force-machine", "",
"Machine/container to deploy initial unit, bypasses constraints")
On 2013/07/11 21:34:15, dimitern wrote:
> s/Machine/machine/ (since all the other are first-lower anyway)

Done.

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go#newcode96
cmd/juju/deploy.go:96: return errors.New("cannot use --num-units with
--force-machine")
On 2013/07/11 21:34:15, dimitern wrote:
> technically --num-units=1 --force-machine=... will still work, so this is a
bit
> misleading..
> 
> maybe the default for NumUnits should be 0 and get set to 1 if 0 (where you
> check for --force-machine=="" and --num-units==1) - or even -1 as an invalid
> value - i don't see checks for negative values below

I've changed the error message to:
cannot use --num-units > 1 with --force-machine

I think this is the best approach

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy.go#newcode99
cmd/juju/deploy.go:99: return fmt.Errorf("invalid force machine parameter %q",
c.ForceMachineSpec)
On 2013/07/11 21:34:15, dimitern wrote:
> s/force machine/--force-machine/

Done.

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy_test.go#ne...
cmd/juju/deploy_test.go:207: ms, err := s.State.AllMachines()
On 2013/07/11 21:34:15, dimitern wrote:
> s/ms/machines/ please

Done.

https://codereview.appspot.com/10777044/diff/20001/cmd/juju/deploy_test.go#ne...
cmd/juju/deploy_test.go:219: ms, err := s.State.AllMachines()
On 2013/07/11 21:34:15, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/10777044/diff/20001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/10777044/diff/20001/juju/conn.go#newcode196
juju/conn.go:196: // Use string for deploy-to machine to avoid ambiguity around
machine 0.
On 2013/07/11 21:34:15, dimitern wrote:
> move this line (and rephrase it probably a bit) after the next one, so the doc
> comment begins properly: // ForceMachineSpec ...

Done.

https://codereview.appspot.com/10777044/diff/20001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/10777044/diff/20001/juju/conn_test.go#newcode553
juju/conn_test.go:553: // The newly created container will use the constraints.
On 2013/07/11 21:34:15, dimitern wrote:
> blank line before this one

Done.

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

https://codereview.appspot.com/10777044/diff/20001/state/state.go#newcode72
state/state.go:72: const containerNameSnippet = "(/[a-z]+)"
On 2013/07/11 21:34:15, dimitern wrote:
> i'd really like if you use this in the line above, so we'll have to update
only
> one const if the name pattern needs to change.

Done.

https://codereview.appspot.com/10777044/diff/20001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10777044/diff/20001/state/unit.go#newcode137
state/unit.go:137: // Series returns the series.
On 2013/07/11 21:34:15, dimitern wrote:
> s/series./deployed charm's series./ ?

Done.

[dimitern, 2013-07-12 07:40:31]

Thanks, LGTM with a suggestion.

https://codereview.appspot.com/10777044/diff/29001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/10777044/diff/29001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:34: Doc:     "Machines are created in a clean state and
ready to have units deployed.",
if nothing else, please add a TODO to add --force-machine syntax examples, lest
we forget later, when the format is decided.

[fwereade, 2013-07-15 14:55:24]

Thanks for all your work on this. Given its pipeline position, LGTM, so long as
there's an immediate followup to change the spelling from 25/lxc to lxc:25.

[wallyworld, 2013-07-16 00:48:17]

On 2013/07/15 14:55:24, fwereade wrote:
> Thanks for all your work on this. Given its pipeline position, LGTM, so long
as
> there's an immediate followup to change the spelling from 25/lxc to lxc:25.

I've made the change is this branch and will carry it forward.

[fwereade, 2013-07-16 13:00:53]

On 2013/07/16 00:48:17, wallyworld wrote:
> On 2013/07/15 14:55:24, fwereade wrote:
> > Thanks for all your work on this. Given its pipeline position, LGTM, so long
> as
> > there's an immediate followup to change the spelling from 25/lxc to lxc:25.
> 
> I've made the change is this branch and will carry it forward.

Perfect, tyvm.