feat(openstack-sync-plugins): neutron router flavor

[haseebsyed12]

No description provided.

[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