Implement deploy -force-machine=2

Implement deploy -force-machine=2

Allows specification of manual machine for a service's first unit when 
deploying.

https://code.launchpad.net/~hazmat/juju-core/deploy-to/+merge/157690

(do not edit description out of merge proposal)

[hazmat, 2013-04-08 16:22:32]

Please take a look.

[dimitern, 2013-04-08 16:59:41]

LGTM modulo the below.
So is it deploy -to or deploy --to=blah ?

https://codereview.appspot.com/8520043/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8520043/diff/1/juju/conn.go#newcode193
juju/conn.go:193: // Use string for deploy-to machine to avoid ambiguity around
machine 0.
put some blanks between blocks of comment/field please

https://codereview.appspot.com/8520043/diff/1/juju/conn.go#newcode314
juju/conn.go:314: // TODO lp:1101139 (units are not assigned transactionally)
I think they are - AssignToNewMachine was merged recently

https://codereview.appspot.com/8520043/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/8520043/diff/1/juju/conn_test.go#newcode393
juju/conn_test.go:393: c.Assert(err, ErrorMatches, `cannot add multiple units of
"testriak" to a single machine`)
s/of /of service /

[hazmat, 2013-04-08 17:11:55]

format is deploy --to. Added some additional deploy cli tests. i have a concern
that its possible to end up failing on deploy --to with a service deployed with
no units since there isn't a transaction block encompassing both parts, but its
not clear transaction boundaries are composable with mgo txns.

Thanks for the review.

https://codereview.appspot.com/8520043/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8520043/diff/1/juju/conn.go#newcode193
juju/conn.go:193: // Use string for deploy-to machine to avoid ambiguity around
machine 0.
On 2013/04/08 16:59:42, dimitern wrote:
> put some blanks between blocks of comment/field please

I'm a little unclear what you mean. The comment matches the rest of the ones in
the struct. Additional spacing between MachineId and string triggers gofmt.

https://codereview.appspot.com/8520043/diff/1/juju/conn.go#newcode314
juju/conn.go:314: // TODO lp:1101139 (units are not assigned transactionally)
On 2013/04/08 16:59:42, dimitern wrote:
> I think they are - AssignToNewMachine was merged recently

yeah.. bug is marked fix released. removed comment.

https://codereview.appspot.com/8520043/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/8520043/diff/1/juju/conn_test.go#newcode393
juju/conn_test.go:393: c.Assert(err, ErrorMatches, `cannot add multiple units of
"testriak" to a single machine`)
On 2013/04/08 16:59:42, dimitern wrote:
> s/of /of service /

done

[hazmat, 2013-04-08 17:14:04]

Please take a look.

[niemeyer, 2013-04-08 17:23:07]

-1 from me, for reasons debated a non-trivial number of times. This should be
addressed by constraints.

Whatever William decides, though.

[niemeyer, 2013-04-08 17:23:59]

NOT LGTM, for reasons debated a non-trivial number of times. This should be
addressed by constraints.

Whatever William decides, though.

[hazmat, 2013-04-08 17:44:24]

Constraints at a service level would prevent add-unit from working
properly.  Are you referencing a unit level constraint independent of the
service? fwiw I did do a preimpl call with william.



On Mon, Apr 8, 2013 at 1:23 PM, <n13m3y3r@gmail.com> wrote:

> -1 from me, for reasons debated a non-trivial number of times. This
> should be addressed by constraints.
>
> Whatever William decides, though.
>
>
https://codereview.appspot.**com/8520043/<https://codereview.appspot.com/8520...
>

[niemeyer, 2013-04-08 17:56:49]

I'm taking this conversation to the list.

[hazmat, 2013-04-08 17:59:36]

per request from william, renaming s/--to/--force-machine to better reflect the
constraints bypass.

[hazmat, 2013-04-08 18:01:07]

Please take a look.

[fwereade, 2013-04-08 18:07:35]

On 2013/04/08 17:23:59, niemeyer wrote:
> NOT LGTM, for reasons debated a non-trivial number of times. This should be
> addressed by constraints.

There will certainly be constraints that address the use cases enabled by this
in a more sophisticated way.

> Whatever William decides, though.

This capability solves real problems and makes real users content; it makes
heavy use of existing code; and it's actually orthogonal to the
constraints/assignment mechanism (so I don't fear serious pollution from the
approach).

IMO the biggest problem is that by implementing it we demonstrate implicit
approval of the approach, and that misrepresents our intent. So, rather than
"--to", please go with "--force-machine".

The users will be happy despite the extra typing, and the option can be clearly
documented to ignore constraints: the eventual impact will be, I hope, that
people graduate from --force-machine as juju becomes more sophisticated.

[hazmat, 2013-04-08 18:13:28]

Please take a look.

[fwereade, 2013-04-09 11:36:04]

LGTM, just a couple of tweaks

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

