docs: fix and clarify holistic example of handling storage

[dwilding]

This PR significantly reworks How to manage storage. The current doc has several problems:

• It's not clear that _update_configuration is supposed to be an example of a holistic handler.

• The code incorrectly tries to access self.model.storages["cache"] as an object (it's a list).

• The doc mixes up machine and Kubernetes charms, which require different approaches because of (1) multiple vs singular storage and (2) how & whether to configure the workload with storage instance paths.

• The doc shows how to specify a location in a storage definition and explains the default location that Juju determines. This can mislead charmers into thinking that locations are predictable and could be hard-coded, when actually it's a better practice to transparently accept the locations that Juju determines.

Since this PR is substantial enough already, I've tried to leave the parts about detaching and tests largely unchanged.

Preview of updated doc

See related discussion in #2083

[tonyandrewmeyer]

Thanks for this!

While we're here, we call the storage "local-cache" in the first section of the doc, and then "cache" in the second. It would be good to be consistent I think.

Similarly while we're here, on line 81 it'd be nice if self.framework was framework, and self.on.cache_storage_attached was self.on['cache'].storage_attached (or "local-cache" if we go with that name).

The main issue here is that the example is more wrong than I initially thought :(. So it needs more fixing than just this.

[dwilding]

Thanks @tonyandrewmeyer for reviewing and explaining where the doc is mixing things up. I worked through the doc more carefully and totally agree with you. Also, I find the structure of the doc quite confusing.

I've tried a partial refactoring of the doc, to make it easier to follow and clearly separate the machine and K8s info. There's more that really ought to be improved, but this is enough for one PR I think.

Preview doc

New/reworked sections:

• Manage storage for a machine charm >
  • Define the storage
  • Access the storage
• Manage storage for a Kubernetes charm >
  • Define the storage
  • Access the storage

Other sections are largely untouched.

[tonyandrewmeyer]

Thanks for working on this - it's going to be much better (and actually correct) once we've done this. A few suggestions, mostly that we should push harder on always getting the location from Juju and not say where that probably will be.

[benhoyt]

We'll wait to progress this one till David is back as it's not urgent.

[Copilot]

Pull request overview

This PR fixes and clarifies the documentation for handling storage in Ops charms. The main improvements are correcting a code bug where storage was incorrectly accessed as an object instead of a list, and adding clarity about using holistic event handlers for storage events.

Key changes:

• Reorganized content with separate sections for machine charms and Kubernetes charms
• Fixed code bug: changed cache.location to properly iterate over cache as a list of storage instances
• Added explanatory comments to clarify that _update_configuration is a holistic handler example

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dwilding]

The approaches are essentially equivalent - choose whichever you prefer.

Can we give better guidance than this? Maybe it's OK. I figured it's good to show both approaches for the sake of clarity about what's going on.

[tonyandrewmeyer]

They feel extra similar because pathops is hiding away that one is using Pebble (other than the error). I think if we're going to use pathops then we should just stick with one of them (and the local one probably makes more sense, because you don't need to worry about Pebble issues).

[dwilding]

Good suggestion, thanks. I've gone with this, to also capture something that Dima and I talked about:

To access the storage instance in charm code, use pathops or standard file operations in the charm container. For example:

    # Prepare the storage instance for use by the workload.
    charm_cache_path = cache[0].location  # Always index 0 in a K8s charm.
    charm_cache_root = pathops.LocalPath(charm_cache_path)
    (charm_cache_root / "uploaded-data").mkdir(exist_ok=True)
    (charm_cache_root / "processed-data").mkdir(exist_ok=True)

Alternatively, use pathops.ContainerPath to access web_cache_path in the workload container. This approach is more appropriate if you need to manipulate lots of data in the workload container.

[dwilding]

On reflecting, this is less about the amount of data. The point is more that there might be other data in the workload container that you need to put into the cache (for example). In which case it makes more sense to do the file operations in the container. I've changed the wording to:

This approach is more appropriate if you need to reference additional data in the workload container.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dwilding]

@dimaqq & @tonyandrewmeyer would you mind re-reviewing this one? I've significantly reworked the doc to address fundamental issues, so it would be best to review from scratch if possible. (I've updated the PR description to reflect the new approach)

Thanks a lot, and please take your time - this is not high priority

[dimaqq]

WDYT about a recommendation like this:

if not cache: return
if not pebble-ready: return

container.replan(plan-with-storage-location)

However, this point is moot if the pod always comes up with storage.

If the storage is guaranteed to be there, and we hard-code the storage location, we could as well have the workload container image to expect the storage at that location, so the charm need not do anything (apart from kicking off the service on pebble ready)

[dwilding]

In my simple test charm, storage was always available when my holistic handler looked for it earlier than storage-attached. However, I don't think this is guaranteed. I can't find any Juju docs that directly address this, but there's a comment in the code:

For CAAS models, we wait until after the start hook has run before running storage-attached hooks since storage is provisioned only after the pod has been created. [...] any charm logic that needs storage cannot be in install or start and if in config-changed, needs to be deferred until storage is available.

I've taken your suggestions and reworked the example like this:

def _update_configuration(self, event: ops.EventBase):
    """Update the workload configuration."""
    cache = self.model.storages["cache"]
    if not cache:
        logger.info("No instance available for storage 'cache'.")
        return
    web_cache_path = self.meta.containers["web"].mounts["cache"].location
    # Configure the workload to use the storage instance path (assuming that
    # the workload container image isn't preconfigured to expect storage at
    # the location specified in charmcraft.yaml).
    # For example, provide the storage instance path in the Pebble layer.
    web_container = self.unit.get_container("web")
    try:
        web_container.add_layer(...)
    except ops.pebble.ConnectionError:
        logger.info("Workload container is not available.")
        return
    web_container.replan()

[dimaqq]

mention that it's only for machines... if that's only for machines

[dwilding]

This is intended to apply to both machine and K8s. While I'm checking other parts in a K8s context, I'll check this too.

[dwilding]

I looked into this further. Detaching storage manually is only possible for machine charms. But a K8s charm might still want to observe the event. I've created a separate issue for improving this section of the doc: #2257

[tonyandrewmeyer]

@dwilding sorry I've been slow on this - I meant to review before our doc session today, but that's looking unlikely now. I'll definitely do so this week before going into my break.

[tonyandrewmeyer]

Looks great! Thanks for doing all the re-work after the previous round.

[tonyandrewmeyer]

They feel extra similar because pathops is hiding away that one is using Pebble (other than the error). I think if we're going to use pathops then we should just stick with one of them (and the local one probably makes more sense, because you don't need to worry about Pebble issues).