Repository: canonical/operator
Pull request #628 (2021-09-30 08:01:59)

Extended and modified Harness storage API
* Added detach, attach and remove functions to simulate storage being
  attached/detached/removed via the Juju CLI.

* Renamed some test names.

* Added firing of hooks post-Harness.begin() when appropriate Harness
  functions are called.

* Hook firing counts changed to mimic Juju behavior, e.g. if 3 storage
  units get attached (even via a single call), 3 events should be
  emitted.

* Some minor tweaks to the existing testing backend implementation for
  tracking storage state.

jnsgruk: This PR is actually from @Vultaire -- I made a big mess, and hence this is reopened.
Vultaire: Based on discussion today re: storage events and #648, I've added appropriate events to the harness code and done some relevant reworks for this plus for other feedback received.

However, as #648 is not merged, I simply pulled Ryan's changes locally and tested against his code without committing it into this branch, thus this **should not be merged right now** since it depends on changes he makes to the Storage and StorageEvent classes.
Vultaire: To be clear, the current failures are believed to be due to the code from #648 not being merged yet.
jameinel: I would probably focus on attach/detach as the Harness methods, as those
are associated with the hooks that are fired (storage-attached,
storage-detached), vs the add/remove Harness methods.

I can certainly see a case for wanting to have the charm handle when it
sees storage that was used previously (rotating out an old postgresql unit
and reattaching its storage to a new postgresql unit). However, from the
perspective of the charm, I'm pretty sure they only ever see attach/detach
where the thing being attached may be pristine or it may come with existing
data.

We may need to simulate actual content blocks at some point in the future,
or have nice ways for testing to give a test/temp directory for where the
data is associated.

John
=:->


On Thu, Dec 9, 2021 at 4:12 AM Jon Seager ***@***.***> wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In ops/testing.py
> <https://github.com/canonical/operator/pull/628#discussion_r765574259>:
>
> > +        It will trigger a storage-attached hook if the storage unit in question exists
> +        and is presently marked as detached.
> +
> +        Args:
> +            storage_id: The full storage ID of the storage unit being attached, including the
> +                storage key, e.g. my-storage/0.
> +        """
> +        if self.charm is None:
> +            raise RuntimeError('cannot attach storage before Harness is initialised')
> +        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))
> +
> +    def remove_storage(self, storage_id: str) -> None:
>
> Yeh, I can see how this might simplify things slightly. Ultimately so long
> as we have a way for charmers to say "hey charm, here's some storage" and
> validate that their hooks do the right thing, then we're good - certainly
> for a first implementation.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/628#discussion_r765574259>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7MJ4KW437CXBOIUS33UQBXIZANCNFSM5FBV6VSQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>


Patch:
diff --git a/ops/model.py b/ops/model.py
index 06f48f2..9974af9 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1021,6 +1021,13 @@ class StorageMapping(Mapping):
                               ' it is not present in the charm metadata').format(storage_name))
         self._backend.storage_add(storage_name, count)
 
+    def _invalidate(self, storage_name):
+        """Remove an entry from the storage map.
+
+        Not meant to be used by charm authors -- this exists mainly for testing purposes.
+        """
+        self._storage_map[storage_name] = None
+
 
 class Storage:
     """Represents a storage as defined in metadata.yaml.


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 84b6382..fde981d 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -178,10 +178,11 @@ class Harness(typing.Generic[CharmType]):
         # Checking if disks have been added
         # storage-attached events happen before install
         for storage_name in self._meta.storages:
-            if len(self._backend.storage_list(storage_name)) > 0:
-                # Storage device(s) detected, emit storage-attached event
-                storage_name = storage_name.replace('-', '_')
-                self._charm.on[storage_name].storage_attached.emit()
+            storage_name = storage_name.replace('-', '_')
+            for storage_index in self._backend.storage_list(storage_name):
+                # 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))
         # 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
@@ -391,7 +392,7 @@ class Harness(typing.Generic[CharmType]):
         self._relation_id_counter += 1
         return rel_id
 
