Issue 6845120: worker/deployer: initial implementation
Descriptionworker/deployer: initial implementation
Please consider this as both a serious proposal, from a logic point of view,
and as a strawman intended to provoke discussion of the best way to get this
merged. Please note that, if we use a Deployer in place of a Machiner:
* the container package becomes redundant
* the machiner package becomes redundant
* Machine.WatchPrincipalUnits becomes redundant, and can be discarded to
make way for the implementation currently known as WatchPrincipalUnits2
...and all of this is great, but the best way to merge this in has caused
some contention in IRC, so I'd appreciate guidance here.
https://code.launchpad.net/~fwereade/juju-core/deployer-worker/+merge/137359
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 51
Patch Set 2 : worker/deployer: initial implementation #
Total comments: 3
Patch Set 3 : worker/deployer: initial implementation #
MessagesTotal messages: 11
Please take a look.
Sign in to reply to this message.
I haven't fully reviewed this, but I like the direction things are going.
Sign in to reply to this message.
So far LGTM, some points discussed on irc. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:12: type Context interface { Context is a very generic name and somehow doesn't match the method names. As discussed on irc I prefer "Workspace". https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:14: // DeployerName identifies the agent represented by the context. This may have to be changed as discussed too. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:70: // changed responds to a reported change in the named unit. This change may // changed notifies the deployer that the named unit has changed. ???
Sign in to reply to this message.
https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:12: type Context interface { I was unhappy with Context too, but I couldn't think of something better. It's not *that* bad though, it's not an unadorned Context, it's deployer.Context, and that makes a big difference. I don't think Workspace is a good name for an interface, for a struct yes, but not for an interface.
Sign in to reply to this message.
looks good. some minor comments below. https://codereview.appspot.com/6845120/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6845120/diff/1/state/unit.go#newcode212 state/unit.go:212: func (u *Unit) PublicAddress() (string, error) { i think we should probably change this so it returns (string, bool) to match the style of DeployerName. that's another CL though. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:20: DeployUnit(name, initialPassword string) error s/name/unitName/ ? https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:70: // changed responds to a reported change in the named unit. This change may On 2012/12/05 10:22:39, TheMue wrote: > // changed notifies the deployer that the named unit has changed. > > ??? +1 or perhaps: // changed notifies the deployer that the named unit has changed. // A dead unit will be removed from the state when // we know that the deployer context is responsible for it. ? https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:136: if err := unit.EnsureDead(); err != nil { this is perhaps a little misleading, as AFAICS, the unit should *always* be dead by the time we get here. i'd be more happy with a simple that Life==Dead, assuming that's the case. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go File worker/deployer/simple.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:40: var _ Context = &SimpleContext{} i'd put this in the tests. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:63: func (ctx *SimpleContext) DeployUnit(name, initialPassword string) (err error) { i'm presuming none of this has changed from the container package code. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:129: re, err := regexp.Compile(pattern) i think i'd lose the deployer name from the pattern; then we can use use MustCompile to make a global variable, and check the deployer name as one of the submatches. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:156: svcName := "jujud-" + entityName + "-x-" + ctx.DeployerName() "-x-" ?
Sign in to reply to this message.
Thanks for the comments; I'd appreciate a little more discussion on some points before I go ahead with changes. Thoughts, anyone? https://codereview.appspot.com/6845120/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6845120/diff/1/state/unit.go#newcode212 state/unit.go:212: func (u *Unit) PublicAddress() (string, error) { On 2012/12/05 16:36:07, rog wrote: > i think we should probably change this so it returns (string, bool) to match the > style of DeployerName. that's another CL though. +1, I'll try to remember it some time :). https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:12: type Context interface { On 2012/12/05 10:49:33, aram wrote: > I was unhappy with Context too, but I couldn't think of something better. It's > not *that* bad though, it's not an unadorned Context, it's deployer.Context, and > that makes a big difference. I don't think Workspace is a good name for an > interface, for a struct yes, but not for an interface. Yeah -- I'm not especially pleased with either, but I'm not quite so sure Workspace is actually a win now... https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:14: // DeployerName identifies the agent represented by the context. On 2012/12/05 10:22:39, TheMue wrote: > This may have to be changed as discussed too. ...partly because of this -- I feel that the identity, at least, belongs with the context. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:20: DeployUnit(name, initialPassword string) error On 2012/12/05 16:36:07, rog wrote: > s/name/unitName/ ? Good idea, thanks. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:70: // changed responds to a reported change in the named unit. This change may On 2012/12/05 16:36:07, rog wrote: > On 2012/12/05 10:22:39, TheMue wrote: > > // changed notifies the deployer that the named unit has changed. > > > > ??? > > +1 > > or perhaps: > > // changed notifies the deployer that the named unit has changed. > // A dead unit will be removed from the state when > // we know that the deployer context is responsible for it. > > ? I'm keen to see the wording improve, but I don't think either suggestion is accurate, and I don't see in what way my doc is inaccurate. Thoughts? https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:136: if err := unit.EnsureDead(); err != nil { On 2012/12/05 16:36:07, rog wrote: > this is perhaps a little misleading, as AFAICS, the unit should *always* be dead > by the time we get here. > > i'd be more happy with a simple that Life==Dead, assuming that's the case. IMO we're also responsible for removing Dying units that aren't yet deployed (unless you think we ought to deploy them and wait for them to kill themselves? seems somewhat wasteful to me, but maybe easier to understand?). https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go File worker/deployer/simple.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:40: var _ Context = &SimpleContext{} On 2012/12/05 16:36:07, rog wrote: > i'd put this in the tests. I thought we made a habit of asserting not-necessarily-obvious interfaces right next to the struct definitions primarily as documentation, eg in the charm package. Not sure we've been consistent with it, but it still seems nicer to me. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:63: func (ctx *SimpleContext) DeployUnit(name, initialPassword string) (err error) { On 2012/12/05 16:36:07, rog wrote: > i'm presuming none of this has changed from the container package code. It's largely similar but not the same: I dropped the tools param and changed the --debug handling into something just as dumb but slightly more compact. Don't think there were any significant ones though. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:129: re, err := regexp.Compile(pattern) On 2012/12/05 16:36:07, rog wrote: > i think i'd lose the deployer name from the pattern; then we can use use > MustCompile to make a global variable, and check the deployer name as one of the > submatches. Sounds reasonable, cheers. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:156: svcName := "jujud-" + entityName + "-x-" + ctx.DeployerName() On 2012/12/05 16:36:07, rog wrote: > "-x-" ? Intended connotation is "out of" or "from", each of which STM to be a much suckier thing to put :). Better suggestions?
Sign in to reply to this message.
LGTM with a few further thoughts below. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:70: // changed responds to a reported change in the named unit. This change may On 2012/12/05 23:00:01, fwereade wrote: > On 2012/12/05 16:36:07, rog wrote: > > On 2012/12/05 10:22:39, TheMue wrote: > > > // changed notifies the deployer that the named unit has changed. > > > > > > ??? > > > > +1 > > > > or perhaps: > > > > // changed notifies the deployer that the named unit has changed. > > // A dead unit will be removed from the state when > > // we know that the deployer context is responsible for it. > > > > ? > > I'm keen to see the wording improve, but I don't think either suggestion is > accurate, and I don't see in what way my doc is inaccurate. Thoughts? the original comment seemed a little like explaining the whole implementation in the doc comment, which seemed a little hard to read. perhaps go with TheMue's suggestion and move the rest of the comments to appropriate places in the code. i needed to stare quite hard at the below logic before i was happy that it worked correctly, and perhaps a comment or two might help. i don't mind much though, if you're attached to the original text. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:136: if err := unit.EnsureDead(); err != nil { On 2012/12/05 23:00:01, fwereade wrote: > On 2012/12/05 16:36:07, rog wrote: > > this is perhaps a little misleading, as AFAICS, the unit should *always* be > dead > > by the time we get here. > > > > i'd be more happy with a simple that Life==Dead, assuming that's the case. > > IMO we're also responsible for removing Dying units that aren't yet deployed > (unless you think we ought to deploy them and wait for them to kill themselves? > seems somewhat wasteful to me, but maybe easier to understand?). good point. and i see you've got a test specifically for that case. perhaps a comment somewhere that mentions this case might be helpful. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go File worker/deployer/simple.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:40: var _ Context = &SimpleContext{} On 2012/12/05 23:00:01, fwereade wrote: > On 2012/12/05 16:36:07, rog wrote: > > i'd put this in the tests. > > I thought we made a habit of asserting not-necessarily-obvious interfaces right > next to the struct definitions primarily as documentation, eg in the charm > package. Not sure we've been consistent with it, but it still seems nicer to me. you're right - i'd remembered we did it in the tests (that's the standard in the Go core) but that's not true. slightly better to do: var _ Context = (*SimpleContext)(nil) though, as it's marginally cheaper at runtime, and that's the idiom we use elsewhere. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:156: svcName := "jujud-" + entityName + "-x-" + ctx.DeployerName() On 2012/12/05 23:00:01, fwereade wrote: > On 2012/12/05 16:36:07, rog wrote: > > "-x-" ? > > Intended connotation is "out of" or "from", each of which STM to be a much > suckier thing to put :). Better suggestions? i'd be tempted to put the deployer name first, so it reads from parent to child. how about using a dot as a separator? something like this perhaps: jujud-machine-1.unit-wordpress-0 jujud-unit-wordpress-0.unit-logging-1 jujud-unit-wordpress-0.unit-another-subordinate-2
Sign in to reply to this message.
Good stuff, thanks! https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:12: type Context interface { I think this should be something like a deployer.Manager. Context doesn't do its importance justice.. it's not just "adding context" to the deployer. This is the thing that knows *how to deploy* at all. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:14: // DeployerName identifies the agent represented by the context. On 2012/12/05 23:00:01, fwereade wrote: > On 2012/12/05 10:22:39, TheMue wrote: > > This may have to be changed as discussed too. > > ...partly because of this -- I feel that the identity, at least, belongs with > the context. That's true, but given that this doesn't look like a context, a different explanation is that this method is misplaced. There's no intrinsic reason why a deployer.Manager has to care about the agent name, or the entity name it represents. This doesn't mean that a given implementation of a deployer.Manager (e.g. SimpleManager) wants to know about that, but that's a parameter to its constructor function, rather than a method on this interface. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:87: if deployerName, ok := unit.DeployerName(); ok { Surprised to see unit.DeployerName here. Haven't reached the method on the review yet, though. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:136: if err := unit.EnsureDead(); err != nil { On 2012/12/05 23:00:01, fwereade wrote: > On 2012/12/05 16:36:07, rog wrote: > > this is perhaps a little misleading, as AFAICS, the unit should *always* be > dead > > by the time we get here. > > > > i'd be more happy with a simple that Life==Dead, assuming that's the case. > > IMO we're also responsible for removing Dying units that aren't yet deployed > (unless you think we ought to deploy them and wait for them to kill themselves? > seems somewhat wasteful to me, but maybe easier to understand?). Good catch. A comment as recommended by Roger would be useful. It would also be good to have a double-check here ensuring that d.deployed[unit.Name()] is indeed false. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:146: func (d *Deployer) loop(w *state.UnitsWatcher) error { As a general comment about this file, the logic here looks remarkably readable. Thanks. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer_test.go File worker/deployer/deployer_test.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer_test.g... worker/deployer/deployer_test.go:50: c.Assert(err, IsNil) Can this be reasonably split into independent tests verifying individual aspects? This looks like another version of the long-tailed table with tons of state that then become really hard to maintain and understand over time. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer_test.g... worker/deployer/deployer_test.go:112: func (s *DeployerSuite) TestDeployRecallRemoveSubordinates(c *C) { Same. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go File worker/deployer/simple.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:20: type SimpleContext struct { SimpleManager, if you take the suggestion. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:59: func (ctx *SimpleContext) DeployerName() string { And this could go away. We already know it internally, and then Deployer can know its own name too. I'd also preserve the term "entity name" in there somehow, so a casual reader can find the way home. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:156: svcName := "jujud-" + entityName + "-x-" + ctx.DeployerName() On 2012/12/06 07:49:39, rog wrote: > On 2012/12/05 23:00:01, fwereade wrote: > > On 2012/12/05 16:36:07, rog wrote: > > > "-x-" ? > > > > Intended connotation is "out of" or "from", each of which STM to be a much > > suckier thing to put :). Better suggestions? > > i'd be tempted to put the deployer name first, so it reads from parent to child. > how about using a dot as a separator? > something like this perhaps: > > jujud-machine-1.unit-wordpress-0 > jujud-unit-wordpress-0.unit-logging-1 > jujud-unit-wordpress-0.unit-another-subordinate-2 Or ":", as in "jujud-unit-wordpress-0:unit-logging-1" "-x-" is a valid part of a name. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple_test.go File worker/deployer/simple_test.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple_test.go#... worker/deployer/simple_test.go:32: func (s *SimpleContextSuite) TestInstallRemove(c *C) { Same about long-winded testing.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:12: type Context interface { On 2012/12/06 17:11:36, niemeyer wrote: > I think this should be something like a deployer.Manager. Context doesn't do its > importance justice.. it's not just "adding context" to the deployer. This is the > thing that knows *how to deploy* at all. I guess -- my personal definition of Context is maybe a little looser than that (consider a graphics context -- I think the parallels are fairly compelling) but I'm not seriously attached to the name :). (You know what I'd really like? the current Deployer to be deployer.Task, and the interface itself to be Deployer... but that's a bit of a break with the other workers) https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:14: // DeployerName identifies the agent represented by the context. On 2012/12/06 17:11:36, niemeyer wrote: > On 2012/12/05 23:00:01, fwereade wrote: > > On 2012/12/05 10:22:39, TheMue wrote: > > > This may have to be changed as discussed too. > > > > ...partly because of this -- I feel that the identity, at least, belongs with > > the context. > > That's true, but given that this doesn't look like a context, a different > explanation is that this method is misplaced. There's no intrinsic reason why a > deployer.Manager has to care about the agent name, or the entity name it > represents. This doesn't mean that a given implementation of a deployer.Manager > (e.g. SimpleManager) wants to know about that, but that's a parameter to its > constructor function, rather than a method on this interface. Yeah, fair enough :). Done. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:70: // changed responds to a reported change in the named unit. This change may On 2012/12/06 07:49:39, rog wrote: > the original comment seemed a little like explaining the whole implementation in > the doc comment, which seemed a little hard to read. perhaps go with TheMue's > suggestion and move the rest of the comments to appropriate places in the code. > i needed to stare quite hard at the below logic before i was happy that it > worked correctly, and perhaps a comment or two might help. i don't mind much > though, if you're attached to the original text. Heh, I moved the comments up there because it seemed neater at the time. You're probably right, though -- thanks. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:87: if deployerName, ok := unit.DeployerName(); ok { On 2012/12/06 17:11:36, niemeyer wrote: > Surprised to see unit.DeployerName here. Haven't reached the method on the > review yet, though. I'm nervous about any prospect of ambiguity re who's actually responsible for a given unit. This was also the thinking behind the DeployerName method on Context (as was). https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:136: if err := unit.EnsureDead(); err != nil { On 2012/12/06 17:11:36, niemeyer wrote: > On 2012/12/05 23:00:01, fwereade wrote: > > On 2012/12/05 16:36:07, rog wrote: > > > this is perhaps a little misleading, as AFAICS, the unit should *always* be > > dead > > > by the time we get here. > > > > > > i'd be more happy with a simple that Life==Dead, assuming that's the case. > > > > IMO we're also responsible for removing Dying units that aren't yet deployed > > (unless you think we ought to deploy them and wait for them to kill > themselves? > > seems somewhat wasteful to me, but maybe easier to understand?). > > Good catch. A comment as recommended by Roger would be useful. It would also be > good to have a double-check here ensuring that d.deployed[unit.Name()] is indeed > false. SGTM. Done. https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer.go#new... worker/deployer/deployer.go:146: func (d *Deployer) loop(w *state.UnitsWatcher) error { On 2012/12/06 17:11:36, niemeyer wrote: > As a general comment about this file, the logic here looks remarkably readable. > Thanks. Cheers -- I was very pleased with it :). https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer_test.go File worker/deployer/deployer_test.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer_test.g... worker/deployer/deployer_test.go:50: c.Assert(err, IsNil) On 2012/12/06 17:11:36, niemeyer wrote: > Can this be reasonably split into independent tests verifying individual > aspects? This looks like another version of the long-tailed table with tons of > state that then become really hard to maintain and understand over time. I'll see what I can do :). https://codereview.appspot.com/6845120/diff/1/worker/deployer/deployer_test.g... worker/deployer/deployer_test.go:112: func (s *DeployerSuite) TestDeployRecallRemoveSubordinates(c *C) { On 2012/12/06 17:11:36, niemeyer wrote: > Same. As above, split into two. Going further STM to be unhelpful re comprehension... https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go File worker/deployer/simple.go (right): https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:20: type SimpleContext struct { On 2012/12/06 17:11:36, niemeyer wrote: > SimpleManager, if you take the suggestion. Yeah -- I'm fine with Manager for now :). https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:40: var _ Context = &SimpleContext{} On 2012/12/06 07:49:39, rog wrote: > On 2012/12/05 23:00:01, fwereade wrote: > > On 2012/12/05 16:36:07, rog wrote: > > > i'd put this in the tests. > > > > I thought we made a habit of asserting not-necessarily-obvious interfaces > right > > next to the struct definitions primarily as documentation, eg in the charm > > package. Not sure we've been consistent with it, but it still seems nicer to > me. > > you're right - i'd remembered we did it in the tests (that's the standard in the > Go core) but that's not true. > > slightly better to do: > > var _ Context = (*SimpleContext)(nil) > > though, as it's marginally cheaper at runtime, and that's the idiom we use > elsewhere. Done. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:59: func (ctx *SimpleContext) DeployerName() string { On 2012/12/06 17:11:36, niemeyer wrote: > And this could go away. We already know it internally, and then Deployer can > know its own name too. I'd also preserve the term "entity name" in there > somehow, so a casual reader can find the way home. Done. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:129: re, err := regexp.Compile(pattern) On 2012/12/05 23:00:01, fwereade wrote: > On 2012/12/05 16:36:07, rog wrote: > > i think i'd lose the deployer name from the pattern; then we can use use > > MustCompile to make a global variable, and check the deployer name as one of > the > > submatches. > > Sounds reasonable, cheers. Done. https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:156: svcName := "jujud-" + entityName + "-x-" + ctx.DeployerName() On 2012/12/06 07:49:39, rog wrote: > On 2012/12/05 23:00:01, fwereade wrote: > > On 2012/12/05 16:36:07, rog wrote: > > > "-x-" ? > > > > Intended connotation is "out of" or "from", each of which STM to be a much > > suckier thing to put :). Better suggestions? > > i'd be tempted to put the deployer name first, so it reads from parent to child. > how about using a dot as a separator? > something like this perhaps: > > jujud-machine-1.unit-wordpress-0 > jujud-unit-wordpress-0.unit-logging-1 > jujud-unit-wordpress-0.unit-another-subordinate-2 I'm -1 on this ordering myself -- I feel the "important" bit of the name (from a human-readability/least-surprise perspective) is what agent it's running, and should therefore come first. Can't decide whether or not I like dots, but fine to switch, I guess -- it's not like -x- is a work of staggering genius or anything ;). https://codereview.appspot.com/6845120/diff/1/worker/deployer/simple.go#newco... worker/deployer/simple.go:156: svcName := "jujud-" + entityName + "-x-" + ctx.DeployerName() On 2012/12/06 17:11:36, niemeyer wrote: > On 2012/12/06 07:49:39, rog wrote: > > On 2012/12/05 23:00:01, fwereade wrote: > > > On 2012/12/05 16:36:07, rog wrote: > > > > "-x-" ? > > > > > > Intended connotation is "out of" or "from", each of which STM to be a much > > > suckier thing to put :). Better suggestions? > > > > i'd be tempted to put the deployer name first, so it reads from parent to > child. > > how about using a dot as a separator? > > something like this perhaps: > > > > jujud-machine-1.unit-wordpress-0 > > jujud-unit-wordpress-0.unit-logging-1 > > jujud-unit-wordpress-0.unit-another-subordinate-2 > > Or ":", as in "jujud-unit-wordpress-0:unit-logging-1" Hmm, I'm still not really keen on putting the deployer name before the agent name, but it's growing on me. I'll give it a try. > "-x-" is a valid part of a name. D'oh. Good point.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6845120/diff/13001/worker/deployer/deployer.go File worker/deployer/deployer.go (right): https://codereview.appspot.com/6845120/diff/13001/worker/deployer/deployer.go... worker/deployer/deployer.go:119: log.Printf("worker/deployer: deploying agent for unit %q", unit) "deploying unit", I suppose. It's not just the agent. https://codereview.appspot.com/6845120/diff/13001/worker/deployer/deployer.go... worker/deployer/deployer.go:140: log.Printf("worker/deployer: recalling agent for unit %q", unitName) "recalling unit" as well. https://codereview.appspot.com/6845120/diff/13001/worker/deployer/deployer.go... worker/deployer/deployer.go:154: panic("must not remove an Alive unit") Nice checks here and above. Thanks.
Sign in to reply to this message.
*** Submitted:
worker/deployer: initial implementation
Please consider this as both a serious proposal, from a logic point of view,
and as a strawman intended to provoke discussion of the best way to get this
merged. Please note that, if we use a Deployer in place of a Machiner:
* the container package becomes redundant
* the machiner package becomes redundant
* Machine.WatchPrincipalUnits becomes redundant, and can be discarded to
make way for the implementation currently known as WatchPrincipalUnits2
...and all of this is great, but the best way to merge this in has caused
some contention in IRC, so I'd appreciate guidance here.
R=aram, TheMue, rog, niemeyer
CC=
https://codereview.appspot.com/6845120
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
