Skip to content
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

Merged
merged 45 commits into from Jul 13, 2018
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
1c0e8c7
Trigger upgrade-series-prepare hook.
ExternalReality Jun 27, 2018
0ac2132
API to check status.
ExternalReality Jun 30, 2018
02ce53c
Uniter identifies upgrade-series status changes.
ExternalReality Jul 2, 2018
be3d968
Trigger hook for upgrade series on build.
ExternalReality Jul 2, 2018
c5f67d3
Add uniter callback to update series upgade state.
ExternalReality Jul 3, 2018
678e376
Pull index to query nested array.
ExternalReality Jul 5, 2018
3d71446
thread through initial unit upgrade status.
ExternalReality Jul 6, 2018
9873531
ensure proper unit gets status set.
ExternalReality Jul 6, 2018
025d928
Resolve uniter status
ExternalReality Jul 6, 2018
095e39e
Ensure the uniter transitions prepare states.f
ExternalReality Jul 6, 2018
132b877
Remove redundant param, and more transactionsOp builder tests.
Jul 7, 2018
9e06d5e
Fix test name.
Jul 7, 2018
e7d842a
defer mutex release.
Jul 7, 2018
afd2b0e
Fix runhook tests.
Jul 8, 2018
ce7f0c9
Fix uniter tests.
ExternalReality Jul 9, 2018
418f215
Upgrade charm v6 reference.
ExternalReality Jul 9, 2018
737461e
Fix debug hooks test.
ExternalReality Jul 9, 2018
d31fefe
Fix api error check.
ExternalReality Jul 9, 2018
cd35901
Fix state transistion tests.
ExternalReality Jul 9, 2018
93b5a64
Fix known hooks test.
ExternalReality Jul 9, 2018
92d223c
Fix snapshot tests.
ExternalReality Jul 9, 2018
f7e1ab8
Remomve redundant lock call.
ExternalReality Jul 9, 2018
1ac1597
Fix abort error of set series upgrade transaction.
ExternalReality Jul 9, 2018
facc529
Do not return string api setter method.
ExternalReality Jul 9, 2018
ff4ac60
No need for status mutex in setUpgradeSeriesStatus
ExternalReality Jul 9, 2018
2eda0a1
Remove unused unit state update function.
ExternalReality Jul 9, 2018
7ca5aae
Fix typo in function name.
ExternalReality Jul 9, 2018
27b7ec7
Use more exact results struct.
ExternalReality Jul 10, 2018
cef8aed
Update test due to default change.
ExternalReality Jul 10, 2018
aa6dcff
Use more specific error resutls type.
ExternalReality Jul 10, 2018
e5c98bb
Remove error logging.
ExternalReality Jul 11, 2018
c622313
Move string contstants to core.
ExternalReality Jul 11, 2018
0d31aea
Pass around strings in the api and add validation.
ExternalReality Jul 11, 2018
71f3958
Refine assertions in update transaction.
ExternalReality Jul 11, 2018
44050d1
Test more upgrade series status assumptions.
ExternalReality Jul 11, 2018
600d130
Consume intial upgrade series notification on server side.
ExternalReality Jul 11, 2018
8cee315
Remove dangerous looking named returned value error
ExternalReality Jul 11, 2018
fb9f7cd
Documentation fix.
ExternalReality Jul 11, 2018
550ecd7
Documentation addition.
ExternalReality Jul 11, 2018
3d830be
Rename series lock to better match other state types.
ExternalReality Jul 12, 2018
b99b95a
Documentation fix.
ExternalReality Jul 12, 2018
2ab106c
Documentation fix.
ExternalReality Jul 12, 2018
6c00b4e
Change array search to switch statement.
ExternalReality Jul 12, 2018
a5c59a6
Allow different statuses.
ExternalReality Jul 13, 2018
8cc0cae
Fix redundant conversion.
ExternalReality Jul 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
70 changes: 70 additions & 0 deletions api/uniter/unit.go
Expand Up @@ -642,6 +642,76 @@ func (u *Unit) WatchActionNotifications() (watcher.StringsWatcher, error) {
return w, nil
}