https://codereview.appspot.com/8520043/diff/5002/cmd/juju/deploy.go#newcode92
cmd/juju/deploy.go:92: }
Add a quick check for an invalid machine id (if it's non-empty). Use
state.IsMachineId() IIRC

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

https://codereview.appspot.com/8520043/diff/5002/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:172: func (s *DeploySuite) TestDeployTo(c *C) {
TestForceMachine

https://codereview.appspot.com/8520043/diff/5002/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8520043/diff/5002/juju/conn.go#newcode194
juju/conn.go:194: MachineId string
ForceMachineId

[hazmat, 2013-04-11 13:23:23]

All done. thanks.

On 2013/04/09 11:36:04, fwereade wrote:
> LGTM, just a couple of tweaks
> 
> https://codereview.appspot.com/8520043/diff/5002/cmd/juju/deploy.go
> File cmd/juju/deploy.go (right):
> 
> https://codereview.appspot.com/8520043/diff/5002/cmd/juju/deploy.go#newcode92
> cmd/juju/deploy.go:92: }
> Add a quick check for an invalid machine id (if it's non-empty). Use
> state.IsMachineId() IIRC
> 
> https://codereview.appspot.com/8520043/diff/5002/cmd/juju/deploy_test.go
> File cmd/juju/deploy_test.go (right):
> 
>
https://codereview.appspot.com/8520043/diff/5002/cmd/juju/deploy_test.go#newc...
> cmd/juju/deploy_test.go:172: func (s *DeploySuite) TestDeployTo(c *C) {
> TestForceMachine
> 
> https://codereview.appspot.com/8520043/diff/5002/juju/conn.go
> File juju/conn.go (right):
> 
> https://codereview.appspot.com/8520043/diff/5002/juju/conn.go#newcode194
> juju/conn.go:194: MachineId string
> ForceMachineId

[hazmat, 2013-04-11 13:26:54]

Please take a look.

[rog, 2013-04-11 17:47:51]

LGTM with the below issues addressed.

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

https://codereview.appspot.com/8520043/diff/28001/cmd/juju/deploy.go#newcode62
cmd/juju/deploy.go:62: f.StringVar(&c.MachineId, "force-machine", "", "Machine
to deploy initial unit, bypasses constraints")
s/unit,/units;/
?

or perhaps we should check that num-units is 1 when using --force-machine?

ah! i see you do check later. please add a check here.
or else just leave all the error checking for the later call.

https://codereview.appspot.com/8520043/diff/28001/cmd/juju/deploy.go#newcode96
cmd/juju/deploy.go:96: return fmt.Errorf("Invalid machine id %q", c.MachineId)
s/Invalid/invalid/
might as well be consistent

https://codereview.appspot.com/8520043/diff/28001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8520043/diff/28001/juju/conn.go#newcode187
juju/conn.go:187: NumUnits    int
// NumUnits specifies the number of units to deploy.
// If ForceMachineId is true, it must be 1.

https://codereview.appspot.com/8520043/diff/28001/juju/conn.go#newcode193
juju/conn.go:193: // Use string for deploy-to machine to avoid ambiguity around
machine 0.
this comment isn't very useful.
the whole struct could do with more docs actually, but for the time being:
// ForceMachineId forces the unit of the service to be
// deployed on the machine with the given id.

https://codereview.appspot.com/8520043/diff/28001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/8520043/diff/28001/state/state.go#newcode935
state/state.go:935: func (st *State) AssignUnitToMachine(u *Unit, mid string)
(err error) {
i'm not sure this method is justified.
we already have Unit.AssignToMachine.
the IsPrincipal check should go there, and we can lose this function.

[hazmat, 2013-04-11 18:03:28]

thanks for the review.

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

https://codereview.appspot.com/8520043/diff/28001/cmd/juju/deploy.go#newcode62
cmd/juju/deploy.go:62: f.StringVar(&c.MachineId, "force-machine", "", "Machine
to deploy initial unit, bypasses constraints")
On 2013/04/11 17:47:51, rog wrote:
> s/unit,/units;/
> ?
> 
> or perhaps we should check that num-units is 1 when using --force-machine?
> 
> ah! i see you do check later. please add a check here.
> or else just leave all the error checking for the later call.

i'll can add an additional check here. i feel the other check should be in place
as its an internal api that should validate its args.

https://codereview.appspot.com/8520043/diff/28001/cmd/juju/deploy.go#newcode96
cmd/juju/deploy.go:96: return fmt.Errorf("Invalid machine id %q", c.MachineId)
On 2013/04/11 17:47:51, rog wrote:
> s/Invalid/invalid/
> might as well be consistent

ok.

https://codereview.appspot.com/8520043/diff/28001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8520043/diff/28001/juju/conn.go#newcode193
juju/conn.go:193: // Use string for deploy-to machine to avoid ambiguity around
machine 0.
On 2013/04/11 17:47:51, rog wrote:
> this comment isn't very useful.
> the whole struct could do with more docs actually, but for the time being:
> // ForceMachineId forces the unit of the service to be
> // deployed on the machine with the given id.

ok.

https://codereview.appspot.com/8520043/diff/28001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/8520043/diff/28001/state/state.go#newcode935
state/state.go:935: func (st *State) AssignUnitToMachine(u *Unit, mid string)
(err error) {
On 2013/04/11 17:47:51, rog wrote:
> i'm not sure this method is justified.
> we already have Unit.AssignToMachine.
> the IsPrincipal check should go there, and we can lose this function.

sounds reasonable.

[jameinel, 2013-04-15 06:02:22]

I feel like the conversation on the list ended up with this being acceptable, if
only a transition step to something more constraint based.

Is there something still blocking it from landing?

[hazmat, 2013-04-16 16:53:00]

*** Submitted:

Implement deploy -force-machine=2

Allows specification of manual machine for a service's first unit when 
deploying.

R=dimitern, niemeyer, fwereade, rog, jameinel
CC=
https://codereview.appspot.com/8520043

[dave_cheney.net, 2013-04-17 00:24:08]

Hazmat, you just checked in some bzr turds.