Repository: canonical/operator

Issue:
[storage] Expand StorageEvent to provide details of disk by default
Hi,

Supposing we are running a configuration such as:

```
storage:
  data:
    type: block
    multiple:
      range: 0- <<<--- I may have several volumes here
```

When I get a storage detached, I will need to treat the case where among several disks, one is going away under that same storage_name ("data").
Currently, there is no way via operator framework to discover which disk does it relates to. 
The model does provide a Storage class, but there is no way to retrieve that information and get an object pointing to the detaching disk.

I've raised with Juju directly, on the following discussion: https://bugs.launchpad.net/juju/+bug/1936876

So, we can recover which disk is being removed if one runs: ```storage-get``` only, without arguments.

In my view, storage-related events should run that storage-get at __init__ and store storage_id, name and location info.

We should have instead something similar to:

```
class StorageEvent(HookEvent):

    def __init__(self):
         self._storage_json = self._run('storage-get', return_output=True, use_json=True)
         
```

Discussion:
jameinel: so "doesn't hand it back" is probably true ,but in attach/detach the *hook
itself* is the name. eg it is "db-storage-attached" for the "db" storage,
not a generic "storage-attached" without the context.
That said, in 'config-changed' if you called 'storage-get' then you
definitely don't have the context.


On Wed, Oct 20, 2021 at 11:21 AM Ryan Barry ***@***.***>
wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In ops/model.py
> <https://github.com/canonical/operator/pull/648#discussion_r732892464>:
>
> > @@ -1008,6 +1008,22 @@ def __getitem__(self, storage_name: str) -> typing.List['Storage']:
>                  storage_list.append(Storage(storage_name, storage_id, self._backend))
>          return storage_list
>
> +    def get_by_name_and_location(self, storage_name: str, location: str) -> "Storage":
> +        """Looks up an entry by the location.
> +
> +        :class:`Storage` objects do not necessarily have any unique identifiers
>
> The issue is, as far as I saw in testing, that storage-get returned:
>
> {
>   "type": "filesystem",
>   "location": "foo/bar/baz",
> }
>
> The name *can* explicitly identify it, but Juju doesn't hand it back on
> attach/detach events, so relating it to a Storage object requires
> iterating through the names OF knows about and the location Juju handed us
> to sync them up.
>
> Fortunately, this sort of lookup only needs to be done once when a disk is
> attached/detached, and a "real" Storage object is yielded where .location
> can be lazily looked up as normal.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/648#discussion_r732892464>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7KOBLPOLLUGL5NFAVTUH3M7NANCNFSM5GDAKORQ>
> .
> 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/charm.py b/ops/charm.py
index e4d22cf..010dc99 100755
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -426,6 +426,38 @@ class StorageEvent(HookEvent):
     of :class:`StorageEvent`.
     """
 