// WatchActionNotifications returns a StringsWatcher for observing the state of
// a series upgrade.
func (u *Unit) WatchUpgradeSeriesNotifications() (watcher.NotifyWatcher, error) {
var results params.NotifyWatchResults
args := params.Entities{
Entities: []params.Entity{{Tag: u.tag.String()}},
}
err := u.st.facade.FacadeCall("WatchUpgradeSeriesNotifications", args, &results)
if err != nil {
return nil, err
}
if len(results.Results) != 1 {
return nil, errors.Errorf("expected 1 result, got %d", len(results.Results))
}
result := results.Results[0]
if result.Error != nil {
return nil, result.Error
}
w := apiwatcher.NewNotifyWatcher(u.st.facade.RawAPICaller(), result)
return w, nil
}

// UpgradeSeriesStatus returns the upgrade series status of a unit from remote state
func (u *Unit) UpgradeSeriesStatus() (params.UnitSeriesUpgradeStatus, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a string instead? params structs shouldn't be returned from an API call. does that extend to consts in params?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just string constants. We should probably move them out of params.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do that now, since they're added in this pr. :-) perhaps @jameinel knows a good spot?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't return any params structs or types from the API package.

If they really need their own type, I'd be tempted to make a core package. Alternatively keep the type in the same api package as the method.

var results params.UpgradeSeriesStatusResults
args := params.Entities{
Entities: []params.Entity{{Tag: u.tag.String()}},
}
err := u.st.facade.FacadeCall("UpgradeSeriesStatus", args, &results)
if err != nil {
return "", err
}
if len(results.Results) != 1 {
return "", errors.Errorf("expected 1 result, got %d", len(results.Results))
}
result := results.Results[0]
if result.Error != nil {
//TODO (externalreality) The code to do convert api errors (with
//error codes) back to normal Go errors is in bad spot and
//causes import cycles which is why we don't use it here and may
//be the reason why it has few uses despite being useful.
if params.IsCodeNotFound(result.Error) {
return "", errors.NewNotFound(result.Error, "")
}
return "", result.Error
}
return result.Status, nil
}

// UpgradeSeriesStatus sets the upgrade series status of the unit in the remote state
func (u *Unit) SetUpgradeSeriesStatus(status params.UnitSeriesUpgradeStatus) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same params comment as line 668

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Both the args and return values from API calls should not expose the params package.

var results params.ErrorResults
args := params.SetUpgradeSeriesStatusParams{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of the bulk calls isn't to apply a particular value to multiple targets, but instead to be able to process multiple actions in a single call. I think this is why the param struct this way feels weird. The bulk nature of the SetUpgradeSeriesStatus should be to take pairs of agents and status values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Entities: []params.Entity{{Tag: u.tag.String()}},
Status: status,
}
err := u.st.facade.FacadeCall("SetUpgradeSeriesStatus", args, &results)
if err != nil {
return err
}
if len(results.Results) != 1 {
return errors.Errorf("expected 1 result, got %d", len(results.Results))
}
result := results.Results[0]
if result.Error != nil {
return result.Error
}
return nil
}

