Implement a add-machine command

Implement a add-machine command

add-machine will add a new machine record to the environment which results
in a new empty machine being spun up. Optional constraints can be supplied,
which are merged with the existing environ constraints and assigned to the
machine. The machine is created with JobHostUnits running on it, so it is
ready to accept new units.

https://code.launchpad.net/~wallyworld/juju-core/create-machine-command/+merge/165518

(do not edit description out of merge proposal)

[wallyworld, 2013-05-24 02:19:23]

Please take a look.

[thumper, 2013-05-27 02:34:26]

https://codereview.appspot.com/9662048/diff/1/cmd/juju/createmachine.go
File cmd/juju/createmachine.go (right):

https://codereview.appspot.com/9662048/diff/1/cmd/juju/createmachine.go#newco...
cmd/juju/createmachine.go:25: Args:    "<machine> ...",
Well, doesn't really take args yet.

https://codereview.appspot.com/9662048/diff/1/cmd/juju/createmachine.go#newco...
cmd/juju/createmachine.go:52: m, err :=
conn.State.AddMachine(conf.DefaultSeries(), &c.Constraints, state.JobHostUnits)
I do think that there should be a method called AddMachineWithConstraints.

This means all the tests below wouldn't have to change.

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

https://codereview.appspot.com/9662048/diff/1/state/state.go#newcode203
state/state.go:203: func (st *State) AddMachine(series string, extraCons
*constraints.Value, jobs ...MachineJob) (m *Machine, err error) {
I seriously think there should be two methods here.

Yes, the main method called from the command would be AddMachineWithConstraints,
but to make the tests readable, and not have people go "what is this nil param?"
at every call site is worth the extra method.

https://codereview.appspot.com/9662048/diff/1/state/state.go#newcode254
state/state.go:254: func (st *State) addMachine(series string, extraCons
*constraints.Value, instanceId InstanceId,
This looks fine to me.

[wallyworld, 2013-05-27 03:02:42]

Please take a look.

https://codereview.appspot.com/9662048/diff/1/cmd/juju/createmachine.go
File cmd/juju/createmachine.go (right):

https://codereview.appspot.com/9662048/diff/1/cmd/juju/createmachine.go#newco...
cmd/juju/createmachine.go:25: Args:    "<machine> ...",
On 2013/05/27 02:34:26, thumper wrote:
> Well, doesn't really take args yet.

Done.

https://codereview.appspot.com/9662048/diff/1/cmd/juju/createmachine.go#newco...
cmd/juju/createmachine.go:52: m, err :=
conn.State.AddMachine(conf.DefaultSeries(), &c.Constraints, state.JobHostUnits)
On 2013/05/27 02:34:26, thumper wrote:
> I do think that there should be a method called AddMachineWithConstraints.
> 
> This means all the tests below wouldn't have to change.

Ok, I added a AddMachineWithConstraints method

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

https://codereview.appspot.com/9662048/diff/1/state/state.go#newcode203
state/state.go:203: func (st *State) AddMachine(series string, extraCons
*constraints.Value, jobs ...MachineJob) (m *Machine, err error) {
On 2013/05/27 02:34:26, thumper wrote:
> I seriously think there should be two methods here.
> 
> Yes, the main method called from the command would be
AddMachineWithConstraints,
> but to make the tests readable, and not have people go "what is this nil
param?"
> at every call site is worth the extra method.

Done.

[thumper, 2013-05-27 04:19:03]

LGTM after fixing the two call sites you forgot.

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/destroymachine_test.go
File cmd/juju/destroymachine_test.go (right):

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/destroymachine_test...
cmd/juju/destroymachine_test.go:67: m1, err := s.State.AddMachine("series", nil,
state.JobManageEnviron)
Missed this one.

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/ssh_test.go
File cmd/juju/ssh_test.go (right):

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/ssh_test.go#newcode123
cmd/juju/ssh_test.go:123: m, err := s.State.AddMachine("series", nil,
state.JobHostUnits)
And here.

https://codereview.appspot.com/9662048/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9662048/diff/6001/state/state.go#newcode210
state/state.go:210: func (st *State) AddMachineWithConstraints(series string,
extraCons *constraints.Value, jobs ...MachineJob) (m *Machine, err error) {
Yep, I'm happier with this now.

[wallyworld, 2013-05-27 04:20:32]

On 2013/05/27 04:19:03, thumper wrote:
> LGTM after fixing the two call sites you forgot.
> 
>
https://codereview.appspot.com/9662048/diff/6001/cmd/juju/destroymachine_test.go
> File cmd/juju/destroymachine_test.go (right):
> 
>
https://codereview.appspot.com/9662048/diff/6001/cmd/juju/destroymachine_test...
> cmd/juju/destroymachine_test.go:67: m1, err := s.State.AddMachine("series",
nil,
> state.JobManageEnviron)
> Missed this one.
> 
> https://codereview.appspot.com/9662048/diff/6001/cmd/juju/ssh_test.go
> File cmd/juju/ssh_test.go (right):
> 
>
https://codereview.appspot.com/9662048/diff/6001/cmd/juju/ssh_test.go#newcode123
> cmd/juju/ssh_test.go:123: m, err := s.State.AddMachine("series", nil,
> state.JobHostUnits)
> And here.
> 
> https://codereview.appspot.com/9662048/diff/6001/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/9662048/diff/6001/state/state.go#newcode210
> state/state.go:210: func (st *State) AddMachineWithConstraints(series string,
> extraCons *constraints.Value, jobs ...MachineJob) (m *Machine, err error) {
> Yep, I'm happier with this now.

I found the callsites and pushed, but this %#!$@%!# code review site doesn't
notice the changes.

[fwereade, 2013-05-27 19:47:35]

I'm not sure about a few of these. Ping me when you're on please, I should be
around tonight.

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/createmachine.go
File cmd/juju/createmachine.go (right):

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/createmachine.go#ne...
cmd/juju/createmachine.go:24: Name:    "create-machine",
add-machine, please, for consistency with the other commands.

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/createmachine.go#ne...
cmd/juju/createmachine.go:51: m, err :=
conn.State.AddMachineWithConstraints(conf.DefaultSeries(), &c.Constraints,
state.JobHostUnits)
series is at least as important as constraints... please add --series in a
followup, if not in this CL.

https://codereview.appspot.com/9662048/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9662048/diff/6001/state/state.go#newcode261
state/state.go:261: func (st *State) addMachine(series string, extraCons
*constraints.Value, instanceId InstanceId,
Why's this a pointer? If you're specifying constraints, you're specifying
constraints; empty constraints are perfectly validm and will work fine with the
env fallbacks.

https://codereview.appspot.com/9662048/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/9662048/diff/6001/state/state_test.go#newcode161
state/state_test.go:161: }()
Do we really need all this? Doesn't StateSuite give us a clean slate?

https://codereview.appspot.com/9662048/diff/6001/state/state_test.go#newcode177
state/state_test.go:177: check(m0, "0", "series", oneJob)
Why wrap this lot up in a func?

[wallyworld, 2013-05-28 02:08:31]

Please take a look.

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/createmachine.go
File cmd/juju/createmachine.go (right):

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/createmachine.go#ne...
cmd/juju/createmachine.go:24: Name:    "create-machine",
On 2013/05/27 19:47:35, fwereade wrote:
> add-machine, please, for consistency with the other commands.

Done.

https://codereview.appspot.com/9662048/diff/6001/cmd/juju/createmachine.go#ne...
cmd/juju/createmachine.go:51: m, err :=
conn.State.AddMachineWithConstraints(conf.DefaultSeries(), &c.Constraints,
state.JobHostUnits)
On 2013/05/27 19:47:35, fwereade wrote:
> series is at least as important as constraints... please add --series in a
> followup, if not in this CL.

Done.

https://codereview.appspot.com/9662048/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9662048/diff/6001/state/state.go#newcode261
state/state.go:261: func (st *State) addMachine(series string, extraCons
*constraints.Value, instanceId InstanceId,
On 2013/05/27 19:47:35, fwereade wrote:
> Why's this a pointer? If you're specifying constraints, you're specifying
> constraints; empty constraints are perfectly validm and will work fine with
the
> env fallbacks.

I just have an aversion to passing structs by value. Ingrained habit. Perhaps
it's ok in Go? Plus I found it easier to type "nil" than "constraints.Value{}"
in those call sites which require it.

I've changed it.

https://codereview.appspot.com/9662048/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/9662048/diff/6001/state/state_test.go#newcode161
state/state_test.go:161: }()
On 2013/05/27 19:47:35, fwereade wrote:
> Do we really need all this? Doesn't StateSuite give us a clean slate?

I removed it and the tests still pass, so looks like we don't need it. I didn't
see any code to do the cleanup so thought I needed to add it.

https://codereview.appspot.com/9662048/diff/6001/state/state_test.go#newcode177
state/state_test.go:177: check(m0, "0", "series", oneJob)
On 2013/05/27 19:47:35, fwereade wrote:
> Why wrap this lot up in a func?

Left over remnant from old code.

Fixed.

[thumper, 2013-05-29 04:08:58]

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

https://codereview.appspot.com/9662048/diff/16001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:34: f.StringVar(&c.Series, "series", "", "the Ubuntu
series")
Hmm... this isn't always going the be the Ubuntu series.  Is this duplicated at
the other call sites that allow specifying --series?  I'm wondering if at some
stage in the future we fix it as we will hopefully have both other linux series
and potentially win8 etc.

https://codereview.appspot.com/9662048/diff/16001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9662048/diff/16001/state/state.go#newcode204
state/state.go:204: return st.addMachine(series, constraints.Value{}, "", "",
jobs)
weren't you going to change this to be a pointer?

[wallyworld, 2013-05-29 05:43:54]

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

https://codereview.appspot.com/9662048/diff/16001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:34: f.StringVar(&c.Series, "series", "", "the Ubuntu
series")
On 2013/05/29 04:08:58, thumper wrote:
> Hmm... this isn't always going the be the Ubuntu series.  Is this duplicated
at
> the other call sites that allow specifying --series?  I'm wondering if at some
> stage in the future we fix it as we will hopefully have both other linux
series
> and potentially win8 etc.

Yeah, I couldn't think of a good description for it. What does series even mean
in a non-Ubuntu context. Suggestions welcome. Perhaps "the operating system
version"? But then we would need to change the arg name too? Does --version make
sense? The current majority of users know what series means. Decisions,
decisions.

https://codereview.appspot.com/9662048/diff/16001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9662048/diff/16001/state/state.go#newcode204
state/state.go:204: return st.addMachine(series, constraints.Value{}, "", "",
jobs)
On 2013/05/29 04:08:58, thumper wrote:
> weren't you going to change this to be a pointer?

It was a pointer but William asked me to change it to a non-pointer.

[fwereade, 2013-05-30 13:51:24]

LGTM; I'll leave InjectMachine up to your judgment.

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

https://codereview.appspot.com/9662048/diff/16001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:34: f.StringVar(&c.Series, "series", "", "the Ubuntu
series")
On 2013/05/29 05:43:54, wallyworld wrote:
> On 2013/05/29 04:08:58, thumper wrote:
> > Hmm... this isn't always going the be the Ubuntu series.  Is this duplicated
> at
> > the other call sites that allow specifying --series?  I'm wondering if at
some
> > stage in the future we fix it as we will hopefully have both other linux
> series
> > and potentially win8 etc.
> 
> Yeah, I couldn't think of a good description for it. What does series even
mean
> in a non-Ubuntu context. Suggestions welcome. Perhaps "the operating system
> version"? But then we would need to change the arg name too? Does --version
make
> sense? The current majority of users know what series means. Decisions,
> decisions.

Call it the charm series perhaps? That's the source of the whole concept, after
all. I don't love the name either, but it's pretty well ingrained and I'd prefer
to stay consistent than to start messing around with alternative terminology at
this stage.

https://codereview.appspot.com/9662048/diff/16001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:52: }
Put this code inside the `series == ""` block, that's the only time we need the
env conf.

https://codereview.appspot.com/9662048/diff/16001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9662048/diff/16001/state/state.go#newcode204
state/state.go:204: return st.addMachine(series, constraints.Value{}, "", "",
jobs)
On 2013/05/29 05:43:54, wallyworld wrote:
> On 2013/05/29 04:08:58, thumper wrote:
> > weren't you going to change this to be a pointer?
> 
> It was a pointer but William asked me to change it to a non-pointer.

The general rationale is that constraints are never optional: they may be empty,
but they're always present, and it's semantically off-kilter to have two
distinct ways of specifying "no constraints". I don't consider the runtime cost
of passing around the odd larger-than-necessary in-memory struct to be
significant, and since they serialize down to ~nothing I'm not bothered by the
impact of sending them down the wire either. Sane?

https://codereview.appspot.com/9662048/diff/16001/state/state.go#newcode221
state/state.go:221: return st.addMachine(series, constraints.Value{},
instanceId, BootstrapNonce, jobs)
This is ~fine so long as it's always called *after* setting env constraints...
which, in practice, it is. But it'd probably be nicer if we passed them in
explicitly in jujud bootstrap-state...

[wallyworld, 2013-05-31 00:15:01]

*** Submitted:

Implement a add-machine command

add-machine will add a new machine record to the environment which results
in a new empty machine being spun up. Optional constraints can be supplied,
which are merged with the existing environ constraints and assigned to the
machine. The machine is created with JobHostUnits running on it, so it is
ready to accept new units.

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

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

https://codereview.appspot.com/9662048/diff/16001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:34: f.StringVar(&c.Series, "series", "", "the Ubuntu
series")
On 2013/05/30 13:51:24, fwereade wrote:
> On 2013/05/29 05:43:54, wallyworld wrote:
> > On 2013/05/29 04:08:58, thumper wrote:
> > > Hmm... this isn't always going the be the Ubuntu series.  Is this
duplicated
> > at
> > > the other call sites that allow specifying --series?  I'm wondering if at
> some
> > > stage in the future we fix it as we will hopefully have both other linux
> > series
> > > and potentially win8 etc.
> > 
> > Yeah, I couldn't think of a good description for it. What does series even
> mean
> > in a non-Ubuntu context. Suggestions welcome. Perhaps "the operating system
> > version"? But then we would need to change the arg name too? Does --version
> make
> > sense? The current majority of users know what series means. Decisions,
> > decisions.
> 
> Call it the charm series perhaps? That's the source of the whole concept,
after
> all. I don't love the name either, but it's pretty well ingrained and I'd
prefer
> to stay consistent than to start messing around with alternative terminology
at
> this stage.

I'll rename the description to "the charm series" as suggested.

https://codereview.appspot.com/9662048/diff/16001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:52: }
On 2013/05/30 13:51:24, fwereade wrote:
> Put this code inside the `series == ""` block, that's the only time we need
the
> env conf.

Done.

https://codereview.appspot.com/9662048/diff/16001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9662048/diff/16001/state/state.go#newcode221
state/state.go:221: return st.addMachine(series, constraints.Value{},
instanceId, BootstrapNonce, jobs)
On 2013/05/30 13:51:24, fwereade wrote:
> This is ~fine so long as it's always called *after* setting env constraints...
> which, in practice, it is. But it'd probably be nicer if we passed them in
> explicitly in jujud bootstrap-state...

Done.