+    def __init__(self, handle, storage):
+        super().__init__(handle)
+        self.storage = storage
+
+    def snapshot(self) -> dict:
+        """Used by the framework to serialize the event to disk.
+
+        Not meant to be called by charm code.
+        """
+        snapshot = {}
+
+        if isinstance(self.storage, model.Storage):
+            snapshot["storage_name"] = self.storage.name
+            snapshot["storage_id"] = self.storage.id
+            snapshot["storage_location"] = str(self.storage.location)
+        return snapshot
+
+    def restore(self, snapshot: dict) -> None:
+        """Used by the framework to deserialize the event from disk.
+
+        Not meant to be called by charm code.
+        """
+        storage_name = snapshot.get("storage_name")
+        storage_id = snapshot.get("storage_id")
+        storage_location = snapshot.get("storage_location")
+
+        if storage_name and storage_id is not None:
+            self.storage = next(
+                (s for s in self.framework.model.storages[storage_name] if s.id == storage_id),
+                None,)
+            self.storage.location = storage_location
+
 
 class StorageAttachedEvent(StorageEvent):
     """Event triggered when new storage becomes available.
diff --git a/ops/main.py b/ops/main.py
index 91e37be..50b0c6c 100755
--- a/ops/main.py
+++ b/ops/main.py
@@ -150,6 +150,23 @@ def _get_event_args(charm, bound_event):
         workload_name = os.environ['JUJU_WORKLOAD_NAME']
         container = model.unit.get_container(workload_name)
         return [container], {}
+    elif issubclass(event_type, ops.charm.StorageEvent):
+        storage_id = os.environ.get("JUJU_STORAGE_ID", "")
+        if storage_id:
+            storage_name = storage_id.split("/")[0]
+        else:
+            # Before JUJU_STORAGE_ID exists
+            storage_name = bound_event.event_kind.split("_", 1)[0]
+
+        storages = model.storages[storage_name]
+        id, storage_location = model._backend._storage_event_details()
+        if len(storages) == 1:
+            storage = storages[0]
+        else:
+            # If there's more than one value, pick the right one. We'll realize the key on lookup
+            storage = next((s for s in storages if s.id == id), None)
+        storage.location = storage_location
+        return [storage], {}
     elif issubclass(event_type, ops.charm.RelationEvent):
         relation_name = os.environ['JUJU_RELATION']
         relation_id = int(os.environ['JUJU_RELATION_ID'].split(':')[-1])
diff --git a/ops/model.py b/ops/model.py
index c467ddc..ec99173 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1037,13 +1037,22 @@ class Storage:
         self._location = None
 
     @property
-    def location(self):
+    def location(self) -> Path:
         """Return the location of the storage."""
         if self._location is None:
             raw = self._backend.storage_get('{}/{}'.format(self.name, self.id), "location")
             self._location = Path(raw)
         return self._location
 
+    @location.setter
+    def location(self, location: str) -> None:
+        """Sets the location for use in events.
+
+        For :class:`StorageAttachedEvent` and :class:`StorageDetachingEvent` in case
+        the actual details are gone from Juju by the time of a dynamic lookup.
+        """
+        self._location = Path(location)
+
 
 class Container:
     """Represents a named container in a unit.
@@ -1669,6 +1678,20 @@ class _ModelBackend:
         return [int(s.split('/')[1]) for s in self._run('storage-list', name,
                                                         return_output=True, use_json=True)]
 
+    def _storage_event_details(self) -> typing.Tuple[int, str]:
+        output = self._run('storage-get', '--help', return_output=True)
+
+        # Match the entire string at once instead of going line by line
+        matcher = re.compile(
+            r'.*^-s\s+\(=\s+(?P<storage_key>.*?)\)\s*?$',
+            re.MULTILINE | re.DOTALL
+        )
+        key = matcher.match(output).groupdict()["storage_key"]
+
+        id = int(key.split("/")[1])
+        location = self.storage_get(key, "location")
+        return id, location
+
     def storage_get(self, storage_name_id, attribute):
         return self._run('storage-get', '-s', storage_name_id, attribute,
                          return_output=True, use_json=True)


Test patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index 8e09bc5..7161359 100755
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -26,7 +26,7 @@ from ops.charm import (
     ContainerStorageMeta,
 )
 from ops.framework import EventBase, EventSource, Framework
-from ops.model import Model, _ModelBackend
+from ops.model import Model, Storage, _ModelBackend
 from ops.storage import SQLiteStorage
 
 from .test_helpers import fake_script, fake_script_calls
@@ -175,6 +175,7 @@ peers:
         ])
 
     def test_storage_events(self):
+        this = self
 
         class MyCharm(CharmBase):
             def __init__(self, *args):
@@ -187,6 +188,7 @@ peers:
 
             def _on_stor1_attach(self, event):
                 self.seen.append(type(event).__name__)
+                this.assertEqual(event.storage.location, Path("/var/srv/stor1/0"))
 
             def _on_stor2_detach(self, event):
                 self.seen.append(type(event).__name__)
