From 0fea12e9671b125b26f03cf6362f209b35911cf2 Mon Sep 17 00:00:00 2001 From: haseeb Date: Wed, 9 Sep 2026 15:10:05 +0530 Subject: [PATCH 1/2] fix: clear orphaned profiles when prune is off --- .../openstack-sync-operator/values.yaml | 1 + .../openstack_sync/hooks/framework.py | 6 +- .../openstack_sync/hooks/router_flavors.py | 8 +- .../plugins/neutron/router_flavors/prune.py | 38 ++++++---- .../neutron/router_flavors/reconcile.py | 61 +++++++--------- python/openstack-sync/tests/test_framework.py | 32 +++++++- python/openstack-sync/tests/test_prune.py | 73 ++++++++++++++++++- python/openstack-sync/tests/test_reconcile.py | 54 ++++++++++---- .../tests/test_router_flavors_hook.py | 67 ++++++++++++++++- 9 files changed, 264 insertions(+), 76 deletions(-) diff --git a/components/openstack-sync-operator/values.yaml b/components/openstack-sync-operator/values.yaml index 30b425505..5e660a8ba 100644 --- a/components/openstack-sync-operator/values.yaml +++ b/components/openstack-sync-operator/values.yaml @@ -55,6 +55,7 @@ pluginData: READY_DELAY: 10 # When true, removing a NeutronRouterFlavor CR also deletes its unused # operator-managed OpenStack flavor. Enable this before removing the CR. + # Gates flavor deletion only; unbound managed profiles go either way. PRUNE: false ironicRunbooks: diff --git a/python/openstack-sync/openstack_sync/hooks/framework.py b/python/openstack-sync/openstack_sync/hooks/framework.py index aee8fe0c5..7ba150103 100644 --- a/python/openstack-sync/openstack_sync/hooks/framework.py +++ b/python/openstack-sync/openstack_sync/hooks/framework.py @@ -663,15 +663,13 @@ def _run_prune( conn = connections.get(credentials) if conn is None: - if not plugin.config.prune: - continue try: conn = get_openstack_connection(secret_name, cloud_name) - plugin.wait_for_api(conn) except Exception as exc: # noqa: BLE001 prune_failed = True LOG.error( - "Cannot reach OpenStack for %s prune cloud=%r secret=%r: %s", + "Cannot build an OpenStack connection for the %s prune " + "cloud=%r secret=%r: %s", noun, cloud_name, secret_name, diff --git a/python/openstack-sync/openstack_sync/hooks/router_flavors.py b/python/openstack-sync/openstack_sync/hooks/router_flavors.py index 1de539ec5..5d5a59e08 100644 --- a/python/openstack-sync/openstack_sync/hooks/router_flavors.py +++ b/python/openstack-sync/openstack_sync/hooks/router_flavors.py @@ -33,9 +33,8 @@ def wait_for_api(self, conn: Any) -> None: ) def new_cache(self) -> reconcile_module.ProfileCache: - # Keyed by driver and shared across every flavor in one credential - # group, so two flavors wanting the same profile share one lookup and - # end up sharing one profile. + # Keyed by driver, shared across the credential group, so two flavors + # wanting the same profile share one lookup and one profile. return {} def reconcile( @@ -51,7 +50,10 @@ def prune( authoritative_empty: bool, ) -> None: if not self.config.prune: + # PRUNE gates flavor deletion, not the orphan sweep. + prune_module.prune_orphaned_profiles(conn) return + # prune_removed_flavors sweeps orphans itself. prune_module.prune_removed_flavors( conn, desired_specs, authoritative_empty=authoritative_empty ) diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py b/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py index 460871db0..8989a6cc4 100644 --- a/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py +++ b/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py @@ -1,9 +1,7 @@ """Delete router flavors and service profiles whose CR was removed. -Everything here is gated on the operator's ownership markers. A hand-made flavor -or profile is untouched until a CR causes the operator to create or adopt it; a -resource carrying the marker is in the operator-managed set, which makes any -further filtering redundant. +Every deletion is gated on the operator's ownership marker, so a hand-made +flavor or profile is never touched. """ from __future__ import annotations @@ -119,18 +117,32 @@ def _delete_flavor( maybe_delete_profile(conn, profile_id, cache, counts) -def _prune_orphaned_profiles( +def _sweep_orphaned_profiles( conn: Any, cache: ProfileCache, counts: Counter[str] ) -> None: - """Delete owned, unattached profiles left behind by an earlier partial failure. + """Delete owned, unattached profiles left behind by a partial failure. - Safe to run every cycle: it only ever touches operator-owned profiles that - no flavor is bound to. + Safe every cycle: only owned profiles with no flavor bound to them. """ LOG.info("Scanning for orphaned operator-owned service profiles") for profile in list(conn.network.service_profiles()): - if is_managed_service_profile(profile): - maybe_delete_profile(conn, resource_id(profile), cache, counts) + if not is_managed_service_profile(profile): + continue + profile_id = resource_id(profile) + # The listing already gave us the profile; skip the per-profile GET. + cache.setdefault(profile_id, profile) + maybe_delete_profile(conn, profile_id, cache, counts) + + +def prune_orphaned_profiles(conn: Any) -> None: + """Sweep owned, unattached service profiles, deleting no flavor. + + The entry point when ``PRUNE`` is off. That flag gates flavor deletion, + which needs a trustworthy desired set; an unbound owned profile needs none. + Only reaches credential groups the run already has a connection for. + """ + flavors = list(conn.network.flavors(service_type=SERVICE_TYPE)) + _sweep_orphaned_profiles(conn, {}, _attachment_counts(flavors)) def prune_removed_flavors( @@ -141,8 +153,8 @@ def prune_removed_flavors( ) -> None: """Delete operator-owned router flavors absent from *desired_specs*. - An empty *desired_specs* is only acted on when *authoritative_empty* says a - CR really was deleted; otherwise it may be a snapshot we could not read, and + An empty *desired_specs* is acted on only when *authoritative_empty* says a + CR really was deleted; otherwise it may be an unreadable snapshot, and pruning against it would delete every managed flavor. """ if not desired_specs and not authoritative_empty: @@ -166,4 +178,4 @@ def prune_removed_flavors( continue _delete_flavor(conn, flavor, cache, counts) - _prune_orphaned_profiles(conn, cache, counts) + _sweep_orphaned_profiles(conn, cache, counts) diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/reconcile.py b/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/reconcile.py index 4f13215b7..b70168dc8 100644 --- a/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/reconcile.py +++ b/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/reconcile.py @@ -47,13 +47,9 @@ class ProfileDrift: """One field of a reused service profile that diverged from the CR spec. - Profile drift is reported, never auto-corrected. Neutron's - ``update_service_profile`` calls ``_ensure_service_profile_not_in_use`` and - raises ``ServiceProfileInUse`` (HTTP 409) while *any* flavor binding exists - -- not merely while a router is using it -- and this operator binds every - profile it manages. An update attempt would fail every cycle. Correcting - drift means unbinding the profile from every flavor first, which is an - operator decision. + Reported, never auto-corrected: Neutron rejects an update to a profile bound + to any flavor (HTTP 409), and this operator binds every profile it manages, + so unbinding first is an operator decision. """ profile_id: str @@ -80,16 +76,21 @@ def profiles_for_driver(conn: Any, driver: str, cache: ProfileCache) -> list[Any return cache[driver] -def find_matching_profile(profiles: list[Any], meta_info: Any) -> Any | None: - """Return a service profile matching *meta_info*, preferring owned profiles. +def find_matching_profile( + profiles: list[Any], driver: str, meta_info: Any +) -> Any | None: + """Return a profile matching *driver* and *meta_info*, preferring owned ones. - A NeutronRouterFlavor CR is an ownership claim for the flavor and the service - profiles described under it. If a matching profile already exists without - the marker, ``ensure_profile`` adopts it before binding or pruning depends on - that marker. + A CR claims ownership of the profiles under it, so an unowned match is + returned for ``ensure_profile`` to adopt. + + *driver* is re-checked, not trusted from the candidate list: binding the + wrong driver would send routers to the wrong service provider. """ unowned_match: Any | None = None for profile in profiles: + if get_value(profile, "driver") != driver: + continue if not meta_info_matches(service_profile_meta_info(profile), meta_info): continue if is_managed_service_profile(profile): @@ -131,14 +132,11 @@ def _profile_drift( ) -> list[ProfileDrift]: """Return the spec fields a reused *profile* disagrees with. - ``meta_info`` is excluded by construction -- the profile was selected by - matching it -- and ``driver`` is excluded because profiles are queried per - driver. That leaves ``is_enabled`` and ``description``. + Only ``is_enabled`` and ``description``; the profile was selected by matching + ``driver`` and ``meta_info``. - ``is_enabled`` is the consequential one: Neutron's - ``get_flavor_next_provider`` raises ``ServiceProfileDisabled`` (HTTP 503) - when the profile it selects is disabled, so every router create against the - flavor fails while the flavor still looks healthy. + ``is_enabled`` is the consequential one: a disabled profile fails every + router create against the flavor (HTTP 503) while the flavor looks healthy. """ checks = ( ( @@ -183,17 +181,15 @@ def ensure_profile( ) -> Any: """Find or create the service profile *spec* describes. - The CRD guarantees ``driver`` and ``is_enabled`` are present; ``description`` - and ``meta_info`` are optional and fall back to empty. Drift on a reused - profile is appended to *drift* -- this is the only place holding both the - desired spec value and the Neutron state, so it is the only place drift can - be detected. + The CRD guarantees ``driver`` and ``is_enabled``; ``description`` and + ``meta_info`` fall back to empty. Drift on a reused profile is appended to + *drift*, the only place holding both the spec value and the Neutron state. """ driver = spec["driver"] meta_info = spec.get("meta_info", {}) profiles = profiles_for_driver(conn, driver, cache) - profile = find_matching_profile(profiles, meta_info) + profile = find_matching_profile(profiles, driver, meta_info) if profile: profile_id = resource_id(profile) if not is_managed_service_profile(profile): @@ -224,8 +220,7 @@ def ensure_profile( meta_info=meta_info_payload(managed_meta_info(meta_info)), is_enabled=spec["is_enabled"], ) - # Visible to any later flavor this run with an identical (driver, meta_info) - # spec, so it reuses this profile instead of creating a duplicate. + # So a later flavor with the same (driver, meta_info) reuses it. profiles.append(created) return created @@ -238,9 +233,8 @@ def ensure_profile( def find_flavor(conn: Any, name: str) -> Any | None: """Return the flavor named *name*, or None. - The SDK passes ``name=`` as a server-side query parameter which Neutron - filters in SQL, so at most one record comes back; the equality check guards - against a future change to substring semantics. + Neutron filters ``name=`` in SQL, so at most one record comes back; the + equality check guards against a switch to substring semantics. """ for flavor in conn.network.flavors(name=name): if get_value(flavor, "name") == name: @@ -354,9 +348,8 @@ def reconcile_flavor_profiles( ) -> Any: """Converge the set of service profiles bound to *flavor*. - Profiles missing from the flavor are bound; operator-owned profiles bound to - it but absent from the desired set are unbound. Profiles attached - out-of-band are left alone -- the operator only unbinds what it owns. + Missing profiles are bound; owned profiles absent from the desired set are + unbound. A profile attached out-of-band is left alone. """ flavor = conn.network.get_flavor(flavor) flavor_name = get_value(flavor, "name", default=resource_id(flavor)) diff --git a/python/openstack-sync/tests/test_framework.py b/python/openstack-sync/tests/test_framework.py index 68b79c886..24462b5e5 100644 --- a/python/openstack-sync/tests/test_framework.py +++ b/python/openstack-sync/tests/test_framework.py @@ -944,15 +944,41 @@ def test_run_sync_skips_prune_for_credentials_with_no_desired_resources(): assert plugin.pruned == [] -def test_run_sync_does_not_connect_for_prune_when_prune_disabled(): - """A deleted-only run must not open a connection just to do nothing.""" +def test_run_sync_connects_to_prune_a_deletion_whatever_prune_says(): + """A deletion reconciles nothing, so the prune opens the connection itself. + + ``PRUNE`` is the plugin's flag, not the driver's: a prune can have work the + flag does not gate, so the driver hands over a connection and lets the + plugin decide. It does not wait for the API first -- the prune's own first + call is the probe. + """ plugin = StubPlugin(make_hook_config(prune=False)) inputs = _inputs([], desired=[], deleted=[_resource("gone")]) code, _, connect = _drive(plugin, inputs) assert code == 0 - assert connect.call_count == 0 + assert connect.call_count == 1 + assert plugin.waits == 0 + assert plugin.pruned == [([], True)] + + +def test_run_sync_reports_a_prune_whose_connection_cannot_be_built(): + """Credentials that cannot be loaded fail the run rather than pruning.""" + plugin = StubPlugin(make_hook_config(prune=True)) + inputs = _inputs([], desired=[], deleted=[_resource("gone")]) + + with ( + mock.patch.object( + framework, + "get_openstack_connection", + side_effect=RuntimeError("no clouds.yaml in secret"), + ), + mock.patch.object(framework, "patch_resource_status"), + ): + code = run_sync(plugin, inputs) + + assert code == 1 assert plugin.pruned == [] diff --git a/python/openstack-sync/tests/test_prune.py b/python/openstack-sync/tests/test_prune.py index 255c28977..9b6508d31 100644 --- a/python/openstack-sync/tests/test_prune.py +++ b/python/openstack-sync/tests/test_prune.py @@ -1,8 +1,7 @@ """Tests for router flavor prune behaviour. -Pruning is gated entirely on the operator's ownership markers, so these tests -are mostly about what must *not* be deleted. Whether pruning runs at all is the -hook's decision (``config.prune``), tested in ``test_framework.py``. +Deletion is gated on the ownership markers, so most of these cover what must +*not* be deleted. Whether prune runs at all is tested in ``test_framework.py``. """ from __future__ import annotations @@ -29,6 +28,7 @@ def __init__(self, flavors: list[dict[str, Any]], profiles: dict[str, Any]): self.deleted_flavors: list[str] = [] self.deleted_profiles: list[str] = [] self.flavor_list_calls = 0 + self.profile_get_calls = 0 def flavors(self, service_type: str | None = None) -> list[dict[str, Any]]: self.flavor_list_calls += 1 @@ -45,6 +45,7 @@ def service_profiles(self) -> list[Any]: return [p for p in self._profiles.values() if p is not None] def get_service_profile(self, profile_id: str) -> Any: + self.profile_get_calls += 1 profile = self._profiles.get(profile_id) if profile is None: raise openstack_exceptions.NotFoundException(f"no profile {profile_id}") @@ -210,6 +211,9 @@ def test_prune_lists_flavors_once_for_all_profile_checks(): prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) assert conn.network.flavor_list_calls == 1 + # Only the deleted flavor's profile needs a GET; the orphan came from the + # sweep's own listing. + assert conn.network.profile_get_calls == 1 assert conn.network.deleted_flavors == ["removed-flavor-id"] assert conn.network.deleted_profiles == [ "removed-profile-id", @@ -226,3 +230,66 @@ def test_prune_skips_flavor_still_used_by_routers(): prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_flavors == [] + + +# --------------------------------------------------------------------------- +# The orphaned profile sweep on its own, with PRUNE off +# --------------------------------------------------------------------------- + + +def test_prune_orphaned_profiles_deletes_an_owned_unattached_profile(): + orphan = _owned_profile("orphan-profile-id") + conn = _conn([], {orphan.id: orphan}) + + prune.prune_orphaned_profiles(conn) + + assert conn.network.deleted_profiles == ["orphan-profile-id"] + + +def test_prune_orphaned_profiles_keeps_an_unowned_profile(): + unowned = SimpleNamespace( + id="unmanaged-profile-id", + driver=_DRIVER, + meta_info={"vni_alloc": "auto"}, # no ownership marker + ) + conn = _conn([], {unowned.id: unowned}) + + prune.prune_orphaned_profiles(conn) + + assert conn.network.deleted_profiles == [] + + +def test_prune_orphaned_profiles_keeps_an_attached_profile(): + """The sweep needs its own attachment counts, or it deletes a bound profile.""" + attached = _owned_profile("attached-profile-id") + kept = _owned_flavor("kept-flavor-id", "kept-flavor", [attached.id]) + conn = _conn([kept], {attached.id: attached}) + + prune.prune_orphaned_profiles(conn) + + assert conn.network.deleted_profiles == [] + + +def test_prune_orphaned_profiles_reuses_the_listing_instead_of_refetching(): + """The sweep already holds the profile, so it must not GET it again.""" + orphan = _owned_profile("orphan-profile-id") + conn = _conn([], {orphan.id: orphan}) + + prune.prune_orphaned_profiles(conn) + + assert conn.network.profile_get_calls == 0 + assert conn.network.deleted_profiles == ["orphan-profile-id"] + + +def test_prune_orphaned_profiles_deletes_no_flavor(): + """The sweep must not delete a flavor; that still needs PRUNE.""" + orphan = _owned_profile("orphan-profile-id") + conn = _conn( + [_owned_flavor("removed-flavor-id", "removed-flavor")], + {orphan.id: orphan}, + ) + + prune.prune_orphaned_profiles(conn) + + assert conn.network.deleted_flavors == [] + assert conn.network.deleted_profiles == ["orphan-profile-id"] diff --git a/python/openstack-sync/tests/test_reconcile.py b/python/openstack-sync/tests/test_reconcile.py index 5e7280846..bd01055e4 100644 --- a/python/openstack-sync/tests/test_reconcile.py +++ b/python/openstack-sync/tests/test_reconcile.py @@ -1,9 +1,7 @@ """Tests for router flavor reconciliation. -Covers the profile cache, create-or-adopt by ``(driver, meta_info)``, profile -ownership transfer, drift reporting on reused profiles, the flavor -``service_type`` guard and ``is_enabled``/description reconcile, and the -flavor-to-profile binding set. +Covers the profile cache, create-or-adopt by ``(driver, meta_info)``, ownership +transfer, drift reporting, the ``service_type`` guard, and the binding set. """ from __future__ import annotations @@ -216,9 +214,8 @@ def test_ensure_profile_reuses_existing_owned_profile(): def test_ensure_profile_appends_created_profile_to_driver_cache(): """A profile created for one flavor must be visible to the next flavor. - The cache is shared across all flavors in a credential group, so two - flavors with an identical ``(driver, meta_info)`` spec share one profile - rather than each creating a duplicate. + The cache is shared across the credential group, so two flavors with the + same ``(driver, meta_info)`` share one profile instead of duplicating it. """ meta_info = {"vni_alloc": "auto"} created = _make_profile("new-profile", meta_info=meta_info) @@ -277,7 +274,7 @@ def test_find_matching_profile_returns_unowned_match(): meta_info = {"vni_alloc": "auto"} unowned = _make_profile("adhoc-profile", meta_info=meta_info, managed=False) - assert reconcile.find_matching_profile([unowned], meta_info) is unowned + assert reconcile.find_matching_profile([unowned], _DRIVER, meta_info) is unowned def test_find_matching_profile_prefers_owned_over_unowned(): @@ -285,7 +282,37 @@ def test_find_matching_profile_prefers_owned_over_unowned(): unowned = _make_profile("adhoc-profile", meta_info=meta_info, managed=False) owned = _make_profile("owned-profile", meta_info=meta_info) - assert reconcile.find_matching_profile([unowned, owned], meta_info) is owned + assert ( + reconcile.find_matching_profile([unowned, owned], _DRIVER, meta_info) is owned + ) + + +def test_find_matching_profile_ignores_a_profile_with_another_driver(): + """The candidate list is Neutron's driver filter; the match must not trust it.""" + meta_info = {"vni_alloc": "auto"} + other_driver = _make_profile( + "other-driver-profile", driver="some.other.Driver", meta_info=meta_info + ) + + assert reconcile.find_matching_profile([other_driver], _DRIVER, meta_info) is None + + +def test_ensure_profile_creates_rather_than_adopting_another_driver(): + """A wrong-driver profile in the candidate list must not be bound to a flavor.""" + meta_info = {"vni_alloc": "auto"} + other_driver = _make_profile( + "other-driver-profile", driver="some.other.Driver", meta_info=meta_info + ) + created = _make_profile("new-profile", meta_info=meta_info) + conn = _create_conn(created, existing=[other_driver]) + + result = reconcile.ensure_profile( + conn, _NAME, _profile_spec(meta_info=meta_info), {}, [] + ) + + assert result is created + conn.network.update_service_profile.assert_not_called() + assert conn.network.create_service_profile.call_args.kwargs["driver"] == _DRIVER def test_ensure_profile_adopts_unowned_match(): @@ -374,9 +401,8 @@ def test_adopted_unowned_match_can_later_be_unbound(): def test_ensure_profile_reports_is_enabled_drift_on_reuse(caplog): """A profile disabled out-of-band is reported, not silently accepted. - Neutron's get_flavor_next_provider raises ServiceProfileDisabled for the - profile it selects, so every router create against the flavor fails while - the flavor itself still looks converged. + A disabled profile fails every router create against the flavor while the + flavor itself still looks converged. """ existing = _make_profile("owned-profile", is_enabled=False) conn = _reuse_conn(existing) @@ -725,8 +751,8 @@ def test_sync_flavor_returns_no_notes_when_nothing_drifted(): def test_sync_flavor_reports_profile_drift(): """Drift found while resolving profiles reaches the caller as notes. - The flavor itself is converged, so this is not a failure -- but the caller - must be able to qualify the status it reports. + Not a failure -- the flavor converged -- but the caller must be able to + qualify the status it reports. """ flavor = _make_flavor(service_profile_ids=["owned-profile"]) drifted_profile = _make_profile("owned-profile", is_enabled=False) diff --git a/python/openstack-sync/tests/test_router_flavors_hook.py b/python/openstack-sync/tests/test_router_flavors_hook.py index 4705ce19b..1050307a8 100644 --- a/python/openstack-sync/tests/test_router_flavors_hook.py +++ b/python/openstack-sync/tests/test_router_flavors_hook.py @@ -156,7 +156,7 @@ def test_plugin_wait_for_api_uses_configured_retry_budget(): wait.assert_called_once_with(conn, retries=5, delay=0.25) -def test_plugin_prune_is_a_noop_when_disabled(): +def test_plugin_prune_deletes_no_flavor_when_disabled(): plugin = hook.RouterFlavorPlugin(make_hook_config(prune=False)) with mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune: @@ -165,15 +165,31 @@ def test_plugin_prune_is_a_noop_when_disabled(): prune.assert_not_called() +def test_plugin_prune_sweeps_orphaned_profiles_when_disabled(): + """PRUNE gates flavor deletion; an unattached owned profile is collected anyway.""" + plugin = hook.RouterFlavorPlugin(make_hook_config(prune=False)) + conn = mock.MagicMock() + + with mock.patch.object(hook.prune_module, "prune_orphaned_profiles") as sweep: + plugin.prune(conn, [{"name": "a"}], authoritative_empty=False) + + sweep.assert_called_once_with(conn) + + def test_plugin_prune_forwards_authoritative_empty_when_enabled(): plugin = hook.RouterFlavorPlugin(make_hook_config(prune=True)) conn = mock.MagicMock() specs = [{"name": "a"}] - with mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune: + with ( + mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune, + mock.patch.object(hook.prune_module, "prune_orphaned_profiles") as sweep, + ): plugin.prune(conn, specs, authoritative_empty=True) prune.assert_called_once_with(conn, specs, authoritative_empty=True) + # prune_removed_flavors sweeps internally; a second call would re-list. + sweep.assert_not_called() def test_plugin_cache_is_per_credential_group(): @@ -243,6 +259,19 @@ def _schedule_context(*names: str) -> list[dict]: ] +def _deleted_context(name: str) -> list[dict]: + """A Deleted event with the empty snapshot that follows the last CR.""" + return [ + { + "binding": BINDING_NAME, + "type": "Event", + "watchEvent": "Deleted", + "object": router_flavor_object(name), + "snapshots": {BINDING_NAME: []}, + } + ] + + def test_main_returns_zero_when_hook_disabled(monkeypatch, tmp_path): clear_env(monkeypatch) conn = _neutron_conn() @@ -328,6 +357,40 @@ def test_main_prunes_after_a_successful_reconcile(monkeypatch, tmp_path): assert [spec["name"] for spec in prune.call_args.args[1]] == ["pa1410"] +def test_main_sweeps_an_orphaned_profile_when_the_last_cr_is_deleted( + monkeypatch, tmp_path +): + """Deleting the last CR still collects an unbound managed profile. + + The run reconciles nothing, so the sweep opens the connection itself. + """ + clear_env(monkeypatch) + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + conn = _neutron_conn() + bound_profile = conn.network.service_profiles.return_value[0] + orphan = types.SimpleNamespace( + id="orphan-profile-id", + driver="neutron_understack.l3_router.vrf.Vrf", + is_enabled=True, + description="pa1410 profile", + meta_info=markers.managed_meta_info({"vni_alloc": "auto"}), + ) + conn.network.service_profiles.return_value = [bound_profile, orphan] + + code, patch_status = _run_main( + monkeypatch, tmp_path, _deleted_context("pa1410"), conn + ) + + assert code == 0 + patch_status.assert_not_called() + # The orphan, and only the orphan: the bound profile stays. + assert [ + call.args[0] for call in conn.network.delete_service_profile.call_args_list + ] == [orphan] + # PRUNE gates flavor deletion, and it is off. + conn.network.delete_flavor.assert_not_called() + + def test_main_fails_loudly_on_a_cr_missing_cloud_credentials(monkeypatch, tmp_path): """A CR without credentials must fail the run, not be skipped. From 68552a8f9f07ad2796fdfaca726f1fe9e86f6afe Mon Sep 17 00:00:00 2001 From: haseeb Date: Thu, 10 Sep 2026 23:20:35 +0530 Subject: [PATCH 2/2] fix(openstack-sync): collapse duplicate CR events and tolerate a 404 status write Shell-operator hands a hook every event it has queued and replays the whole backlog while a run keeps failing, so a batch can carry dozens of events for one CR. Key the changed and deleted maps by CR uid so one CR is one reconcile, and drop a CR whose Deleted event is in the same batch: the prune still removes it. A status write for a CR that is already gone now logs at info and returns, rather than reporting a failure nothing is waiting on. --- .../openstack-sync-operator/DESIGN-NOTES.md | 64 ++-- python/openstack-sync/README.md | 31 +- .../openstack_sync/hooks/common.py | 26 ++ .../openstack_sync/hooks/framework.py | 173 ++++++++--- .../openstack_sync/hooks/ironic_runbooks.py | 8 +- .../openstack_sync/hooks/router_flavors.py | 8 +- .../plugins/ironic/runbooks/prune.py | 36 ++- .../plugins/neutron/router_flavors/prune.py | 33 ++- python/openstack-sync/tests/test_framework.py | 274 ++++++++++++++++-- .../openstack-sync/tests/test_hook_common.py | 137 +++++++++ .../tests/test_ironic_runbooks_hook.py | 25 +- .../tests/test_ironic_runbooks_prune.py | 87 +++++- python/openstack-sync/tests/test_prune.py | 111 ++++++- .../tests/test_router_flavors_hook.py | 66 ++++- 14 files changed, 923 insertions(+), 156 deletions(-) diff --git a/components/openstack-sync-operator/DESIGN-NOTES.md b/components/openstack-sync-operator/DESIGN-NOTES.md index 3a7b8e8d1..5198350b2 100644 --- a/components/openstack-sync-operator/DESIGN-NOTES.md +++ b/components/openstack-sync-operator/DESIGN-NOTES.md @@ -30,10 +30,22 @@ is turned on in the operator values and its script is in the image. ### Runtime model -There's no always-running loop or work queue. shell-operator watches the CRDs and +There's no always-running loop of our own. shell-operator watches the CRDs and runs a hook script on each `Added` / `Modified` / `Deleted` event, plus a -periodic full resync on a timer (`SYNC_CRONTAB`). Retries are implicit: if a run -fails, it waits for the next event or the next scheduled resync. +periodic full resync on a timer (`SYNC_CRONTAB`). + +Retries are not implicit, and this matters. Per the +[shell-operator docs](https://github.com/flant/shell-operator/blob/main/docs/src/HOOKS.md), +each queue runs its hooks strictly in sequence, and a hook that exits non-zero is +re-run every few seconds until it succeeds, with everything else in that queue +blocked until it does. `allowFailure` would change that and we don't set it. Each +hook gets its own queue (`queue: `), so the blockage is contained to +one resource type. + +The consequence: exiting non-zero is only useful for a fault that a retry could +clear. For a permanent one -- a malformed CR already stored in etcd, say -- a +non-zero exit buys nothing and pins the queue, which stops the healthy CRs from +reconciling too. Those get reported in the log and the run exits zero. ### Core framework @@ -44,12 +56,20 @@ resource type is a `SyncPlugin` subclass that provides four things: - `reconcile(conn, spec, cache)` — bring one CR spec in line with OpenStack, and return any notes about things it won't fix on its own. - `new_cache()` — a scratch cache shared by all CRs using the same credentials. -- `prune(conn, desired_specs, authoritative_empty)` — delete resources whose CR - is gone (optional; does nothing by default). +- `prune(conn, desired_specs, deleted_specs, sweep_unseen)` — delete resources + whose CR is gone: `deleted_specs` names the CRs just lost, `desired_specs` is + what must survive, and `sweep_unseen` additionally allows deleting anything + managed that `desired_specs` does not name, which catches a CR whose removal + was never observed (optional; does nothing by default). `run_sync()` handles the rest: grouping CRs by credentials, opening one OpenStack connection per group, reconciling each CR and updating its status, and running a -guarded prune at the end. +guarded prune at the end. The guard scales with how trustworthy the desired set +is: everything reconciled means a full prune; a failed reconcile withholds +`sweep_unseen` but still lets deletions through, since those name their resources +and the failing CR is still in the desired set; an unreadable CR withholds the +prune entirely, because its resource names are unknown and so cannot be protected +from a deletion naming one of them. ### Reconcile behavior (per resource) @@ -79,8 +99,9 @@ and are otherwise left alone. The CRDs have a status subresource with `syncStatus` (Synced/Failed/Unknown), `lastSyncTime`, `observedGeneration`, `message`, and a standard `conditions[]` -list. Status is written by running `kubectl patch --subresource status` in a -subprocess (`hooks/common.py`). +list. The hook writes status through the Kubernetes Python client's status +subresource API (`hooks/common.py`). Failed status writes are logged but do not +fail the reconcile itself, because status is reporting, not the OpenStack work. ### Safety details worth noting @@ -92,6 +113,9 @@ The framework handles a few tricky cases carefully: endless loop. - **Skips no-op status writes**: it doesn't rewrite status when the important fields already match, which avoids extra Modified events. +- **Tolerates stale status targets**: if the CR disappears between reconcile and + status patch, a 404 for that CR is logged at info and ignored; a missing CRD or + other API failure is still reported. - **Guards prune**: if any CR failed to reconcile or couldn't be read, prune is skipped completely, since it can't know the full desired set and might delete something it shouldn't. @@ -128,20 +152,15 @@ The framework handles a few tricky cases carefully: an hour depending on `SYNC_CRONTAB`. A short OpenStack hiccup can leave a CR `Failed` for a while. -3. **Status uses a `kubectl` subprocess.** This starts a process per patch and - needs the `kubectl` binary in the image, even though the code already uses the - Python Kubernetes client to read Secrets. `common.py` even has a - "kubectl not found" branch to handle its absence. - -4. **Single replica, no leader election.** `replicaCount: 1` and no HA. That's +3. **Single replica, no leader election.** `replicaCount: 1` and no HA. That's fine for config sync, but together with the missed-delete gap, any downtime is a window where deletes get lost. -5. **No per-resource metrics.** Only shell-operator's built-in metrics (port +4. **No per-resource metrics.** Only shell-operator's built-in metrics (port 9115) and TCP probes are available. There's nothing per-CRD like reconcile count, failure count, or drift-note count for dashboards or alerts. -6. **Markers are defined per plugin.** Each plugin rolls its own marker scheme +5. **Markers are defined per plugin.** Each plugin rolls its own marker scheme (router flavors in `meta_info`, flavors in `description`, runbooks similar). There's no shared, versioned marker format, so a new plugin could do it a little differently. @@ -156,26 +175,21 @@ Roughly in order of value. None of these mean dropping shell-operator. (patching `metadata.finalizers`), so it's worth checking the leak actually matters for a resource before adding it everywhere. -2. **Switch status writes to the Python Kubernetes client.** The client is - already a dependency. This drops the per-patch subprocess, removes the - `kubectl` binary requirement, gives cleaner error handling, and gets rid of the - "kubectl not found" case. - -3. **Add retry/backoff for temporary failures.** shell-operator doesn't do +2. **Add retry/backoff for temporary failures.** shell-operator doesn't do per-object requeue timing, but its queue retry settings can be tuned, or `SYNC_CRONTAB` shortened, so a temporary failure retries sooner than the next full resync. At least document how long a retry actually takes. -4. **Add per-resource metrics.** Reconcile count, failure count, and drift-note +3. **Add per-resource metrics.** Reconcile count, failure count, and drift-note count per CRD would make the operator easier to watch. shell-operator can export hook metrics; surface them in the chart. -5. **Say the single-replica choice out loud.** If missed deletes matter and +4. **Say the single-replica choice out loud.** If missed deletes matter and finalizers aren't added, HA on its own doesn't fully fix it (the event is still lost during a gap). Writing down that this is single-replica on purpose, and why, helps operators reason about the tradeoff. -6. **Make the marker scheme a shared, versioned contract.** A shared marker module +5. **Make the marker scheme a shared, versioned contract.** A shared marker module with one versioned key format keeps adoption and prune rules consistent across plugins and easier to check. diff --git a/python/openstack-sync/README.md b/python/openstack-sync/README.md index 2b8e0cbee..32a14bd73 100644 --- a/python/openstack-sync/README.md +++ b/python/openstack-sync/README.md @@ -31,13 +31,28 @@ openstack_sync/ `run_sync` groups CRs by the credentials in `spec.cloudCredentialsRef`, opens one connection per credential group, waits for the OpenStack service, reconciles each CR, patches `Synced`/`Failed` onto the CR status, and then calls the plugin's -prune step, which most plugins gate on `PRUNE`. If any reconcile fails, or any CR -could not be read at all, it **skips the prune entirely** - either way the -desired state is unknown, so deleting anything would be unsafe. +prune step, which most plugins gate on `PRUNE`. + +How much of that prune runs depends on how trustworthy the desired set is: + +- **All CRs reconciled.** The full prune: resources a deletion names, plus any + owned resource the desired set does not name, which catches a CR whose removal + was never observed. +- **A reconcile failed.** Deleting by absence is withheld, because the failing + CR's resource would read as unwanted. Deletions still go through: they name + their resources, and the failing CR is still in the desired set and so still + protected. The run exits non-zero so shell-operator retries it. +- **A CR could not be read.** No prune at all. The desired set is short by + however many CRs were dropped, and their resource names are unknown, so they + cannot be protected from a deletion that happens to name one of them. A CR whose spec does not satisfy the framework's contract is named in the log and -dropped, and the run exits non-zero. The remaining CRs still reconcile: one -unusable object must not stall a whole namespace. +dropped. The remaining CRs still reconcile: one unusable object must not stall a +whole namespace. The run does **not** exit non-zero for this alone, because the +object is stored that way and would be dropped again on every retry -- and +shell-operator re-runs a failing hook every few seconds while blocking the rest +of its queue, so reporting it as a failure would stop the healthy CRs from +reconciling for as long as the malformed CR exists. Alert on the error log. `run_hook` handles the shell-operator calling convention: `--config`, logging, reading the binding context, and the exit code. @@ -91,10 +106,12 @@ reading the binding context, and the exit code. def reconcile(self, conn, spec, cache) -> list[str]: return reconcile_module.sync(conn, spec, cache) - def prune(self, conn, desired_specs, *, authoritative_empty) -> None: + def prune(self, conn, desired_specs, *, deleted_specs, + sweep_unseen) -> None: if self.config.prune: prune_module.prune(conn, desired_specs, - authoritative_empty=authoritative_empty) + deleted_specs=deleted_specs, + sweep_unseen=sweep_unseen) def main() -> int: def run(contexts): diff --git a/python/openstack-sync/openstack_sync/hooks/common.py b/python/openstack-sync/openstack_sync/hooks/common.py index df0480e63..b0b45ad22 100644 --- a/python/openstack-sync/openstack_sync/hooks/common.py +++ b/python/openstack-sync/openstack_sync/hooks/common.py @@ -228,6 +228,22 @@ def _api_error_detail(exc: ApiException, max_body: int = 512) -> str: return f"{detail}: {truncate_message(body, max_body)}" +def _api_error_is_missing_object(exc: ApiException, name: str) -> bool: + """Return whether a 404 is for *name* itself rather than for its CRD. + + A missing object answers with a Status naming it in ``details.name``; an + unserved plural, group or version answers with plain text. A 403 Status + names it the same way, so the status check is not redundant. + """ + if exc.status != 404: + return False + try: + return json.loads(exc.body or "")["details"]["name"] == name + except (ValueError, TypeError, LookupError): + # Not JSON, or JSON the API server did not shape like a Status. + return False + + def patch_resource_status( *, name: str, @@ -309,6 +325,16 @@ def patch_resource_status( body={"status": status}, ) except ApiException as exc: + if _api_error_is_missing_object(exc, name): + # The CR went away between the reconcile and this write, so nothing + # is waiting on its status. A 404 for the CRD still warns below. + LOG.info( + "not patching %s status for %s; the CR is gone: %s", + crd_kind, + name, + _api_error_detail(exc), + ) + return LOG.warning( "failed to patch %s status for %s: %s", crd_kind, diff --git a/python/openstack-sync/openstack_sync/hooks/framework.py b/python/openstack-sync/openstack_sync/hooks/framework.py index 7ba150103..b92e62bf3 100644 --- a/python/openstack-sync/openstack_sync/hooks/framework.py +++ b/python/openstack-sync/openstack_sync/hooks/framework.py @@ -164,6 +164,15 @@ class SyncResource: def credentials(self) -> CredentialKey: return (self.secret_name, self.cloud_name) + @property + def identity(self) -> str: + """Key identifying this CR across a batch of events. + + Unique among live objects. It does not separate a recreated CR from the + one it replaced, so :func:`_split_events` reads events in order. + """ + return f"{self.namespace}/{self.name}" + @property def display_name(self) -> str: """Return the OpenStack resource name, falling back to the CR name.""" @@ -256,7 +265,7 @@ def _resource_from_object(obj: dict[str, Any]) -> SyncResource: f"got secretName={secret_name!r}, cloudName={cloud_name!r}" ) - metadata = obj.get("metadata", {}) + metadata = obj.get("metadata") or {} return SyncResource( spec=spec, @@ -285,13 +294,24 @@ class _ResourceReader: def __init__(self) -> None: self.unreadable: set[str] = set() - def read(self, obj: dict[str, Any], description: str = "CR") -> SyncResource | None: - """Return the resource for *obj*, or None when it cannot be read.""" + def read( + self, + obj: dict[str, Any], + description: str = "CR", + *, + in_desired_set: bool = True, + ) -> SyncResource | None: + """Return the resource for *obj*, or None when it cannot be read. + + ``in_desired_set`` is False for a departing object: its read failure + says nothing about the desired set, so it must not hold back the prune. + """ try: return _resource_from_object(obj) except _MalformedResourceError as exc: identity = _resource_identity(obj) - self.unreadable.add(identity) + if in_desired_set: + self.unreadable.add(identity) LOG.error("Ignoring unreadable %s %s: %s", description, identity, exc) return None @@ -329,10 +349,23 @@ def _status_is_current(resource: SyncResource) -> bool: def _split_events( contexts: list[dict[str, Any]], config: HookConfig, reader: _ResourceReader ) -> tuple[list[SyncResource], list[SyncResource], bool]: - """Split this binding's Event contexts into changed and deleted resources.""" - changed: list[SyncResource] = [] - deleted: list[SyncResource] = [] + """Split this binding's Event contexts into changed and deleted resources. + + Shell-operator replays its whole backlog, so a batch can carry dozens of + events for one CR. Both maps are keyed by identity to collapse them. + + Order decides the rest. A Deleted cancels an earlier change -- removing the + resource is the prune's business, which ``deleted`` still carries -- while a + change after a Deleted is a recreated CR that must still reconcile. The + binding's queue is FIFO, so the last context for a CR is the current one. + """ + changed: dict[str, SyncResource] = {} + deleted: dict[str, SyncResource] = {} saw_event_context = False + # Counted apart from ``changed`` so a cancelled CR reads as neither a + # duplicate nor a reconcile. + changed_events = 0 + changed_identities: set[str] = set() for context in contexts: if context.get("binding") != config.binding_name: @@ -355,12 +388,21 @@ def _split_events( ) continue - resource = reader.read(obj, f"{watch_event} CR") + resource = reader.read( + obj, f"{watch_event} CR", in_desired_set=watch_event != "Deleted" + ) if resource is None: continue if watch_event == "Deleted": - deleted.append(resource) + deleted[resource.identity] = resource + superseded = changed.pop(resource.identity, None) + if superseded is not None: + LOG.info( + "Not reconciling %s %s; a later event in this batch deleted it", + config.crd_kind, + superseded.display_name, + ) elif watch_event == "Modified" and _status_is_current(resource): LOG.info( "Skipping %s Modified event; generation %s is already Synced", @@ -368,10 +410,20 @@ def _split_events( resource.generation, ) else: - changed.append(resource) + changed_events += 1 + changed_identities.add(resource.identity) + changed[resource.identity] = resource + + if changed_events > len(changed_identities): + LOG.info( + "Collapsed %s %s event(s) into %s changed CR(s)", + changed_events, + config.crd_kind, + len(changed_identities), + ) - changed.sort(key=lambda r: str(r.spec.get("name", ""))) - return changed, deleted, saw_event_context + resources = sorted(changed.values(), key=lambda r: str(r.spec.get("name", ""))) + return resources, list(deleted.values()), saw_event_context def hook_inputs(contexts: list[dict[str, Any]], config: HookConfig) -> HookInputs: @@ -452,31 +504,31 @@ def prune( conn: Any, desired_specs: list[dict[str, Any]], *, - authoritative_empty: bool, + deleted_specs: list[dict[str, Any]], + sweep_unseen: bool, ) -> None: """Delete resources whose CR was removed. - *desired_specs* is every credential group's desired specs, not just - those of the credentials *conn* authenticates as. A plugin prunes by its - own ownership marker, which records no credential, and what a connection - lists depends on its token, so a resource one group manages is reachable - from another group's connection. The union is what keeps each group's - prune to the resources no group asked for. - - One consequence: two credentials managing the same resource name keep - each other's resource off the prune list. If they are separate clouds - the resource leaks instead. That is the safer direction, since the - alternative is deleting a resource whose CR still exists. - - *authoritative_empty* is scoped to this credential group, not to - *desired_specs*: it says a CR using *these* credentials was deleted, so - an empty desired set is a real one rather than a snapshot that could not - be read. Because *desired_specs* is the union, it can be non-empty while - this is True; a plugin only needs it to decide whether an empty - *desired_specs* may be acted on. + *deleted_specs* names the CRs this credential group just lost. Delete + what they name; this is the bounded, ordinary case. + + *desired_specs* is what must survive. It is every group's specs, not + just this one's, because a plugin prunes by an ownership marker that + records no credential and what a connection lists depends on its token, + so one group's resource is reachable from another's. Two credentials + sharing a resource name therefore keep it off each other's prune, and it + leaks rather than being deleted while a CR still wants it. + + *sweep_unseen* additionally allows deleting by absence: anything managed + that *desired_specs* does not name, which catches a CR that went away + while the hook was down and left no *deleted_specs* behind. It requires + *desired_specs* to be complete, so the framework clears it when a CR + failed to reconcile -- that CR's resource would otherwise read as + unwanted. An empty *desired_specs* is never sweepable for the same + reason: nothing can be diffed against it. Optional: the default does nothing, which is correct for a plugin whose - resources outlive their CR or that has nothing safe to delete. + resources outlive their CR. """ LOG.debug("%s defines no prune step", type(self).__name__) @@ -604,16 +656,40 @@ def run_sync(plugin: SyncPlugin, inputs: HookInputs) -> int: ) _patch_status(plugin, resource, "Synced", synced_message(noun, notes)) - if failed or unreadable: - # Pruning deletes resources absent from the desired set. A CR that - # failed to reconcile or could not be read at all means the desired set - # could not be established, so deleting anything now risks removing a - # resource that should exist. + # Two separate questions below. What may be pruned depends on how complete + # the desired set is; the exit code depends on whether retrying this run + # could ever help. They do not have the same answer. + if unreadable: + # Nothing may be pruned. The desired set is short by however many CRs + # could not be read, and their resource names are unknown, so a deletion + # naming one of them cannot be told apart from a real removal. + LOG.error("Skipping %s prune; %s could not be read", noun, unreadable) + # Exit 0 unless something retryable also failed. An unreadable CR is + # stored state, not a transient fault, so it is still unreadable on the + # next run: shell-operator re-runs a failing hook every few seconds and + # blocks the rest of its queue until it succeeds, so reporting this as a + # failure would stop the readable CRs from reconciling for as long as + # the malformed CR exists. The error log above is the signal. + return 1 if failed else 0 + + if failed: + # Deleting by absence is off: a CR that failed to reconcile may want a + # resource this run would otherwise read as unwanted. A deletion is + # different, because it names its resources, and the failing CR is still + # in the desired set and so still protected from being one of them. LOG.error( - "Skipping %s prune; %s failed to reconcile and %s could not be read", + "Pruning only %s deletions; %s failed to reconcile so the desired " + "set cannot be swept against", noun, failed, - unreadable, + ) + _run_prune( + plugin, + inputs, + grouped_desired, + grouped_deleted, + connections, + sweep_unseen=False, ) return 1 @@ -631,6 +707,8 @@ def _run_prune( grouped_desired: dict[CredentialKey, list[SyncResource]], grouped_deleted: dict[CredentialKey, list[SyncResource]], connections: dict[CredentialKey, Any], + *, + sweep_unseen: bool = True, ) -> int: noun = plugin.noun prune_failed = False @@ -648,13 +726,15 @@ def _run_prune( for credentials in sorted(inputs.prune_credentials): secret_name, cloud_name = credentials desired = grouped_desired.get(credentials, []) - # An empty desired set is only authoritative when we know a CR was - # deleted; otherwise it may just be a snapshot we could not read, and - # pruning against it would delete everything. - authoritative_empty = credentials in grouped_deleted and not desired - if not desired and not authoritative_empty: + deleted = grouped_deleted.get(credentials, []) + # With nothing deleted, a prune has work to do only if it may sweep, and + # only then against a desired set that exists. Nothing desired and + # nothing deleted means the snapshot does not yet show the CR whose event + # brought us here; sweeping against it would delete resources it simply + # has not listed. + if not deleted and (not desired or not sweep_unseen): LOG.info( - "Skipping %s prune for cloud=%r secret=%r; no desired resources", + "Skipping %s prune for cloud=%r secret=%r; nothing to delete", noun, cloud_name, secret_name, @@ -682,7 +762,8 @@ def _run_prune( plugin.prune( conn, all_desired_specs, - authoritative_empty=authoritative_empty, + deleted_specs=[resource.spec for resource in deleted], + sweep_unseen=sweep_unseen, ) except Exception as exc: # noqa: BLE001 prune_failed = True diff --git a/python/openstack-sync/openstack_sync/hooks/ironic_runbooks.py b/python/openstack-sync/openstack_sync/hooks/ironic_runbooks.py index e9101e74f..70a537677 100644 --- a/python/openstack-sync/openstack_sync/hooks/ironic_runbooks.py +++ b/python/openstack-sync/openstack_sync/hooks/ironic_runbooks.py @@ -40,12 +40,16 @@ def prune( conn: Any, desired_specs: list[dict[str, Any]], *, - authoritative_empty: bool, + deleted_specs: list[dict[str, Any]], + sweep_unseen: bool, ) -> None: if not self.config.prune: return prune_module.prune_removed_runbooks( - conn, desired_specs, authoritative_empty=authoritative_empty + conn, + desired_specs, + deleted_specs=deleted_specs, + sweep_unseen=sweep_unseen, ) diff --git a/python/openstack-sync/openstack_sync/hooks/router_flavors.py b/python/openstack-sync/openstack_sync/hooks/router_flavors.py index 5d5a59e08..2aeb5abfe 100644 --- a/python/openstack-sync/openstack_sync/hooks/router_flavors.py +++ b/python/openstack-sync/openstack_sync/hooks/router_flavors.py @@ -47,7 +47,8 @@ def prune( conn: Any, desired_specs: list[dict[str, Any]], *, - authoritative_empty: bool, + deleted_specs: list[dict[str, Any]], + sweep_unseen: bool, ) -> None: if not self.config.prune: # PRUNE gates flavor deletion, not the orphan sweep. @@ -55,7 +56,10 @@ def prune( return # prune_removed_flavors sweeps orphans itself. prune_module.prune_removed_flavors( - conn, desired_specs, authoritative_empty=authoritative_empty + conn, + desired_specs, + deleted_specs=deleted_specs, + sweep_unseen=sweep_unseen, ) diff --git a/python/openstack-sync/openstack_sync/plugins/ironic/runbooks/prune.py b/python/openstack-sync/openstack_sync/plugins/ironic/runbooks/prune.py index c5b11ff3c..c21c1adbf 100644 --- a/python/openstack-sync/openstack_sync/plugins/ironic/runbooks/prune.py +++ b/python/openstack-sync/openstack_sync/plugins/ironic/runbooks/prune.py @@ -24,34 +24,46 @@ LOG = logging.getLogger(__name__) +def _spec_names(specs: list[dict[str, Any]]) -> set[str]: + return {str(spec["runbookName"]) for spec in specs if spec.get("runbookName")} + + def prune_removed_runbooks( conn: Any, desired_specs: list[dict[str, Any]], *, - authoritative_empty: bool = False, + deleted_specs: list[dict[str, Any]], + sweep_unseen: bool = True, ) -> None: - """Delete operator-owned runbooks absent from *desired_specs*. + """Delete operator-owned runbooks their CR no longer wants. + + *deleted_specs* names the runbooks whose CR just went away. *desired_specs* + is what must survive, and is never deleted from. - An empty *desired_specs* is only acted on when *authoritative_empty* says a - CR really was deleted; otherwise it may be a snapshot we could not read, and - pruning against it would delete every managed runbook. + When *sweep_unseen* holds and there is a desired set to diff, this also + deletes anything managed that the desired set does not name, catching a CR + whose removal was never observed. The caller clears *sweep_unseen* when the + desired set may be incomplete; an empty desired set is treated the same way, + since it must never mean "delete every managed runbook". """ - if not desired_specs and not authoritative_empty: + desired_names = _spec_names(desired_specs) + deleted_names = _spec_names(deleted_specs) - desired_names + sweep = sweep_unseen and bool(desired_names) + + if not sweep and not deleted_names: LOG.warning( - "No desired Ironic runbooks found; skipping prune to avoid deleting " - "all managed runbooks" + "Skipping Ironic runbook prune; nothing was deleted and there is no " + "desired set to sweep against" ) return - desired_names = { - str(spec["runbookName"]) for spec in desired_specs if spec.get("runbookName") - } - LOG.info("Pruning removed Ironic runbooks") for runbook in client.list_runbooks(conn): name = get_value(runbook, "name") if not name or name in desired_names: continue + if not sweep and name not in deleted_names: + continue if not is_managed_runbook(runbook): LOG.info("Keeping Ironic runbook %s; it is not operator-owned", name) continue diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py b/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py index 8989a6cc4..9a1ff0790 100644 --- a/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py +++ b/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py @@ -145,26 +145,39 @@ def prune_orphaned_profiles(conn: Any) -> None: _sweep_orphaned_profiles(conn, {}, _attachment_counts(flavors)) +def _spec_names(specs: list[dict[str, Any]]) -> set[str]: + return {str(spec["name"]) for spec in specs if spec.get("name")} + + def prune_removed_flavors( conn: Any, desired_specs: list[dict[str, Any]], *, - authoritative_empty: bool = False, + deleted_specs: list[dict[str, Any]], + sweep_unseen: bool = True, ) -> None: - """Delete operator-owned router flavors absent from *desired_specs*. + """Delete operator-owned router flavors their CR no longer wants. + + *deleted_specs* names the flavors whose CR just went away. *desired_specs* + is what must survive, and is never deleted from. - An empty *desired_specs* is acted on only when *authoritative_empty* says a - CR really was deleted; otherwise it may be an unreadable snapshot, and - pruning against it would delete every managed flavor. + When *sweep_unseen* holds and there is a desired set to diff, this also + deletes anything managed that the desired set does not name, catching a CR + whose removal was never observed. The caller clears *sweep_unseen* when the + desired set may be incomplete; an empty desired set is treated the same way, + since it must never mean "delete every managed flavor". """ - if not desired_specs and not authoritative_empty: + desired_names = _spec_names(desired_specs) + deleted_names = _spec_names(deleted_specs) - desired_names + sweep = sweep_unseen and bool(desired_names) + + if not sweep and not deleted_names: LOG.warning( - "No desired router flavors found; skipping prune to avoid deleting " - "all managed router flavors" + "Skipping router flavor prune; nothing was deleted and there is no " + "desired set to sweep against" ) return - desired_names = {str(spec["name"]) for spec in desired_specs if spec.get("name")} cache: ProfileCache = {} LOG.info("Pruning removed router flavors") @@ -174,6 +187,8 @@ def prune_removed_flavors( name = get_value(flavor, "name") if not name or name in desired_names: continue + if not sweep and name not in deleted_names: + continue if not is_managed_flavor(flavor): continue _delete_flavor(conn, flavor, cache, counts) diff --git a/python/openstack-sync/tests/test_framework.py b/python/openstack-sync/tests/test_framework.py index 24462b5e5..293b4dd5e 100644 --- a/python/openstack-sync/tests/test_framework.py +++ b/python/openstack-sync/tests/test_framework.py @@ -7,6 +7,7 @@ from __future__ import annotations import json +import logging from pathlib import Path from typing import Any from unittest import mock @@ -72,7 +73,9 @@ def __init__( self.notes_for = notes_for or {} self.prune_raises = prune_raises self.reconciled: list[str] = [] - self.pruned: list[tuple[list[str], bool]] = [] + self.pruned: list[tuple[list[str], list[str]]] = [] + # One entry per prune call, in step with ``pruned``. + self.swept: list[bool] = [] self.waits = 0 self.caches: list[Any] = [] @@ -96,13 +99,18 @@ def prune( conn: Any, desired_specs: list[dict[str, Any]], *, - authoritative_empty: bool, + deleted_specs: list[dict[str, Any]], + sweep_unseen: bool, ) -> None: if self.prune_raises: raise RuntimeError("prune exploded") self.pruned.append( - ([spec["name"] for spec in desired_specs], authoritative_empty) + ( + [spec["name"] for spec in desired_specs], + [spec["name"] for spec in deleted_specs], + ) ) + self.swept.append(sweep_unseen) def _resource( @@ -442,6 +450,162 @@ def test_deleted_event_reconciles_nothing_but_prunes(): assert inputs.prune_credentials == frozenset({("infrasetup", "understack")}) +def _event(watch_event: str, obj: dict, snapshot: list[dict] | None = None) -> dict: + """Build one Event context. + + *snapshot* is the desired set the prune compares against. It defaults to the + event's own object, as a live CR really sends. A Deleted event must say what + is left instead: a snapshot still listing it would make a prune test pass + while the prune does nothing. + """ + assert not ( + watch_event == "Deleted" and snapshot is None + ), "a Deleted event needs its snapshot spelled out" + return { + "binding": BINDING, + "type": "Event", + "watchEvent": watch_event, + "object": obj, + "snapshots": {BINDING: [{"object": obj}] if snapshot is None else snapshot}, + } + + +def test_repeated_events_for_one_cr_reconcile_it_once(): + """A failing run accumulates a backlog; one CR must stay one reconcile.""" + config = make_hook_config() + live = _cr("probe", generation=3) + contexts = [ + _event("Added", _cr("probe", generation=1), [{"object": live}]), + _event("Modified", _cr("probe", generation=2), [{"object": live}]), + _event("Modified", live, [{"object": live}]), + ] + + inputs = hook_inputs(contexts, config) + + assert [r.spec["name"] for r in inputs.resources_to_reconcile] == ["probe"] + # The last event is the current one. + assert inputs.resources_to_reconcile[0].generation == 3 + + +def test_collapsed_batch_still_prunes_against_the_whole_snapshot(): + """A backlog for one CR must not narrow the desired set the prune sees. + + The snapshot names every CR that should exist; pruning against less deletes + a resource whose CR is still there. + """ + config = make_hook_config(prune=True, status_enabled=True) + live = _cr("probe", generation=3) + snapshot = [{"object": live}, {"object": _cr("untouched")}] + contexts = [_event("Added", _cr("probe", generation=1), snapshot)] + contexts += [_event("Modified", live, snapshot) for _ in range(29)] + + inputs = hook_inputs(contexts, config) + plugin = StubPlugin(config) + code, patch_status, _ = _drive(plugin, inputs) + + assert code == 0 + assert plugin.reconciled == ["probe"] + assert patch_status.call_count == 1 + assert plugin.pruned == [(["probe", "untouched"], [])] + + +def test_a_recreate_after_a_delete_in_the_same_batch_is_reconciled(): + """Order decides: a change after a Deleted is a new CR under the same name.""" + config = make_hook_config() + recreated = _cr("probe", generation=1) + contexts = [ + _event("Deleted", _cr("probe", generation=7), []), + _event("Added", recreated, [{"object": recreated}]), + ] + + inputs = hook_inputs(contexts, config) + + assert [r.generation for r in inputs.resources_to_reconcile] == [1] + assert [r.spec["name"] for r in inputs.deleted_resources] == ["probe"] + + +def test_deleted_event_cancels_an_earlier_change_in_the_same_batch(): + config = make_hook_config() + contexts = [ + _event("Added", _cr("probe"), []), + _event("Modified", _cr("probe"), []), + _event("Deleted", _cr("probe"), []), + ] + + inputs = hook_inputs(contexts, config) + + assert inputs.resources_to_reconcile == [] + assert [r.spec["name"] for r in inputs.deleted_resources] == ["probe"] + assert inputs.prune_credentials == frozenset({("infrasetup", "understack")}) + + +def test_repeated_delete_events_prune_once(): + config = make_hook_config() + contexts = [ + _event("Deleted", _cr("probe"), []), + _event("Deleted", _cr("probe"), []), + ] + + inputs = hook_inputs(contexts, config) + + assert [r.spec["name"] for r in inputs.deleted_resources] == ["probe"] + + +def test_distinct_crs_in_one_batch_are_all_reconciled(): + config = make_hook_config() + snapshot = [{"object": _cr("a")}, {"object": _cr("b")}] + contexts = [ + _event("Added", _cr("a"), snapshot), + _event("Modified", _cr("b"), snapshot), + ] + + inputs = hook_inputs(contexts, config) + + assert [r.spec["name"] for r in inputs.resources_to_reconcile] == ["a", "b"] + + +def test_a_deleted_cr_does_not_wedge_the_reconcile(caplog): + """Replays a backlog of events for a CR that was deleted mid-backlog. + + Shell-operator replays the whole backlog on every retry, so a failing run + keeps being handed events for an object that is already gone. None of them + is reconciled and no status is written for it. The prune still reports, + because a cloud it cannot reach is not one it can safely sweep -- that is + the only failure left, where there were once one per queued event. + """ + config = make_hook_config(prune=True, status_enabled=True) + + def gone() -> dict: + return _cr("zz-probe", secret="missing-secret") + + contexts = [_event("Added", gone(), [])] + contexts += [_event("Modified", gone(), []) for _ in range(30)] + contexts += [_event("Deleted", gone(), [])] + + inputs = hook_inputs(contexts, config) + + assert inputs.resources_to_reconcile == [] + assert [r.spec["name"] for r in inputs.deleted_resources] == ["zz-probe"] + + plugin = StubPlugin(config) + with ( + mock.patch.object( + framework, + "get_openstack_connection", + side_effect=RuntimeError('secrets "missing-secret" not found'), + ), + mock.patch.object(framework, "patch_resource_status") as patch_status, + caplog.at_level(logging.ERROR, logger="openstack_sync.hooks.framework"), + ): + code = run_sync(plugin, inputs) + + assert plugin.reconciled == [] + assert patch_status.call_count == 0 + assert code == 1 + assert "Cannot build an OpenStack connection for the widget prune" in caplog.text + assert "failed to reconcile" not in caplog.text + + def test_event_without_watch_event_is_ignored_without_snapshot_reconcile(caplog): config = make_hook_config() contexts = [ @@ -635,8 +799,12 @@ def test_unreadable_cr_event_is_dropped(): assert inputs.unreadable_resources == frozenset({"openstack/legacy"}) -def test_unreadable_delete_event_is_dropped(): - """A CR that cannot be read cannot be used to drive a deletion either.""" +def test_unreadable_delete_event_is_dropped_without_holding_back_the_prune(caplog): + """A CR that cannot be read cannot drive a deletion, but is not missing either. + + A deleted CR was never in the desired set, so counting it as unreadable + would skip the prune on every replay of that batch -- forever. + """ config = make_hook_config() contexts = [ { @@ -648,10 +816,12 @@ def test_unreadable_delete_event_is_dropped(): } ] - inputs = hook_inputs(contexts, config) + with caplog.at_level(logging.ERROR, logger="openstack_sync.hooks.framework"): + inputs = hook_inputs(contexts, config) assert inputs.deleted_resources == [] - assert inputs.unreadable_resources == frozenset({"openstack/legacy"}) + assert inputs.unreadable_resources == frozenset() + assert "Ignoring unreadable Deleted CR openstack/legacy" in caplog.text def test_unreadable_cr_is_reported_once_across_event_and_snapshot(): @@ -704,7 +874,9 @@ def test_unreadable_crs_do_not_stall_a_whole_namespace(): assert plugin.reconciled == ["firmware-bios-r740xd", "firmware-idrac9"] statuses = {call.kwargs["sync_status"] for call in patch_status.call_args_list} assert statuses == {"Synced"} - assert code == 1 + # Reported in the log, not the exit code: these three stay unreadable on + # every retry, and a failing run blocks this hook's queue. + assert code == 0 assert plugin.pruned == [] assert inputs.unreadable_resources == frozenset( { @@ -806,8 +978,12 @@ def test_run_sync_reports_notes_without_failing(): assert "thing drifted" in message -def test_run_sync_marks_failure_and_skips_prune(): - """A failed reconcile means the desired set is unknown, so prune must not run.""" +def test_run_sync_marks_failure_and_does_not_sweep(): + """A failed reconcile leaves the desired set unsafe to delete by absence. + + Nothing was deleted here, so there is no prune to run at all -- but the run + still reports failure so shell-operator retries it. + """ plugin = StubPlugin(make_hook_config(prune=True), fail_for=("b",)) code, patch_status, _ = _drive(plugin, _inputs([_resource("a"), _resource("b")])) @@ -821,6 +997,24 @@ def test_run_sync_marks_failure_and_skips_prune(): assert by_name == {"a": "Synced", "b": "Failed"} +def test_run_sync_still_deletes_a_removed_cr_when_another_failed(): + """A deletion names its resource, so an unrelated failure cannot make it wrong. + + The failing CR stays in the desired set and so stays protected; only the + absence-based sweep is withheld. + """ + plugin = StubPlugin(make_hook_config(prune=True), fail_for=("broken",)) + broken = _resource("broken") + gone = _resource("gone") + inputs = _inputs([broken], desired=[broken], deleted=[gone]) + + code, _, _ = _drive(plugin, inputs) + + assert code == 1 + assert plugin.pruned == [(["broken"], ["gone"])] + assert plugin.swept == [False] + + def test_run_sync_continues_after_one_failure(): plugin = StubPlugin(make_hook_config(), fail_for=("a",)) @@ -871,10 +1065,10 @@ def test_run_sync_prunes_after_successful_reconcile(): code, _, _ = _drive(plugin, _inputs([_resource("a")])) assert code == 0 - assert plugin.pruned == [(["a"], False)] + assert plugin.pruned == [(["a"], [])] -def test_run_sync_prune_is_authoritative_for_deleted_credentials(): +def test_run_sync_passes_the_deleted_specs_for_deleted_credentials(): """A confirmed deletion lets prune act on an empty desired set.""" plugin = StubPlugin(make_hook_config(prune=True)) deleted = _resource("gone") @@ -883,16 +1077,16 @@ def test_run_sync_prune_is_authoritative_for_deleted_credentials(): code, _, _ = _drive(plugin, inputs) assert code == 0 - assert plugin.pruned == [([], True)] + assert plugin.pruned == [([], ["gone"])] def test_run_sync_prunes_against_every_credentials_desired_resources(): """Prune is scoped by ownership marker, not by credentials, so the set is the union. - Here the credential whose only CR was deleted has an authoritative empty - desired set of its own, and can still list what the other credential manages. - It is handed every group's desired names, which is what keeps the resource - the other group still wants from being a prune candidate. + Here the credential whose only CR was deleted has an empty desired set of + its own, and can still list what the other credential manages. It is handed + every group's desired names, which is what keeps the resource the other + group still wants from being a prune candidate. """ plugin = StubPlugin(make_hook_config(prune=True)) keeper = _resource("keeper", secret="infrasetup", cloud="understack") @@ -903,8 +1097,8 @@ def test_run_sync_prunes_against_every_credentials_desired_resources(): assert code == 0 # Sorted by credentials: infrasetup, then infrasetup-system. The second - # group's desired set is empty and authoritative, and it still sees keeper. - assert plugin.pruned == [(["keeper"], False), (["keeper"], True)] + # group's own desired set is empty, and it still sees keeper. + assert plugin.pruned == [(["keeper"], []), (["keeper"], ["gone"])] def test_run_sync_keeps_a_resource_another_credential_wants_off_the_prune_list(): @@ -960,7 +1154,7 @@ def test_run_sync_connects_to_prune_a_deletion_whatever_prune_says(): assert code == 0 assert connect.call_count == 1 assert plugin.waits == 0 - assert plugin.pruned == [([], True)] + assert plugin.pruned == [([], ["gone"])] def test_run_sync_reports_a_prune_whose_connection_cannot_be_built(): @@ -991,11 +1185,47 @@ def test_run_sync_returns_error_when_prune_fails(): def test_run_sync_skips_prune_when_a_cr_was_unreadable(): + """An unreadable CR withholds every prune, deletions included. + + Its resource names cannot be read, so they cannot be subtracted from the + deletions either, and a deletion naming one of them is indistinguishable + from a real removal. + """ + plugin = StubPlugin(make_hook_config(prune=True)) + inputs = _inputs( + [_resource("a")], + deleted=[_resource("gone")], + unreadable=frozenset({"openstack/legacy"}), + ) + + code, _, _ = _drive(plugin, inputs) + + assert plugin.pruned == [] + assert code == 0 + + +def test_run_sync_does_not_fail_the_run_for_an_unreadable_cr_alone(): + """A malformed CR is stored state, so a non-zero exit could only wedge us. + + Shell-operator re-runs a failing hook and blocks the rest of its queue until + it succeeds. An unreadable CR is unreadable on every retry, so reporting it + as a failure would stop the readable CRs from reconciling for good. + """ plugin = StubPlugin(make_hook_config(prune=True)) inputs = _inputs([_resource("a")], unreadable=frozenset({"openstack/legacy"})) code, _, _ = _drive(plugin, inputs) + assert code == 0 + + +def test_run_sync_fails_when_a_reconcile_failed_alongside_an_unreadable_cr(): + """The retryable failure still decides the exit code.""" + plugin = StubPlugin(make_hook_config(prune=True), fail_for=("a",)) + inputs = _inputs([_resource("a")], unreadable=frozenset({"openstack/legacy"})) + + code, _, _ = _drive(plugin, inputs) + assert code == 1 assert plugin.pruned == [] @@ -1008,9 +1238,9 @@ def test_run_sync_reconciles_readable_crs_despite_an_unreadable_one(): code, patch_status, _ = _drive(plugin, inputs) - # Non-zero keeps the problem visible, but the healthy CRs still converge and + # The error log keeps the problem visible; the healthy CRs still converge and # still get their status patched. - assert code == 1 + assert code == 0 assert plugin.reconciled == ["a", "b"] statuses = {call.kwargs["sync_status"] for call in patch_status.call_args_list} assert statuses == {"Synced"} diff --git a/python/openstack-sync/tests/test_hook_common.py b/python/openstack-sync/tests/test_hook_common.py index 1cc1cf96b..ec37229b5 100644 --- a/python/openstack-sync/tests/test_hook_common.py +++ b/python/openstack-sync/tests/test_hook_common.py @@ -339,6 +339,143 @@ def test_patch_resource_status_logs_api_errors(caplog): assert "Forbidden" in caplog.text +# Bodies below are what a real API server sends, read off a live cluster. The +# body is the whole basis for telling a deleted CR from a misconfigured chart. + +GROUP = API_VERSION.split("/")[0] +PLURAL = RESOURCE.split(".")[0] + +#: A plural, group or version the API server does not serve. Not JSON. +MISSING_RESOURCE_BODY = "404 page not found" + + +def _status_body(name: str, *, code: int, reason: str, message: str) -> str: + """Build the Status the API server returns for a named object.""" + return json.dumps( + { + "kind": "Status", + "apiVersion": "v1", + "metadata": {}, + "status": "Failure", + "message": message, + "reason": reason, + "details": {"name": name, "group": GROUP, "kind": PLURAL}, + "code": code, + } + ) + + +def _missing_object(name: str) -> ApiException: + exc = ApiException(status=404, reason="Not Found") + exc.body = _status_body( + name, code=404, reason="NotFound", message=f'{RESOURCE} "{name}" not found' + ) + return exc + + +def _forbidden(name: str) -> ApiException: + """A 403 names the object in ``details.name`` exactly as a 404 does.""" + exc = ApiException(status=403, reason="Forbidden") + exc.body = _status_body( + name, + code=403, + reason="Forbidden", + message=f'{RESOURCE} "{name}" is forbidden: User "sa" cannot patch', + ) + return exc + + +def _not_found(body: str) -> ApiException: + exc = ApiException(status=404, reason="Not Found") + exc.body = body + return exc + + +def _patch_status_with(exc: ApiException, name: str = "deleted-flavor") -> None: + api = mock.MagicMock() + api.patch_namespaced_custom_object_status.side_effect = exc + with mock.patch.object(hc, "_customobjects_api", return_value=api): + hc.patch_resource_status( + name=name, + namespace="openstack", + generation=None, + sync_status="Failed", + message="OpenStack connection failed", + crd_api_version=API_VERSION, + crd_resource=RESOURCE, + crd_kind="NeutronRouterFlavor", + status_enabled=True, + ) + + +def test_patch_resource_status_does_not_error_when_the_cr_is_gone(caplog): + """A CR deleted mid-reconcile is a race, not a fault worth an error line.""" + with caplog.at_level(logging.INFO, logger="openstack_sync.hooks.common"): + _patch_status_with(_missing_object("deleted-flavor")) + + assert "the CR is gone" in caplog.text + assert "404" in caplog.text + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +def test_patch_resource_status_warns_when_the_crd_is_the_thing_missing(caplog): + """A 404 for the resource, not the object, is a chart misconfiguration. + + It leaves every CR without a status and nothing else reports it, since the + patch never fails a reconcile. + """ + with caplog.at_level(logging.INFO, logger="openstack_sync.hooks.common"): + _patch_status_with(_not_found(MISSING_RESOURCE_BODY)) + + assert "failed to patch" in caplog.text + assert [r for r in caplog.records if r.levelno >= logging.WARNING] + + +def test_patch_resource_status_warns_when_denied_for_the_very_same_object(caplog): + """A 403 names the object just as a 404 does, and is not a deleted CR. + + Matching on the body alone would read an RBAC gap as a race. + """ + with caplog.at_level(logging.INFO, logger="openstack_sync.hooks.common"): + _patch_status_with(_forbidden("deleted-flavor")) + + assert "failed to patch" in caplog.text + assert "403" in caplog.text + + +def test_patch_resource_status_warns_when_a_404_names_a_different_object(caplog): + """The name has to match; a 404 about something else is not this CR's race.""" + with caplog.at_level(logging.INFO, logger="openstack_sync.hooks.common"): + _patch_status_with(_missing_object("other-flavor")) + + assert "failed to patch" in caplog.text + assert [r for r in caplog.records if r.levelno >= logging.WARNING] + + +@pytest.mark.parametrize( + ("body", "shape"), + [ + ( + json.dumps({"kind": "Status", "reason": "NotFound", "code": 404}), + "no details", + ), + (json.dumps({"kind": "Status", "details": None, "code": 404}), "null details"), + (json.dumps(["not", "a", "status"]), "not an object"), + ], +) +def test_patch_resource_status_warns_when_a_404_body_names_nothing(body, shape, caplog): + """A body naming no object cannot clear a CR as gone, and must not raise. + + This runs inside the ApiException handler, where a raise escapes the handler + below it and fails the reconcile. + """ + with caplog.at_level(logging.INFO, logger="openstack_sync.hooks.common"): + _patch_status_with(_not_found(body)) + + assert "failed to patch" in caplog.text, shape + assert [r for r in caplog.records if r.levelno >= logging.WARNING] + + def test_patch_resource_status_logs_unexpected_errors(caplog): with mock.patch.object( hc, "_customobjects_api", side_effect=RuntimeError("no kubeconfig") diff --git a/python/openstack-sync/tests/test_ironic_runbooks_hook.py b/python/openstack-sync/tests/test_ironic_runbooks_hook.py index 51652df36..615b1c925 100644 --- a/python/openstack-sync/tests/test_ironic_runbooks_hook.py +++ b/python/openstack-sync/tests/test_ironic_runbooks_hook.py @@ -208,14 +208,33 @@ def test_plugin_prunes_only_when_the_chart_enabled_it(): with mock.patch.object(hook.prune_module, "prune_removed_runbooks") as do_prune: hook.IronicRunbookPlugin(make_ironic_config(prune=False)).prune( - conn, specs, authoritative_empty=False + conn, specs, deleted_specs=[], sweep_unseen=True ) do_prune.assert_not_called() + deleted = [{"runbookName": "CUSTOM_GONE", "steps": []}] hook.IronicRunbookPlugin(make_ironic_config(prune=True)).prune( - conn, specs, authoritative_empty=True + conn, specs, deleted_specs=deleted, sweep_unseen=True ) - do_prune.assert_called_once_with(conn, specs, authoritative_empty=True) + do_prune.assert_called_once_with( + conn, specs, deleted_specs=deleted, sweep_unseen=True + ) + + +def test_plugin_prune_forwards_a_withheld_sweep(): + """The framework's decision not to delete by absence must reach the module.""" + conn = mock.MagicMock() + specs = [{"runbookName": "CUSTOM_KEEP", "steps": []}] + deleted = [{"runbookName": "CUSTOM_GONE", "steps": []}] + + with mock.patch.object(hook.prune_module, "prune_removed_runbooks") as do_prune: + hook.IronicRunbookPlugin(make_ironic_config(prune=True)).prune( + conn, specs, deleted_specs=deleted, sweep_unseen=False + ) + + do_prune.assert_called_once_with( + conn, specs, deleted_specs=deleted, sweep_unseen=False + ) def test_main_returns_zero_when_hook_disabled( diff --git a/python/openstack-sync/tests/test_ironic_runbooks_prune.py b/python/openstack-sync/tests/test_ironic_runbooks_prune.py index 340a0f665..a0d54ff38 100644 --- a/python/openstack-sync/tests/test_ironic_runbooks_prune.py +++ b/python/openstack-sync/tests/test_ironic_runbooks_prune.py @@ -34,8 +34,19 @@ def _spec(name: str) -> dict[str, Any]: return {"runbookName": name, "steps": []} -def _prune(fake: FakeBaremetal, specs: list[dict[str, Any]], **kwargs: Any) -> None: - prune.prune_removed_runbooks(_conn(fake), specs, **kwargs) +def _prune( + fake: FakeBaremetal, + specs: list[dict[str, Any]], + deleted: list[dict[str, Any]] | None = None, + *, + sweep_unseen: bool = True, +) -> None: + prune.prune_removed_runbooks( + _conn(fake), + specs, + deleted_specs=deleted if deleted is not None else [], + sweep_unseen=sweep_unseen, + ) def test_owned_runbook_absent_from_the_desired_set_is_deleted(): @@ -86,16 +97,82 @@ def test_runbook_without_a_name_is_skipped(): assert fake.calls_for("DELETE") == [] -def test_empty_desired_set_is_refused_unless_a_cr_was_deleted(): +def test_empty_desired_set_deletes_nothing_on_its_own(): """An unreadable snapshot must not read as "delete everything".""" fake = FakeBaremetal([_owned("CUSTOM_GONE")]) _prune(fake, []) + assert sorted(fake.runbooks) == ["CUSTOM_GONE"] assert fake.calls == [] - _prune(fake, [], authoritative_empty=True) - assert fake.runbooks == {} + +def test_empty_desired_set_deletes_only_the_named_deleted_runbooks(): + """Without a desired set to diff, only the lost CRs may be acted on.""" + fake = FakeBaremetal([_owned("CUSTOM_GONE"), _owned("CUSTOM_UNRELATED")]) + + _prune(fake, [], [_spec("CUSTOM_GONE")]) + + assert sorted(fake.runbooks) == ["CUSTOM_UNRELATED"] + assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE-uuid"] + + +def test_a_deleted_runbook_another_cr_still_wants_is_kept(): + """The desired set wins over a deletion naming the same runbook.""" + fake = FakeBaremetal([_owned("CUSTOM_SHARED")]) + + _prune(fake, [_spec("CUSTOM_SHARED")], [_spec("CUSTOM_SHARED")]) + + assert sorted(fake.runbooks) == ["CUSTOM_SHARED"] + assert fake.calls_for("DELETE") == [] + + +# --------------------------------------------------------------------------- +# A withheld sweep +# --------------------------------------------------------------------------- + + +def test_without_the_sweep_only_the_deleted_runbooks_go(): + """With sweeping off, a desired set is a protection list and nothing more. + + The framework withholds the sweep when a CR failed to reconcile, so the + desired set may be missing names. An owned runbook absent from it is left + alone; only the runbooks a deletion names go. + """ + fake = FakeBaremetal([_owned("CUSTOM_GONE"), _owned("CUSTOM_ABSENT")]) + + _prune(fake, [_spec("CUSTOM_KEEP")], [_spec("CUSTOM_GONE")], sweep_unseen=False) + + assert sorted(fake.runbooks) == ["CUSTOM_ABSENT"] + assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE-uuid"] + + +def test_without_the_sweep_the_desired_set_still_protects(): + """A deletion cannot remove a runbook the desired set still names. + + This is what makes deleting by name safe while a reconcile is failing: the + failing CR is still in the desired set, so it cannot be deleted. + """ + fake = FakeBaremetal([_owned("CUSTOM_SHARED")]) + + _prune( + fake, + [_spec("CUSTOM_SHARED")], + [_spec("CUSTOM_SHARED")], + sweep_unseen=False, + ) + + assert sorted(fake.runbooks) == ["CUSTOM_SHARED"] + assert fake.calls_for("DELETE") == [] + + +def test_without_the_sweep_and_nothing_deleted_nothing_happens(): + fake = FakeBaremetal([_owned("CUSTOM_GONE")]) + + _prune(fake, [_spec("CUSTOM_KEEP")], [], sweep_unseen=False) + + assert sorted(fake.runbooks) == ["CUSTOM_GONE"] + assert fake.calls == [] def test_a_runbook_deleted_out_of_band_is_not_an_error(): diff --git a/python/openstack-sync/tests/test_prune.py b/python/openstack-sync/tests/test_prune.py index 9b6508d31..cf3917495 100644 --- a/python/openstack-sync/tests/test_prune.py +++ b/python/openstack-sync/tests/test_prune.py @@ -87,6 +87,21 @@ def _conn(flavors: list[dict[str, Any]], profiles: dict[str, Any] | None = None) return SimpleNamespace(network=FakeNetwork(flavors, profiles or {})) +def _prune( + conn: Any, + desired: list[dict[str, Any]], + deleted: list[dict[str, Any]] | None = None, + *, + sweep_unseen: bool = True, +) -> None: + prune.prune_removed_flavors( + conn, + desired, + deleted_specs=deleted if deleted is not None else [], + sweep_unseen=sweep_unseen, + ) + + # --------------------------------------------------------------------------- # Ownership gates deletion # --------------------------------------------------------------------------- @@ -103,7 +118,7 @@ def test_prune_keeps_unowned_flavor_even_with_owned_profile(): profile = _owned_profile("owned-profile-id") conn = _conn([flavor], {profile.id: profile}) - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_flavors == [] @@ -111,7 +126,7 @@ def test_prune_keeps_unowned_flavor_even_with_owned_profile(): def test_prune_deletes_removed_owned_flavor(): conn = _conn([_owned_flavor("managed-flavor-id", "removed-managed-flavor")]) - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_flavors == ["managed-flavor-id"] @@ -121,7 +136,7 @@ def test_prune_deletes_removed_flavor_and_its_unused_profile(): flavor = _owned_flavor("managed-flavor-id", "removed-managed-flavor", [profile.id]) conn = _conn([flavor], {profile.id: profile}) - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_flavors == ["managed-flavor-id"] assert conn.network.deleted_profiles == ["managed-profile-id"] @@ -136,20 +151,90 @@ def test_prune_keeps_owned_flavors_when_desired_list_is_empty(): """An empty desired set may be an unreadable snapshot, not a deletion.""" conn = _conn([_owned_flavor("managed-flavor-id", "removed-managed-flavor")]) - prune.prune_removed_flavors(conn, []) + _prune(conn, []) assert conn.network.deleted_flavors == [] -def test_prune_deletes_when_empty_desired_is_authoritative(): - """A confirmed CR deletion makes the empty desired set actionable.""" - conn = _conn([_owned_flavor("managed-flavor-id", "removed-managed-flavor")]) +def test_prune_deletes_only_the_named_deleted_flavors_without_a_desired_set(): + """Without a desired set to diff, only the lost CRs may be acted on.""" + conn = _conn( + [ + _owned_flavor("managed-flavor-id", "removed-managed-flavor"), + _owned_flavor("unrelated-flavor-id", "unrelated-flavor"), + ] + ) - prune.prune_removed_flavors(conn, [], authoritative_empty=True) + _prune(conn, [], [{"name": "removed-managed-flavor"}]) assert conn.network.deleted_flavors == ["managed-flavor-id"] +def test_prune_keeps_a_deleted_flavor_another_cr_still_wants(): + """The desired set wins over a deletion naming the same flavor.""" + conn = _conn([_owned_flavor("shared-flavor-id", "shared-flavor")]) + + _prune(conn, [{"name": "shared-flavor"}], [{"name": "shared-flavor"}]) + + assert conn.network.deleted_flavors == [] + + +# --------------------------------------------------------------------------- +# A withheld sweep +# --------------------------------------------------------------------------- + + +def test_prune_without_the_sweep_deletes_only_what_was_deleted(): + """With sweeping off, a desired set is a protection list and nothing more. + + The framework withholds the sweep when a CR failed to reconcile, so the + desired set may be missing names. An owned flavor absent from it is left + alone; only the flavors a deletion names go. + """ + conn = _conn( + [ + _owned_flavor("gone-flavor-id", "gone-flavor"), + _owned_flavor("unnamed-flavor-id", "absent-from-desired"), + ] + ) + + _prune( + conn, + [{"name": "kept-flavor"}], + [{"name": "gone-flavor"}], + sweep_unseen=False, + ) + + assert conn.network.deleted_flavors == ["gone-flavor-id"] + + +def test_prune_without_the_sweep_still_protects_the_desired_set(): + """A deletion cannot remove a flavor the desired set still names. + + This is what makes deleting by name safe while a reconcile is failing: the + failing CR is still in the desired set, so it cannot be deleted. + """ + conn = _conn([_owned_flavor("shared-flavor-id", "shared-flavor")]) + + _prune( + conn, + [{"name": "shared-flavor"}], + [{"name": "shared-flavor"}], + sweep_unseen=False, + ) + + assert conn.network.deleted_flavors == [] + + +def test_prune_without_the_sweep_and_nothing_deleted_does_nothing(): + conn = _conn([_owned_flavor("managed-flavor-id", "removed-managed-flavor")]) + + _prune(conn, [{"name": "kept-flavor"}], [], sweep_unseen=False) + + assert conn.network.deleted_flavors == [] + assert conn.network.deleted_profiles == [] + + # --------------------------------------------------------------------------- # Orphaned profile sweep # --------------------------------------------------------------------------- @@ -160,7 +245,7 @@ def test_prune_deletes_orphaned_owned_profile(): orphan = _owned_profile("orphan-profile-id") conn = _conn([], {orphan.id: orphan}) - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_profiles == ["orphan-profile-id"] @@ -174,7 +259,7 @@ def test_prune_keeps_unowned_profile(): ) conn = _conn([], {unowned.id: unowned}) - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_profiles == [] @@ -185,7 +270,7 @@ def test_prune_keeps_attached_profile(): kept = _owned_flavor("kept-flavor-id", "kept-flavor", [attached.id]) conn = _conn([kept], {attached.id: attached}) - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_flavors == [] assert conn.network.deleted_profiles == [] @@ -208,7 +293,7 @@ def test_prune_lists_flavors_once_for_all_profile_checks(): }, ) - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.flavor_list_calls == 1 # Only the deleted flavor's profile needs a GET; the orphan came from the @@ -227,7 +312,7 @@ def test_prune_skips_flavor_still_used_by_routers(): conn = _conn([flavor]) conn.network.routers = lambda flavor_id: [{"id": "router-1"}] - prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}]) + _prune(conn, [{"name": "kept-flavor"}]) assert conn.network.deleted_flavors == [] diff --git a/python/openstack-sync/tests/test_router_flavors_hook.py b/python/openstack-sync/tests/test_router_flavors_hook.py index 1050307a8..f5d722b3e 100644 --- a/python/openstack-sync/tests/test_router_flavors_hook.py +++ b/python/openstack-sync/tests/test_router_flavors_hook.py @@ -8,6 +8,7 @@ import importlib import json +import logging import types from pathlib import Path from typing import Any @@ -160,7 +161,9 @@ def test_plugin_prune_deletes_no_flavor_when_disabled(): plugin = hook.RouterFlavorPlugin(make_hook_config(prune=False)) with mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune: - plugin.prune(mock.MagicMock(), [{"name": "a"}], authoritative_empty=False) + plugin.prune( + mock.MagicMock(), [{"name": "a"}], deleted_specs=[], sweep_unseen=True + ) prune.assert_not_called() @@ -171,27 +174,58 @@ def test_plugin_prune_sweeps_orphaned_profiles_when_disabled(): conn = mock.MagicMock() with mock.patch.object(hook.prune_module, "prune_orphaned_profiles") as sweep: - plugin.prune(conn, [{"name": "a"}], authoritative_empty=False) + plugin.prune(conn, [{"name": "a"}], deleted_specs=[], sweep_unseen=True) + + sweep.assert_called_once_with(conn) + + +def test_plugin_prune_ignores_a_deletion_when_disabled(): + """PRUNE off means a lost CR's flavor is left in place, not deleted by name.""" + plugin = hook.RouterFlavorPlugin(make_hook_config(prune=False)) + conn = mock.MagicMock() + + with ( + mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune, + mock.patch.object(hook.prune_module, "prune_orphaned_profiles") as sweep, + ): + plugin.prune(conn, [], deleted_specs=[{"name": "gone"}], sweep_unseen=True) + prune.assert_not_called() sweep.assert_called_once_with(conn) -def test_plugin_prune_forwards_authoritative_empty_when_enabled(): +def test_plugin_prune_forwards_deleted_specs_when_enabled(): plugin = hook.RouterFlavorPlugin(make_hook_config(prune=True)) conn = mock.MagicMock() specs = [{"name": "a"}] + deleted = [{"name": "gone"}] with ( mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune, mock.patch.object(hook.prune_module, "prune_orphaned_profiles") as sweep, ): - plugin.prune(conn, specs, authoritative_empty=True) + plugin.prune(conn, specs, deleted_specs=deleted, sweep_unseen=True) - prune.assert_called_once_with(conn, specs, authoritative_empty=True) + prune.assert_called_once_with(conn, specs, deleted_specs=deleted, sweep_unseen=True) # prune_removed_flavors sweeps internally; a second call would re-list. sweep.assert_not_called() +def test_plugin_prune_forwards_a_withheld_sweep(): + """The framework's decision not to delete by absence must reach the module.""" + plugin = hook.RouterFlavorPlugin(make_hook_config(prune=True)) + conn = mock.MagicMock() + specs = [{"name": "a"}] + deleted = [{"name": "gone"}] + + with mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune: + plugin.prune(conn, specs, deleted_specs=deleted, sweep_unseen=False) + + prune.assert_called_once_with( + conn, specs, deleted_specs=deleted, sweep_unseen=False + ) + + def test_plugin_cache_is_per_credential_group(): plugin = hook.RouterFlavorPlugin(make_hook_config()) @@ -391,14 +425,19 @@ def test_main_sweeps_an_orphaned_profile_when_the_last_cr_is_deleted( conn.network.delete_flavor.assert_not_called() -def test_main_fails_loudly_on_a_cr_missing_cloud_credentials(monkeypatch, tmp_path): - """A CR without credentials must fail the run, not be skipped. +def test_main_reports_a_cr_missing_cloud_credentials_without_failing( + monkeypatch, tmp_path, caplog +): + """A CR without credentials is reported and skipped, and prunes nothing. The CRD marks cloudCredentialsRef required, so the API server should reject - it first; this guards the case where something bypasses that. + it first; this guards the case where something bypasses that. The run does + not fail, because the object is stored that way and would be unreadable on + every retry, and a failing hook blocks its own queue in shell-operator. """ clear_env(monkeypatch) monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + monkeypatch.setenv(f"{ENV_PREFIX}_PRUNE", "true") obj = router_flavor_object("pa1410") del obj["spec"]["cloudCredentialsRef"] contexts = [ @@ -409,9 +448,16 @@ def test_main_fails_loudly_on_a_cr_missing_cloud_credentials(monkeypatch, tmp_pa } ] - code, _ = _run_main(monkeypatch, tmp_path, contexts, _neutron_conn()) + with ( + caplog.at_level(logging.ERROR, logger="openstack_sync.hooks.framework"), + mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune, + ): + code, patch_status = _run_main(monkeypatch, tmp_path, contexts, _neutron_conn()) - assert code == 1 + assert code == 0 + prune.assert_not_called() + patch_status.assert_not_called() + assert "openstack/pa1410" in caplog.text def test_main_uses_the_credentials_named_by_each_cr(monkeypatch, tmp_path):