chore: remplement best guess for idle timer

Author: dimaqq

juju/model/__init__.py

@@ -3298,7 +3298,8 @@ async def new_wait_for_idle(
         if any(not isinstance(o, str) for o in apps):
             raise TypeError(f"apps must be a Iterable[str], got {apps=}")
 
-        deadline = None if timeout is None else time.monotonic() + timeout
+        started = time.monotonic()
+        deadline = None if timeout is None else started + timeout
 
         async def status_on_demand():
             yield _idle.check(
@@ -3316,6 +3317,7 @@ async def status_on_demand():
             wait_for_units=wait_for_units,
             idle_period=idle_period,
         ):
+            logger.info(f"wait_for_idle start{time.monotonic() - started:+.1f} {done=}")
             if done:
                 break
 

juju/model/_idle.py

@@ -44,59 +44,58 @@ async def loop(
     idle_since: dict[str, float] = {}
 
     async for status in foo:
-        logger.warning("FIXME unit test debug %r", status)
+        logger.info("wait_for_idle iteration %s", status)
         now = time.monotonic()
 
         if not status:
             yield False
             continue
 
         expected_idle_since = now - idle_period
-        rv = True
 
         # FIXME there's some confusion about what a "busy" unit is
         # are we ready when over the last idle_period, every time sampled:
         # a. >=N units were ready (possibly different each time), or
         # b. >=N units were ready each time
         for name in status.units:
-            if name in status.ready_units:
+            if name in status.idle_units:
                 idle_since[name] = min(now, idle_since.get(name, float("inf")))
             else:
                 idle_since[name] = float("inf")
 
+        if busy := {n for n, t in idle_since.items() if t > expected_idle_since}:
+            logger.info("Waiting for units to be idle enough: %s", busy)
+            yield False
+            continue
+
         for app_name in apps:
             ready_units = [
                 n for n in status.ready_units if n.startswith(f"{app_name}/")
             ]
             if len(ready_units) < wait_for_units:
-                logger.warn(
+                logger.info(
                     "Waiting for app %r units %s >= %s",
                     app_name,
                     len(status.ready_units),
                     wait_for_units,
                 )
-                rv = False
+                yield False
+                continue
 
             if (
                 wait_for_exact_units is not None
                 and len(ready_units) != wait_for_exact_units
             ):
-                logger.warn(
+                logger.info(
                     "Waiting for app %r units %s == %s",
                     app_name,
                     len(ready_units),
                     wait_for_exact_units,
                 )
-                rv = False
-
-        # FIXME possible interaction between "wait_for_units" and "idle_period"
-        # Assume that we've got some units ready and some busy
-        # What are the semantics for returning True?
-        if busy := [n for n, t in idle_since.items() if t > expected_idle_since]:
-            logger.warn("Waiting for %s to be idle enough", busy)
-            rv = False
+                yield False
+                continue
 
-        yield rv
+        yield True
 
 
 def check(
@@ -175,24 +174,19 @@ def check(
     rv = CheckStatus(set(), set(), set())
 
     for app_name in apps:
-        ready_units = []
         app = full_status.applications[app_name]
         assert isinstance(app, ApplicationStatus)
         for unit_name, unit in _app_units(full_status, app_name).items():
             rv.units.add(unit_name)
             assert unit.agent_status
             assert unit.workload_status
 
-            if unit.agent_status.status != "idle":
-                continue
-            if status and unit.workload_status.status != status:
-                continue
+            if unit.agent_status.status == "idle":
+                rv.idle_units.add(unit_name)
 
-            ready_units.append(unit)
-            rv.ready_units.add(unit_name)
+            if not status or unit.workload_status.status == status:
+                rv.ready_units.add(unit_name)
 
-    # FIXME
-    # rv.idle_units -- depends on agent status only, not workload status
     return rv
 
 

juju/version.py

@@ -6,4 +6,4 @@
 
 DEFAULT_ARCHITECTURE = "amd64"
 
-CLIENT_VERSION = "3.6.1.0rc2"
+CLIENT_VERSION = "3.6.1.0rc3"

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "juju"
-version = "3.6.1.0rc2"  # Stop-gap until dynamic versioning is done; must be in sync with juju/version.py:CLIENT_VERSION
+version = "3.6.1.0rc3"  # Stop-gap until dynamic versioning is done; must be in sync with juju/version.py:CLIENT_VERSION
 description = "Python library for Juju"
 readme = "docs/readme.rst"
 license = { file = "LICENSE" }

tests/unit/test_idle_check.py

@@ -5,6 +5,7 @@
 import copy
 import json
 from typing import Any
+from unittest.mock import ANY
 
 import pytest
 
@@ -31,7 +32,12 @@ def test_check_status(full_status: FullStatus, kwargs):
             "mysql-test-app/0",
             "mysql-test-app/1",
         },
-        set(),
+        {
+            "grafana-agent-k8s/0",
+            "hexanator/0",
+            "mysql-test-app/0",
+            "mysql-test-app/1",
+        },
     )
 
 
@@ -44,7 +50,7 @@ def test_check_status_missing_app(full_status: FullStatus, kwargs):
 def test_check_status_is_selective(full_status: FullStatus, kwargs):
     kwargs["apps"] = ["hexanator"]
     status = check(full_status, **kwargs)
-    assert status == CheckStatus({"hexanator/0"}, {"hexanator/0"}, set())
+    assert status == CheckStatus({"hexanator/0"}, {"hexanator/0"}, {"hexanator/0"})
 
 
 def test_no_apps(full_status: FullStatus, kwargs):
@@ -80,33 +86,21 @@ def test_app_error(response: dict[str, Any], kwargs):
     assert "big problem" in str(e)
 
 
-def test_exact_count(response: dict[str, Any], kwargs):
-    units = response["response"]["applications"]["hexanator"]["units"]
-    units["hexanator/1"] = units["hexanator/0"]
-
-    kwargs["apps"] = ["hexanator"]
-
-    status = check(convert(response), **kwargs)
-    assert status == CheckStatus(
-        {"hexanator/0", "hexanator/1"}, {"hexanator/0", "hexanator/1"}, set()
-    )
-
-
 def test_ready_units(full_status: FullStatus, kwargs):
     kwargs["apps"] = ["mysql-test-app"]
     status = check(full_status, **kwargs)
     assert status == CheckStatus(
         {"mysql-test-app/0", "mysql-test-app/1"},
         {"mysql-test-app/0", "mysql-test-app/1"},
-        set(),
+        {"mysql-test-app/0", "mysql-test-app/1"},
     )
 
 
 def test_active_units(full_status: FullStatus, kwargs):
     kwargs["apps"] = ["mysql-test-app"]
     kwargs["status"] = "active"
     status = check(full_status, **kwargs)
-    assert status == CheckStatus({"mysql-test-app/0", "mysql-test-app/1"}, set(), set())
+    assert status == CheckStatus({"mysql-test-app/0", "mysql-test-app/1"}, set(), ANY)
 
 
 def test_ready_unit_requires_idle_agent(response: dict[str, Any], kwargs):
@@ -118,9 +112,11 @@ def test_ready_unit_requires_idle_agent(response: dict[str, Any], kwargs):
     kwargs["status"] = "active"
 
     status = check(convert(response), **kwargs)
-    assert status
-    assert status.units == {"hexanator/0", "hexanator/1"}
-    assert status.ready_units == {"hexanator/0"}
+    assert status == CheckStatus(
+        {"hexanator/0", "hexanator/1"},
+        {"hexanator/0", "hexanator/1"},
+        {"hexanator/0"},
+    )
 
 
 def test_ready_unit_requires_workload_status(response: dict[str, Any], kwargs):
@@ -132,7 +128,7 @@ def test_ready_unit_requires_workload_status(response: dict[str, Any], kwargs):
     kwargs["status"] = "active"
 
     status = check(convert(response), **kwargs)
-    assert status == CheckStatus({"hexanator/0", "hexanator/1"}, {"hexanator/0"}, set())
+    assert status == CheckStatus({"hexanator/0", "hexanator/1"}, {"hexanator/0"}, ANY)
 
 
 def test_agent_error(response: dict[str, Any], kwargs):
@@ -182,7 +178,7 @@ def test_machine_ok(response: dict[str, Any], kwargs):
     kwargs["raise_on_error"] = True
 
     status = check(convert(response), **kwargs)
-    assert status == CheckStatus({"hexanator/0"}, {"hexanator/0"}, set())
+    assert status == CheckStatus({"hexanator/0"}, {"hexanator/0"}, ANY)
 
 
 def test_machine_error(response: dict[str, Any], kwargs):
@@ -244,7 +240,7 @@ def test_maintenance(response: dict[str, Any], kwargs):
     kwargs["status"] = "maintenance"
 
     status = check(convert(response), **kwargs)
-    assert status == CheckStatus({"hexanator/0"}, {"hexanator/0"}, set())
+    assert status == CheckStatus({"hexanator/0"}, {"hexanator/0"}, ANY)
 
 
 @pytest.fixture

tests/unit/test_idle_check_subordinate.py

@@ -16,7 +16,11 @@
 
 def test_subordinate_apps(response: dict[str, Any], kwargs):
     status = check(convert(response), **kwargs)
-    assert status == CheckStatus({"ntp/0", "ubuntu/0"}, {"ntp/0", "ubuntu/0"}, set())
+    assert status == CheckStatus(
+        {"ntp/0", "ubuntu/0"},
+        {"ntp/0", "ubuntu/0"},
+        {"ntp/0", "ubuntu/0"},
+    )
 
 
 def test_subordinate_is_selective(response, kwargs):
@@ -25,7 +29,11 @@ def test_subordinate_is_selective(response, kwargs):
     ]
     subordinates["some-other/0"] = subordinates["ntp/0"]
     status = check(convert(response), **kwargs)
-    assert status == CheckStatus({"ntp/0", "ubuntu/0"}, {"ntp/0", "ubuntu/0"}, set())
+    assert status == CheckStatus(
+        {"ntp/0", "ubuntu/0"},
+        {"ntp/0", "ubuntu/0"},
+        {"ntp/0", "ubuntu/0"},
+    )
 
 
 @pytest.fixture