Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Using Harness.add_storage(..., attach=True) before begin_with_initial_hooks gives confusing error #1127

Closed
2 tasks done
tonyandrewmeyer opened this issue Feb 9, 2024 · 0 comments · Fixed by #1150
Closed
2 tasks done
Assignees
Labels
bug Something isn't working small item testing Related to ops.testing

Comments

@tonyandrewmeyer
Copy link
Contributor

tonyandrewmeyer commented Feb 9, 2024

For example:

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
@tonyandrewmeyer tonyandrewmeyer added bug Something isn't working testing Related to ops.testing small item labels Feb 9, 2024
tonyandrewmeyer added a commit that referenced this issue Mar 18, 2024
…e begin (#1150)

One minor fix to the `storage` metadata:
[charmcraft.yaml](https://juju.is/docs/sdk/charmcraft-yaml#heading--storage)
and
[metadata.yaml](https://juju.is/docs/sdk/metadata-yaml#heading--storage)
support a `multiple.range` value in the form `<n>+` - this would
previously error out when we attempted to read the metadata. We now
support this (as an alias for `<n>-`, which is an open-ended range
rather than "n or less", and have a test for it.

Adds a minor clarification in the API docs when `Harness.add_storage`
will emit a storage-attached event.

In `begin_with_initial_hooks`, if the storage has already been attached,
then don't try to attach it a second time, but *do* emit the
storage-attached event, since that will not have been done previously.

Make the `_storage_attach` method safe to call multiple times. This
appears to have been the intention based on comments in the code, but,
in practice, attempting to symlink the mount would fail. Now, if the
symlink already exists (and has the same target) then we continue.

Adds a variety of tests for Harness `add_storage` and `attach_storage`
behaviour.

Fixes #1127

---------

Co-authored-by: Ben Hoyt <ben.hoyt@canonical.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working small item testing Related to ops.testing
Projects
None yet
1 participant