feat(openstack-sync): full CRUD plugin for Neutron router flavors

[haseebsyed12]

Implements full CRUD for Neutron router flavors in the openstack-sync operator.
Extracts shared hook/plugin logic into common modules, fixes a status patch feedback loop

Module structure

openstack_sync/
  hooks/
    common.py               # new: shared hook base logic
    router_flavors.py       # updated: wires CRUD ops, fixes feedback loop
    placeholder.py          # updated
  plugins/
    common.py               # new: shared plugin base logic
    neutron/
      router_flavors/
        create.py               # new
        update.py               # new
        delete.py               # new
        router_flavors_common.py  # new: flavor-specific shared utils

Data flow

CRD (NeutronRouterFlavor)
        |
        v
hooks/router_flavors.py   (reconcile entry point)
        |
        +--[create]--> plugins/neutron/router_flavors/create.py
        +--[update]--> plugins/neutron/router_flavors/update.py
        +--[delete]--> plugins/neutron/router_flavors/delete.py
                                |
                       router_flavors_common.py
                       plugins/common.py
                       hooks/common.py

Reconciliation Engine and Neutron Router Flavor Process

+------------------------------------------------------------------------+
|                    SHELL-OPERATOR (Reconciliation Engine)              |
|                                                                        |
|   K8s watch: NeutronRouterFlavor  --+                                 |
|   (Added/Modified/Deleted)          |                                 |
|                                      +--> queue: "neutron-router-      |
|   Cron tick (SYNC_CRONTAB)      ----+       flavors" --> HookRun      |
|   e.g. "0 * * * *"                           |                        |
|                                               |   (dedicated queue,    |
|                                               |    NOT "main" -- see   |
|                                               |    section 9)          |
|                                               v                        |
|                              writes BINDING_CONTEXT_PATH (JSON)       |
|                              [Synchronization | Event | Schedule]     |
+---------------------------------+--------------------------------------+
                                  |  exec
                                  v
+------------------------------------------------------------------------+
|                     router_flavors.py  (the hook)                     |
|                                                                        |
|  read BINDING_CONTEXT_PATH                                            |
|    +--> router_flavor_resources_from_binding_context()                |
|          1. snapshot_items()          (Event/Schedule -> full list)   |
|          2. synchronization_items()   (startup fallback)              |
|                                                                        |
|  --> list[RouterFlavorResource]  (one per NeutronRouterFlavor CR)      |
+---------------------------------+--------------------------------------+
                                  v
