Repository: canonical/operator
Pull request #366 (2020-08-05 16:00:50)

version-check to determine dispatch support
This branch introudces a change, and a simplification (or at least a
separation of concerns) for how the operator framework determines that
a juju is dispatch-aware, and how it determines what hook juju is
asking us to run.

In the existing code we used sys.argv[0] to determine the hook to run
unless JUJU_DISPATCH_PATH was set and a dispatch binary existed, in
which case that environment variable was used. This leads to
charmcraft setting this environment variable to signal what hook to
run (because it will be indirecting the call to the charm code to set
up PYTHONPATH etc). Together with it always creating a dispatch
binary, this means operator always thought it was running in a
dispatch-aware juju, which meant it never created all the missing
symlinks, which meant it never got events beyond those created by
charmcraft itself on non-dispatch-aware jujus.

With this branch, instead JUJU_VERSION is looked at to determine
dispatch-awareness, and JUJU_DISPATCH_PATH is used instead of argv[0]
to know which hook is wanted, and this separation means
non-dispatch-aware jujus should work with dispatch-enabled charms such
as what charmcraft produces.

One last change introduced is that, when creating the symlinks for
non-dispatch-aware jujus (or non-dispatch-enabled charms), the target
of the symlink will be the hook that is currently running, rather than
the charm entry point. This is because the hooks, like dispatch, may
well be shims that set up the environment (etc), without which the
charm entry point won't run.

fixes: #362


Patch:
diff --git a/ops/jujuversion.py b/ops/jujuversion.py
index b2b8177..1a8ed66 100755
--- a/ops/jujuversion.py
+++ b/ops/jujuversion.py
@@ -96,3 +96,7 @@ class JujuVersion:
     def has_app_data(self) -> bool:
         """Determine whether this juju version knows about app data."""
         return (self.major, self.minor, self.patch) >= (2, 7, 0)
+
+    def is_dispatch_aware(self) -> bool:
+        """Determine whether this juju version knows about dispatch."""
+        return (self.major, self.minor, self.patch) >= (2, 8, 0)
diff --git a/ops/main.py b/ops/main.py
index 6dc31c3..6841cf1 100755
--- a/ops/main.py
+++ b/ops/main.py
@@ -28,6 +28,7 @@ import ops.model
 import ops.storage
 
 from ops.log import setup_root_logging
+from ops.jujuversion import JujuVersion
 
 CHARM_STATE_FILE = '.unit-state.db'
 
@@ -45,11 +46,12 @@ def _get_charm_dir():
     return charm_dir
 
 
-def _create_event_link(charm, bound_event):
+def _create_event_link(charm, bound_event, link_to):
     """Create a symlink for a particular event.
 
     charm -- A charm object.
     bound_event -- An event for which to create a symlink.
+    link_to -- What the event link should point to
     """
     if issubclass(bound_event.event_type, ops.charm.HookEvent):
         event_dir = charm.framework.charm_dir / 'hooks'
@@ -70,7 +72,7 @@ def _create_event_link(charm, bound_event):
         # CPython has different implementations for populating sys.argv[0] for Linux and Windows.
         # For Windows it is always an absolute path (any symlinks are resolved)
         # while for Linux it can be a relative path.
-        target_path = os.path.relpath(os.path.realpath(sys.argv[0]), str(event_dir))
+        target_path = os.path.relpath(link_to, str(event_dir))
 
         # Ignore the non-symlink files or directories
         # assuming the charm author knows what they are doing.
@@ -94,10 +96,11 @@ def _setup_event_links(charm_dir, charm):
     charm -- An instance of the Charm class.
 
     """
+    link_to = os.path.realpath(os.environ.get("JUJU_DISPATCH_PATH", sys.argv[0]))
     for bound_event in charm.on.events().values():
         # Only events that originate from Juju need symlinks.
         if issubclass(bound_event.event_type, (ops.charm.HookEvent, ops.charm.ActionEvent)):
-            _create_event_link(charm, bound_event)
+            _create_event_link(charm, bound_event, link_to)
 
 
 def _emit_charm_event(charm, event_name):
@@ -159,15 +162,15 @@ class _Dispatcher:
     Attributes:
         event_name: the name of the event to run
         is_dispatch_aware: are we running under a Juju that knows about the
-            dispatch binary?
+            dispatch binary, and is that binary present?
 
     """
 
     def __init__(self, charm_dir: Path):
         self._charm_dir = charm_dir
