operator icon indicating copy to clipboard operation
operator copied to clipboard

Some events should not be deferrable

Open pengale opened this issue 3 years ago • 2 comments

When Juju is tearing down a unit, deferring does not work as one might naively expect. Since the controller does not have a view of "deferred" or "non deferred" events, the controller will simply assume that the "deferred" hook finished, and will continue the teardown.

When charm authors call event.defer on events like storage-detached, and possibly relation-departed and relation-broken, the framework should raise an error.

Error states are visible in the controller. This means that the code will do what the charm author probably "intended", which is to block the teardown until something else happens.

pengale avatar Apr 21 '22 16:04 pengale

We already have code that actions cannot be deferred, we can probably use the same errors in this situation.

On Thu, Apr 21, 2022 at 12:13 PM Pen Gale @.***> wrote:

When Juju is tearing down a unit, deferring does not work as one might naively expect. Since the controller does not have a view of "deferred" or "non deferred" events, the controller will simply assume that the "deferred" hook finished, and will continue the teardown.

When charm authors call event.defer on events like storage-detached, and possibly relation-departed and relation-broken, the framework should raise an error.

Error states are visible in the controller. This means that the code will do what the charm author probably "intended", which is to block the teardown until something else happens.

— Reply to this email directly, view it on GitHub https://github.com/canonical/operator/issues/740, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7NYBCK7UBLQEP6KL73VGF5BDANCNFSM5T7XVM2Q . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jameinel avatar Apr 21 '22 21:04 jameinel

We already have code that actions cannot be deferred, we can probably use the same errors in this situation.

Excellent. Writing less code is always good :-)

pengale avatar Apr 22 '22 13:04 pengale

Events like ActionEvent.defer raise RuntimeError for this, so we could follow that precedent for these "tearing down" events.

benhoyt avatar May 01 '23 10:05 benhoyt

This is still an issue. However, we're thinking of (maybe!) deprecating defer in general, see https://github.com/canonical/operator/issues/966

benhoyt avatar Oct 04 '23 04:10 benhoyt

This was done in #1122, and I missed linking the PR to the ticket, sorry :disappointed:.

tonyandrewmeyer avatar Mar 12 '24 06:03 tonyandrewmeyer