// RequestReboot sets the reboot flag for its machine agent
func (u *Unit) RequestReboot() error {
machineId, err := u.AssignedMachine()
Expand Down
80 changes: 80 additions & 0 deletions api/uniter/unit_test.go
Expand Up @@ -684,6 +684,86 @@ 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?!?
Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Histerical raisins.

Copy link
Contributor

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fixed now.

_, ok := <-notifyWatcher.Watcher.Changes()
Copy link
Member

Choose a reason for hiding this comment

The 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.
I don't know exactly how your code is initialized, but I'll note that the NotifyWatcher's that I know of don't transmit an event over-the-wire for the first event. They intentionally swallow the first event on the server side, and trust that the client side knows its a NotifyWatcher and thus triggers the event on the client side.
I'm guessing you have the same "always trigger an event" on your client side, but don't have the "swallow the initial event" on the server side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()

s.CreateUpgradeSeriesLock(c)

// 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()
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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).
Shouldn't the status of a unit start in something like Pending?

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)
or clarify that we just treat setting the status to Started as a no-op?
Do we support going from Completed back to Started? Or should that be considered an error? (I'd tend toward the latter, though re-running a pre upgrade step is something we've discussed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestUpgradeSeriesStatus tests that a call to the api gets the current remote state - regardless of what it is.

It just so happens that the remote state starts in the UnitStartedState. When you create a lock, an upgrade is considered started. The uniter see's that and syncs its local state accordingly, so the initial states are as follows:

Uniter: UnitNotStarted
Controller: UnitStarted (as soon as a lock is created)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

s.CreateUpgradeSeriesLock(c)

// 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, params.UnitStarted)
}

func (s *unitSuite) TestSetUpgradeSeriesStatus(c *gc.C) {
// First we create the prepare lock or the required state will not exists
s.CreateUpgradeSeriesLock(c)

err := s.apiUnit.SetUpgradeSeriesStatus(params.UnitStarted)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use UnitCompleted instead, as UnitStarted is now the "default"

c.Assert(err, jc.ErrorIsNil)

// Check to see that the upgrade status has been set appropriately
status, err := s.apiUnit.UpgradeSeriesStatus()
c.Assert(err, jc.ErrorIsNil)
c.Assert(status, gc.Equals, params.UnitStarted)
}

func (s *unitSuite) TestSetUpgradeSeriesStatusShouldOnlySetSpecifiedUnit(c *gc.C) {
// add another unit
unit2, err := s.wordpressApplication.AddUnit(state.AddUnitParams{})
c.Assert(err, jc.ErrorIsNil)

err = unit2.AssignToMachine(s.wordpressMachine)
c.Assert(err, jc.ErrorIsNil)

// Creating a lock for the machine transitions all units to started state
s.CreateUpgradeSeriesLock(c, unit2.Name())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that this syntax is correct, as it is a locally defined function that knows about the various machines/units in play, and only creates a lock for s.wordpressMachine and takes additional unit names to include.
I will say that the signature of the method is confusing, as just reading through the code looks like you're taking an UpgradeSeriesLock for unit2, which isn't even a machine.
Maybe its ok, but it does mean I originally had a comment about correctness, and had to come back and reevaluate.
Maybe an issue is that we are using exactly the same function name, so I was expecting a similar signature? CreateTestUpgradeSeriesLock ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this invocation also pass in a unit name? It isn't clear to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its because the Suite function takes a list of "additional units to include in the preparation"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It compares the passed in values with the known values, and thus if someone has added a machine then we can't start the update. The user must confirm again in light of the new information, that is, a newly created machine. Its just used for validation.


// Complete one unit
err = unit2.SetUpgradeSeriesStatus(params.UnitCompleted)
c.Assert(err, jc.ErrorIsNil)

// The other unit should still be in the started state
status, err := s.wordpressUnit.UpgradeSeriesStatus()
c.Assert(err, jc.ErrorIsNil)
c.Assert(status, gc.Equals, params.UnitStarted)
}

func (s *unitSuite) CreateUpgradeSeriesLock(c *gc.C, additionalUnits ...string) {
unitNames := additionalUnits
unitNames = append(unitNames, s.wordpressUnit.Name())
series := "trust"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: trusty

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to add validation that the series mentioned is one that juju knows about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is validation at the CLI level. Will add validation at the DB level too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably make sense to do the validation at the API server level rather than the DB level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do validate the series via an api call, before calling prepare, as the machine lock is created... perhaps it can be validated again when creating the machine lock document?


err := s.wordpressMachine.CreateUpgradeSeriesLock(unitNames, series)
c.Assert(err, jc.ErrorIsNil)
}

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())
Expand Down
105 changes: 105 additions & 0 deletions apiserver/facades/agent/uniter/uniter.go
Expand Up @@ -984,6 +984,111 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc string: WatchActionNotifications creates a NotifyWatcher

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per your earlier question of "why do I have 2 events":
https://github.com/juju/juju/blob/develop/apiserver/common/watch.go#L39
"
// Consume the initial event. Technically, API
// calls to Watch 'transmit' the initial event
// in the Watch response. But NotifyWatchers
// have no state to transmit.
if _, ok := <-watch.Changes(); ok {
"

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
}