@@ -217,6 +219,47 @@ storage:
     type: filesystem
 ''')
 
+        fake_script(
+            self,
+            "storage-get",
+            """
+            if [ "$1" = "-s" ]; then
+                id=${2#*/}
+                key=${2%/*}
+                echo "\\"/var/srv/${key}/${id}\\"" # NOQA: test_quote_backslashes
+            elif [ "$1" = '--help' ]; then
+                printf '%s\\n' \\
+                'Usage: storage-get [options] [<key>]' \\
+                '   ' \\
+                'Summary:' \\
+                'print information for storage instance with specified id' \\
+                '   ' \\
+                'Options:' \\
+                '--format  (= smart)' \\
+                '    Specify output format (json|smart|yaml)' \\
+                '-o, --output (= "")' \\
+                '    Specify an output file' \\
+                '-s  (= test-stor/0)' \\
+                '    specify a storage instance by id' \\
+                '   ' \\
+                'Details:' \\
+                'When no <key> is supplied, all keys values are printed.'
+            else
+                # Return the same path for all disks since `storage-get`
+                # on attach and detach takes no parameters and is not
+                # deterministically faked with fake_script
+                exit 1
+            fi
+            """,
+        )
+        fake_script(
+            self,
+            "storage-list",
+            """
+            echo '["disks/0"]'
+            """,
+        )
+
         self.assertIsNone(self.meta.storages['stor1'].multiple_range)
         self.assertEqual(self.meta.storages['stor2'].multiple_range, (2, 2))
         self.assertEqual(self.meta.storages['stor3'].multiple_range, (2, None))
@@ -224,10 +267,10 @@ storage:
 
         charm = MyCharm(self.create_framework())
 
-        charm.on['stor1'].storage_attached.emit()
-        charm.on['stor2'].storage_detaching.emit()
-        charm.on['stor3'].storage_attached.emit()
-        charm.on['stor-4'].storage_attached.emit()
+        charm.on['stor1'].storage_attached.emit(Storage("stor1", 0, charm.model._backend))
+        charm.on['stor2'].storage_detaching.emit(Storage("stor2", 0, charm.model._backend))
+        charm.on['stor3'].storage_attached.emit(Storage("stor3", 0, charm.model._backend))
+        charm.on['stor-4'].storage_attached.emit(Storage("stor-4", 0, charm.model._backend))
 
         self.assertEqual(charm.seen, [
             'StorageAttachedEvent',
diff --git a/test/test_main.py b/test/test_main.py
index fbfa01e..65f5a80 100755
--- a/test/test_main.py
+++ b/test/test_main.py
@@ -710,6 +710,46 @@ class TestMainWithNoDispatch(_TestMain, unittest.TestCase):
         event_file = self.JUJU_CHARM_DIR / rel_path
         # Note that sys.executable is used to make sure we are using the same
         # interpreter for the child process to support virtual environments.
+        fake_script(
+            self,
+            "storage-get",
+            """
+            if [ "$1" = "-s" ]; then
+                id=${2#*/}
+                key=${2%/*}
+                echo "\\"/var/srv/${key}/${id}\\"" # NOQA: test_quote_backslashes
+            elif [ "$1" = '--help' ]; then
+                printf '%s\\n' \\
+                'Usage: storage-get [options] [<key>]' \\
+                '   ' \\
+                'Summary:' \\
+                'print information for storage instance with specified id' \\
+                '   ' \\
+                'Options:' \\
+                '--format  (= smart)' \\
+                '    Specify output format (json|smart|yaml)' \\
+                '-o, --output (= "")' \\
+                '    Specify an output file' \\
+                '-s  (= test-stor/0)' \\
+                '    specify a storage instance by id' \\
+                '   ' \\
+                'Details:' \\
+                'When no <key> is supplied, all keys values are printed.'
+            else
+                # Return the same path for all disks since `storage-get`
+                # on attach and detach takes no parameters and is not
+                # deterministically faked with fake_script
+                exit 1
+            fi
+            """,
+        )
+        fake_script(
+            self,
+            "storage-list",
+            """
+            echo '["disks/0"]'
+            """,
+        )
         subprocess.run(
             [sys.executable, str(event_file)],
             check=True, env=env, cwd=str(self.JUJU_CHARM_DIR))
@@ -960,6 +1000,46 @@ class TestMainWithDispatch(_TestMainWithDispatch, unittest.TestCase):
         env['JUJU_DISPATCH_PATH'] = str(rel_path)
         env['JUJU_VERSION'] = '2.8.0'
         dispatch = self.JUJU_CHARM_DIR / 'dispatch'
+        fake_script(
+            self,
+            "storage-get",
+            """
+            if [ "$1" = "-s" ]; then
+                id=${2#*/}
+                key=${2%/*}
+                echo "\\"/var/srv/${key}/${id}\\"" # NOQA: test_quote_backslashes
+            elif [ "$1" = '--help' ]; then
+                printf '%s\\n' \\
+                'Usage: storage-get [options] [<key>]' \\
+                '   ' \\
+                'Summary:' \\
+                'print information for storage instance with specified id' \\
+                '   ' \\
+                'Options:' \\
+                '--format  (= smart)' \\
+                '    Specify output format (json|smart|yaml)' \\
+                '-o, --output (= "")' \\
+                '    Specify an output file' \\
+                '-s  (= test-stor/0)' \\
+                '    specify a storage instance by id' \\
+                '   ' \\
+                'Details:' \\
+                'When no <key> is supplied, all keys values are printed.'
+            else
+                # Return the same path for all disks since `storage-get`
+                # on attach and detach takes no parameters and is not
+                # deterministically faked with fake_script
+                exit 1
+            fi
+            """,
+        )
+        fake_script(
+            self,
+            "storage-list",
+            """
+            echo '["disks/0"]'
+            """,
+        )
         subprocess.run(
             [sys.executable, str(dispatch)],
             # stdout=self.stdout,
@@ -990,6 +1070,46 @@ class TestMainWithDispatchAsScript(_TestMainWithDispatch, unittest.TestCase):
     def _call_event(self, rel_path, env):
         env['JUJU_DISPATCH_PATH'] = str(rel_path)
         env['JUJU_VERSION'] = '2.8.0'
+        fake_script(
+            self,
+            "storage-get",
+            """
+            if [ "$1" = "-s" ]; then
+                id=${2#*/}
+                key=${2%/*}
+                echo "\\"/var/srv/${key}/${id}\\"" # NOQA: test_quote_backslashes
+            elif [ "$1" = '--help' ]; then
+                printf '%s\\n' \\
+                'Usage: storage-get [options] [<key>]' \\
+                '   ' \\
+                'Summary:' \\
+                'print information for storage instance with specified id' \\
+                '   ' \\
+                'Options:' \\
+                '--format  (= smart)' \\
+                '    Specify output format (json|smart|yaml)' \\
+                '-o, --output (= "")' \\
+                '    Specify an output file' \\
+                '-s  (= test-stor/0)' \\
+                '    specify a storage instance by id' \\
+                '   ' \\
+                'Details:' \\
+                'When no <key> is supplied, all keys values are printed.'
+            else
+                # Return the same path for all disks since `storage-get`
+                # on attach and detach takes no parameters and is not
+                # deterministically faked with fake_script
+                exit 1
+            fi
+            """,
+        )
+        fake_script(
+            self,
+            "storage-list",
+            """
+            echo '["disks/0"]'
+            """,
+        )
         dispatch = (self.JUJU_CHARM_DIR / 'dispatch').with_suffix(self.suffix)
         subprocess.check_call([str(dispatch)], env=env, cwd=str(self.JUJU_CHARM_DIR))