Repository: canonical/operator

Issue:
Using Harness.add_storage(..., attach=True) before begin_with_initial_hooks gives confusing error
For example:

```python
import ops
import ops.testing

class MyCharm(ops.CharmBase):
    pass

meta = """
            containers:
                test-container:
                    mounts:
                        - storage: test-storage
                          location: /mounts/foo
            storage:
                test-storage:
                    type: filesystem
"""

h = ops.testing.Harness(MyCharm, meta=meta)
try:
    h.add_storage("test-storage", attach=True)
    h.begin_with_initial_hooks()
finally:
    h.cleanup()
```

If `begin()` is used instead of `begin_with_initial_hooks`, everything is ok, because `begin_with_initial_hooks` does the attaching (to then emit the `StorageAttached` event) and `begin` does not. Similarly, if `attach=True` is not used, everything is ok.

However, when run as above, an error like this is generated:

```
Traceback (most recent call last):
  File "/tmp/storagetest/test.py", line 41, in <module>
    h.begin_with_initial_hooks()
  File "/tmp/storagetest/.venv/lib/python3.11/site-packages/ops/testing.py", line 407, in begin_with_initial_hooks
    self.attach_storage(s.full_id)
  File "/tmp/storagetest/.venv/lib/python3.11/site-packages/ops/testing.py", line 752, in attach_storage
    if not self._backend._storage_attach(storage_id):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/storagetest/.venv/lib/python3.11/site-packages/ops/testing.py", line 2348, in _storage_attach
    mounting_dir.symlink_to(target_dir)
  File "/usr/lib/python3.11/pathlib.py", line 1199, in symlink_to
    os.symlink(target, self, target_is_directory)
FileExistsError: [Errno 17] File exists: '/tmp/ops-harness-z2tflymi/storages/test-storage/0' -> '/tmp/ops-harness-z2tflymi/containers/test-container/mounts/foo'
```

This happens because the `add_storage` call mounts the storage, and then the `begin_with_initial_hooks` attempts to mount it again, but that's already happened, so the symlinking fails.

We should:

* [ ] Adjust the API documentation so that it explicitly says that `attach` does nothing if `begin` hasn't yet been called
* [ ] Verify that `begin` has been called (ie. `._charm is not None` before trying to mount the storage

Discussion:
IronCore864: Great test cases, I've learned a lot :)

Patch:
diff --git a/ops/charm.py b/ops/charm.py
index 59ad4c5..eae71a2 100644
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -1540,7 +1540,9 @@ class StorageMeta:
         self.multiple_range = None
         if 'multiple' in raw:
             range = raw['multiple']['range']
-            if '-' not in range:
+            if range[-1] == '+':
+                self.multiple_range = (int(range[:-1]), None)
+            elif '-' not in range:
                 self.multiple_range = (int(range), int(range))
             else:
                 range = range.split('-')


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index e3c9556..cb395be 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -405,7 +405,11 @@ class Harness(Generic[CharmType]):
         for storage_name in self._meta.storages:
             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)
+                if self._backend._storage_is_attached(storage_name, storage_index):
+                    # Attaching was done already, but we still need the event to be emitted.
+                    self.charm.on[storage_name].storage_attached.emit(s)
+                else:
+                    self.attach_storage(s.full_id)
         # Storage done, emit install event
         charm.on.install.emit()
 
@@ -690,8 +694,8 @@ class Harness(Generic[CharmType]):
         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.
+            attach: True to also attach the storage mount; if :meth:`begin`
+                has been called a True value will also emit storage-attached
 
         Return:
             A list of storage IDs, e.g. ["my-storage/1", "my-storage/2"].
