Repository: canonical/operator
Pull request #59 (2019-11-27 16:26:11)

Optimize is_leader checks by caching temporarily
In order to avoid is-leader hook tool executions on every write to
application relation data bags caching of is-leader return values is
introduced. It relies on the implementation detail in Juju where it
grants a lease for 30 seconds when either of the following happens:

1) a unit agent renews the lease;
2) is-leader is called;
3) leader-set is called.

dshcherb: Rebased on top of the latest master (had to update the first commit to avoid test failures).

Patch:
diff --git a/op/model.py b/op/model.py
index 0291fda..354dd3e 100644
--- a/op/model.py
+++ b/op/model.py
@@ -3,6 +3,10 @@ import weakref
 import os
 import shutil
 import tempfile
+import time
+import datetime
+
+
 from abc import ABC, abstractmethod
 from collections.abc import Mapping, MutableMapping
 from pathlib import Path
@@ -293,10 +297,16 @@ class RelationNotFound(ModelError):
 
 
 class ModelBackend:
+
+    LEASE_RENEWAL_PERIOD = datetime.timedelta(seconds=30)
+
     def __init__(self):
         self.unit_name = os.environ['JUJU_UNIT_NAME']
         self.app_name = self.unit_name.split('/')[0]
 
+        self._is_leader = None
+        self._leader_check_time = 0
+
     def _run(self, *args, capture_output=True, use_json=True):
         if capture_output:
             kwargs = dict(stdout=PIPE, stderr=PIPE)
@@ -353,7 +363,19 @@ class ModelBackend:
         return self._run('config-get')
 
     def is_leader(self):
-        return self._run('is-leader')
+        """Obtain the current leadership status for the unit the charm code is executing on.
+
+        The value is cached for the duration of a lease which is 30s in Juju.
+        """
+        now = time.monotonic()
+        time_since_check = datetime.timedelta(seconds=now - self._leader_check_time)
+        if time_since_check > self.LEASE_RENEWAL_PERIOD or self._is_leader is None:
+            # Current time MUST be saved before running is-leader to ensure the cache
+            # is only used inside the window that is-leader itself asserts.
+            self._leader_check_time = now
+            self._is_leader = self._run('is-leader')
+
+        return self._is_leader
 
     def resource_get(self, resource_name):
         return self._run('resource-get', resource_name, use_json=False).strip()


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 7681864..1f017f3 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -6,6 +6,7 @@ import subprocess
 import pathlib
 import shutil
 import unittest
+import time
 
 import op.model
 import op.charm
@@ -217,6 +218,28 @@ class TestModel(unittest.TestCase):
             ['relation-list', '-r', '4', '--format=json'],
         ])
 
+    def test_is_leader_refresh(self):
+        self.backend = op.model.ModelBackend()
+        meta = op.charm.CharmMeta()
+        meta.relations = {'db0': None, 'db1': None, 'db2': None}
+        self.model = op.model.Model('myapp/0', meta, self.backend)
+
+        # A sanity check.
+        self.assertGreater(time.monotonic(), op.model.ModelBackend.LEASE_RENEWAL_PERIOD.total_seconds())
+
+        fake_script(self, 'is-leader', 'echo false')
+        self.assertFalse(self.model.unit.is_leader())
+
+        # Change the leadership status and force a recheck.
+        fake_script(self, 'is-leader', 'echo true')
+        self.backend._leader_check_time = 0
+        self.assertTrue(self.model.unit.is_leader())
+
+        # Force a recheck without changing the leadership status.
+        fake_script(self, 'is-leader', 'echo true')
+        self.backend._leader_check_time = 0
+        self.assertTrue(self.model.unit.is_leader())
+
     def test_resources(self):
         backend = op.model.ModelBackend()
         meta = op.charm.CharmMeta()