Issue 7425044: uniter, state: handle upgrades better (Closed)
Descriptionuniter, state: handle upgrades better
The big change here is uniter/filter is now
responsible for changing the unit's charm URL,
so that it can stop and start the config
watcher on the same goroutine; if we don't
sync these operations, then a unit which
sets a charm URL such that no references to
the relevent service settings exist, will
die because its config watcher dies.
This isn't a real concern yet, but it will
be in a follow-up branch.
https://code.launchpad.net/~dimitern/juju-core/010-uniter-handle-config-upgrades/+merge/151069
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 39
Patch Set 2 : uniter, state: handle upgrades better #Patch Set 3 : uniter, state: handle upgrades better #
Total comments: 29
Patch Set 4 : uniter, state: handle upgrades better #Patch Set 5 : uniter, state: handle upgrades better #
Total comments: 32
Patch Set 6 : uniter, state: handle upgrades better #
Total comments: 8
Patch Set 7 : uniter, state: handle upgrades better #Patch Set 8 : uniter, state: handle upgrades better #
Total comments: 13
Patch Set 9 : uniter, state: handle upgrades better #Patch Set 10 : uniter, state: handle upgrades better #
MessagesTotal messages: 17
LGTM, only some minor comments. https://codereview.appspot.com/7425044/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/7425044/diff/1/state/unit.go#newcode120 state/unit.go:120: cfg, err := charm.Config().Validate(nil) Not happy with expressions like Validate(nil). Prefer at least a speaking constant with value nil here, if no explicit value is possible. https://codereview.appspot.com/7425044/diff/1/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/7425044/diff/1/state/watcher.go#newcode1013 state/watcher.go:1013: func (u *Unit) WatchServiceConfig() *ConfigWatcher { Yep, the name is better. Would you please add the already before missing comment? https://codereview.appspot.com/7425044/diff/1/state/watcher.go#newcode1014 state/watcher.go:1014: // TODO: Will use a better way to get the key in a follow-up. Better TODO(initials) to see who is addressed here or who made this statement. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode132 worker/uniter/filter.go:132: func (f *filter) WantUpgradeEvent(mustForce bool) { Why is the comment gone? https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode225 worker/uniter/filter.go:225: // TODO: configw may be nil, so panic here - how can we get around it? See above. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go File worker/uniter/filter_test.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go#ne... worker/uniter/filter_test.go:190: // TODO: is this needed here? See above.
Sign in to reply to this message.
On 2013/03/01 10:31:07, TheMue wrote: > LGTM, only some minor comments. > > https://codereview.appspot.com/7425044/diff/1/state/unit.go > File state/unit.go (right): > > https://codereview.appspot.com/7425044/diff/1/state/unit.go#newcode120 > state/unit.go:120: cfg, err := charm.Config().Validate(nil) > Not happy with expressions like Validate(nil). Prefer at least a speaking > constant with value nil here, if no explicit value is possible. > > https://codereview.appspot.com/7425044/diff/1/state/watcher.go > File state/watcher.go (right): > > https://codereview.appspot.com/7425044/diff/1/state/watcher.go#newcode1013 > state/watcher.go:1013: func (u *Unit) WatchServiceConfig() *ConfigWatcher { > Yep, the name is better. Would you please add the already before missing > comment? > > https://codereview.appspot.com/7425044/diff/1/state/watcher.go#newcode1014 > state/watcher.go:1014: // TODO: Will use a better way to get the key in a > follow-up. > Better TODO(initials) to see who is addressed here or who made this statement. > > https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go > File worker/uniter/filter.go (right): > > https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode132 > worker/uniter/filter.go:132: func (f *filter) WantUpgradeEvent(mustForce bool) { > Why is the comment gone? > > https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode225 > worker/uniter/filter.go:225: // TODO: configw may be nil, so panic here - how > can we get around it? > See above. > > https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go > File worker/uniter/filter_test.go (right): > > https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go#ne... > worker/uniter/filter_test.go:190: // TODO: is this needed here? > See above. Thanks for the review! Please, note this is still very much WIP and it'll change slightly as it progresses. I proposed it for a discussion with William on how to tackle what's left to do. I'll take all your suggestions into account once I propose it officially.
Sign in to reply to this message.
On 2013/03/01 10:40:57, dimitern wrote: > Thanks for the review! Please, note this is still very much WIP and it'll change > slightly as it progresses. I proposed it for a discussion with William on how to > tackle what's left to do. I'll take all your suggestions into account once I > propose it officially. Hehe, yep, I've seen that it is WIP. I only made it my habit long years ago to even do drafts like I'm working on the final. Otherwise often draft parts are forgotten or not changed. Only a private experience, so never mind. *smile*
Sign in to reply to this message.
Coming along very nicely. We need to beef up the filter tests, and be a bit careful about the failures that will become possible in the followup, but the heart of it looks very solid to me. Thanks. https://codereview.appspot.com/7425044/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/7425044/diff/1/state/unit.go#newcode120 state/unit.go:120: cfg, err := charm.Config().Validate(nil) This bit is a little weird, and would probably benefit from a comment. (I know it's what we were already doing...) https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go File worker/uniter/filter.go (left): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#oldcode136 worker/uniter/filter.go:136: func (f *filter) WantUpgradeEvent(url *charm.URL, mustForce bool) { // WantUpgradeEvent controls whether the filter will generate upgrade events for unforced service charm changes. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#oldcode145 worker/uniter/filter.go:145: func (f *filter) WantAllRelationsEvents() { // SetCharm notifies the filter that the unit is running a new charm. It causes the unit's charm URL to be set in state, and the following changes to the filter's behaviour: * Upgrade events will only be generated for charms different to that supplied * A fresh event will be generated for every relation known to the service * A fresh configuration event will be generated, and subsequent events will only be sent in response to changes in the version of the service's settings that is specific to that charm. ...ok, that isn't quite right, because we don't have per-charm service settings yet, but I think it's better to make the intent clear here and now rather than have to remember to update these docs when we fix service settings. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode186 worker/uniter/filter.go:186: var configw *state.ConfigWatcher var configChanges <-chan *state.Settings https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode188 worker/uniter/filter.go:188: configw = f.unit.WatchServiceConfig() configChanges = configw.Changes() https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode226 worker/uniter/filter.go:226: case _, ok = <-configw.Changes(): case _, ok = <-configChanges https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode260 worker/uniter/filter.go:260: // We need to restart the config watcher after setting the charm. ...because service config settings are distinct for different service charms. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode267 worker/uniter/filter.go:267: configw = f.unit.WatchServiceConfig() configChanges = configw.Changes() https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode269 worker/uniter/filter.go:269: // Restart the relations watcher. ...in order to generate fresh events for every relation, so that previously-ignored events (ie those not applicable to the previous charm) can be handled. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go File worker/uniter/filter_test.go (left): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go#ol... worker/uniter/filter_test.go:186: assertNoChange() I think these tests could use a bit of beefing up -- making the charm-fiddling independent of the force-fiddling is great, but we ought to test the two independently. We should also add some explicit tests for config/discardConfig handling in the context of units starting with/without a charm URL. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go File worker/uniter/filter_test.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go#ne... worker/uniter/filter_test.go:336: // TODO: remove if safe We definitely need to test this behaviour, as we do with the config stuff that is also hit by the SetCharm changes, but I think it's best to do it in an independent test https://codereview.appspot.com/7425044/diff/1/worker/uniter/modes.go File worker/uniter/modes.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/modes.go#newcode234 worker/uniter/modes.go:234: u.f.SetCharm(url) Nope, we can drop all the SetCharms in this file. https://codereview.appspot.com/7425044/diff/1/worker/uniter/uniter.go File worker/uniter/uniter.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/uniter.go#newcode209 worker/uniter/uniter.go:209: u.f.SetCharm(curl) Sorry long comment: This needs to happen *before* we write local state referencing the charm... and, actually, we need to be sure it's *happened* before we write local state, so SetCharm should probably only return when that charm has actually been set. After that we can do a u.unit.Refresh() to pick up the new charm URL for use in subsequent runHooks. Please note that the scheme I'm proposing is still unavoidably racy: it's always possible that the service's charm will be reset to something new before any unit starts to use the new settings, and thus that SetCharm could fail (assuming the beefier charm-settings-existence check in the followup). However, this is OK: so long as we detect this situation and barf *before* writing local state, we'll pick up a new, sane charm when the worker is restarted. https://codereview.appspot.com/7425044/diff/1/worker/uniter/uniter.go#newcode210 worker/uniter/uniter.go:210: if err := u.unit.SetCharmURL(curl); err != nil { We shouldn't SetCharmURL here if we're doing it in the filter as well. Just refresh once you're sure the SetCharm has stuck.
Sign in to reply to this message.
Now all tests pass, except the filter tests. Need more work there anyway. But I need to understand what to test and when with the new behavior. https://codereview.appspot.com/7425044/diff/1/state/unit.go File state/unit.go (right): https://codereview.appspot.com/7425044/diff/1/state/unit.go#newcode120 state/unit.go:120: cfg, err := charm.Config().Validate(nil) On 2013/03/01 11:59:56, fwereade wrote: > This bit is a little weird, and would probably benefit from a comment. (I know > it's what we were already doing...) Added a comment. https://codereview.appspot.com/7425044/diff/1/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/7425044/diff/1/state/watcher.go#newcode1013 state/watcher.go:1013: func (u *Unit) WatchServiceConfig() *ConfigWatcher { On 2013/03/01 10:31:07, TheMue wrote: > Yep, the name is better. Would you please add the already before missing > comment? Added doc comment. https://codereview.appspot.com/7425044/diff/1/state/watcher.go#newcode1014 state/watcher.go:1014: // TODO: Will use a better way to get the key in a follow-up. On 2013/03/01 10:31:07, TheMue wrote: > Better TODO(initials) to see who is addressed here or who made this statement. Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go File worker/uniter/filter.go (left): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#oldcode136 worker/uniter/filter.go:136: func (f *filter) WantUpgradeEvent(url *charm.URL, mustForce bool) { On 2013/03/01 11:59:56, fwereade wrote: > // WantUpgradeEvent controls whether the filter will generate upgrade events for > unforced service charm changes. Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#oldcode145 worker/uniter/filter.go:145: func (f *filter) WantAllRelationsEvents() { On 2013/03/01 11:59:56, fwereade wrote: > // SetCharm notifies the filter that the unit is running a new charm. It causes > the unit's charm URL to be set in state, and the following changes to the > filter's behaviour: > > * Upgrade events will only be generated for charms different to that supplied > * A fresh event will be generated for every relation known to the service > * A fresh configuration event will be generated, and subsequent events will > only be sent in response to changes in the version of the service's settings > that is specific to that charm. > > ...ok, that isn't quite right, because we don't have per-charm service settings > yet, but I think it's better to make the intent clear here and now rather than > have to remember to update these docs when we fix service settings. Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode132 worker/uniter/filter.go:132: func (f *filter) WantUpgradeEvent(mustForce bool) { On 2013/03/01 10:31:07, TheMue wrote: > Why is the comment gone? Because we changed the behavior and I wasn't sure the exact wording. Added now. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode186 worker/uniter/filter.go:186: var configw *state.ConfigWatcher On 2013/03/01 11:59:56, fwereade wrote: > var configChanges <-chan *state.Settings Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode188 worker/uniter/filter.go:188: configw = f.unit.WatchServiceConfig() On 2013/03/01 11:59:56, fwereade wrote: > configChanges = configw.Changes() Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode225 worker/uniter/filter.go:225: // TODO: configw may be nil, so panic here - how can we get around it? On 2013/03/01 10:31:07, TheMue wrote: > See above. Gone now. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode226 worker/uniter/filter.go:226: case _, ok = <-configw.Changes(): On 2013/03/01 11:59:56, fwereade wrote: > case _, ok = <-configChanges Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode260 worker/uniter/filter.go:260: // We need to restart the config watcher after setting the charm. On 2013/03/01 11:59:56, fwereade wrote: > ...because service config settings are distinct for different service charms. Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode267 worker/uniter/filter.go:267: configw = f.unit.WatchServiceConfig() On 2013/03/01 11:59:56, fwereade wrote: > configChanges = configw.Changes() Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter.go#newcode269 worker/uniter/filter.go:269: // Restart the relations watcher. On 2013/03/01 11:59:56, fwereade wrote: > ...in order to generate fresh events for every relation, so that > previously-ignored events (ie those not applicable to the previous charm) can be > handled. Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go File worker/uniter/filter_test.go (left): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go#ol... worker/uniter/filter_test.go:186: assertNoChange() On 2013/03/01 11:59:56, fwereade wrote: > I think these tests could use a bit of beefing up -- making the charm-fiddling > independent of the force-fiddling is great, but we ought to test the two > independently. > > We should also add some explicit tests for config/discardConfig handling in the > context of units starting with/without a charm URL. I'll add better tests, but I need to discuss this I think a bit more. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go File worker/uniter/filter_test.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go#ne... worker/uniter/filter_test.go:190: // TODO: is this needed here? On 2013/03/01 10:31:07, TheMue wrote: > See above. Gone now. https://codereview.appspot.com/7425044/diff/1/worker/uniter/filter_test.go#ne... worker/uniter/filter_test.go:336: // TODO: remove if safe On 2013/03/01 11:59:56, fwereade wrote: > We definitely need to test this behaviour, as we do with the config stuff that > is also hit by the SetCharm changes, but I think it's best to do it in an > independent test I'll do a separate test. https://codereview.appspot.com/7425044/diff/1/worker/uniter/modes.go File worker/uniter/modes.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/modes.go#newcode234 worker/uniter/modes.go:234: u.f.SetCharm(url) On 2013/03/01 11:59:56, fwereade wrote: > Nope, we can drop all the SetCharms in this file. Done. https://codereview.appspot.com/7425044/diff/1/worker/uniter/uniter.go File worker/uniter/uniter.go (right): https://codereview.appspot.com/7425044/diff/1/worker/uniter/uniter.go#newcode209 worker/uniter/uniter.go:209: u.f.SetCharm(curl) On 2013/03/01 11:59:56, fwereade wrote: > Sorry long comment: > > This needs to happen *before* we write local state referencing the charm... and, > actually, we need to be sure it's *happened* before we write local state, so > SetCharm should probably only return when that charm has actually been set. > After that we can do a u.unit.Refresh() to pick up the new charm URL for use in > subsequent runHooks. > > Please note that the scheme I'm proposing is still unavoidably racy: it's always > possible that the service's charm will be reset to something new before any unit > starts to use the new settings, and thus that SetCharm could fail (assuming the > beefier charm-settings-existence check in the followup). However, this is OK: so > long as we detect this situation and barf *before* writing local state, we'll > pick up a new, sane charm when the worker is restarted. I think I understood that correctly, take a look at SetCharm's doc comment and body. https://codereview.appspot.com/7425044/diff/1/worker/uniter/uniter.go#newcode210 worker/uniter/uniter.go:210: if err := u.unit.SetCharmURL(curl); err != nil { On 2013/03/01 11:59:56, fwereade wrote: > We shouldn't SetCharmURL here if we're doing it in the filter as well. Just > refresh once you're sure the SetCharm has stuck. Done.
Sign in to reply to this message.
Only minor quibbles, but we should probably split up the filter tests a bit more... https://codereview.appspot.com/7425044/diff/13001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/7425044/diff/13001/state/unit.go#newcode108 state/unit.go:108: func (u *Unit) ServiceConfig() (map[string]interface{}, error) { This doesn't have tests yet..? https://codereview.appspot.com/7425044/diff/13001/state/unit.go#newcode120 state/unit.go:120: // We pass nil here, because we need just the config defaults. // Build a dictionary containing charm defaults, and // overwrite any values that have actually been set. https://codereview.appspot.com/7425044/diff/13001/state/unit.go#newcode131 state/unit.go:131: return m, nil for k, v := range settings.Map() { cfg[k] = v } return cfg, nil ? https://codereview.appspot.com/7425044/diff/13001/worker/uniter/context_test.go File worker/uniter/context_test.go (right): https://codereview.appspot.com/7425044/diff/13001/worker/uniter/context_test.... worker/uniter/context_test.go:619: err = s.unit.SetCharmURL(sch.URL()) // Note: the unit must always have a charm URL set, because this happens as part of the installation process (that happens before the initial install hook) https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:39: setCharm chan *charm.URL // Request to upgrade to a charm URL Maybe move setCharm away from the want* block and give it its own comment -- something like: // setCharm is used to request that the unit's charm URL be set to a new value. This must be done on the filter's goroutine, so that config watches can be stopped and restarted pointing to the new charm URL. If we don't stop the watch before the (potentially) last reference to that settings document is removed, we'll see spurious errors (and even in the best case, we risk getting notifications for the wrong settings version). https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:48: charmChanged chan error Yeah, put setCharm next to this. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:163: for { No need to loop here. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:172: case err := <-f.charmChanged: I don't think we need the actual error here - we can have a chan struct{} and return nil if we receive on it, or return f.tomb.Err() if the tomb's dying. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:223: f.upgradeRequested.url = curl s/upgradeRequested/upgradeFrom/ (throughout) https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:303: f.charmChanged <- err This gets a bit cleaner if you make the change suggested in SetCharm https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (right): https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:250: c.Assert(err, IsNil) Check no config event here? https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:255: c.Assert(ok, Equals, true) Not necessary, this stuff is tested in state. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:323: // Filter died after the error, so restart it. Should probably be checking that this actually killed the filter.
Sign in to reply to this message.
So some tests still fail, not sure why, but most of the other non-filter stuff is done. I'd appreciate a look at it. https://codereview.appspot.com/7425044/diff/13001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/7425044/diff/13001/state/unit.go#newcode108 state/unit.go:108: func (u *Unit) ServiceConfig() (map[string]interface{}, error) { On 2013/03/06 18:01:05, fwereade wrote: > This doesn't have tests yet..? Done. https://codereview.appspot.com/7425044/diff/13001/state/unit.go#newcode120 state/unit.go:120: // We pass nil here, because we need just the config defaults. On 2013/03/06 18:01:05, fwereade wrote: > // Build a dictionary containing charm defaults, and > // overwrite any values that have actually been set. Done. https://codereview.appspot.com/7425044/diff/13001/state/unit.go#newcode131 state/unit.go:131: return m, nil On 2013/03/06 18:01:05, fwereade wrote: > for k, v := range settings.Map() { > cfg[k] = v > } > return cfg, nil > > ? Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/context_test.go File worker/uniter/context_test.go (right): https://codereview.appspot.com/7425044/diff/13001/worker/uniter/context_test.... worker/uniter/context_test.go:619: err = s.unit.SetCharmURL(sch.URL()) On 2013/03/06 18:01:05, fwereade wrote: > // Note: the unit must always have a charm URL set, because this happens as part > of the installation process (that happens before the initial install hook) Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:39: setCharm chan *charm.URL // Request to upgrade to a charm URL On 2013/03/06 18:01:05, fwereade wrote: > Maybe move setCharm away from the want* block and give it its own comment -- > something like: > > // setCharm is used to request that the unit's charm URL be set to a new value. > This must be done on the filter's goroutine, so that config watches can be > stopped and restarted pointing to the new charm URL. If we don't stop the watch > before the (potentially) last reference to that settings document is removed, > we'll see spurious errors (and even in the best case, we risk getting > notifications for the wrong settings version). Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:48: charmChanged chan error On 2013/03/06 18:01:05, fwereade wrote: > Yeah, put setCharm next to this. Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:163: for { On 2013/03/06 18:01:05, fwereade wrote: > No need to loop here. Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:172: case err := <-f.charmChanged: On 2013/03/06 18:01:05, fwereade wrote: > I don't think we need the actual error here - we can have a chan struct{} and > return nil if we receive on it, or return f.tomb.Err() if the tomb's dying. Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:223: f.upgradeRequested.url = curl On 2013/03/06 18:01:05, fwereade wrote: > s/upgradeRequested/upgradeFrom/ > > (throughout) Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter.go#new... worker/uniter/filter.go:303: f.charmChanged <- err On 2013/03/06 18:01:05, fwereade wrote: > This gets a bit cleaner if you make the change suggested in SetCharm Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (right): https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:192: // TODO: is this needed here? Removing this. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:220: // TODO: is this needed here? Removing this. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:227: // TODO: is this needed here? Removing this. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:250: c.Assert(err, IsNil) On 2013/03/06 18:01:05, fwereade wrote: > Check no config event here? Done. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:255: c.Assert(ok, Equals, true) On 2013/03/06 18:01:05, fwereade wrote: > Not necessary, this stuff is tested in state. Removed the whole block. https://codereview.appspot.com/7425044/diff/13001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:323: // Filter died after the error, so restart it. On 2013/03/06 18:01:05, fwereade wrote: > Should probably be checking that this actually killed the filter. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Very nearly there: just trivials, really, but I'd like to see the final result and the missing tests before you submit. https://codereview.appspot.com/7425044/diff/24001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/7425044/diff/24001/state/unit.go#newcode121 state/unit.go:121: settings, err := readSettings(u.st, "s#"+u.doc.Service) Add a TODO here similar to that in watcher.go https://codereview.appspot.com/7425044/diff/24001/state/unit_test.go File state/unit_test.go (right): https://codereview.appspot.com/7425044/diff/24001/state/unit_test.go#newcode47 state/unit_test.go:47: "foo": "bar", It's evil and wrong that we can do this in the first place, but that's out of scope. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:159: // * A fresh event will be generated for every relation s/event/relations event/ s/for/containing/ https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:160: // known to the service; s/known to// s/;/ is participating in;/ https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:268: if ok { I'm pretty sure the original behaviour should stand. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:303: watcher.Stop(configw, &f.tomb) I think it's clearer to explicitly configw.Stop(), and return the error if not nil; this also ensures we don't do another loop through the select with weird state. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:317: watcher.Stop(relationsw, &f.tomb) Ditto. If we got an error, we'll hit the loop again and *might* hit <-f.tomb.Dying()... or we might hit <-relationsw.Changes(), in which case boom splat panic because we MustErr it. Much clearer to just return the error immediately. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:402: log.Debugf("worker/uniter/filter: charm check skipped, no yet installed.") s/no yet/not yet/ https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (left): https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:240: assertChange := func() { If the last thing you do in assertChange is assertNoChange(), you don't have to keep calling it explicitly below. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:300: func (s *FilterSuite) TestRelationsEvents(c *C) { Oh, ok, there is a relations events test. Check that setting a new charm causes a fresh event with all relations (and, hmm, also that setting the same charm doesn't -- that should be a trivial check-and-abort in the <-f.setCharm branch of the main select). https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (right): https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:169: func (s *FilterSuite) TestUpgradeEvents(c *C) { ? https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:266: c.Assert(ok, Equals, false) Don't think this test is necessary, but as you like. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:274: _, ok = s.unit.CharmURL() Test the charm URL as well please here. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:319: c.Assert(err, IsNil) defer f.Stop() https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:344: f.Stop() Don't need this now. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:355: // TODO: add discard + nochange test Yep, do those ;p. Also check the relations events (but those each feel like separate test cases to me -- one for config events including discard and one for relations events -- then this one can just be TestSetCharmErrors).
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7425044/diff/24001/state/unit.go File state/unit.go (right): https://codereview.appspot.com/7425044/diff/24001/state/unit.go#newcode121 state/unit.go:121: settings, err := readSettings(u.st, "s#"+u.doc.Service) On 2013/03/14 10:53:54, fwereade wrote: > Add a TODO here similar to that in watcher.go Done. https://codereview.appspot.com/7425044/diff/24001/state/unit_test.go File state/unit_test.go (right): https://codereview.appspot.com/7425044/diff/24001/state/unit_test.go#newcode47 state/unit_test.go:47: "foo": "bar", On 2013/03/14 10:53:54, fwereade wrote: > It's evil and wrong that we can do this in the first place, but that's out of > scope. I agree! https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:159: // * A fresh event will be generated for every relation On 2013/03/14 10:53:54, fwereade wrote: > s/event/relations event/ > s/for/containing/ Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:160: // known to the service; On 2013/03/14 10:53:54, fwereade wrote: > s/known to// > s/;/ is participating in;/ Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:268: if ok { On 2013/03/14 10:53:54, fwereade wrote: > I'm pretty sure the original behaviour should stand. Yeah, at first I thought configw can be nil here, but since we have the configChanges set, it should be not nil. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:303: watcher.Stop(configw, &f.tomb) On 2013/03/14 10:53:54, fwereade wrote: > I think it's clearer to explicitly configw.Stop(), and return the error if not > nil; this also ensures we don't do another loop through the select with weird > state. Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:317: watcher.Stop(relationsw, &f.tomb) On 2013/03/14 10:53:54, fwereade wrote: > Ditto. If we got an error, we'll hit the loop again and *might* hit > <-f.tomb.Dying()... or we might hit <-relationsw.Changes(), in which case boom > splat panic because we MustErr it. Much clearer to just return the error > immediately. Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter.go#new... worker/uniter/filter.go:402: log.Debugf("worker/uniter/filter: charm check skipped, no yet installed.") On 2013/03/14 10:53:54, fwereade wrote: > s/no yet/not yet/ Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (left): https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:240: assertChange := func() { On 2013/03/14 10:53:54, fwereade wrote: > If the last thing you do in assertChange is assertNoChange(), you don't have to > keep calling it explicitly below. Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:300: func (s *FilterSuite) TestRelationsEvents(c *C) { On 2013/03/14 10:53:54, fwereade wrote: > Oh, ok, there is a relations events test. Check that setting a new charm causes > a fresh event with all relations (and, hmm, also that setting the same charm > doesn't -- that should be a trivial check-and-abort in the <-f.setCharm branch > of the main select). Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (right): https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:169: func (s *FilterSuite) TestUpgradeEvents(c *C) { On 2013/03/14 10:53:54, fwereade wrote: > ? A leftover. Removed. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:266: c.Assert(ok, Equals, false) On 2013/03/14 10:53:54, fwereade wrote: > Don't think this test is necessary, but as you like. Removed. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:274: _, ok = s.unit.CharmURL() On 2013/03/14 10:53:54, fwereade wrote: > Test the charm URL as well please here. Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:319: c.Assert(err, IsNil) On 2013/03/14 10:53:54, fwereade wrote: > defer f.Stop() Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:344: f.Stop() On 2013/03/14 10:53:54, fwereade wrote: > Don't need this now. Done. https://codereview.appspot.com/7425044/diff/24001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:355: // TODO: add discard + nochange test On 2013/03/14 10:53:54, fwereade wrote: > Yep, do those ;p. Also check the relations events (but those each feel like > separate test cases to me -- one for config events including discard and one for > relations events -- then this one can just be TestSetCharmErrors). Done.
Sign in to reply to this message.
LGTM, just minors (assuming the SetCharm move doesn't invalidate our assumptions) https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter.go#new... worker/uniter/filter.go:168: // TODO(dimitern): Once we have per-charm service settings in a coming I don't think this is necessary. The above is accurate even now; the motivation becomes real in a followup, but that doesn't need to go in the documentation. https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter.go#new... worker/uniter/filter.go:322: // the previous charm) can be handled. I think we can drop this comment, it just restates the SetCharm documentation. https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (left): https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:286: s.State.Sync() I wonder whether we should keep this sync, but one line up, in order to match the commented behaviour in the previous hunk (which looks right to me). https://codereview.appspot.com/7425044/diff/33001/worker/uniter/uniter.go File worker/uniter/uniter.go (right): https://codereview.appspot.com/7425044/diff/33001/worker/uniter/uniter.go#new... worker/uniter/uniter.go:209: if err := u.f.SetCharm(curl); err != nil { Wait, this is happening too late. We need to do it *before* we download the charm -- that's is how we grab a ref and ensure the settings will still be valid (should only make a difference once we land the followup).
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter.go#new... worker/uniter/filter.go:168: // TODO(dimitern): Once we have per-charm service settings in a coming On 2013/03/15 14:48:29, fwereade wrote: > I don't think this is necessary. The above is accurate even now; the motivation > becomes real in a followup, but that doesn't need to go in the documentation. Removed. https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter.go#new... worker/uniter/filter.go:322: // the previous charm) can be handled. On 2013/03/15 14:48:29, fwereade wrote: > I think we can drop this comment, it just restates the SetCharm documentation. Removed most of it, kept just // Restart the relations watcher. https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter_test.go File worker/uniter/filter_test.go (left): https://codereview.appspot.com/7425044/diff/33001/worker/uniter/filter_test.g... worker/uniter/filter_test.go:286: s.State.Sync() On 2013/03/15 14:48:29, fwereade wrote: > I wonder whether we should keep this sync, but one line up, in order to match > the commented behaviour in the previous hunk (which looks right to me). Done. https://codereview.appspot.com/7425044/diff/33001/worker/uniter/uniter.go File worker/uniter/uniter.go (right): https://codereview.appspot.com/7425044/diff/33001/worker/uniter/uniter.go#new... worker/uniter/uniter.go:209: if err := u.f.SetCharm(curl); err != nil { On 2013/03/15 14:48:29, fwereade wrote: > Wait, this is happening too late. We need to do it *before* we download the > charm -- that's is how we grab a ref and ensure the settings will still be valid > (should only make a difference once we land the followup). Moved just before "...fetching charm..." above.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM with a few minor points, but i'm afraid i haven't had time for a deep reading of the code yet. i think i'll trust fwereade on that though :-) https://codereview.appspot.com/7425044/diff/47001/state/unit_test.go File state/unit_test.go (right): https://codereview.appspot.com/7425044/diff/47001/state/unit_test.go#newcode53 state/unit_test.go:53: unit, err := s.service.AddUnit() c.Assert(err, IsNil) https://codereview.appspot.com/7425044/diff/47001/worker/uniter/context.go File worker/uniter/context.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/context.go#ne... worker/uniter/context.go:76: ctx.config, err = ctx.unit.ServiceConfig() should this be overwriting ctx.config on error? https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go#new... worker/uniter/filter.go:47: // a new value. This must be done on the filter's goroutine, so s/on/in/ ? https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go#new... worker/uniter/filter.go:219: // configw and relationsw can get restarted, so we need to bind s/bind their current/use their eventual/ ? https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go#new... worker/uniter/filter.go:302: } configw = nil ? https://codereview.appspot.com/7425044/diff/47001/worker/uniter/uniter.go File worker/uniter/uniter.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/uniter.go#new... worker/uniter/uniter.go:189: u.unit.Refresh() check error here.
Sign in to reply to this message.
couple of responses to rog's suggestions https://codereview.appspot.com/7425044/diff/47001/worker/uniter/context.go File worker/uniter/context.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/context.go#ne... worker/uniter/context.go:76: ctx.config, err = ctx.unit.ServiceConfig() On 2013/03/15 18:00:49, rog wrote: > should this be overwriting ctx.config on error? It's sure to be nil to begin with, and I think it's reasonable to trust that ServiceConfig returns nil on error. https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go#new... worker/uniter/filter.go:302: } On 2013/03/15 18:00:49, rog wrote: > configw = nil > ? if we did, we'd need to do configChanges as well; but in fact we always either error or restart the watcher in this branch of the select, so I don't think it's necessary. (a deferred second stop won't hurt, after all.)
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7425044/diff/47001/state/unit_test.go File state/unit_test.go (right): https://codereview.appspot.com/7425044/diff/47001/state/unit_test.go#newcode53 state/unit_test.go:53: unit, err := s.service.AddUnit() On 2013/03/15 18:00:49, rog wrote: > c.Assert(err, IsNil) Done. https://codereview.appspot.com/7425044/diff/47001/worker/uniter/context.go File worker/uniter/context.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/context.go#ne... worker/uniter/context.go:76: ctx.config, err = ctx.unit.ServiceConfig() On 2013/03/15 18:19:57, fwereade wrote: > On 2013/03/15 18:00:49, rog wrote: > > should this be overwriting ctx.config on error? > > It's sure to be nil to begin with, and I think it's reasonable to trust that > ServiceConfig returns nil on error. Well said, sir. https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go File worker/uniter/filter.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go#new... worker/uniter/filter.go:47: // a new value. This must be done on the filter's goroutine, so On 2013/03/15 18:00:49, rog wrote: > s/on/in/ ? Done. https://codereview.appspot.com/7425044/diff/47001/worker/uniter/filter.go#new... worker/uniter/filter.go:219: // configw and relationsw can get restarted, so we need to bind On 2013/03/15 18:00:49, rog wrote: > s/bind their current/use their eventual/ ? Done. https://codereview.appspot.com/7425044/diff/47001/worker/uniter/uniter.go File worker/uniter/uniter.go (right): https://codereview.appspot.com/7425044/diff/47001/worker/uniter/uniter.go#new... worker/uniter/uniter.go:189: u.unit.Refresh() On 2013/03/15 18:00:49, rog wrote: > check error here. Done.
Sign in to reply to this message.
*** Submitted: uniter, state: handle upgrades better The big change here is uniter/filter is now responsible for changing the unit's charm URL, so that it can stop and start the config watcher on the same goroutine; if we don't sync these operations, then a unit which sets a charm URL such that no references to the relevent service settings exist, will die because its config watcher dies. This isn't a real concern yet, but it will be in a follow-up branch. R=TheMue, fwereade, rog CC= https://codereview.appspot.com/7425044
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
