fix(openstack-sync-operator): clear orphaned profiles when prune is off and few improvements

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: <binding name>`), 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.
 

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:

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):

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,