-        self._exec_path = Path(sys.argv[0])
+        self._exec_path = Path(os.environ.get('JUJU_DISPATCH_PATH', sys.argv[0]))
 
-        if 'JUJU_DISPATCH_PATH' in os.environ and (charm_dir / 'dispatch').exists():
+        if JujuVersion.from_environ().is_dispatch_aware() and (charm_dir / 'dispatch').exists():
             self._init_dispatch()
         else:
             self._init_legacy()
@@ -211,7 +214,7 @@ class _Dispatcher:
             logger.warning("Legacy %s exists but is not executable.", self._dispatch_path)
             return
 
-        if dispatch_path.resolve() == self._exec_path.resolve():
+        if dispatch_path.resolve() == Path(sys.argv[0]).resolve():
             logger.debug("Legacy %s is just a link to ourselves.", self._dispatch_path)
             return
 


Test patch:
diff --git a/test/test_jujuversion.py b/test/test_jujuversion.py
index 1e38210..cb3a60d 100755
--- a/test/test_jujuversion.py
+++ b/test/test_jujuversion.py
@@ -60,6 +60,10 @@ class TestJujuVersion(unittest.TestCase):
         self.assertTrue(JujuVersion('2.7.0').has_app_data())
         self.assertFalse(JujuVersion('2.6.9').has_app_data())
 
+    def test_is_dispatch_aware(self):
+        self.assertTrue(JujuVersion('2.8.0').is_dispatch_aware())
+        self.assertFalse(JujuVersion('2.7.9').is_dispatch_aware())
+
     def test_parsing_errors(self):
         invalid_versions = [
             "xyz",
@@ -131,15 +135,17 @@ class TestJujuVersion(unittest.TestCase):
             ("2.0.0.0", "2.0.0.0", False, True),
             ("2.0.0.1", "2.0.0.0", False, False),
             ("2.0.1.10", "2.0.0.0", False, False),
+            ("2.10.0", "2.8.0", False, False),
         ]
 
         for a, b, expected_strict, expected_weak in test_cases:
-            self.assertEqual(JujuVersion(a) < JujuVersion(b), expected_strict)
-            self.assertEqual(JujuVersion(a) <= JujuVersion(b), expected_weak)
-            self.assertEqual(JujuVersion(b) > JujuVersion(a), expected_strict)
-            self.assertEqual(JujuVersion(b) >= JujuVersion(a), expected_weak)
-            # Implicit conversion.
-            self.assertEqual(JujuVersion(a) < b, expected_strict)
-            self.assertEqual(JujuVersion(a) <= b, expected_weak)
-            self.assertEqual(b > JujuVersion(a), expected_strict)
-            self.assertEqual(b >= JujuVersion(a), expected_weak)
+            with self.subTest(a=a, b=b):
+                self.assertEqual(JujuVersion(a) < JujuVersion(b), expected_strict)
+                self.assertEqual(JujuVersion(a) <= JujuVersion(b), expected_weak)
+                self.assertEqual(JujuVersion(b) > JujuVersion(a), expected_strict)
+                self.assertEqual(JujuVersion(b) >= JujuVersion(a), expected_weak)
+                # Implicit conversion.
+                self.assertEqual(JujuVersion(a) < b, expected_strict)
+                self.assertEqual(JujuVersion(a) <= b, expected_weak)
+                self.assertEqual(b > JujuVersion(a), expected_strict)
+                self.assertEqual(b >= JujuVersion(a), expected_weak)
diff --git a/test/test_main.py b/test/test_main.py
index 7b80dda..50a9c37 100755
--- a/test/test_main.py
+++ b/test/test_main.py
@@ -88,6 +88,7 @@ class CharmInitTestCase(unittest.TestCase):
         fake_environ = {
             'JUJU_UNIT_NAME': 'test_main/0',
             'JUJU_MODEL_NAME': 'mymodel',
+            'JUJU_VERSION': '2.7.0',
         }
         fake_script(self, 'juju-log', "exit 0")
         with patch.dict(os.environ, fake_environ):
