uniter: Relations between subordinates shouldn't keep the sub units alive #7603
Conversation
| // OtherApplication returns the name of the application on the other | ||
| // end of the relation (from this unit's perspective). | ||
| func (r *Relation) OtherApplication() string { | ||
| return r.otherApp |
There was a problem hiding this comment.
Just curious, what is otherApp for peer relations?
There was a problem hiding this comment.
It'll be populated with the same app. It's obtained using relation.RelatedEndpoints(thisApp), which uses counterpartRole - for peer that returns peer. I think that's correct. It won't have any impact for this change though, since a peer relation couldn't ever be a principal one - the app would have to be both subordinate and insubordinate. :)
apiserver/params/internal.go
Outdated
|
|
||
| // RelationResults holds the result of an API call that returns | ||
| // information about multiple relations. | ||
| type RelationResults struct { |
There was a problem hiding this comment.
Maybe move this above RelationResult so that all the RelationResult structs are next to each other?
| // NewUniterAPIV4 creates an instance of the V4 uniter API. | ||
| func NewUniterAPIV4(st *state.State, resources facade.Resources, authorizer facade.Authorizer) (*UniterAPIV4, error) { | ||
| uniterAPI, err := NewUniterAPI(st, resources, authorizer) | ||
| uniterAPI, err := NewUniterAPIV5(st, resources, authorizer) |
There was a problem hiding this comment.
This is weird (constructing V5 in V4) but I think I get it.
There was a problem hiding this comment.
It's what was happening before anyway (there was an invisible V5 at the end of NewUniterAPI that became visible when V6 sprang into existence). The differences between the API versions are cumulative - if we just embedded UniterAPI in UniterAPIV4 we'd need to copy the V5 methods into it as well.
We could do it the other way around, implementing V5 in terms of V4 and V6 embedding V5, but it seems clearer to have the current version be the default case and the older (hopefully smaller) ones the deltas.
This required changing the return type of Relation and RelationById and bumping the uniter API to version 6 (leaving a V5 that returns the old types). The new field will be used in the uniter worker to determine whether the relation can keep a subordinate unit alive.
babbageclunk
force-pushed
the
uniter-subsub
branch
from
9b8b1af
to
2798d2b
Compare
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Grr, why are all of these tests making assertions about the version of the API? |
This will be used by the uniter to determine whether the relation to the principal unit is still alive.
Previously the uniter would only destroy the unit when all of its container-scoped relations died - this meant that a relation to another subordinate unit under the same principal could keep it alive. Make ordering of relations deterministic for testing.
babbageclunk
force-pushed
the
uniter-subsub
branch
from
2798d2b
to
3c17558
Compare
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Description of change
Relating two subordinates and then trying to remove the principal application would leave the units of the application in terminating state. The principal units wouldn't be cleaned up until their subordinates were, and they wouldn't die because the sub-sub relation would keep them alive. For example, if you had this configuration:
...and nrpe and ntp were also related, then
mysql/0wouldn't get removed if you removedmysql.Fix this by making the subordinate units only consider the relations to their principal when checking whether they should destroy themselves.
QA steps
Bug reference
Fixes part of https://bugs.launchpad.net/juju/+bug/1702636