canonical
Are application relation settings inherantly racy?
I've been pondering code like this:
class MyCharm(CharmBase):
def __init__(self, *args, **kw):
super().__init__(*args, **kw)
self.framework.observe(self.on.my_relation_joined, self)
def on_my_relation_joined(self, event):
if self.model.unit.is_leader():
event.relation[event.unit.app]['secret'] = mkpasswd(128)
I very much like the idea of application level relation settings, as it avoids a lot of mess dealing with eventual consistency. And I think the above is how the operator framework guides charm authors to set this data. My concern is that I believe it is possible for all units to run the on_my_relation_joined handler while none of them are actually leader, and the setting never gets set. Unlike the reactive framework, which encourages charm authors to decouple this sort of thing from hooks, and ends up with different race condition that are not relevant here.
When running the example charm, if the leader becomes unavailable when a relation is being added (netsplit, reboot), then other units will run their joined hooks while leadership remains with the isolated unit. A period later, one of the remaining units will gain leadership after some time. When the deposed leader again becomes available, it will run the joined hook as non-leader. All units have run the joined hook, and none of them set the application relation setting.
Does Juju actually prevent this somehow? Could it?
Do we need charms to be provably correct? The above race is certainly rather unlikely to occur, highly improbable outside of a test case.
One way of addressing the issue is to defer the relation-joined event:
def on_my_relation_joined(self, event):
if self.model.unit.is_leader():
event.relation[event.unit.app]['secret'] = mkpasswd(128)
elif event.relation[event.unit.app].get('secret') is None:
event.defer()
Can the operator framework guide charm authors to correct code, even if only in documentation? There is a push for more code to only be run on lead units, so far relation application settings and pod-set-spec, and I have encountered developers tripping over both of these and neglecting the 'is_leader()' guard. Perhaps the charm code to be run on the leader could be decoupled from the charm code to be run on all units? Or is this solution worse than the problem?
class MyCharm(CharmBase):
[...]
class MyCharmLeader(CharmLeader):
[...]
def on_my_relation_joined(self, event):
event.relation[event.unit.app]['secret'] = mkpasswd(128)
I think you raise valid points.
- We've known that leadership is decoupled from events, and we have even seen in the wild a case where you have unit/1 being the leader, unit/2 sees relation-joined, then before unit/1 sees relation-joined, it loses leadership, and then restarts to see relation-joined. We only saw that in the wild before the new leadership changes, when under significant load it could take >45s to extend your lease (30s+45s > 1m which meant you would lose leadership.)
- With the improvements to lease handling, we haven't explicitly seen that failure, but it does exist. It has always been possible to use 'leader-elected' to have a back-stop that would let you validate if any relations aren't set up properly, but it isn't a good experience for charm developers.
- I've brought up the idea internally of having events that a leader must 'ack'. (eg, relation-joined will try to fire on the current leader, if it is unable to, then that event is still pending until a unit that is the leader has successfully run the hook and exited 0.)
- You can do exactly that with defer() + handling deferred events in on.leader_elected. However, you have to deal with the fact that the leader could very well have run the code that you've deferred, and you would need something like a peer relation for the leader to indicate to the other units that the event has been handled. (Otherwise, on every event, everything gets deferred, and you just build up an ever-growing 'things I might want to do if I become the leader.)
- I do think we've started seeing more and more of the pattern that "all I really care about is events as the leader". This is especially apparent on K8s charms, where the only real interaction you have is to set the pod spec, and that can only be done by the leader. I've considered that it is probably worthwhile to promote that information all the way up into the charm metadata.yaml, so that Juju doesn't try to fire hooks that will just do 'if not self.is_leader(): return".
- The other advantage of promoting it to to metadata.yaml is that it quickly lets us have the behavior I mentioned above. The hook will only ever fire on a leader, but it is guaranteed to fire while a unit has leadership, and will continue to fire on a unit that has leadership until it exits successfully.
Doing it only at the Charm layer would be possible, but would require some cleverness to get the peer relation data correct. (How do you communicate from the current leader to other potential leaders that you've already done the work, so they don't need to worry about it anymore, you could track things like the hash of content to the output, and then check if that content has already been processed, but the 'list of what has been processed' also potentially grows as well.)
John =:->
On Thu, Mar 5, 2020 at 3:25 AM Stub [email protected] wrote:
I've been pondering code like this:
class MyCharm(CharmBase): def init(self, *args, **kw): super().init(*args, **kw) self.framework.observe(self.on.my_relation_joined, self)
def on_my_relation_joined(self, event): if self.model.unit.is_leader(): event.relation[event.unit.app]['secret'] = mkpasswd(128)I very much like the idea of application level relation settings, as it avoids a lot of mess dealing with eventual consistency. And I think the above is how the operator framework guides charm authors to set this data. My concern is that I believe it is possible for all units to run the on_my_relation_joined handler while none of them are actually leader, and the setting never gets set. Unlike the reactive framework, which encourages charm authors to decouple this sort of thing from hooks, and ends up with different race condition that are not relevant here.
When running the example charm, if the leader becomes unavailable when a relation is being added (netsplit, reboot), then other units will run their joined hooks while leadership remains with the isolated unit. A period later, one of the remaining units will gain leadership after some time. When the deposed leader again becomes available, it will run the joined hook as non-leader. All units have run the joined hook, and none of them set the application relation setting.
Does Juju actually prevent this somehow? Could it?
Do we need charms to be provably correct? The above race is certainly rather unlikely to occur, highly improbable outside of a test case.
One way of addressing the issue is to defer the relation-joined event:
def on_my_relation_joined(self, event): if self.model.unit.is_leader(): event.relation[event.unit.app]['secret'] = mkpasswd(128) elif event.relation[event.unit.app].get('secret') is None: event.defer()Can the operator framework guide charm authors to correct code, even if only in documentation? There is a push for more code to only be run on lead units, so far relation application settings and pod-set-spec, and I have encountered developers tripping over both of these and neglecting the 'is_leader()' guard. Perhaps the charm code to be run on the leader could be decoupled from the charm code to be run on all units? Or is this solution worse than the problem?
class MyCharm(CharmBase): [...] class MyCharmLeader(CharmLeader): [...] def on_my_relation_joined(self, event): event.relation[event.unit.app]['secret'] = mkpasswd(128)
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/canonical/operator/issues/169?email_source=notifications&email_token=AABRQ7MXLA4UQJY756SJXTLRF3PPVA5CNFSM4LB5UY6KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ISTERVQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7NY76YCQ3HGWY6MWWTRF3PPVANCNFSM4LB5UY6A .
I think this could be fixed in the Operator Framework, and prototyped with a decorator, implementing some of the previous comment's ideas.
class MyCharm(CharmBase):
[...]
@leader_only
def on_config_changed(self, event):
[...]
If called on the leader, the @leader_only decorator would call the wrapped method, and set a flag via leader-set.
If called on a non-leader, the @leader_only decorator would check for the flag via leader-get. If the flag is set, it returns and does nothing. If the flag is not set, it defers the event.
Or instead of using a decorator, the Operator Framework could implement the same logic. For example adding a leader_only argument to Framework.observe, or adding a leader_observe method to the Framework class.
(for the above, the trick would be calculating what key to use for the flag, probably involving the event handle. And how to clear the flag, possibly by clearing all the flags in leader-elected, before reemitting any deferred events)
Given the fact this describes an edge case which hasn't been commented on for 3 years, and IMO it's clearer to just write it out as shown below (especially with the ever-problematic defer involved!), we're going to close this.
def on_changed_changed(self, event):
if not self.unit.is_leader():
event.defer()
return
# rest of code