@@ -138,6 +139,61 @@ class CharmInitTestCase(unittest.TestCase):
         self.assertFalse(warn_cm)
 
 
+@patch('sys.argv', new=("hooks/config-changed",))
+class TestDispatch(unittest.TestCase):
+    def _check(self, *, with_dispatch=False, dispatch_path=''):
+        """Helper for below tests."""
+        class MyCharm(CharmBase):
+            def __init__(self, framework):
+                super().__init__(framework)
+
+        fake_environ = {
+            'JUJU_UNIT_NAME': 'test_main/0',
+            'JUJU_MODEL_NAME': 'mymodel',
+        }
+        if dispatch_path:
+            fake_environ['JUJU_DISPATCH_PATH'] = dispatch_path
+            fake_environ['JUJU_VERSION'] = '2.8.0'
+        else:
+            fake_environ['JUJU_VERSION'] = '2.7.0'
+        fake_script(self, 'juju-log', "exit 0")
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            tmpdir = Path(tmpdir)
+            fake_metadata = tmpdir / 'metadata.yaml'
+            with fake_metadata.open('wb') as fh:
+                fh.write(b'name: test')
+            if with_dispatch:
+                dispatch = tmpdir / 'dispatch'
+                with dispatch.open('wt', encoding='utf8') as fh:
+                    fh.write('')
+                    os.fchmod(fh.fileno(), 0o755)
+
+            with patch.dict(os.environ, fake_environ):
+                with patch('ops.main._emit_charm_event') as mock_charm_event:
+                    with patch('ops.main._get_charm_dir') as mock_charmdir:
+                        mock_charmdir.return_value = tmpdir
+                        main(MyCharm)
+
+        self.assertEqual(mock_charm_event.call_count, 1)
+        return mock_charm_event.call_args[0][1]
+
+    def test_most_legacy(self):
+        """Without dispatch, sys.argv[0] is used."""
+        event = self._check()
+        self.assertEqual(event, 'config_changed')
+
+    def test_with_dispatch(self):
+        """With dispatch, dispatch is used."""
+        event = self._check(with_dispatch=True, dispatch_path='hooks/potatos')
+        self.assertEqual(event, 'potatos')
+
+    def test_with_dispatch_path_but_no_dispatch(self):
+        """Dispatch path overwrites sys.argv[0] even if no actual dispatch."""
+        event = self._check(with_dispatch=False, dispatch_path='hooks/foo')
+        self.assertEqual(event, 'foo')
+
+
 class _TestMain(abc.ABC):
 
     @abc.abstractmethod
@@ -435,8 +491,7 @@ class _TestMain(abc.ABC):
                                  expected_event_data[event_spec.event_name])
 
     def test_event_not_implemented(self):
-        """Make sure events without implementation do not cause non-zero exit.
-        """
+        """Make sure events without implementation do not cause non-zero exit."""
         # Simulate a scenario where there is a symlink for an event that
         # a charm does not know how to handle.
         hook_path = self.JUJU_CHARM_DIR / 'hooks/not-implemented-event'
@@ -564,6 +619,7 @@ class TestMainWithNoDispatch(_TestMain, unittest.TestCase):
         path.symlink_to(self.charm_exec_path)
 
     def _call_event(self, rel_path, env):
+        env['JUJU_VERSION'] = '2.7.0'
         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.
@@ -572,8 +628,7 @@ class TestMainWithNoDispatch(_TestMain, unittest.TestCase):
             check=True, env=env, cwd=str(self.JUJU_CHARM_DIR))
 
     def test_setup_event_links(self):
