From 9c02fb1a4034ca06991eb58fbc5ae73a387d0bfe Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 13 Mar 2024 19:57:21 +1300 Subject: [PATCH 1/5] Support 'range: +' in storage multiple. --- ops/charm.py | 4 +++- test/test_charm.py | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ops/charm.py b/ops/charm.py index db02e13d9..c294d072e 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1539,7 +1539,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/test/test_charm.py b/test/test_charm.py index 627d2d52c..765fbc99a 100644 --- a/test/test_charm.py +++ b/test/test_charm.py @@ -258,6 +258,10 @@ def _on_stor4_attach(self, event: ops.StorageAttachedEvent): multiple: range: 2- type: filesystem + stor-plus: + multiple: + range: 10+ + type: filesystem ''') fake_script( @@ -305,6 +309,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()) From a0522aabd4b91399732276f36a366646beb8cb41 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 13 Mar 2024 20:19:07 +1300 Subject: [PATCH 2/5] Improve storage attaching and extend the tests around it. --- ops/testing.py | 23 ++++++++--- test/test_testing.py | 90 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 7 deletions(-) diff --git a/ops/testing.py b/ops/testing.py index 5014ef516..202a129bf 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -690,8 +690,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,21 +739,22 @@ 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. + If called before :meth:`begin`, or with hooks disabled, it will have no effect. 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 + inconsistencies compared to the actual charm. Users should be cognisant of this potential discrepancy. Args: storage_id: The full storage ID of the storage unit being attached, including the storage key, e.g. my-storage/0. """ - 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 + if not self._backend._storage_attach(storage_id): + return # storage was already attached storage_name, storage_index = storage_id.split('/', 1) @@ -2322,7 +2323,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_testing.py b/test/test_testing.py index 47dd36e16..5cb7376ed 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -4496,9 +4496,9 @@ def test_container_storage_mounts(self): self.addCleanup(harness.cleanup) store_id = harness.add_storage('store1')[0] - harness.attach_storage(store_id) harness.begin() + harness.attach_storage(store_id) harness.set_can_connect('c1', True) harness.set_can_connect('c2', True) harness.set_can_connect('c3', True) @@ -4738,6 +4738,94 @@ def test_storage_mount(self): self.harness.attach_storage(storage_id) self.assertTrue((self.root / "mounts/foo/bar").read_text(), "foobar") + def test_storage_attach(self): + 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) + + meta = ''' + name: test + containers: + test-container: + mounts: + - storage: test-storage + location: /mounts/foo + storage: + test-storage: + type: filesystem + ''' + # If `begin()` hasn't been called, `attach` does not emit storage-attached. + harness = ops.testing.Harness(MyCharm, meta=meta) + self.addCleanup(harness.cleanup) + harness.add_storage('test-storage', attach=True) + harness.begin() + self.assertNotIn('test-storage/0', harness.charm.attached) + + # `attach` doesn't emit storage-attached before `begin_with_initial_hooks`. + harness = ops.testing.Harness(MyCharm, meta=meta) + self.addCleanup(harness.cleanup) + 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]) + + # We can add the storage and attach it later. + harness = ops.testing.Harness(MyCharm, meta=meta) + self.addCleanup(harness.cleanup) + 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) + + # Machine charms have slightly different metadata. + meta = ''' + name: test + storage: + test-storage: + type: filesystem + mount: /mounts/foo + ''' + harness = ops.testing.Harness(MyCharm, meta=meta) + self.addCleanup(harness.cleanup) + harness.begin() + harness.add_storage('test-storage', attach=True) + self.assertIn('test-storage/0', harness.charm.attached) + + # In a machine charm, we can request multiple storage instances. + meta = ''' + name: test + storage: + test-storage: + type: filesystem + mount: /mounts/foo + multiple: + range: 2-4 + ''' + harness = ops.testing.Harness(MyCharm, meta=meta) + self.addCleanup(harness.cleanup) + 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): From 25b9f31c0c47416fb51571a8c06c80452f74a863 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 13 Mar 2024 20:35:10 +1300 Subject: [PATCH 3/5] Make backwards compatible. --- ops/testing.py | 14 +++++++++----- test/test_testing.py | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/ops/testing.py b/ops/testing.py index 202a129bf..bbe513728 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() @@ -739,8 +743,8 @@ 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. - If called before :meth:`begin`, or with hooks disabled, it will have no effect. - 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 @@ -751,10 +755,10 @@ 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 not self._charm or not self._hooks_enabled: - return # don't need to run hook callback 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) diff --git a/test/test_testing.py b/test/test_testing.py index 5cb7376ed..9a2daccb3 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -4496,9 +4496,9 @@ def test_container_storage_mounts(self): self.addCleanup(harness.cleanup) store_id = harness.add_storage('store1')[0] + harness.attach_storage(store_id) harness.begin() - harness.attach_storage(store_id) harness.set_can_connect('c1', True) harness.set_can_connect('c2', True) harness.set_can_connect('c3', True) From 2485fc9752f770a50b833ff64250da291f70079a Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 14 Mar 2024 17:58:45 +1300 Subject: [PATCH 4/5] Remove doc sentence as per review. --- ops/testing.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ops/testing.py b/ops/testing.py index 16def3177..a7d2927a8 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -748,8 +748,7 @@ def attach_storage(self, storage_id: str) -> None: 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 cognisant 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 From 5f7161fdf37510f963a1f599652ab58b171bef42 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 14 Mar 2024 18:07:25 +1300 Subject: [PATCH 5/5] Split into separate tests, as per code review. --- test/test_testing.py | 52 +++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/test/test_testing.py b/test/test_testing.py index 9a2daccb3..3f6763fe4 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -4738,7 +4738,7 @@ def test_storage_mount(self): self.harness.attach_storage(storage_id) self.assertTrue((self.root / "mounts/foo/bar").read_text(), "foobar") - def test_storage_attach(self): + def _make_storage_attach_harness(self, meta: typing.Optional[str] = None): class MyCharm(ops.CharmBase): def __init__(self, framework: ops.Framework): super().__init__(framework) @@ -4750,35 +4750,39 @@ def _on_attach(self, event: ops.StorageAttachedEvent): self.attached.append(event.storage.full_id) self.locations.append(event.storage.location) - meta = ''' - name: test - containers: - test-container: - mounts: - - storage: test-storage - location: /mounts/foo - storage: - test-storage: - type: filesystem - ''' - # If `begin()` hasn't been called, `attach` does not emit storage-attached. + 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) - # `attach` doesn't emit storage-attached before `begin_with_initial_hooks`. - harness = ops.testing.Harness(MyCharm, meta=meta) - self.addCleanup(harness.cleanup) + 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]) - # We can add the storage and attach it later. - harness = ops.testing.Harness(MyCharm, meta=meta) - self.addCleanup(harness.cleanup) + 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) @@ -4790,7 +4794,7 @@ def _on_attach(self, event: ops.StorageAttachedEvent): harness.attach_storage(s_id) self.assertEqual(harness.charm.attached.count('test-storage/0'), 1) - # Machine charms have slightly different metadata. + def test_storage_machine_charm_metadata(self): meta = ''' name: test storage: @@ -4798,13 +4802,12 @@ def _on_attach(self, event: ops.StorageAttachedEvent): type: filesystem mount: /mounts/foo ''' - harness = ops.testing.Harness(MyCharm, meta=meta) - self.addCleanup(harness.cleanup) + harness = self._make_storage_attach_harness(meta) harness.begin() harness.add_storage('test-storage', attach=True) self.assertIn('test-storage/0', harness.charm.attached) - # In a machine charm, we can request multiple storage instances. + def test_storage_multiple_storage_instances(self): meta = ''' name: test storage: @@ -4814,8 +4817,7 @@ def _on_attach(self, event: ops.StorageAttachedEvent): multiple: range: 2-4 ''' - harness = ops.testing.Harness(MyCharm, meta=meta) - self.addCleanup(harness.cleanup) + 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'])