func (u *UniterAPI) SetUpgradeSeriesStatus(args params.SetUpgradeSeriesStatusParams) (params.UpgradeSeriesStatusResults, error) {
result := params.UpgradeSeriesStatusResults{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest using params.ErrorsResults, the status string isn't returned with Set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed.

Results: make([]params.UpgradeSeriesStatusResult, len(args.Entities)),
}
canAccess, err := u.accessUnit()
if err != nil {
return params.UpgradeSeriesStatusResults{}, err
}
for i, entity := range args.Entities {
//TODO[externalreality] refactor all of this, its being copied often.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certainly it can be shared by all methods on this type.
if err := u.canAccessEntity(&Results[i], entity); err != nil {
// canAccessEntity has already set Results[i].Error appropriately
continue
}
?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably refactor the next three if statements into something like:

getUnitOrServerError(tag names.Tag) (*state.Unit, *params.Error)

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
}
err = unit.SetUpgradeSeriesStatus(args.Status)
if err != nil {
result.Results[i].Error = common.ServerError(err)
continue
}
}

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) {
Expand Down
14 changes: 14 additions & 0 deletions apiserver/params/params.go
Expand Up @@ -1276,3 +1276,17 @@ type DumpModelRequest struct {
Entities []Entity `json:"entities"`
Simplified bool `json:"simplified"`
}

type UpgradeSeriesStatusResult struct {
Error *Error `json:"error,omitempty"`
Status UnitSeriesUpgradeStatus `json:"status,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming in late, but what is the rationale for having a particular type here rather thatn just a string?

Since this is only wire protocol, different types give us no benefit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a bit weird returning the status in a set result call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, we just pass around base string now. With validations to convert it into a typed const.

}

type UpgradeSeriesStatusResults struct {
Results []UpgradeSeriesStatusResult `json:"results,omitempty"`
}

type SetUpgradeSeriesStatusParams struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why assume that every entity will receive the same status? Suggest
type SetUpgradeSeriesStatusArgs struct
Args SetUpgradeSeriesStatusArg

type SetUpgradeSeriesStatusArg struct
Entity Entity
Status UnitSeriesUpgradeStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're probably not going to need that flexibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it follows the pattern of current params structs and will could be useful for the juju piece results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we probably don't need that flexibility, but the pattern generally used is to have arrays of structs where the embedded struct would have entity and status.

In reality, we are really only going to be calling this with a single entity from the uniter anyway.

Entities []Entity `json:"entities"`
Status UnitSeriesUpgradeStatus `json:"status"`
}
26 changes: 26 additions & 0 deletions apiserver/params/upgradeseries.go
@@ -0,0 +1,26 @@
// Copyright 2018 Canonical Ltd.
// Licensed under the AGPLv3, see LICENCE file for details.

package params

//MachineSeriesUpgradeStatus is the current status a machine series upgrade
type MachineSeriesUpgradeStatus string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps these constants would be better suited in core/model package?


const (
MachineSeriesUpgradeNotStarted MachineSeriesUpgradeStatus = "NotStarted"
MachineSeriesUpgradeStarted MachineSeriesUpgradeStatus = "Started"
MachineSeriesUpgradeUnitsRunning MachineSeriesUpgradeStatus = "UnitsRunning"
MachineSeriesUpgradeJujuComplete MachineSeriesUpgradeStatus = "JujuComplete"
MachineSeriesUpgradeAgentsStopped MachineSeriesUpgradeStatus = "AgentsStopped"
MachineSeriesUpgradeComplete MachineSeriesUpgradeStatus = "Complete"
)

//UnitSeriesUpgradeStatus is the current status of an upgrade
type UnitSeriesUpgradeStatus string

const (
UnitNotStarted UnitSeriesUpgradeStatus = "NotStarted"
UnitStarted UnitSeriesUpgradeStatus = "Started"
UnitErrored UnitSeriesUpgradeStatus = "Errored"
UnitCompleted UnitSeriesUpgradeStatus = "Completed"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants may eventually get replaced with int iota for ordering. This would require a stringer a custom BSON marshaling. Of course, ordering can be done in other ways too.

2 changes: 1 addition & 1 deletion cmd/juju/commands/debughooks_test.go
Expand Up @@ -121,7 +121,7 @@ var debugHooksTests = []struct {
}, {
info: `invalid hook`,
args: []string{"mysql/0", "invalid-hook"},
error: `unit "mysql/0" contains neither hook nor action "invalid-hook", valid actions are [fakeaction] and valid hooks are [collect-metrics config-changed install juju-info-relation-broken juju-info-relation-changed juju-info-relation-departed juju-info-relation-joined leader-deposed leader-elected leader-settings-changed meter-status-changed server-admin-relation-broken server-admin-relation-changed server-admin-relation-departed server-admin-relation-joined server-relation-broken server-relation-changed server-relation-departed server-relation-joined start stop update-status upgrade-charm]`,
error: `unit "mysql/0" contains neither hook nor action "invalid-hook", valid actions are [fakeaction] and valid hooks are [collect-metrics config-changed install juju-info-relation-broken juju-info-relation-changed juju-info-relation-departed juju-info-relation-joined leader-deposed leader-elected leader-settings-changed meter-status-changed post-series-upgrade pre-series-upgrade server-admin-relation-broken server-admin-relation-changed server-admin-relation-departed server-admin-relation-joined server-relation-broken server-relation-changed server-relation-departed server-relation-joined start stop update-status upgrade-charm]`,
}, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list is getting long enough, I have to question if it is being useful vs having something you can run to find out the list...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just a regex unit "mysql/0" contains neither hook nor action "invalid-hook", valid actions are .*. Unless we don't trust the list generation and want to test it here, I'd say we don't need the list at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its more a signal that we're not giving a helpful message to the user if we're drowning them in a list that wraps multiple times.
Occasionally validating long lists is good, as it makes sure things are wired up/listed

info: `no args at all`,
args: nil,
Expand Down
2 changes: 1 addition & 1 deletion dependencies.tsv
Expand Up @@ -111,7 +111,7 @@ gopkg.in/goose.v2 git 36df5d12fc6d0ef1c102e1c18a22ddf345b18821 2018-03-22T12:54:
gopkg.in/httprequest.v1 git 1a21782420ea13c3c6fb1d03578f446b3248edb1 2018-03-08T16:26:44Z
gopkg.in/ini.v1 git 776aa739ce9373377cd16f526cdf06cb4c89b40f 2016-02-22T23:24:41Z
gopkg.in/juju/blobstore.v2 git 51fa6e26128d74e445c72d3a91af555151cc3654 2016-01-25T02:37:03Z
gopkg.in/juju/charm.v6 git 5fbb467f7f759a4a2890fc5da0abe4b9b1df20fa 2018-06-29T06:08:46Z
gopkg.in/juju/charm.v6 git e6a4837dfe5ac4394d9861a97c45a887448bdc3c 2018-07-09T02:22:58Z
gopkg.in/juju/charmrepo.v2 git 653bbd81990d2d7d48e0fb931a3b0f52c694572f 2017-11-14T18:40:45Z
gopkg.in/juju/charmrepo.v3 git 8bb46aa94f3c679069e80fe35aac6d0581096d65 2018-06-29T07:07:28Z
gopkg.in/juju/charmstore.v5 git d93d0fd2b81b8230bdf9b1be91b50538dd8782fa 2018-06-28T08:11:03Z
Expand Down