-    def add_storage(self, storage_name: str, count: int = 1) -> int:
+    def add_storage(self, storage_name: str, count: int = 1) -> typing.List[str]:
         """Declare a new storage device attached to this unit.
 
         To have repeatable tests, each device will be initialized with
@@ -403,9 +404,82 @@ class Harness(typing.Generic[CharmType]):
             count: Number of disks being added
 
         Return:
-            The storage_id created
+            A list of storage IDs, e.g. ["my-storage/1", "my-storage/2"].
         """
-        return self._backend.storage_add(storage_name, count)
+        if storage_name not in self._meta.storages:
+            raise RuntimeError(
+                "the key '{}' is not specified as a storage key in metadata".format(storage_name))
+        storage_indices = self._backend.storage_add(storage_name, count)
+
+        # Reset associated cached value in the storage mappings.  If we don't do this,
+        # Model._storages won't return Storage objects for subsequently-added storage.
+        self._model._storages._invalidate(storage_name)
+
+        if self.charm is not None and self._hooks_enabled:
+            for storage_index in storage_indices:
+                self.charm.on[storage_name].storage_attached.emit(
+                    model.Storage(storage_name, storage_index, self._backend))
+        return ["{}/{}".format(storage_name, storage_index) for storage_index in storage_indices]
+
+    def detach_storage(self, storage_id: str) -> None:
+        """Detach a storage device.
+
+        The intent of this function is to simulate a "juju detach-storage" call.
+        It will trigger a storage-detaching hook if the storage unit in question exists
+        and is presently marked as attached.
+
+        Args:
+            storage_id: The full storage ID of th e storage unit being detached, including the
+                storage key, e.g. my-storage/0.
+        """
+        if self.charm is None:
+            raise RuntimeError('cannot detach storage before Harness is initialised')
+        storage_name, storage_index = storage_id.split('/', 1)
+        storage_index = int(storage_index)
+        if self._backend._storage_is_attached(storage_name, storage_index) and self._hooks_enabled:
+            self.charm.on[storage_name].storage_detaching.emit(
+                model.Storage(storage_name, storage_index, self._backend))
+        self._backend._storage_detach(storage_id)
+
+    def attach_storage(self, storage_id: str) -> None:
+        """Attach a storage device.
+
+        The intent of this function is to simulate a "juju attach-storage" call.
+        It will trigger a storage-attached hook if the storage unit in question exists
+        and is presently marked as detached.
+
+        Args:
+            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))
+
+    def remove_storage(self, storage_id: str) -> None:
+        """Attach a storage device.
+
+        The intent of this function is to simulate a "juju remove-storage" call.
+        It will trigger a storage-detaching hook if the storage unit in question exists
+        and is presently marked as detached.  Additionally, it will remove the storage
+        unit from the testing backend.
+
+        Args:
+            storage_id: The full storage ID of the storage unit being removed, including the
+                storage key, e.g. my-storage/0.
+        """
+        storage_name, storage_index = storage_id.split('/', 1)
+        storage_index = int(storage_index)
+        if storage_name not in self._meta.storages:
+            raise RuntimeError(
+                "the key '{}' is not specified as a storage key in metadata".format(storage_name))
+        is_attached = self._backend._storage_is_attached(storage_name, storage_index)
+        if self.charm is not None and self._hooks_enabled and is_attached:
+            self.charm.on[storage_name].storage_detaching.emit(
+                model.Storage(storage_name, storage_index, self._backend))
+        self._backend._storage_remove(storage_id)
 
     def add_relation(self, relation_name: str, remote_app: str) -> int:
         """Declare that there is a new relation between this app and `remote_app`.
@@ -949,9 +1023,7 @@ class _TestingModelBackend:
         # <ID1>: device id that is key for given storage_name
         # Initialize the _storage_list with values present on metadata.yaml
         self._storage_list = {k: {} for k in self._meta.storages}
-        # Every new storage device gets an id from the _storage_id_counter.
-        # That id is mapped back to the storage name on _storage_ids_map
-        self._storage_ids_map = {}
+        self._storage_detached = {k: set() for k in self._meta.storages}
         self._storage_id_counter = 0
         # {socket_path : _TestingPebbleClient}
         # socket_path = '/charm/containers/{container_name}/pebble.socket'
@@ -1057,24 +1129,64 @@ class _TestingModelBackend:
             self._unit_status = {'status': status, 'message': message}
 
     def storage_list(self, name):
