|
|
|
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)
Patch Set 1 #
Total comments: 6
Patch Set 2 : Implement deploy -to #Patch Set 3 : Implement deploy -to #Patch Set 4 : Implement deploy -to #
Total comments: 3
Patch Set 5 : Implement deploy -to #Patch Set 6 : Implement deploy -force-machine=2 #
Total comments: 9
Patch Set 7 : Implement deploy -force-machine=2 #Patch Set 8 : Implement deploy -force-machine=2 #Patch Set 9 : Implement deploy -force-machine=2 #
Total messages: 20
|
hazmat
Please take a look.
|
12 years, 9 months ago (2013年04月08日 16:22:32 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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 /
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
Please take a look.
-1 from me, for reasons debated a non-trivial number of times. This should be addressed by constraints. Whatever William decides, though.
NOT LGTM, for reasons debated a non-trivial number of times. This should be addressed by constraints. Whatever William decides, though.
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... >
I'm taking this conversation to the list.
per request from william, renaming s/--to/--force-machine to better reflect the constraints bypass.
Please take a look.
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.
Please take a look.
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
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
Please take a look.
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.
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.
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?
*** 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