+------------------------------------------------------------------------+
|              reconcile_router_flavor_resources(resources)             |
|                                                                        |
|   group by (secret_name, cloud_name)  --+  avoid 1 conn per CR        |
|                                          v                            |
|   for each credential group:                                         |
|     conn = get_openstack_connection(secret, cloud)                   |
|     wait_for_openstack_network(conn)     # retry until Neutron ready |
|                                                                        |
|     for each resource (CR) in group:                                 |
|       +--------------------------------------------------------+     |
|       |        reconcile_router_flavor_resource(conn, resource)|     |
|       |                     = sync_flavor()                    |     |
|       |                                                        |     |
|       |   desired (CR.spec) ---------> actual (Neutron)        |     |
|       |                                                        |     |
|       |   1. ensure_profile()      create.ensure_profile       |     |
|       |        found by profile_id/driver+meta_info?           |     |
|       |        +-- yes -> reuse (warn if drifted, can't update)|     |
|       |        +-- no  -> create_service_profile()             |     |
|       |                                                        |     |
|       |   2. ensure_flavor()       update.ensure_flavor        |     |
|       |        find_flavor(name)?                              |     |
|       |        +-- yes, desc matches -> no-op                  |     |
|       |        +-- yes, desc drifted -> update_flavor()        |     |
|       |        +-- no  -> create_flavor()                      |     |
|       |                                                        |     |
|       |   3. ensure_profile_attached()  create.ensure_profile_ |     |
|       |        attached                                        |     |
|       |        profile_id in flavor.service_profiles?          |     |
|       |        +-- yes -> no-op                                |     |
|       |        +-- no  -> associate_flavor_with_service_profile()|   |
|       +--------------------------------------------------------+     |
|              | success                    | exception              |
|              v                            v                          |
|   patch_flavor_status(Synced)   patch_flavor_status(Failed)          |
|   (skipped if status already      (added to failed_resources)        |
|    current -- avoids patch-loop)                                     |
|                                                                        |
|   after all CRs in all groups:                                       |
|     if failed_resources: skip prune, exit non-zero (retry)           |
|     else: prune_removed_flavors()  # delete operator-owned            |
|                                       # flavors not in any CR         |
+------------------------------------------------------------------------+

[cardoe]

Code review

Found 6 issues. The theme: a lot of machinery is built and wired into the chart, but never called.

1. Four functions have no non-test callers, so three chart env knobs are inert.
git grep finds only definitions for patch_flavor_status, prune_removed_flavors, load_router_flavor_resources, and wait_for_openstack_network. reconcile_router_flavor calls sync_flavor and returns. Consequences: STATUS_ENABLED does nothing and .status.syncStatus / the SyncStatus printer column stay empty on success and failure; NEUTRON_ROUTER_FLAVOR_PRUNE does nothing and all 238 lines of delete.py are unreachable, so deleting a CR orphans the Neutron flavor and profile forever (the "D" of the commit title); READY_RETRIES/READY_DELAY do nothing, and wait_for_openstack_network is duplicated in plugins/common.py:276.

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

Lines 218 to 224 in fa81b55

	def reconcile_router_flavor(event: dict[str, Any]) -> None:
	"""Reconcile a single NeutronRouterFlavor resource against OpenStack."""
	resource = _resource_from_object(event["object"], "event.object")
	conn = get_openstack_connection(resource.secret_name, resource.cloud_name)
	sync_flavor(conn, resource.flavor)

2. Disabling the plugin no longer disables the hook.
The gate moved from NEUTRON_ROUTER_FLAVOR_ENABLED to "is SYNC_CRONTAB non-empty", but configuredHooks injects env for every configured hook regardless of enabled, and SYNC_CRONTAB defaults to "0 * * * *". helm template with stock values yields ENABLED="false" alongside SYNC_CRONTAB="0 * * * *", so with plugins.neutronRouterFlavors: false the hook still registers its watch and schedule — while rbac.yaml.tpl gates the neutronrouterflavors get/list/watch rules on $hook.enabled, so that watch 403s. This undoes the <PREFIX>_ENABLED-derives-from-$plugin.enabled fix from #2205. placeholder.py still uses OPENSTACK_PLACEHOLDER_ENABLED.

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

Lines 78 to 87 in fa81b55

	is_sync_enabled = bool(
	os.environ.get("NEUTRON_ROUTER_FLAVOR_SYNC_CRONTAB", "").strip()
	)
	if not is_sync_enabled:
	# Shell-operator requires at least one binding.
	hook_config["onStartup"] = 10
	return hook_config
	namespace = os.environ.get("POD_NAMESPACE")

3. Abort-on-first-failure is back.
return 1 inside the loop abandons every remaining object in a Synchronization/Schedule batch, so one malformed CR leaves the rest unsynced — with no status written, per issue 1. This was flagged on #2205 and went away with the split. The new test only covers a single-bad-item batch.

understack/python/openstack-sync/openstack_sync/hooks/common.py

Lines 218 to 222 in fa81b55

	context_type = context.get("type", "")
	if context_type == "Synchronization":
	for item in context.get("objects", []):

4. The hourly sync never dispatches.
The filter compares context["binding"] against "neutron-router-flavors", but the schedule is registered as "name": "hourly sync" and shell-operator sets binding to the schedule's own name, not the k8s binding referenced via includeSnapshotsFrom. The else: branch added here for Schedule snapshots is unreachable; only Add/Modify events reconcile. Worth confirming against a real pod's BINDING_CONTEXT_PATH. Note snapshot_items() scans all contexts for the snapshot key, which is the correct pattern — but it's only reachable from the dead load_router_flavor_resources.

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

Lines 102 to 108 in fa81b55

	hook_config["schedule"] = [
	{
	"name": "hourly sync",
	"crontab": os.environ["NEUTRON_ROUTER_FLAVOR_SYNC_CRONTAB"],
	"includeSnapshotsFrom": [CRD_BINDING_NAME],
	}
	]

5. Replacement service profiles are created without ownership markers.
meta_info if configured_profile_id else managed_meta_info(meta_info) — when profile_id is set but that profile is gone, the newly created one gets no markers, so is_managed_service_profile never recognizes it and prune can never reclaim it. Relatedly, a stale profile_id silently degrades to discover-or-create, contradicting the CRD's "to attach instead of creating or discovering one".

understack/python/openstack-sync/openstack_sync/plugins/neutron/router_flavors/create.py

Lines 106 to 118 in fa81b55

	service_profile_meta = (
	meta_info if configured_profile_id else managed_meta_info(meta_info)
	)
	log(f"Creating service profile for {name} driver={driver}")
	return conn.network.create_service_profile(
	description=description,
	driver=driver,
	meta_info=meta_info_payload(service_profile_meta),
	is_enabled=True,
	)

6. comparable_meta_info is a no-op that claims otherwise.
Docstring says it strips operator-managed keys; the body is {k: v for k, v in normalized.items()} with no filter. Unused today (callers import the real one from router_flavors_common), so it's a trap for the next plugin author rather than a live bug.

understack/python/openstack-sync/openstack_sync/plugins/common.py

Lines 160 to 172 in fa81b55

	def comparable_meta_info(value: Any) -> Any:
	"""Strip operator-managed keys from *value* before comparison.
	Operator marker keys (e.g. ``_understack_router_flavor_operator``) are
	injected at creation time and must not trigger spurious updates when
	comparing desired vs current state. The caller is responsible for
	passing the set of keys to strip via the module-level constant in the
	plugin's ``common`` module.
	"""
	normalized = normalize_meta_info(value)
	if isinstance(normalized, dict):

Checked and clear: the empty-desired-set prune guard from #2205 is present; DEFAULT_SECRET/DEFAULT_CLOUD just wire up values added in 497d6cd; framework conventions (hooks baked into the image, CRDs with the operator, no prefix-based discovery) are all respected. Minor: the PR body is empty, and the commit headline isn't Conventional Commit style.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[cardoe]

Follow-up: use stdlib logging instead of log_fn

Design note, not a defect. This package should adopt logging with output to stderr, dropping the log_fn plumbing.

Today there are three ways to emit a line: bare print(..., file=sys.stderr) (pre-existing in placeholder.py), a log() wrapper hardcoding a [router_flavors] prefix, and log_fn parameters defaulting to lambda msg: print(msg, file=sys.stderr) threaded through signatures.

log_fn is hand-rolled dependency injection for what logging already provides. logger = logging.getLogger(__name__) handles per-module routing so nothing gets passed down; __name__ plus a formatter replaces the per-plugin prefix wrapper; and caplog covers the test capture that log_fn is currently carrying (test_hook_common.py passes logs.append). Levels also stop being faked as "WARNING: " string prefixes (hooks/common.py:139, create.py:88) and become filterable.

This is also the rest of the repo's convention — 64 files under python/ use getLogger(__name__); openstack-sync is the only package with no logging at all.

Two constraints when doing it:

• Configure via logging.basicConfig(stream=sys.stderr, ...) inside each hook's main(), not at import time. shell-operator captures the hook's stderr, so stderr keeps current behavior; import-time setup would compound the read-env-at-import staleness trap from feat(openstack-sync-operator): Operator for syncing openstack data as kubernetes custom resources #2205.
• Nothing may log to stdout — build_hook_config output is parsed as JSON from stdout.

🤖 Generated with Claude Code

[cardoe]

Prune computes its desired set only from this pod's POD_NAMESPACE snapshot but prune_removed_flavors operates on the shared Neutron cloud. With PRUNE on and two namespaces managing CRs against the same (secretName, cloudName), this deletes flavors owned by the other namespace's CRs — and the authoritative-empty path at line 408 (authoritative_empty_desired=True) wipes all managed L3_ROUTER_NAT flavors in the cloud. We only run in the openstack cloud for now so it's not a live risk, but noting it: we need to come back and scope prune to marker + namespace ownership, not just the marker.

[cardoe]

Well my skill went sideways here. This is merely a comment so that we don't forget about this if we start using multiple namespaces. So it'd be good to capture this as an issue and not something to fix before merging.

[cardoe]

These module-level env reads (the os.environ[...] hard lookups here plus the env_bool/int/float parsing further down) run at import, and hooks/router_flavors.py imports this module at load. A missing or malformed var therefore raises before the hook can emit its config — including the --config path shell-operator needs just to register the hook. A bad value should degrade, not stop the hook from loading. Defer these reads into the config-building/run path.

[cardoe]

This comment was supposed to be a question if this is a problem with the design of the shell-operator or not.

[cardoe]

ensure_flavor only reconciles description drift. find_flavor matches on name alone, so a pre-existing flavor with a different service_type or is_enabled=False is returned unchanged, then gets stamped with the operator marker + profile and reported Synced (and becomes prune-eligible) while still mismatched/disabled. Flagging for a follow-up — should reconcile service_type/is_enabled, or refuse to adopt a mismatched flavor.

[cardoe]

This is one to identify as an issue and not a blocker to merge.

[cardoe]

The reconcile loop at line 332 isolates connection failures per credential group, but this deleted-only prune calls get_openstack_connection/wait_for_openstack_network bare. A transient Keystone/Neutron hiccup here raises after the current resources were already patched Synced, propagates to main()'s broad except → return 1 → shell-operator re-runs the whole hook. Wrap it like the reconcile loop.

[cardoe]

This seems like it should be fixed. I'm also okay as a fast follow up.

[cardoe]

This prune is O(profiles × flavors) in Neutron calls: service_profile_attached_to_any_flavor re-lists every L3 flavor per candidate profile, and prune_orphaned_service_profiles scans all profiles every reconcile. On a busy cloud the hourly sync hammers the API. This needs an immediate follow-up to this PR — build the flavor→profile map once per prune pass.

[cardoe]

This is a follow up fix up to me.

[cardoe]

Minor: is_prunable_flavor takes conn but never uses it, so callers thread a connection through for nothing and it misleadingly implies API calls. Not worried about it — drop the param when convenient.