Skip to content

Commit 0c842b3

Browse files
authored
Merge pull request #2332 from rackerlabs/fix-router-flavor-orphaned-profiles-without-prune
fix(openstack-neutron-plugin): common case when orphaned profiles are never cleaned up when prune is disabled
2 parents 483761c + c9b91da commit 0c842b3

5 files changed

Lines changed: 166 additions & 6 deletions

File tree

‎python/openstack-sync/openstack_sync/hooks/framework.py‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,15 @@ def prune(
480480
"""
481481
LOG.debug("%s defines no prune step", type(self).__name__)
482482

483+
def needs_prune_connection(self) -> bool:
484+
"""Return whether prune can do useful work and needs a connection.
485+
486+
Most plugins only prune when their chart prune flag is enabled. Plugins
487+
with narrower cleanup that is safe without destructive pruning can
488+
override this so deletion-only runs still get a connection.
489+
"""
490+
return self.config.prune
491+
483492

484493
# ---------------------------------------------------------------------------
485494
# Status
@@ -663,7 +672,7 @@ def _run_prune(
663672

664673
conn = connections.get(credentials)
665674
if conn is None:
666-
if not plugin.config.prune:
675+
if not plugin.needs_prune_connection():
667676
continue
668677
try:
669678
conn = get_openstack_connection(secret_name, cloud_name)

‎python/openstack-sync/openstack_sync/hooks/router_flavors.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,15 @@ def prune(
5151
authoritative_empty: bool,
5252
) -> None:
5353
if not self.config.prune:
54+
prune_module.prune_orphaned_profiles(conn)
5455
return
5556
prune_module.prune_removed_flavors(
5657
conn, desired_specs, authoritative_empty=authoritative_empty
5758
)
5859

60+
def needs_prune_connection(self) -> bool:
61+
return True
62+
5963

6064
def main() -> int:
6165
def run(contexts: list[dict[str, Any]]) -> int:

‎python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/prune.py‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,17 @@ def _prune_orphaned_profiles(
129129
"""
130130
LOG.info("Scanning for orphaned operator-owned service profiles")
131131
for profile in list(conn.network.service_profiles()):
132-
if is_managed_service_profile(profile):
133-
maybe_delete_profile(conn, resource_id(profile), cache, counts)
132+
if not is_managed_service_profile(profile):
133+
continue
134+
profile_id = resource_id(profile)
135+
cache.setdefault(profile_id, profile)
136+
maybe_delete_profile(conn, profile_id, cache, counts)
137+
138+
139+
def prune_orphaned_profiles(conn: Any) -> None:
140+
"""Delete owned, unattached service profiles without deleting any flavor."""
141+
current = list(conn.network.flavors(service_type=SERVICE_TYPE))
142+
_prune_orphaned_profiles(conn, {}, _attachment_counts(current))
134143

135144

136145
def prune_removed_flavors(

‎python/openstack-sync/tests/test_prune.py‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ def __init__(self, flavors: list[dict[str, Any]], profiles: dict[str, Any]):
2929
self.deleted_flavors: list[str] = []
3030
self.deleted_profiles: list[str] = []
3131
self.flavor_list_calls = 0
32+
self.profile_get_calls = 0
3233

3334
def flavors(self, service_type: str | None = None) -> list[dict[str, Any]]:
3435
self.flavor_list_calls += 1
@@ -45,6 +46,7 @@ def service_profiles(self) -> list[Any]:
4546
return [p for p in self._profiles.values() if p is not None]
4647

4748
def get_service_profile(self, profile_id: str) -> Any:
49+
self.profile_get_calls += 1
4850
profile = self._profiles.get(profile_id)
4951
if profile is None:
5052
raise openstack_exceptions.NotFoundException(f"no profile {profile_id}")
@@ -226,3 +228,66 @@ def test_prune_skips_flavor_still_used_by_routers():
226228
prune.prune_removed_flavors(conn, [{"name": "kept-flavor"}])
227229

228230
assert conn.network.deleted_flavors == []
231+
232+
233+
# ---------------------------------------------------------------------------
234+
# The orphaned profile sweep on its own, with PRUNE off
235+
# ---------------------------------------------------------------------------
236+
237+
238+
def test_prune_orphaned_profiles_deletes_an_owned_unattached_profile():
239+
orphan = _owned_profile("orphan-profile-id")
240+
conn = _conn([], {orphan.id: orphan})
241+
242+
prune.prune_orphaned_profiles(conn)
243+
244+
assert conn.network.deleted_profiles == ["orphan-profile-id"]
245+
246+
247+
def test_prune_orphaned_profiles_keeps_an_unowned_profile():
248+
unowned = SimpleNamespace(
249+
id="unmanaged-profile-id",
250+
driver=_DRIVER,
251+
meta_info={"vni_alloc": "auto"}, # no ownership marker
252+
)
253+
conn = _conn([], {unowned.id: unowned})
254+
255+
prune.prune_orphaned_profiles(conn)
256+
257+
assert conn.network.deleted_profiles == []
258+
259+
260+
def test_prune_orphaned_profiles_keeps_an_attached_profile():
261+
"""The sweep needs its own attachment counts, or it deletes a bound profile."""
262+
attached = _owned_profile("attached-profile-id")
263+
kept = _owned_flavor("kept-flavor-id", "kept-flavor", [attached.id])
264+
conn = _conn([kept], {attached.id: attached})
265+
266+
prune.prune_orphaned_profiles(conn)
267+
268+
assert conn.network.deleted_profiles == []
269+
270+
271+
def test_prune_orphaned_profiles_reuses_the_listing_instead_of_refetching():
272+
"""The sweep already holds the profile, so it must not GET it again."""
273+
orphan = _owned_profile("orphan-profile-id")
274+
conn = _conn([], {orphan.id: orphan})
275+
276+
prune.prune_orphaned_profiles(conn)
277+
278+
assert conn.network.profile_get_calls == 0
279+
assert conn.network.deleted_profiles == ["orphan-profile-id"]
280+
281+
282+
def test_prune_orphaned_profiles_deletes_no_flavor():
283+
"""The sweep must not delete a flavor; that still needs PRUNE."""
284+
orphan = _owned_profile("orphan-profile-id")
285+
conn = _conn(
286+
[_owned_flavor("removed-flavor-id", "removed-flavor")],
287+
{orphan.id: orphan},
288+
)
289+
290+
prune.prune_orphaned_profiles(conn)
291+
292+
assert conn.network.deleted_flavors == []
293+
assert conn.network.deleted_profiles == ["orphan-profile-id"]

‎python/openstack-sync/tests/test_router_flavors_hook.py‎

Lines changed: 76 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717

1818
import openstack_sync.utils as utils
1919
from openstack_sync.hooks import router_flavors as hook
20+
from openstack_sync.hooks.framework import HookInputs
21+
from openstack_sync.hooks.framework import SyncResource
2022
from openstack_sync.plugins.neutron.router_flavors import markers
2123
from openstack_sync.plugins.neutron.router_flavors.config import BINDING_NAME
2224
from openstack_sync.plugins.neutron.router_flavors.config import ENV_PREFIX
@@ -156,13 +158,29 @@ def test_plugin_wait_for_api_uses_configured_retry_budget():
156158
wait.assert_called_once_with(conn, retries=5, delay=0.25)
157159

158160

159-
def test_plugin_prune_is_a_noop_when_disabled():
161+
def test_plugin_prune_deletes_no_flavor_when_disabled():
160162
plugin = hook.RouterFlavorPlugin(make_hook_config(prune=False))
163+
conn = mock.MagicMock()
161164

162-
with mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune:
163-
plugin.prune(mock.MagicMock(), [{"name": "a"}], authoritative_empty=False)
165+
with (
166+
mock.patch.object(hook.prune_module, "prune_removed_flavors") as prune,
167+
mock.patch.object(hook.prune_module, "prune_orphaned_profiles") as sweep,
168+
):
169+
plugin.prune(conn, [{"name": "a"}], authoritative_empty=False)
164170

165171
prune.assert_not_called()
172+
sweep.assert_called_once_with(conn)
173+
174+
175+
def test_plugin_prune_sweeps_orphaned_profiles_when_disabled():
176+
"""PRUNE gates flavor deletion; an unattached owned profile is collected anyway."""
177+
plugin = hook.RouterFlavorPlugin(make_hook_config(prune=False))
178+
conn = mock.MagicMock()
179+
180+
with mock.patch.object(hook.prune_module, "prune_orphaned_profiles") as sweep:
181+
plugin.prune(conn, [{"name": "a"}], authoritative_empty=False)
182+
183+
sweep.assert_called_once_with(conn)
166184

167185

168186
def test_plugin_prune_forwards_authoritative_empty_when_enabled():
@@ -183,6 +201,61 @@ def test_plugin_cache_is_per_credential_group():
183201
assert plugin.new_cache() is not plugin.new_cache()
184202

185203

204+
# ---------------------------------------------------------------------------
205+
# Integration through run_sync()
206+
# ---------------------------------------------------------------------------
207+
208+
209+
def test_run_sync_sweeps_orphaned_profile_on_deletion_when_prune_disabled():
210+
"""Deletion-only input must still open a connection for safe orphan cleanup."""
211+
plugin = hook.RouterFlavorPlugin(make_hook_config(prune=False))
212+
deleted = SyncResource(
213+
spec={"name": "pa1410"},
214+
name="pa1410",
215+
namespace="openstack",
216+
generation=3,
217+
secret_name="infrasetup",
218+
cloud_name="understack",
219+
)
220+
conn = _neutron_conn()
221+
orphan = types.SimpleNamespace(
222+
id="orphan-profile-id",
223+
driver="neutron_understack.l3_router.vrf.Vrf",
224+
is_enabled=True,
225+
description="pa1410 profile",
226+
meta_info=markers.managed_meta_info({"vni_alloc": "auto"}),
227+
)
228+
conn.network.service_profiles.return_value = [
229+
conn.network.service_profiles.return_value[0],
230+
orphan,
231+
]
232+
inputs = HookInputs(
233+
resources_to_reconcile=[],
234+
desired_resources_for_prune=[],
235+
deleted_resources=[deleted],
236+
prune_credentials=frozenset({deleted.credentials}),
237+
unreadable_resources=frozenset(),
238+
)
239+
240+
with (
241+
mock.patch(
242+
"openstack_sync.hooks.framework.get_openstack_connection",
243+
return_value=conn,
244+
) as connect,
245+
mock.patch("openstack_sync.hooks.framework.patch_resource_status"),
246+
mock.patch.object(hook, "wait_for_openstack_network") as wait,
247+
):
248+
code = hook.run_sync(plugin, inputs)
249+
250+
assert code == 0
251+
connect.assert_called_once_with("infrasetup", "understack")
252+
wait.assert_called_once_with(conn, retries=30, delay=10.0)
253+
conn.network.delete_flavor.assert_not_called()
254+
assert [
255+
call.args[0] for call in conn.network.delete_service_profile.call_args_list
256+
] == [orphan]
257+
258+
186259
# ---------------------------------------------------------------------------
187260
# End to end through main()
188261
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)