diff --git a/ops/charm.py b/ops/charm.py index 59ad4c5e7..eae71a2f6 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1540,7 +1540,9 @@ def __init__(self, name: str, raw: '_StorageMetaDict'): 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('-') diff --git a/ops/testing.py b/ops/testing.py index e3c9556e8..cb395be51 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -405,7 +405,11 @@ def begin_with_initial_hooks(self) -> None: 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 @@ def add_storage(self, storage_name: str, count: int = 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. + 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 @@ 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 + 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 @@ def _storage_attach(self, storage_id: str): 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 1a9ac52c5..9767588a2 100644 --- a/test/test_charm.py +++ b/test/test_charm.py @@ -282,6 +282,10 @@ def _on_stor4_attach(self, event: ops.StorageAttachedEvent): multiple: range: 2- type: filesystem + stor-plus: + multiple: + range: 10+ + type: filesystem ''') fake_script( @@ -329,6 +333,7 @@ def _on_stor4_attach(self, event: ops.StorageAttachedEvent): 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 fcb33694f..d531e08d5 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -4749,6 +4749,96 @@ def test_storage_mount(self): 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 ` 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):