-        return list(self._storage_list[name])
+        return list(index for index in self._storage_list[name]
+                    if self._storage_is_attached(name, index))
 
     def storage_get(self, storage_name_id, attribute):
-        name = self._storage_ids_map[storage_name_id]
-        id = storage_name_id.split("/")[1]
-        return self._storage_list[name][id][attribute]
+        name, index = storage_name_id.split("/", 1)
+        index = int(index)
+        try:
+            if index in self._storage_detached[name]:
+                raise KeyError()  # Pretend the key isn't there
+            else:
+                return self._storage_list[name][index][attribute]
+        except KeyError:
+            raise model.ModelError(
+                'ERROR invalid value "{}/{}" for option -s: storage not found'.format(name, index))
 
-    def storage_add(self, name, count=1):
+    def storage_add(self, name: str, count: int = 1):
         if name not in self._storage_list:
             self._storage_list[name] = {}
+        result = []
         for i in range(count):
-            storage_id = self._storage_id_counter
+            index = self._storage_id_counter
             self._storage_id_counter += 1
-            self._storage_list[name][str(storage_id)] = {
-                "location": "/{}{}".format(name, i)
+            self._storage_list[name][index] = {
+                "location": "/{}/{}".format(name, index)
             }
-            self._storage_ids_map['{}/{}'.format(name, storage_id)] = name
-        return storage_id
+            result.append(index)
+        return result
+
+    def _storage_detach(self, storage_id: str):
+        # NOTE: This is an extra function for _TestingModelBackend to simulate
+        # detachment of a storage unit.  This is not present in ops.model._ModelBackend.
+        name, index = storage_id.split('/', 1)
+        index = int(index)
+        if self._storage_is_attached(name, index):
+            self._storage_detached[name].add(index)
+
+    def _storage_attach(self, storage_id: str):
+        # NOTE: This is an extra function for _TestingModelBackend to simulate
+        # re-attachment of a storage unit.  This is not present in
+        # ops.model._ModelBackend.
+        name, index = storage_id.split('/', 1)
+        index = int(index)
+        if not self._storage_is_attached(name, index):
+            self._storage_detached[name].remove(index)
+            return True
+        return False
+
+    def _storage_is_attached(self, storage_name, storage_index):
+        return storage_index not in self._storage_detached[storage_name]
+
+    def _storage_remove(self, storage_id: str):
+        # NOTE: This is an extra function for _TestingModelBackend to simulate
+        # full removal of a storage unit.  This is not present in
+        # ops.model._ModelBackend.
+        self._storage_detach(storage_id)
+        name, index = storage_id.split('/', 1)
+        index = int(index)
+        self._storage_list[name].pop(index, None)
 
     def action_get(self):
         raise NotImplementedError(self.action_get)
diff --git a/test/test_testing.py b/test/test_testing.py
index f8784b9..9b0407b 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -27,7 +27,13 @@ from io import BytesIO, StringIO
 import yaml
 
 from ops import pebble
-from ops.charm import CharmBase, PebbleReadyEvent, RelationEvent
+from ops.charm import (
+    CharmBase,
+    PebbleReadyEvent,
+    RelationEvent,
+    StorageAttachedEvent,
+    StorageDetachingEvent,
+)
 from ops.framework import Object
 from ops.model import (
     ActiveStatus,
@@ -48,6 +54,24 @@ from ops.testing import (
 )
 
 
+class StorageTester(CharmBase):
+    """Record the relation-changed events."""
+
+    def __init__(self, framework):
+        super().__init__(framework)
+        self.observed_events = []
+        self.framework.observe(self.on.test_storage_attached,
+                               self._on_test_storage_attached)
+        self.framework.observe(self.on.test_storage_detaching,
+                               self._on_test_storage_detaching)
+
+    def _on_test_storage_attached(self, event):
+        self.observed_events.append(event)
+
+    def _on_test_storage_detaching(self, event):
+        self.observed_events.append(event)
+
+
 class TestHarness(unittest.TestCase):
 
     def test_add_relation(self):
@@ -1088,7 +1112,227 @@ class TestHarness(unittest.TestCase):
         self.assertEqual(harness.model.name, 'foo')
         self.assertEqual(harness.model.uuid, '96957e90-e006-11eb-ba80-0242ac130004')
 
-    def test_storage_add(self):
+    def test_add_storage_before_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)
+
+        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")))
+        self.assertEqual("/test/0", harness._backend.storage_get("test/0", "location"))
+
+        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))
+
+    def test_add_storage_without_metadata_key_fails(self):
+        harness = Harness(CharmBase, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        with self.assertRaises(RuntimeError) as cm:
+            harness.add_storage("test")
+        self.assertEqual(
+            cm.exception.args[0],
+            "the key 'test' is not specified as a storage key in metadata")
+
+    def test_add_storage_after_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)
+
+        # Set up initial storage
+        stor_id = harness.add_storage("test")[0]
+        self.assertIn(self._extract_storage_index(stor_id),
+                      harness._backend.storage_list("test"))
+        self.assertEqual("/test/0", harness._backend.storage_get(stor_id, "location"))
+
+        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)
+        # 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?)
+
+        added_indices = {self._extract_storage_index(stor_id) for stor_id in stor_ids}
+        self.assertTrue(added_indices.issubset(set(harness._backend.storage_list("test"))))
+        self.assertEqual("/test/1", harness._backend.storage_get("test/1", "location"))
+        self.assertEqual("/test/2", harness._backend.storage_get("test/2", "location"))
+        self.assertEqual("/test/3", harness._backend.storage_get("test/3", "location"))
+        self.assertEqual(len(harness.charm.observed_events), 4)
+        for i in range(1, 4):
+            self.assertTrue(isinstance(harness.charm.observed_events[i], StorageAttachedEvent))
+
+    def test_detach_storage(self):
+        harness = Harness(StorageTester, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            storage:
+                test:
+                    type: filesystem
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        # Set up initial storage
+        stor_id = 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))
+
+        # Detach storage
+        harness.detach_storage(stor_id)
+        self.assertEqual(len(harness.charm.observed_events), 2)
+        self.assertTrue(isinstance(harness.charm.observed_events[1], StorageDetachingEvent))
+
+        # Verify backend functions return appropriate values.
+        # Real backend would return info only for actively attached storage units.
+        self.assertNotIn(stor_id, harness._backend.storage_list("test"))
+        with self.assertRaises(ModelError) as cm:
+            harness._backend.storage_get("test/0", "location")
+        # Error message modeled after output of
+        # "storage-get -s <invalid/inactive id> location" on real deployment
+        self.assertEqual(
+            cm.exception.args[0],
+            'ERROR invalid value "test/0" for option -s: storage not found')
+
+        # Retry detach
+        # Since already detached, no more hooks should fire
+        harness.detach_storage(stor_id)
+        self.assertEqual(len(harness.charm.observed_events), 2)
+        self.assertTrue(isinstance(harness.charm.observed_events[1], StorageDetachingEvent))
+
+    def test_detach_storage_before_harness_begin(self):
+        harness = Harness(StorageTester, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            storage:
+                test:
+                    type: filesystem
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        stor_id = harness.add_storage("test")[0]
+        with self.assertRaises(RuntimeError) as cm:
+            harness.detach_storage("test/{}".format(stor_id))
+        self.assertEqual(cm.exception.args[0],
+                         "cannot detach storage before Harness is initialised")
+
+    def test_attach_storage(self):
+        harness = Harness(StorageTester, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            storage:
+                test:
+                    type: filesystem
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        # Set up initial storage
+        stor_id = 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))
+
+        # Detach storage
+        harness.detach_storage(stor_id)
+        self.assertEqual(len(harness.charm.observed_events), 2)
+        self.assertTrue(isinstance(harness.charm.observed_events[1], StorageDetachingEvent))
+
+        # Re-attach storage
+        harness.attach_storage(stor_id)
+        self.assertEqual(len(harness.charm.observed_events), 3)
+        self.assertTrue(isinstance(harness.charm.observed_events[2], StorageAttachedEvent))
+
+        # Verify backend functions return appropriate values.
+        # Real backend would return info only for actively attached storage units.
+        self.assertIn(self._extract_storage_index(stor_id), harness._backend.storage_list("test"))
+        self.assertEqual("/test/0", harness._backend.storage_get("test/0", "location"))
+
+        # Retry attach
+        # Since already detached, no more hooks should fire
+        harness.attach_storage(stor_id)
+        self.assertEqual(len(harness.charm.observed_events), 3)
+        self.assertTrue(isinstance(harness.charm.observed_events[2], StorageAttachedEvent))
+
+    def test_attach_storage_before_harness_begin(self):
+        harness = Harness(StorageTester, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            storage:
+                test:
+                    type: filesystem
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        # We deliberately don't guard against attaching storage before the harness begins,
+        # as there are legitimate reasons to do so.
+        stor_id = harness.add_storage("test")[0]
+        self.assertTrue(stor_id)
+
+    def test_remove_storage_before_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)
+
+        stor_ids = harness.add_storage("test", count=2)
+        harness.remove_storage(stor_ids[0])
+        # Note re: delta between real behavior and Harness: Juju doesn't allow removal
+        # of the last attached storage unit while a workload is still running.  To more
+        # easily allow testing of storage removal, I am presently ignoring this detail.
+        # (Otherwise, the user would need to flag somehow that they are intentionally
+        # removing the final unit as part of a shutdown procedure, else it'd block the
+        # removal.  I'm not sure such behavior is productive.)
+
+        harness.begin_with_initial_hooks()
+        # Only one hook will fire; one won't since it was removed
+        self.assertEqual(len(harness.charm.observed_events), 1)
+        self.assertTrue(isinstance(harness.charm.observed_events[0], StorageAttachedEvent))
+
+    def test_remove_storage_without_metadata_key_fails(self):
         harness = Harness(CharmBase, meta='''
             name: test-app
             requires:
@@ -1097,11 +1341,67 @@ class TestHarness(unittest.TestCase):
             ''')
         self.addCleanup(harness.cleanup)
 
