From e8246ce1456ad516cbde80656fd92804b338676a Mon Sep 17 00:00:00 2001 From: "Bartosz \"mastier\" Woronicz" <1609672+mastier@users.noreply.github.com> Date: Tue, 19 May 2026 19:44:45 +0200 Subject: [PATCH] fix(core/juju) Introduce model_exists_live to avoid stale cache on destroy Extract _get_model_uncached() as the raw lookup logic, keep get_model() cached via functools.cache for normal use, and add model_exists_live() which always bypasses the cache. Use model_exists_live() in wait_model_gone() and in DestroyControlPlaneStep.run() where the model state is actively changing and cached results would be stale. Add unit tests covering _get_model_uncached, model_exists_live, and wait_model_gone including a regression test for the original bug. Fixes: LP#2146354 Co-authored-by: Claude (OpenCode AI) --- sunbeam-python/sunbeam/core/juju.py | 31 ++++- sunbeam-python/sunbeam/steps/openstack.py | 2 +- .../tests/unit/sunbeam/core/test_juju.py | 107 ++++++++++++++++++ 3 files changed, 134 insertions(+), 6 deletions(-) diff --git a/sunbeam-python/sunbeam/core/juju.py b/sunbeam-python/sunbeam/core/juju.py index 96a7c9d72..0af8a01b6 100644 --- a/sunbeam-python/sunbeam/core/juju.py +++ b/sunbeam-python/sunbeam/core/juju.py @@ -309,9 +309,8 @@ def models(self) -> list[dict]: raise JujuException(e.stderr) return models.get("models", []) - @functools.cache - def get_model(self, model: str) -> "dict": - """Fetch model. + def _get_model_uncached(self, model: str) -> "dict": + """Fetch model without caching. :model: Name of the model """ @@ -320,8 +319,16 @@ def get_model(self, model: str) -> "dict": return m raise ModelNotFoundException(f"Model {model!r} not found") + @functools.cache + def get_model(self, model: str) -> "dict": + """Fetch model (cached). + + :model: Name of the model + """ + return self._get_model_uncached(model) + def model_exists(self, model: str) -> bool: - """Check if model exists. + """Check if model exists (uses cache). :model: Name of the model """ @@ -331,6 +338,20 @@ def model_exists(self, model: str) -> bool: return False return True + def model_exists_live(self, model: str) -> bool: + """Check if model exists without cache. + + Use this in destroy/removal paths where the model state + is actively changing and cached results would be stale. + + :model: Name of the model + """ + try: + self._get_model_uncached(model) + except JujuException: + return False + return True + def add_model( self, model: str, @@ -1297,7 +1318,7 @@ def wait_model_gone( timeout = 60 * 15 start = time.monotonic() - while self.model_exists(model): + while self.model_exists_live(model): if time.monotonic() - start > timeout: raise TimeoutError( f"Timed out while waiting for model {model!r} to be gone" diff --git a/sunbeam-python/sunbeam/steps/openstack.py b/sunbeam-python/sunbeam/steps/openstack.py index d9c8ce950..1aef54f40 100644 --- a/sunbeam-python/sunbeam/steps/openstack.py +++ b/sunbeam-python/sunbeam/steps/openstack.py @@ -1540,7 +1540,7 @@ def run(self, context: StepContext) -> Result: LOG.debug( "Timeout waiting for model to be removed, trying through provider sdk" ) - if not self.jhelper.model_exists(self._MODEL): + if not self.jhelper.model_exists_live(self._MODEL): return Result(ResultType.COMPLETED) self.jhelper.destroy_model(self._MODEL, destroy_storage=True, force=True) try: diff --git a/sunbeam-python/tests/unit/sunbeam/core/test_juju.py b/sunbeam-python/tests/unit/sunbeam/core/test_juju.py index 858c42f7b..6b8b317e3 100644 --- a/sunbeam-python/tests/unit/sunbeam/core/test_juju.py +++ b/sunbeam-python/tests/unit/sunbeam/core/test_juju.py @@ -152,6 +152,113 @@ def test_model_exists_false(jhelper): assert not jhelper.model_exists("nope") +def test_get_model_uncached_found(jhelper): + result = jhelper._get_model_uncached("test-model") + assert result == { + "short-name": "test-model", + "name": "admin/test-model", + "model-uuid": "1234", + } + + +def test_get_model_uncached_not_found(jhelper): + with pytest.raises(jujulib.ModelNotFoundException): + jhelper._get_model_uncached("nope") + + +def test_get_model_uncached_always_calls_models(jhelper): + """_get_model_uncached does not cache; each call queries models().""" + jhelper._get_model_uncached("test-model") + jhelper._get_model_uncached("test-model") + assert jhelper.models.call_count == 2 + + +def test_model_exists_live_true(jhelper): + assert jhelper.model_exists_live("test-model") + + +def test_model_exists_live_false(jhelper): + assert not jhelper.model_exists_live("nope") + + +def test_model_exists_live_bypasses_cache(jhelper): + """model_exists_live sees fresh data even when get_model cache is stale.""" + # Seed the cache + assert jhelper.get_model("test-model") is not None + + # Now model disappears + jhelper.models = Mock(return_value=[]) + + # Cached model_exists still returns True (stale cache) + assert jhelper.model_exists("test-model") is True + + # Live check sees the model is gone + assert jhelper.model_exists_live("test-model") is False + + +@patch("time.sleep") +def test_wait_model_gone_model_already_gone(mock_sleep, jhelper): + """Returns immediately if model does not exist.""" + jhelper.models = Mock(return_value=[]) + jhelper.get_model.cache_clear() + + jhelper.wait_model_gone("test-model", timeout=60) + + mock_sleep.assert_not_called() + + +@patch("time.sleep") +def test_wait_model_gone_model_disappears(mock_sleep, jhelper): + """Exits loop once model is gone, verifying live checks on each iteration.""" + model_data = [ + {"short-name": "test-model", "name": "admin/test-model", "model-uuid": "1234"} + ] + jhelper.models = Mock( + side_effect=[ + model_data, # first check: model still present + [], # second check: model gone + ] + ) + jhelper.get_model.cache_clear() + + jhelper.wait_model_gone("test-model", timeout=60) + + assert jhelper.models.call_count == 2 + mock_sleep.assert_called_once() + + +@patch("time.sleep") +@patch("time.monotonic") +def test_wait_model_gone_timeout_raises(mock_monotonic, mock_sleep, jhelper): + """TimeoutError is raised when model does not disappear in time.""" + # start=0, first iteration: model exists, check timeout -> 9999 > 1 -> raise + mock_monotonic.side_effect = [0, 9999] + + with pytest.raises(TimeoutError): + jhelper.wait_model_gone("test-model", timeout=1) + + +@patch("time.sleep") +def test_wait_model_gone_does_not_use_cache(mock_sleep, jhelper): + """Regression test: wait_model_gone uses live checks, not cached model_exists. + + This is the exact bug scenario (see LP#2146354) + get_model cache has the model, but the model + is actually gone. wait_model_gone must detect this via model_exists_live. + """ + # Seed the get_model cache so cached model_exists would return True forever + assert jhelper.get_model("test-model") is not None + + # Now model disappears from the controller + jhelper.models = Mock(return_value=[]) + + # wait_model_gone should return immediately (model is gone per live check) + # If it used cached model_exists, it would loop until timeout + jhelper.wait_model_gone("test-model", timeout=5) + + mock_sleep.assert_not_called() + + def test_get_model_name_with_owner(jhelper): assert jhelper.get_model_name_with_owner("test-model") == "admin/test-model"