New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trigger upgrade-series-prepare hook. #8864
Changes from 4 commits
1c0e8c7
0ac2132
02ce53c
be3d968
c5f67d3
678e376
3d71446
9873531
025d928
095e39e
132b877
9e06d5e
e7d842a
afd2b0e
ce7f0c9
418f215
737461e
d31fefe
cd35901
93b5a64
92d223c
f7e1ab8
1ac1597
facc529
ff4ac60
2eda0a1
7ca5aae
27b7ec7
cef8aed
aa6dcff
e5c98bb
c622313
0d31aea
71f3958
44050d1
600d130
8cee315
fb9f7cd
550ecd7
3d830be
b99b95a
2ab106c
6c00b4e
a5c59a6
8cc0cae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -684,6 +684,44 @@ func (s *unitSuite) TestWatchActionNotificationsMoreResults(c *gc.C) { | |
c.Assert(err.Error(), gc.Equals, "expected 1 result, got 2") | ||
} | ||
|
||
func (s *unitSuite) TestWatchUpgradeSeriesNotifications(c *gc.C) { | ||
watcher, err := s.apiUnit.WatchUpgradeSeriesNotifications() | ||
c.Assert(err, jc.ErrorIsNil) | ||
|
||
notifyWatcher := watchertest.NewNotifyWatcherC(c, watcher, s.BackingState.StartSync) | ||
defer notifyWatcher.AssertStops() | ||
|
||
// Why do I have two initial events?!? | ||
_, ok := <-notifyWatcher.Watcher.Changes() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a bare channel operation is almost always dangerous, as it can block indefinitely (and then you only find out when the test suite is killed after 25min). However, getting 2 events is even worse, as it throws off everyone's expectations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent! Yes, the other watchers do consume an initial event on the server side - subsequently, on the client side, the additional event is not observed. Thank you @jameinel. |
||
notifyWatcher.Assert(ok, jc.IsTrue) | ||
|
||
notifyWatcher.AssertOneChange() | ||
|
||
err = s.wordpressMachine.CreateUpgradeSeriesLock() | ||
c.Assert(err, jc.ErrorIsNil) | ||
|
||
// Expect a notification that the document was created (i.e. a lock was placed) | ||
notifyWatcher.AssertOneChange() | ||
|
||
err = s.wordpressMachine.RemoveUpgradeSeriesLock() | ||
c.Assert(err, jc.ErrorIsNil) | ||
|
||
// A notification that the document was removed (i.e. the lock was released) | ||
notifyWatcher.AssertOneChange() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. other than the initial double event, this test looks good. |
||
} | ||
|
||
func (s *unitSuite) TestUpgradeSeriesStatus(c *gc.C) { | ||
// First we create the prepare lock | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth checking the pre-condition of s.apiUnit.UpgradeSeriesStatus() before we create the lock? So that we see the value changes to Started? It seems odd that you have this test, which says the status is Started, even though there isn't a call to SetUpgradeSeriesStatus(UnitStarted). If we want to just have "started" as the initial state, then I'd say we don't need a test that you can set the state to Started (drop TestSetUpgradeSeriesStatus, or change it to set the status to Completed) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It just so happens that the remote state starts in the Uniter: UnitNotStarted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but correct, lets test our assumption! We will put in a check of the pre-condition. I only note it in a comment. |
||
err := s.wordpressMachine.CreateUpgradeSeriesLock() | ||
c.Assert(err, jc.ErrorIsNil) | ||
|
||
// Then we check to see the status of our upgrade. We note that creating | ||
// the lock essentially kicks off an upgrade for the perspective of assigned units. | ||
status, err := s.apiUnit.UpgradeSeriesStatus() | ||
c.Assert(err, jc.ErrorIsNil) | ||
c.Assert(status, gc.Equals, "preparing") | ||
} | ||
|
||
func (s *unitSuite) TestApplicationNameAndTag(c *gc.C) { | ||
c.Assert(s.apiUnit.ApplicationName(), gc.Equals, s.wordpressApplication.Name()) | ||
c.Assert(s.apiUnit.ApplicationTag(), gc.Equals, s.wordpressApplication.Tag()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -984,6 +984,77 @@ func (u *UniterAPI) WatchActionNotifications(args params.Entities) (params.Strin | |
return common.WatchActionNotifications(args, canAccess, watchOne), nil | ||
} | ||
|
||
// WatchActionNotifications returns a StringsWatcher for observing the status of an Upgrade Series | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doc string: WatchActionNotifications creates a NotifyWatcher There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bah, the copy pasta monster - or creepy pasta - depending on which way you want to look at it. |
||
func (u *UniterAPI) WatchUpgradeSeriesNotifications(args params.Entities) (params.NotifyWatchResults, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we can put these into an api shared between the uniter and the new upgrade series worker? Both need to WatchUpgradeSeriesNotifications(), UpgradeSeriesStatus() etc. Entities deal in tags, so we can get different data if necessary for machines and units. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is being moved into a new API as per the above, therefor there is no reason to bump version here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ExternalReality I'm not sure what you are meaning "as per the above". If we are adding methods to the uniter API, then we need to bump the facade version, even if they are brought in through a common implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @howbazaar, the above comment by @hmlanigan. The code is being moved out of the UniterAPI entirely in the next PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally we have a facade per-worker, and while they might both embed a common.Methods, they would be separate grouping of methods. |
||
result := params.NotifyWatchResults{ | ||
Results: make([]params.NotifyWatchResult, len(args.Entities)), | ||
} | ||
canAccess, err := u.accessUnit() | ||
if err != nil { | ||
return params.NotifyWatchResults{}, err | ||
} | ||
for i, entity := range args.Entities { | ||
tag, err := names.ParseUnitTag(entity.Tag) | ||
if err != nil { | ||
result.Results[i].Error = common.ServerError(common.ErrPerm) | ||
continue | ||
} | ||
watcherId := "" | ||
if !canAccess(tag) { | ||
result.Results[i].Error = common.ServerError(common.ErrPerm) | ||
continue | ||
} | ||
unit, err := u.getUnit(tag) | ||
if err != nil { | ||
result.Results[i].Error = common.ServerError(err) | ||
continue | ||
} | ||
w, err := unit.WatchUpgradeSeriesNotifications() | ||
if err != nil { | ||
result.Results[i].Error = common.ServerError(err) | ||
continue | ||
} | ||
watcherId = u.resources.Register(w) | ||
result.Results[i].NotifyWatcherId = watcherId | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per your earlier question of "why do I have 2 events": There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (put another way, the call to Watch() should return the watcher identity, and the content of the first Next() result, and then on the client side, those should be split up into the appropriate responses. A NotifyWatcher doesn't have any actual content to return just an event that gets consumed on the server and regenerated on the client) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jameinel I'm unclear on what if anything should be added to the code here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You want to consume the first Next() event here, and "transmit" the value along with the return of Watch() which the transmit is a no-op for us. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @howbazaar, I swallowed the extra event on the server side, here I was swallowing both the servers initial event and the clients initial event on the client. My fault. |
||
} | ||
return result, nil | ||
} | ||
|
||
func (u *UniterAPI) UpgradeSeriesStatus(args params.Entities) (params.UpgradeSeriesStatusResults, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc comment needed. |
||
result := params.UpgradeSeriesStatusResults{ | ||
Results: make([]params.UpgradeSeriesStatusResult, len(args.Entities)), | ||
} | ||
canAccess, err := u.accessUnit() | ||
if err != nil { | ||
return params.UpgradeSeriesStatusResults{}, err | ||
} | ||
for i, entity := range args.Entities { | ||
tag, err := names.ParseUnitTag(entity.Tag) | ||
if err != nil { | ||
result.Results[i].Error = common.ServerError(common.ErrPerm) | ||
continue | ||
} | ||
|
||
if !canAccess(tag) { | ||
result.Results[i].Error = common.ServerError(common.ErrPerm) | ||
continue | ||
} | ||
unit, err := u.getUnit(tag) | ||
if err != nil { | ||
result.Results[i].Error = common.ServerError(err) | ||
continue | ||
} | ||
status, err := unit.UpgradeSeriesStatus() | ||
if err != nil { | ||
result.Results[i].Error = common.ServerError(err) | ||
continue | ||
} | ||
result.Results[i].Status = status | ||
} | ||
|
||
return result, nil | ||
} | ||
|
||
// ConfigSettings returns the complete set of application charm config | ||
// settings available to each given unit. | ||
func (u *UniterAPI) ConfigSettings(args params.Entities) (params.ConfigSettingsResults, error) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2203,6 +2203,11 @@ func (m *Machine) RemoveUpgradeSeriesLock() error { | |
return nil | ||
} | ||
|
||
// UpgradeSeriesStatus returns the status of a series upgrade. | ||
func (m *Machine) UpgradeSeriesStatus() (string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why pass back a string? the two status types are typed. |
||
return "preparing", nil | ||
} | ||
|
||
func createUpgradeSeriesLockTxnOps(machineDocId string, data *upgradeSeriesLock) []txn.Op { | ||
return []txn.Op{ | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -170,7 +170,7 @@ func (w *RemoteStateWatcher) CommandCompleted(completed string) { | |
} | ||
|
||
func (w *RemoteStateWatcher) setUp(unitTag names.UnitTag) (err error) { | ||
// TODO(dfc) named return value is a time bomb | ||
// TODO(dfc) named return value is a time bomb (externalreality) I second the notion | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any reason we can't just remove 'err error' and change it to 'error' ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only a hesitance to dabble. Will change. |
||
// TODO(axw) move this logic. | ||
defer func() { | ||
cause := errors.Cause(err) | ||
|
@@ -317,6 +317,17 @@ func (w *RemoteStateWatcher) loop(unitTag names.UnitTag) (err error) { | |
// returned by the leadership tracker. | ||
requiredEvents++ | ||
|
||
// TODO(externalreality) This pattern should probably be extracted | ||
var seenUpgradeSeriesChange bool | ||
upgradeSeriesw, err := w.unit.WatchUpgradeSeriesNotifications() | ||
if err != nil { | ||
return errors.Trace(err) | ||
} | ||
if err := w.catacomb.Add(upgradeSeriesw); err != nil { | ||
return errors.Trace(err) | ||
} | ||
requiredEvents++ | ||
|
||
var eventsObserved int | ||
observedEvent := func(flag *bool) { | ||
if flag != nil && !*flag { | ||
|
@@ -407,6 +418,12 @@ func (w *RemoteStateWatcher) loop(unitTag names.UnitTag) (err error) { | |
if err != nil { | ||
return err | ||
} | ||
case _, ok := <-upgradeSeriesw.Changes(): | ||
logger.Debugf("got upgrade series change") | ||
if !ok { | ||
return errors.New("upgrades series watcher closed") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who is going to close this? And is it really an error? The other watchers don't follow this pattern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The loop where this code lives has 12 other watchers that follow this pattern. They all return a similar error. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is an appropriate safeguard that the event we got was an actual event rather than just a closed channel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably, just saw the one above. Happy to have consistency here. |
||
} | ||
observedEvent(&seenUpgradeSeriesChange) | ||
case _, ok := <-addressesChanges: | ||
logger.Debugf("got address change: ok=%t", ok) | ||
if !ok { | ||
|
@@ -845,3 +862,14 @@ func (w *RemoteStateWatcher) watchStorageAttachment( | |
w.storageAttachmentWatchers[tag] = innerSAW | ||
return nil | ||
} | ||
|
||
func (w *RemoteStateWatcher) watchUpgradeSeries() error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where is this function used? |
||
w.mu.Lock() | ||
defer w.mu.Unlock() | ||
status, err := w.unit.UpgradeSeriesStatus() | ||
if err != nil { | ||
return errors.Trace(err) | ||
} | ||
w.current.UpgradeSeriesStatus = status | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are apparently two initial events coming out of this puppy. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you find out why this is happening? I noticed the TestWatchActionNotificationsMoreResults() is testing that only 1 watch notification happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is what I mean above. No I don't know why, its innocuous (since the Uniter will just loop twice), but I don't know why its sending two initial events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Histerical raisins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All watchers send an initial event to trigger the checking of initial state. Many times this isn't useful, and you'll see comments like "consume initial event".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, fixed now.