-        """Test auto-creation of symlinks caused by initial events.
-        """
+        """Test auto-creation of symlinks caused by initial events."""
         all_event_hooks = ['hooks/' + e.replace("_", "-")
                            for e in self.charm_module.Charm.on.events().keys()]
         initial_events = {
@@ -582,6 +637,7 @@ class TestMainWithNoDispatch(_TestMain, unittest.TestCase):
             EventSpec(StartEvent, 'start'),
             EventSpec(UpgradeCharmEvent, 'upgrade-charm'),
         }
+        initial_hooks = {'hooks/' + ev.event_name for ev in initial_events}
 
         def _assess_event_links(event_spec):
             self.assertTrue(self.hooks_dir / event_spec.event_name in self.hooks_dir.iterdir())
@@ -589,7 +645,15 @@ class TestMainWithNoDispatch(_TestMain, unittest.TestCase):
                 hook_path = self.JUJU_CHARM_DIR / event_hook
                 self.assertTrue(hook_path.exists(), 'Missing hook: ' + event_hook)
                 if self.hooks_are_symlinks:
+                    self.assertTrue(hook_path.is_symlink())
                     self.assertEqual(os.readlink(str(hook_path)), self.charm_exec_path)
+                elif event_hook in initial_hooks:
+                    self.assertFalse(hook_path.is_symlink())
+                else:
+                    # hooks are not symlinks, and this hook is not one of the initial ones
+                    # check that it's a symlink to the inital ones
+                    self.assertTrue(hook_path.is_symlink())
+                    self.assertEqual(os.readlink(str(hook_path)), event_spec.event_name)
 
         for initial_event in initial_events:
             self._setup_charm_dir()
@@ -609,7 +673,14 @@ class TestMainWithNoDispatch(_TestMain, unittest.TestCase):
 
 class TestMainWithNoDispatchButJujuIsDispatchAware(TestMainWithNoDispatch):
     def _call_event(self, rel_path, env):
-        env["JUJU_DISPATCH_PATH"] = str(rel_path)
+        env['JUJU_DISPATCH_PATH'] = str(rel_path)
+        env['JUJU_VERSION'] = '2.8.0'
+        super()._call_event(rel_path, env)
+
+
+class TestMainWithNoDispatchButDispatchPathIsSet(TestMainWithNoDispatch):
+    def _call_event(self, rel_path, env):
+        env['JUJU_DISPATCH_PATH'] = str(rel_path)
         super()._call_event(rel_path, env)
 
 
@@ -631,7 +702,8 @@ class TestMainWithDispatch(_TestMain, unittest.TestCase):
             path.symlink_to('src/charm.py')
 
     def _call_event(self, rel_path, env):
-        env["JUJU_DISPATCH_PATH"] = str(rel_path)
+        env['JUJU_DISPATCH_PATH'] = str(rel_path)
+        env['JUJU_VERSION'] = '2.8.0'
         dispatch = self.JUJU_CHARM_DIR / 'dispatch'
         subprocess.run(
             [sys.executable, str(dispatch)],
@@ -767,8 +839,7 @@ class TestMainWithDispatch(_TestMain, unittest.TestCase):
 
 
 class TestMainWithDispatchAsScript(TestMainWithDispatch):
-    """Here dispatch is a script that execs the charm.py instead of a symlink.
-    """
+    """Here dispatch is a script that execs the charm.py instead of a symlink."""
 
     has_dispatch = True
 
@@ -781,7 +852,8 @@ class TestMainWithDispatchAsScript(TestMainWithDispatch):
             path.chmod(0o755)
 
     def _call_event(self, rel_path, env):
-        env["JUJU_DISPATCH_PATH"] = str(rel_path)
+        env['JUJU_DISPATCH_PATH'] = str(rel_path)
+        env['JUJU_VERSION'] = '2.8.0'
         dispatch = self.JUJU_CHARM_DIR / 'dispatch'
         subprocess.check_call([str(dispatch)],
                               env=env, cwd=str(self.JUJU_CHARM_DIR))