-        stor_id = harness.add_storage("test")
-        self.assertIsNotNone(stor_id)
+        # Doesn't really make sense since we already can't add storage which isn't in the metadata,
+        # but included for completeness.
+        with self.assertRaises(RuntimeError) as cm:
+            harness.remove_storage("test/0")
+        self.assertEqual(
+            cm.exception.args[0],
+            "the key 'test' is not specified as a storage key in metadata")
+
+    def test_remove_storage_after_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)
+
+        stor_ids = harness.add_storage("test", count=2)
+        harness.begin_with_initial_hooks()
+        self.assertEqual(len(harness.charm.observed_events), 2)
+        self.assertTrue(isinstance(harness.charm.observed_events[0], StorageAttachedEvent))
+        self.assertTrue(isinstance(harness.charm.observed_events[1], StorageAttachedEvent))
+
+        harness.remove_storage(stor_ids[1])
+        self.assertEqual(len(harness.charm.observed_events), 3)
+        self.assertTrue(isinstance(harness.charm.observed_events[2], StorageDetachingEvent))
+
+        attached_storage_ids = harness._backend.storage_list("test")
+        self.assertIn(self._extract_storage_index(stor_ids[0]), attached_storage_ids)
+        self.assertNotIn(self._extract_storage_index(stor_ids[1]), attached_storage_ids)
 
-        self.assertIn(str(stor_id), harness._backend.storage_list("test"))
-        self.assertEqual("/test0", harness._backend.storage_get("test/0", "location"))
+    def _extract_storage_index(self, stor_id):
+        return int(stor_id.split('/')[-1])
+
+    def test_remove_detached_storage(self):
+        harness = Harness(StorageTester, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            storage:
+                test:
+                    type: filesystem
+                    multiple:
+                        range: 1-3
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        stor_ids = harness.add_storage("test", count=2)
+        harness.begin_with_initial_hooks()
+        harness.detach_storage(stor_ids[0])
+        harness.remove_storage(stor_ids[0])  # Already detached, so won't fire a hook
+        self.assertEqual(len(harness.charm.observed_events), 3)
+        self.assertTrue(isinstance(harness.charm.observed_events[0], StorageAttachedEvent))
+        self.assertTrue(isinstance(harness.charm.observed_events[1], StorageAttachedEvent))
+        self.assertTrue(isinstance(harness.charm.observed_events[2], StorageDetachingEvent))
 
     def test_actions_from_directory(self):
         tmp = pathlib.Path(tempfile.mkdtemp())