Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
relation status implemented as struct rather than relation field #7831
Conversation
| @@ -644,13 +644,14 @@ func (u *Unit) RequestReboot() error { | ||
| return result.OneError() | ||
| } | ||
| -// JoinedRelations returns the tags of the relations the unit has joined. | ||
| -func (u *Unit) JoinedRelations() ([]names.RelationTag, error) { | ||
| +// RelationsInScopeOrSuspended returns the tags of the relations the unit has joined |
axw
Sep 7, 2017
Member
so when a relation is suspended, it's no longer in scope? this is starting to get pretty messy, and if you're talking about making "error" also count towards this, it gets even less clear. can it not be in scope, but also suspended? and have the uniter filter suspended relations out as appropriate
wallyworld
Sep 7, 2017
Owner
When a relation is suspended, it has to leave scope and have the departed / broken hooks run.
What we really care about is should the relation state dir be removed when the uniter initialises. If a relation is not in scope, but could be again, the state dir won't be empty and so we shouldn't delete it. So I guess the concept is "active" vs "non-active" relations rather than "in scope" vs "out of scope". We make a single api call off unit which only returns tags - I guess a new bespoke call could be done that returns tag,inscope,status
axw
Sep 7, 2017
Member
OK, sounds fair, but please come up with a name that describes that, and not "isXorYorZ". Not sure about (non-)active, because something that's non-active can become active again.
wallyworld
Sep 7, 2017
Owner
I've added a new uniter api call "RelationStatus" which returns the scope and status info as needed by the uniter.
| @@ -35,8 +35,16 @@ func PublishRelationChange(backend Backend, relationTag names.Tag, change params | ||
| // Update the relation status if necessary. | ||
| relStatus := status.Status(change.Status) | ||
| - if rel.Status() != relStatus && relStatus != "" { | ||
| - if err := rel.SetStatus(relStatus); err != nil { | ||
| + currentStatus, err := rel.Status() |
| + if err != nil { | ||
| + return false | ||
| + } | ||
| + k, err := backend.strictLocalID(doc.Id) |
axw
Sep 7, 2017
Member
maybe s/Id/DocID/ in relationIdDoc? I was confused by the fact that relationDoc has an "Id" field that is an int, but this one refers to the id_ field.
| + | ||
| + // status filter is used to filter changes based on the | ||
| + // relation id number, ie status changes from the status collection | ||
| + relationStatusFilter func(key interface{}) bool | ||
| } | ||
| // WatchStatus returns a watcher that notifies of changes to the life | ||
| // or status of the relation. |
axw
Sep 7, 2017
Member
I think it would be much simpler if we had a separate status watcher, and then you combined that with a watcher that observes life changes.
wallyworld
Sep 7, 2017
Owner
It needs to be in the one state watcher because we are watching 2 different collections, and need to maintain current state which covers life and status combined. Otherwise we would end up firing duplicate events when first life changes and then status (or visa versa).
axw
Sep 7, 2017
Member
When you combine the two watchers, you wait for the initial events from both.
wallyworld
Sep 7, 2017
Owner
It's a bit more complicated. The watcher can be set up from either a relation (watch only one entity) or an application (watch all relations for that application). The status collection is keyed on r# not key. In the application case, we have a string match for the relation key, and so need to first query the relation collection to get the superset of things to watch and thus the set of ids. And when a new relation is created, we need that info in the combined watcher also to track the id. So things are quite tightly all bound up together because of the different ids used in each of the watched collections and how they are related.
axw
Sep 7, 2017
Member
That's not how watchers are typically created. It would be less complicated (here, in state), if you went with the more standard approach:
- Application.WatchRelations watches lifecycle changes, as described in the doc comment
- For each active relation, watch that relation's status (Relation.WatchStatus only watches the status doc for the relation)
That's what I was trying to suggest earlier. This is what we do in various workers: we have a top-level lifecycle watcher which is notified of things being created or destroyed; we then start watchers for those individual entities that we care about (usually the Alive and Dying ones).
Is there a problem with the approach I describe?
wallyworld
Sep 7, 2017
Owner
I was trying to implement a "bulk" watcher to minimise the number of workers as it seems to me our typical O(N) approach to doing things like this is a scalability issue and has always bothered me. My approach is more complex under the covers but scales much better. Nonetheless, I'll rework.
wallyworld
Sep 7, 2017
Owner
state watchers reworked so that the WatchRelations() and WatchLifeStatus() apis construct a standard life watcher according to what they need - for each newly alive relation, a standard status watcher is started. Everything is managed by a new relationLifeStatus watcher.
wallyworld
added some commits
Sep 6, 2017
| + for iter.Next(&doc) { | ||
| + rid := relationGlobalScope(doc.Id) | ||
| + switch doc.Life { | ||
| + case Alive: |
wallyworld
Sep 8, 2017
Owner
I was thinking not since once the relation goes not alive, the status isn't used for anything in practice. I guess it won't hurt to keep the watcher until it goes dead.
| + return stateWatcherDeadError(w.watcher.Err()) | ||
| + case <-w.tomb.Dying(): | ||
| + return tomb.ErrDying | ||
| + case changes, ok = <-w.lifeWatcher.Changes(): |
axw
Sep 8, 2017
Member
this is going to wipe out previous changes, which may not have been sent out yet
| + if err = w.lifeChanged(changes); err != nil { | ||
| + return err | ||
| + } | ||
| + if !sentInitial || len(changes) > 0 { |
axw
Sep 8, 2017
Member
maybe just put this at the end of the main loop, to avoid having it in two places?
| + // Look up the relation key. | ||
| + for _, rid := range rids { | ||
| + if key, ok := w.relationKeys[rid]; ok { | ||
| + changes = append(changes, key) |
axw
Sep 8, 2017
Member
you should probably use a set.Strings, because this is going to get duplicates (two status changes = two changes in the list; we should coalesce and see only one)
| + filter func(interface{}) bool | ||
| +} | ||
| + | ||
| +func newStatusWatcher(backend modelBackend, |
axw
Sep 8, 2017
Member
It would be great if we could stop growing snowflake watchers. I'd prefer if this were built on something like the doc watcher, but I guess we don't want to react to changes to the status doc's "since" field, only changes to the status value. Maybe we could build a more general doc watcher that can pull out and compare fields.
| - case known && (newLifeStatus.life != oldLifeStatus.life || newLifeStatus.status != oldLifeStatus.status): | ||
| - w.knownLifeStatus[key] = newLifeStatus | ||
| + w.knownStatus[id] = newStatus | ||
| + case known && newStatus != oldStatus: |
axw
Sep 8, 2017
Member
this case shouldn't fire if "gone" is true, right? I think you want to swap this with the one below
| - return errors.Trace(err) | ||
| + } else { | ||
| + switch relationStatusValues[id] { | ||
| + case relation.Joined, relation.Suspended, relation.Error: |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
wallyworld commentedSep 6, 2017
•
Edited 1 time
-
wallyworld
Sep 6, 2017
Description of change
Relation status was being modelled as a field in the state relation doc.
We now model it like for other entities as an entry in the statuses collection.
Most of the changes are mechanical, but the state relation status watcher needed to be rewritten to account for needed to get its data from 2 collections.
The juju/description repo has been modified to model status the new way and the dep for this pr is updated. The juju/decription is juju/description#22
The second commit, 24bdeb8, fixes an issue that came up in testing. When the uniter is initialising, it needs to treat any suspended relations as still active, not just those in scope. Otherwise it attempts to incorrectly remove the unit state dir.
QA steps
Run up a cmr scenario.
Set status for relation on offering side to suspended.
Ensure relation on consuming side is broken.
Set relation to resumed. See that consuming relation recovers.
Delete relation on offering side.
Relation on consuming side is removed.