Repository: canonical/operator (language: python)

Issue:
Harness to auto attach storage?
Following up on https://github.com/canonical/operator/pull/734, I tried it out in some unit tests and had the following recurring code:

```python
storage_id = self.harness.add_storage("name")[0]
self.harness.attach_storage(storage_id)
```

Would it be feasible/sensible to auto attach all storages in metadata (perhaps with `count=1` by default)?

For example,

```python
self.harness.auto_attach_storages()
self.harness.begin_with_initial_hooks()
```


Discussion / hints:
Shouldn't the storage-attached events be part of the 'initial hooks' in the `begin_with_initial_hooks` sense?
If that method is meant to be a 'simulate a happy-path situation where all is good', and https://github.com/PietroPasotti/charm-events is correct, then I'd expect all relevant storage-attached events to be the first thing to fire when I call `begin_with_initial_hooks`.
Thanks @PietroPasotti. 
I do see that `begin_with_initial_hooks` [emits](https://github.com/canonical/operator/blob/eb33286800bf94b60b7354b350f2725ab5b81865/ops/testing.py#L233) `storage_attached`, so I guess all I need to do is 
`self.harness.add_storage...`?

My [code](https://github.com/canonical/cos-configuration-k8s-operator/pull/21) is still a draft but unit tests currently fail if I do not have the `add_storage` calls.
Yes - you will need to use `add_storage` before the begin_with_initial_hooks call even if you already have the storage in your metadata.  This was a design decision that was made before I was around, so I'm not sure the reasoning behind it.  But it definitely seems reasonable to consider adding all meta-data storage automatically in begin_with_initial_hooks.  I'll have to think about it more first though.  At a minimum it may break tests for existing charms :-/
The charm should behave well (i.e. set a useful error message) if the person deploying the charm has neglected to add storage to the cloud. I think that it is reasonable to expect a charm author to remember to do something if that charm author is also expecting an operator to do that thing :-)

We can make the harness error message as friendly as possible, though. The harness might even suggest that a charm author check for the existence of the storage, and set model.application.status to something like `ErrorStatus("this charm requires storage: see docs @ ..."`
Agreed. Not sure what's best for harness.
- on one hand, `juju deploy` takes care of storages defined in metadata, and `begin_with_initial_hooks` does take care of many other aspects of startup;
- on the other hand, we do need to manually emit the pebble-ready event anyway.
Yeah - there is definitely a case for `begin_with_initial_hooks` to just simulate what juju does in production (i.e. automatically add storage in metadata.yaml) - since that is sort of the benchmark for how it operates generally.
I have some issues with my unit tests (they fail if I omit a call to `attach_storage`) so I was wondering:
1. Seems like `storage_name = storage_name.replace('-', '_')` is missing from `add_storage` and `attach_storage`.
1. Currently `add_storage` calls `storage_attached.emit()`. Is that intentional?
1. Should `add_storage` and `begin_with_initial_hooks` use `attach_storage` instead?

`add_storage`:
https://github.com/canonical/operator/blob/eb33286800bf94b60b7354b350f2725ab5b81865/ops/testing.py#L473-L477

`attach_storage`:
https://github.com/canonical/operator/blob/eb33286800bf94b60b7354b350f2725ab5b81865/ops/testing.py#L511-L515

`begin_with_initial_hooks`:
https://github.com/canonical/operator/blob/eb33286800bf94b60b7354b350f2725ab5b81865/ops/testing.py#L229-L234
1. The -/_ replacement occurs automatically with the `.on[...]` call: https://github.com/canonical/operator/blob/eb33286800bf94b60b7354b350f2725ab5b81865/ops/framework.py#L453
2. I'll need to look into this more.  At first glance, it seems like we should definitely use more care with how we handle add/attach/detach - it looks like there may be a few subtle bugs lurking.
3. Possibly.  I'll dig into this as well.

Fix patch:
diff --git a/HACKING.md b/HACKING.md
index dc26a8965..6688c8b5f 100644
--- a/HACKING.md
+++ b/HACKING.md
@@ -121,3 +121,5 @@ This will trigger an automatic build for the Python package and publish it to Py
 See [.github/workflows/publish.yml](https://github.com/canonical/operator/blob/main/.github/workflows/publish.yml) for details. (Note that the versions in publish.yml refer to versions of the github actions, not the versions of the Operator Framework.)
 
 You can troubleshoot errors on the [Actions Tab](https://github.com/canonical/operator/actions).
+
+Announce the release on discourse.


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index c11080cca..933718302 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -227,11 +227,9 @@ def begin_with_initial_hooks(self) -> None:
         # Checking if disks have been added
         # storage-attached events happen before install
         for storage_name in self._meta.storages:
-            for storage_index in self._backend.storage_list(storage_name):
-                storage_name = storage_name.replace('-', '_')
-                # Storage device(s) detected, emit storage-attached event(s)
-                self._charm.on[storage_name].storage_attached.emit(
-                    model.Storage(storage_name, storage_index, self._backend))
+            for storage_index in self._backend.storage_list(storage_name, include_detached=True):
+                s = model.Storage(storage_name, storage_index, self._backend)
+                self.attach_storage(s.full_id)
         # Storage done, emit install event
         self._charm.on.install.emit()
         # Juju itself iterates what relation to fire based on a map[int]relation, so it doesn't
@@ -446,16 +444,19 @@ def _next_relation_id(self):
         self._relation_id_counter += 1
         return rel_id
 
-    def add_storage(self, storage_name: str, count: int = 1) -> typing.List[str]:
-        """Declare a new storage device attached to this unit.
+    def add_storage(self, storage_name: str, count: int = 1,
+                    *, attach: bool = False) -> typing.List[str]:
+        """Create a new storage device and attach it to this unit.
 
         To have repeatable tests, each device will be initialized with
-        location set to /<storage_name>N, where N is the counter and
-        will be a number from [0,total_num_disks-1]
+        location set to /[tmpdir]/<storage_name>N, where N is the counter and
+        will be a number from [0,total_num_disks-1].
 
         Args:
             storage_name: The storage backend name on the Charm
             count: Number of disks being added
+            attach: True to also attach the storage mount and emit storage-attached if
+                harness.begin() has been called.
 
         Return:
             A list of storage IDs, e.g. ["my-storage/1", "my-storage/2"].
@@ -463,6 +464,10 @@ def add_storage(self, storage_name: str, count: int = 1) -> typing.List[str]:
         if storage_name not in self._meta.storages:
             raise RuntimeError(
                 "the key '{}' is not specified as a storage key in metadata".format(storage_name))
+        if not self.charm and attach:
+            msg = 'cannot run add_storage with attach=True before calling Harness.begin()'
+            raise RuntimeError(msg)
+
         storage_indices = self._backend.storage_add(storage_name, count)
 
         # Reset associated cached value in the storage mappings.  If we don't do this,
@@ -473,8 +478,8 @@ def add_storage(self, storage_name: str, count: int = 1) -> typing.List[str]:
         for storage_index in storage_indices:
             s = model.Storage(storage_name, storage_index, self._backend)
             ids.append(s.full_id)
-            if self.charm is not None and self._hooks_enabled:
-                self.charm.on[storage_name].storage_attached.emit(s)
+            if attach:
+                self.attach_storage(s.full_id)
         return ids
 
     def detach_storage(self, storage_id: str) -> None:
@@ -508,11 +513,16 @@ def attach_storage(self, storage_id: str) -> None:
             storage_id: The full storage ID of the storage unit being attached, including the
                 storage key, e.g. my-storage/0.
         """
-        if self._backend._storage_attach(storage_id) and self._hooks_enabled:
-            storage_name, storage_index = storage_id.split('/', 1)
-            storage_index = int(storage_index)
-            self.charm.on[storage_name].storage_attached.emit(
-                model.Storage(storage_name, storage_index, self._backend))
+        if not self._backend._storage_attach(storage_id):
+            return  # storage was already attached
+        if not self.charm or not self._hooks_enabled:
+            return  # don't need to run hook callback
+
+        storage_name, storage_index = storage_id.split('/', 1)
+
+        storage_index = int(storage_index)
+        self.charm.on[storage_name].storage_attached.emit(
+            model.Storage(storage_name, storage_index, self._backend))
 
     def remove_storage(self, storage_id: str) -> None:
         """Detach a storage device.
@@ -1106,7 +1116,7 @@ def __init__(self, unit_name, meta):
         # Initialize the _storage_list with values present on metadata.yaml
         self._storage_list = {k: {} for k in self._meta.storages}
 
-        self._storage_detached = {k: set() for k in self._meta.storages}
+        self._storage_attached = {k: set() for k in self._meta.storages}
         self._storage_index_counter = 0
         # {container_name : _TestingPebbleClient}
         self._pebble_clients = {}  # type: {str: _TestingPebbleClient}
@@ -1258,15 +1268,21 @@ def status_set(self, status, message='', *, is_app=False):
         else:
             self._unit_status = {'status': status, 'message': message}
 
-    def storage_list(self, name):
+    def storage_list(self, name: str, include_detached: bool = False):
+        """Returns a list of all attached storage mounts for the given storage name.
+
+        Args:
+            name: name (i.e. from metadata.yaml).
+            include_detached: True to include unattached storage mounts as well.
+        """
         return list(index for index in self._storage_list[name]
-                    if self._storage_is_attached(name, index))
+                    if all or self._storage_is_attached(name, index))
 
     def storage_get(self, storage_name_id, attribute):
         name, index = storage_name_id.split("/", 1)
         index = int(index)
         try:
-            if index in self._storage_detached[name]:
+            if index not in self._storage_attached[name]:
                 raise KeyError()  # Pretend the key isn't there
             else:
                 return self._storage_list[name][index][attribute]
@@ -1300,9 +1316,10 @@ def _storage_detach(self, storage_id: str):
             client._fs.remove_mount(name)
 
         if self._storage_is_attached(name, index):
-            self._storage_detached[name].add(index)
+            self._storage_attached[name].remove(index)
 
     def _storage_attach(self, storage_id: str):
+        """Mark the named storage_id as attached and return True if it was previously detached."""
         # NOTE: This is an extra function for _TestingModelBackend to simulate
         # re-attachment of a storage unit.  This is not present in
         # ops.model._ModelBackend.
@@ -1317,12 +1334,12 @@ def _storage_attach(self, storage_id: str):
 
         index = int(index)
         if not self._storage_is_attached(name, index):
-            self._storage_detached[name].remove(index)
+            self._storage_attached[name].add(index)
             return True
         return False
 
     def _storage_is_attached(self, storage_name, storage_index):
-        return storage_index not in self._storage_detached[storage_name]
+        return storage_index in self._storage_attached[storage_name]
 
     def _storage_remove(self, storage_id: str):
         # NOTE: This is an extra function for _TestingModelBackend to simulate
diff --git a/test/test_testing.py b/test/test_testing.py
index 32326b9bb..25fb09cdf 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -1317,16 +1317,40 @@ def test_add_storage_before_harness_begin(self):
         self.addCleanup(harness.cleanup)
 
         stor_ids = harness.add_storage("test", count=3)
-        self.assertSetEqual(set(self._extract_storage_index(stor_id) for stor_id in stor_ids),
-                            set(harness._backend.storage_list("test")))
-        want = str(pathlib.PurePath('test', '0'))
-        self.assertEqual(want, harness._backend.storage_get("test/0", "location")[-6:])
+        for s in stor_ids:
+            # before begin, adding storage does not attach it.
+            self.assertNotIn(s, harness._backend.storage_list("test"))
+
+        with self.assertRaises(ops.model.ModelError):
+            harness._backend.storage_get("test/0", "location")[-6:]
+
+    def test_add_storage_then_harness_begin(self):
+        harness = Harness(StorageTester, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            storage:
+                test:
+                    type: filesystem
+                    multiple:
+                        range: 1-3
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        harness.add_storage("test", count=3)
+
+        with self.assertRaises(ops.model.ModelError):
+            harness._backend.storage_get("test/0", "location")[-6:]
 
         harness.begin_with_initial_hooks()
         self.assertEqual(len(harness.charm.observed_events), 3)
         for i in range(3):
             self.assertTrue(isinstance(harness.charm.observed_events[i], StorageAttachedEvent))
 
+        want = str(pathlib.PurePath('test', '0'))
+        self.assertEqual(want, harness._backend.storage_get("test/0", "location")[-6:])
+
     def test_add_storage_without_metadata_key_fails(self):
         harness = Harness(CharmBase, meta='''
             name: test-app
@@ -1357,18 +1381,13 @@ def test_add_storage_after_harness_begin(self):
         self.addCleanup(harness.cleanup)
 
         # Set up initial storage
-        stor_id = harness.add_storage("test")[0]
-        self.assertIn(self._extract_storage_index(stor_id),
-                      harness._backend.storage_list("test"))
-        want = str(pathlib.PurePath('test', '0'))
-        self.assertEqual(want, harness._backend.storage_get(stor_id, "location")[-6:])
-
+        harness.add_storage("test")[0]
         harness.begin_with_initial_hooks()
         self.assertEqual(len(harness.charm.observed_events), 1)
         self.assertTrue(isinstance(harness.charm.observed_events[0], StorageAttachedEvent))
 
         # Add additional storage
-        stor_ids = harness.add_storage("test", count=3)
+        stor_ids = harness.add_storage("test", count=3, attach=True)
         # NOTE: stor_id now reflects the 4th ID.  The 2nd and 3rd IDs are created and
         # used, but not returned by Harness.add_storage.
         # (Should we consider changing its return type?)
@@ -1379,7 +1398,7 @@ def test_add_storage_after_harness_begin(self):
         for i in ['1', '2', '3']:
             storage_name = 'test/' + i
             want = str(pathlib.PurePath('test', i))
-            self.assertEqual(want, harness._backend.storage_get(storage_name, "location")[-6:])
+            self.assertTrue(harness._backend.storage_get(storage_name, "location").endswith(want))
         self.assertEqual(len(harness.charm.observed_events), 4)
         for i in range(1, 4):
             self.assertTrue(isinstance(harness.charm.observed_events[i], StorageAttachedEvent))
@@ -1459,7 +1478,7 @@ def test_storage_with_hyphens_works(self):
         # Set up initial storage
         harness.begin()
         helper = StorageWithHyphensHelper(harness.charm, "helper")
-        harness.add_storage("test-with-hyphens")[0]
+        harness.add_storage("test-with-hyphens", attach=True)[0]
 
         self.assertEqual(len(helper.changes), 1)