docs: clarify storage-detaching applies to both machine and K8s charms#2554
Conversation
… and K8s charms
Verified against Juju 3.6.23: `juju remove-storage` on attached storage is rejected unless `--force` is passed (the user is prompted to run `juju detach-storage` first). Tighten the storage-detaching docs to reflect that the event fires from `juju detach-storage` or `juju remove-storage --force`.
|
3.10 failures are unrelated (as one would hope for a docs PR). Will be fixed by another PR and rebasing will fix here. |
| This section applies to both machine and Kubernetes charms. | ||
|
|
||
| For a machine charm, Juju emits a storage-detaching event whenever attached storage is being released: when a user runs `juju detach-storage` (or `juju remove-storage --force`), and during unit teardown. Observing the event lets the charm release resources cleanly before the storage goes away. | ||
|
|
||
| For a Kubernetes charm, `juju detach-storage` isn't supported (it returns `ERROR Juju command "detach-storage" not supported on container models`), so storage is only detached during unit teardown, and cleanup can be done in the `stop` or `remove` events. |
There was a problem hiding this comment.
WDYT about frontloading the common info + recommendation, and then mentioning the difference?
| This section applies to both machine and Kubernetes charms. | |
| For a machine charm, Juju emits a storage-detaching event whenever attached storage is being released: when a user runs `juju detach-storage` (or `juju remove-storage --force`), and during unit teardown. Observing the event lets the charm release resources cleanly before the storage goes away. | |
| For a Kubernetes charm, `juju detach-storage` isn't supported (it returns `ERROR Juju command "detach-storage" not supported on container models`), so storage is only detached during unit teardown, and cleanup can be done in the `stop` or `remove` events. | |
| Juju emits a `storage-detaching` event whenever attached storage is being released. This is emitted during unit teardown for both machine and Kubernetes charms. Observe this event to cleanly release resources before the storage goes away. | |
| On machine charms only, `storage-detaching` is also emitted when a user runs `juju detach-storage` (this command isn't currently supported for Kubernetes). |
I'm not clear on what we're aiming to convey beyond this with the 'cleanup can be done in the stop or remove events` bit for Kubernetes. I've dropped it in the suggestion, but maybe it should instead be expanded on?
There was a problem hiding this comment.
I'm not clear on what we're aiming to convey beyond this with the 'cleanup can be done in the
stoporremoveevents` bit for Kubernetes. I've dropped it in the suggestion, but maybe it should instead be expanded on?
It's a modified version of a suggestion in the issue. The issue suggests saying that storage-detaching can be used for cleanup in K8s, so is still useful there even though you only get it in teardown, since storage lifetime is tied to the life of the container. David doesn't remember the source of that suggestion.
I don't agree with that suggestion. I don't see any benefit in doing storage tidying in storage-detaching on K8s rather than in stop or remove. So I added the version in the PR.
Another way of handling this would be to focus the events specifically on machines, and a small comment mentioning that they happen on K8s but are not generally useful, and why.
There was a problem hiding this comment.
I think it would be easier to explain if we lead with this being about storage cleanup, rather than storage detachment.
The first part of the doc is organised into a machine section and a K8s section. How about adding a subsection in both sections called "Clean up storage".
The machine version could talk about storage-detaching and how it's emitted on demand and on teardown.
The K8s version could essentially be a recommendation to handle storage cleanup in stop or remove, as there's no on-demand way to request storage detachment. I think it's fine to mention storage-detaching here, but emphasise that we don't recommend observing it in K8s charms.
There was a problem hiding this comment.
Why don't we recommend observing storage-detaching in K8s charms? Because you should put all your cleanup logic in stop and/or remove and assume storage will be detached afterwards (and hasn't been already)?
There was a problem hiding this comment.
David's suggestion sounds reasonable to me. My suggestion should be ignored if we don't recommend storage-detaching for K8s charms.
There was a problem hiding this comment.
Why don't we recommend observing
storage-detachingin K8s charms? Because you should put all your cleanup logic instopand/orremoveand assume storage will be detached afterwards (and hasn't been already)?
Well, the issue said we should recommend it. I talked to David about it last week (because he opened the issue) arguing that since you get the storage detached at the same time (more or less) as stop/remove, why would you bother doing it separately. Like I would do all my setup in pebble-ready, not storage-attached.
If you feel observing storage-detached is better, I'm happy to hear the argument.
There was a problem hiding this comment.
I suppose an argument in favour of observing it would be to reduce differences between substrates. I don't know how substrate specific the non-observing code would be, which might change things, though the charm could always do K8s/machine specific helpers.
That said, maybe we don't really know what the right recommendation is here? At least I'm not convinced we should be recommending avoiding observing this event in K8s charms.
3 participants
I double-checked the items in the issue against Juju 3.6.23. A minimal
opscharm observedstorage-attached,storage-detaching,stop, andremoveand logged each event.juju detach-storageemitsstorage-detachingSTORAGE-TEST: storage-detaching event for /var/lib/storage-test/data/3juju remove-storageemitsstorage-detachingSTORAGE-TEST: storage-detaching event for /var/lib/storage-test/data/4(requires--forcewhile attached)data/5storage-detaching fired beforestop/removeonjuju remove-application --destroy-storagejuju detach-storagereturnsERROR Juju command "detach-storage" not supported on container models(verbatim)storage-detachingfires during unit teardowndata/0storage-detaching fired beforestop/removeonjuju remove-application --destroy-storagePreview
Fixes #2257