@@ -739,12 +743,12 @@ class Harness(Generic[CharmType]):
         """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
+        If called after :meth:`begin` and hooks are not disabled, it will trigger
+        a storage-attached hook if the storage unit in question exists
         and is presently marked as detached.
 
         The test harness uses symbolic links to imitate storage mounts, which may lead to some
-        inconsistencies compared to the actual charm. Users should be cognizant of
-        this potential discrepancy.
+        inconsistencies compared to the actual charm.
 
         Args:
             storage_id: The full storage ID of the storage unit being attached, including the
@@ -2339,7 +2343,17 @@ class _TestingModelBackend:
                     mounting_dir.parent.mkdir(parents=True, exist_ok=True)
                     target_dir = pathlib.Path(store["location"])
                     target_dir.mkdir(parents=True, exist_ok=True)
-                    mounting_dir.symlink_to(target_dir)
+                    try:
+                        mounting_dir.symlink_to(target_dir, target_is_directory=True)
+                    except FileExistsError:
+                        # If the symlink is already the one we want, then we
+                        # don't need to do anything here.
+                        # NOTE: In Python 3.9, this can use `mounting_dir.readlink()`
+                        if (
+                            not mounting_dir.is_symlink()
+                            or os.readlink(mounting_dir) != str(target_dir)
+                        ):
+                            raise
 
         index = int(index)
         if not self._storage_is_attached(name, index):
diff --git a/test/test_charm.py b/test/test_charm.py
index 1a9ac52..9767588 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -282,6 +282,10 @@ storage:
     multiple:
       range: 2-
     type: filesystem
+  stor-plus:
+    multiple:
+      range: 10+
+    type: filesystem
 ''')
 
         fake_script(
@@ -329,6 +333,7 @@ storage:
         self.assertEqual(self.meta.storages['stor2'].multiple_range, (2, 2))
         self.assertEqual(self.meta.storages['stor3'].multiple_range, (2, None))
         self.assertEqual(self.meta.storages['stor-4'].multiple_range, (2, 4))
+        self.assertEqual(self.meta.storages['stor-plus'].multiple_range, (10, None))
 
         charm = MyCharm(self.create_framework())
 
diff --git a/test/test_testing.py b/test/test_testing.py
index fcb3369..d531e08 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -4749,6 +4749,96 @@ class TestFilesystem(unittest.TestCase, _TestingPebbleClientMixin):
         self.harness.attach_storage(storage_id)
         self.assertTrue((self.root / "mounts/foo/bar").read_text(), "foobar")
 
+    def _make_storage_attach_harness(self, meta: typing.Optional[str] = None):
+        class MyCharm(ops.CharmBase):
+            def __init__(self, framework: ops.Framework):
+                super().__init__(framework)
+                self.attached: typing.List[str] = []
+                self.locations: typing.List[pathlib.Path] = []
+                framework.observe(self.on['test-storage'].storage_attached, self._on_attach)
+
+            def _on_attach(self, event: ops.StorageAttachedEvent):
+                self.attached.append(event.storage.full_id)
+                self.locations.append(event.storage.location)
+
+        if meta is None:
+            meta = '''
+                name: test
+                containers:
+                    test-container:
+                        mounts:
+                            - storage: test-storage
+                              location: /mounts/foo
+                storage:
+                    test-storage:
+                        type: filesystem
+                '''
+        harness = ops.testing.Harness(MyCharm, meta=meta)
+        self.addCleanup(harness.cleanup)
+        return harness
+
+    def test_storage_attach_begin_no_emit(self):
+        """If `begin()` hasn't been called, `attach` does not emit storage-attached."""
+        harness = self._make_storage_attach_harness()
+        harness.add_storage('test-storage', attach=True)
+        harness.begin()
+        self.assertNotIn('test-storage/0', harness.charm.attached)
+
+    def test_storage_attach_begin_with_hooks_emits(self):
+        """`attach` doesn't emit storage-attached before `begin_with_initial_hooks`."""
+        harness = self._make_storage_attach_harness()
+        harness.add_storage('test-storage', attach=True)
+        harness.begin_with_initial_hooks()
+        self.assertIn('test-storage/0', harness.charm.attached)
+        self.assertTrue(harness.charm.locations[0])
+
+    def test_storage_add_with_later_attach(self):
+        harness = self._make_storage_attach_harness()
+        harness.begin()
+        storage_ids = harness.add_storage('test-storage', attach=False)
+        self.assertNotIn('test-storage/0', harness.charm.attached)
+        for s_id in storage_ids:
+            harness.attach_storage(s_id)
+            # It's safe to call `attach_storage` more than once, and this will
+            # only trigger the event once - this is the same as executing
+            # `juju attach-storage <unit> <storage>` more than once.
+            harness.attach_storage(s_id)
+        self.assertEqual(harness.charm.attached.count('test-storage/0'), 1)
+
+    def test_storage_machine_charm_metadata(self):
+        meta = '''
+            name: test
+            storage:
+                test-storage:
+                    type: filesystem
+                    mount: /mounts/foo
+            '''
+        harness = self._make_storage_attach_harness(meta)
+        harness.begin()
+        harness.add_storage('test-storage', attach=True)
+        self.assertIn('test-storage/0', harness.charm.attached)
+
+    def test_storage_multiple_storage_instances(self):
+        meta = '''
+            name: test
+            storage:
+                test-storage:
+                    type: filesystem
+                    mount: /mounts/foo
+                    multiple:
+                        range: 2-4
+            '''
+        harness = self._make_storage_attach_harness(meta)
+        harness.begin()
+        harness.add_storage('test-storage', 2, attach=True)
+        self.assertEqual(harness.charm.attached, ['test-storage/0', 'test-storage/1'])
+        self.assertNotEqual(harness.charm.locations[0], harness.charm.locations[1])
+        harness.add_storage('test-storage', 2, attach=True)
+        self.assertEqual(
+            harness.charm.attached, [
+                'test-storage/0', 'test-storage/1', 'test-storage/2', 'test-storage/3'])
+        self.assertEqual(len(set(harness.charm.locations)), 4)
+
 
 class TestSecrets(unittest.TestCase):
     def test_add_model_secret_by_app_name_str(self):