docs: define control plane reconciliation contract by jsell-rh · Pull Request #200 · openshift-online/hypershell

jsell-rh · 2026-08-25T14:49:30Z

Executive Summary

Define one shared, level-based reconciliation contract for HyperShell control-plane reconcilers.
Apply multiversion concurrency control at API write boundaries through immutable UIDs, resource versions, generations, and conditional status and finalizer writes.
Align health, deletion, namespace garbage collection, watch events, and end-to-end expectations with restart-safe convergence.

Specification Summary

Add specs/standards/control-plane/reconciliation-contract.spec.md as the canonical behavior contract.
Require current desired-state reads, initial listing, periodic resync, idempotent operations, per-resource serialization, bounded retries, truthful status, durable finalization, and fail-closed destructive actions.
Keep typed domain reconcilers while allowing a shared driver to provide keying, serialization, retry, and resync.
Update the control-plane architecture and conventions to reference the contract.
Update Gateway health behavior so a healthy phase does not suppress drift repair.
Update delete-watch and namespace-GC specifications so events schedule work while the durable deleting resource remains authoritative.
Update end-to-end deletion expectations so 404 follows successful finalization, not only delete-event processing.

Validation

Validated all seven changed specification files for local links, required sections, requirement scenarios, and excess blank lines.
git diff --check origin/main...HEAD passed.
Pre-commit repository policy checks passed, including 20 policy unit tests, dependency checks, and CI component registration.
No runtime tests were required because this pull request changes specifications only.

Jira

No Jira ticket is associated with this specification change.

Scope

This pull request defines desired reconciliation behavior. Control-plane and API implementation changes are separate work.

Checklist

Added or updated the relevant specifications.
Aligned related cross-component behavior.
Ran repository policy and specification validation.

coderabbitai · 2026-08-25T14:49:38Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: aea4caee-8f76-4325-9d0d-5bbc7a4ae83c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

rh-amarin · 2026-08-25T16:12:18Z

+deletion intent, and fields required to locate owned state.

-#### Scenario: Soft-deleted record no longer exists
+#### Scenario: Gateway delete event carries identity


I don't fully get what does "carries identity" mean
The description says that includes Gateway ID (identity), UID, namespace and ownership data...

What I understood by "event carries identity" is that the event is a light event, or anemic/skinny event which only carries an ID...

So, do you refer that it:

Carries identities of objects to be deleted

Or that is contains data?

A fat event, or sometimes a more pompous name of Event-Carried State Transfer

Which will be enough for a consumer to do their work

Yeah it should be more accurately titled Gateway delete event carries resource snapshot or something.

Is this bad practice?

rh-amarin · 2026-08-25T16:32:04Z

+- THEN the API SHALL reject or supersede that stale write
+- AND a later pass SHALL reconcile generation 5
+
+### Requirement: Deletion Uses Durable Finalization


When I reviewed @jjaggars #197 PR, there the tombstone concept for deleting managedDatabases is introduced

I was curious how aligned is to your specs here. In LLMs wordy format...

The alignment is strong and direct. Here's how they connect:

Jesse's tombstone fix (effdbce): When a ManagedDatabase delete event arrives with a resource snapshot, the reconciler now calls rememberManagedDatabase after the event arrives (not before, as in the test setup). This means a failed cleanup retains the tombstone, so a subsequent retry event carrying only the resource ID can still find the data it needs to finish cleanup.

This PR formalizes exactly that behavior as a platform-wide contract. The new reconciliation-contract.spec.md is the key document — specifically:

"Deletion Uses Durable Finalization" — states that partial cleanup or process restart SHALL leave deletion pending and retryable. Jesse's in-memory tombstone retention is a direct implementation of this: it keeps finalization data alive across failed attempts.

"Destructive Actions Fail Closed" — if observation fails, defer and error. Jesse's fix: cleanup failure doesn't remove the tombstone, so the next pass isn't blind.

"Missing Event Payload Does Not Complete Cleanup" (in the updated watch-delete-events.spec.md) — if the snapshot is absent, the reconciler SHALL fetch durable state and SHALL NOT treat the missing payload as success. Jesse's tombstone is the in-memory analog of this: it's the fallback when the retry event carries no Resource.

The key tension: Jesse's tombstone is an in-memory cache — it doesn't survive a process restart. This PR's contract goes further: it specifies that the API resource itself (with a finalizer) must remain readable and listable as the durable source of truth, not a process-local cache. The updated watch-delete-events.spec.md makes this explicit: "the durable deleting resource and its finalizer are authoritative", while the snapshot in the event is just a latency optimization.

So Jesse's fix is correctly aligned in spirit — it implements the "fail closed, retain cleanup data, retry until confirmed absent" principle — but it's an intermediate step. The reconciliation contract this PR defines points toward a more robust, restart-safe finalization model where the API server (with finalizers) holds the tombstone durably, not process memory.

rh-amarin · 2026-08-25T16:37:46Z

+#### Scenario: Actual state drifts
+
+- GIVEN a resource reports healthy at its current generation
+- AND an owned actual resource is later removed or changed


How is ownership defined?
Imagine someone creates one resource (say a namespace) and then the reconcile process wants to create/update that same resource... does it take ownership of it?

In the previous scenario we also mention the reconciliation SHALL "adopt" that resource

Should we define ownership at this spec level, or should we leave it to individual resource reconcilers how they'd like to define ownership?

rh-amarin · 2026-08-25T16:43:03Z

+
+The control plane SHALL run at most one reconcile pass for one resource UID at a time. It MAY reconcile different resources concurrently with a bounded worker count. A slow or failed resource SHALL NOT block watch consumption or unrelated resources.
+
+A failed required operation SHALL return an error and SHALL be retried with bounded exponential backoff. A newer event SHALL cause another pass against current state. A delete request SHALL take precedence over older queued updates for the same UID.


A delete request SHALL take precedence over older queued updates for the same UID
The word "older" makes me wonder... is it possible to receive an update event after a delete request?

I guess the API will reject updates to a resource marked for deletion, so my case may be not possible.... or maybe request come out of order and it is still possible

Yeah I'm not sure how it works today, but agree that if something is either marked for deletion, or actually deleted, update events for that object should 404 or something

rh-amarin · 2026-08-25T16:43:49Z

+
+A failed required operation SHALL return an error and SHALL be retried with bounded exponential backoff. A newer event SHALL cause another pass against current state. A delete request SHALL take precedence over older queued updates for the same UID.
+
+If more than one control-plane replica can mutate state, the deployment SHALL provide one active leader or equivalent cross-process exclusion.


Are there control-plane instances that do not mutate state?

no, I think that's just saying that in the event we have more than 1 replica of a control plane, there must be some leader/follower mechanism

rh-amarin · 2026-08-25T16:46:01Z

+- GIVEN a pass observes generation 4
+- AND desired state changes to generation 5 before the pass records success
+- WHEN the pass attempts to write status for generation 4
+- THEN the API SHALL reject or supersede that stale write


What does it mean "or supersede" can the API behave differently than rejecting?

rh-amarin · 2026-08-25T16:50:13Z

+- GIVEN deletion requires several cleanup operations
+- WHEN one required operation fails
+- THEN the finalizer SHALL remain
+- AND reconciliation SHALL retry until all required state is confirmed absent


This makes me wonder if that retry means there is a gorutine/thread alive retrying the deletion... or if it returns and retries on a later "resync" event

rh-amarin · 2026-08-25T16:58:36Z

+
+Every reconciled API resource SHALL have an immutable UID, a resource version that changes on every write, and a generation that changes only when desired state changes. Controller status SHALL identify the generation it observed.
+
+Status and finalizer writes SHALL be conditional on the resource identity and relevant version or generation. A stale pass SHALL NOT publish success or complete deletion for a newer generation. Controllers SHALL update only fields they own and SHALL NOT use whole-resource replacement for independent status fields.


I don't know if "finalizers" here refer to:

Kubernetes finalizers in created resources

Another concept of finalizer tied to HyperShell API objects

Controllers SHALL update only fields they own
Does this mean different reconcilers can modify the same resource?

jsell-rh · 2026-08-27T18:40:09Z

Amber review

Status: Complete

Verdict

COMMENT (do not merge without cross-PR coordination). This is a clean, well-structured, spec-only change that establishes a genuinely useful canonical reconciliation contract; the specification text itself is sound. However, it directly overlaps three other open control-plane spec PRs and, in a few places, rewrites specs out from under already-shipped behavior (the GetUnscoped delete-enrichment path) and mandates data-model primitives (finalizers, UID/resourceVersion/generation) that no spec currently defines. Those coordination items should be resolved by maintainers before this merges.

Hi, Amber here. I loaded CLAUDE.md, the security and control-plane conventions specs, and the review guidance, then reviewed all seven changed spec files at a5dbb5c. Because this PR is spec-only, most code-level conventions (panics, error wrapping, SecurityContext, secret handling) are N/A; the review focuses on spec consistency, plan/change-order, and cross-PR conflicts as requested.

Summary

The new reconciliation-contract.spec.md is a strong, coherent, level-based contract and the cross-references added to conventions.spec.md and control-plane.spec.md are appropriate. The risk in this PR is not its prose but its coordination surface: it re-specifies the phase gate, periodic resync, resource versioning, and delete/finalization semantics that are simultaneously being defined by PRs #151 and #185, and it deprecates a delete-enrichment design (GetUnscoped) that is already merged and running.

Findings

1. [Major] watch-delete-events.spec.md rewrite supersedes a shipped design without a migration/transition note — Spec Consistency / Change Order (specs/platform/watch-delete-events.spec.md L12-L44)

The prior spec described soft-delete + an unscoped DAO read (GetUnscoped) to enrich delete events, plus removal of the in-memory namespace cache. That design is already implemented: components/api-server/plugins/gateways/{dao.go,service.go,grpc_handler.go} and components/api-server/plugins/roleBindings/grpc_handler.go all use GetUnscoped to load soft-deleted rows for delete events. The rewrite replaces this with a finalizer-based durable-deletion model and deletes the Design Decisions table that recorded the shipped rationale. No finalizer support exists anywhere in the codebase today (grep finds finalizers only in these specs). The spec should either (a) keep/annotate the GetUnscoped approach as the current transitional mechanism, or (b) add an explicit note that finalizer-based deletion supersedes it and that GetUnscoped is now legacy — otherwise the spec silently contradicts running behavior.

2. [Major] Contract mandates data-model primitives (immutable UID, per-write resource version, generation, finalizers) that data-model.spec.md does not define — Spec Completeness (specs/standards/control-plane/reconciliation-contract.spec.md L58, L70-L76)

"Resource Versions Prevent Stale Commits" and "Deletion Uses Durable Finalization" require every reconciled API resource to carry an immutable UID, a resource version that changes on every write, a generation that changes only on desired-state change, deletion intent, and a control-plane finalizer. None of these exist in data-model.spec.md, and the API server is PostgreSQL/rh-trex-backed (today it orders on updated_at). As written, the canonical contract is currently unsatisfiable and floats above the data model. It should reference the data-model additions that provide these primitives (see cross-PR note on #151 and #185, which each introduce part of this vocabulary under different names).

3. [Minor] New canonical spec is not registered in the Spec Registry — Spec Completeness (specs/index.spec.md, unchanged)

specs/index.spec.md is the machine-readable registry the /reconcile skill consumes, and it already lists standards/control-plane/conventions.spec.md. The new standards/control-plane/reconciliation-contract.spec.md is not added there, so autonomous reconciliation and the dependency graph won't see the contract. PR #185 registered its new spec in the same index; this PR should add a row (Domain standards, Components CP, Depends On control-plane, data-model).

4. [Minor] e2e-testing.spec.md scenario now asserts finalization-based 404 that the running system does not yet produce — Spec Consistency / Change Order (specs/platform/e2e-testing.spec.md L336-L383)

The existing e2e expectation ("404 once the control plane has processed the delete event") is changed to "404 once the control plane completes cleanup and removes its finalizer." This is consistent with the new deletion model but describes behavior that depends on the unimplemented finalizer capability. Fine as a forward-looking target, but it should land in step with the finalizer implementation so the e2e suite's desired state isn't ahead of what any component can satisfy. This is not a test-assertion regression (it's a spec scenario, not a _test.go change), but it is a guarantee change worth calling out explicitly.

Cross-PR coordination

I compared PR #200 against the other 23 open PRs (listed below). Three are in the same control-plane-reconciliation design cluster and have material conflicts that need a maintainer decision; the rest do not conflict.

Other open PRs reviewed: #216 fix(console) OpenShift Route; #214 Hypershell-112 UI; #212 e2e performance harness; #211 fix(kind) metrics/DNS; #210 gateway-matched CLI install; #209 Dashboard UI; #208 sandbox-connect UI; #207 reconcile-to-request trace correlation; #206 hsctl login; #201 Red Hat openshell images; #194 adopt upstream OpenShell Helm chart; #189/#188/#135 PatternFly/react-intl dep bumps; #185 periodic world synchronization; #182 JWT audience; #179 reconcile Keycloak clients on gated gateways; #151 gate re-provisioning on desired-state convergence; #150 kind LOCAL_IMAGES; #148 OpenShell branch build spec; #109 security tools; #75/#73 dep bumps.

Material conflict — PR #151 (spec(control-plane): gate gateway re-provisioning on desired-state convergence): Both PRs edit the same two files and the same two requirements. In openshell-gateway-health.spec.md, #200 replaces the requirement Health Reconciliation Not Suppressed By Phase with Health Status Does Not Suppress Reconciliation, while #151 replaces the same requirement with Provisioning Gate Keyed On Desired State. In control-plane.spec.md both rewrite the Status Synchronization requirement. Beyond the textual collision this is a design conflict: #151 gates re-apply strictly on observed_generation == generation and explicitly defers out-of-band drift repair to a later decision; #200's contract requires drift repair even for out-of-band changes ("If owned actual state changes after initial convergence, a later pass SHALL repair it" / "an owned actual resource is later removed or changed … it SHALL restore the desired state"). Maintainers need to decide: is out-of-band drift repair in-scope now (#200) or deferred (#151), and which requirement heading/data model is canonical. They also each add versioning fields (#151 persists generation/observed_generation; #200 requires UID + resourceVersion + generation) that must be reconciled into one data model.

Material conflict — PR #185 (docs(control-plane): specify periodic world synchronization): #200's "Desired State Is Authoritative" requirement (initial list + periodic resync as recovery) and #185's entire control-plane-world-sync.spec.md specify the same mechanism at different levels of detail, and both edit control-plane.spec.md and data-model.spec.md. #185 introduces a resource_revision primitive (explicitly "transport/concurrency metadata rather than persisted domain fields") plus an inventory watermark; #200 requires "a resource version that changes on every write" as a property of the resource. These are the same concept with divergent names and layering. Maintainers should decide ownership: make reconciliation-contract.spec.md the umbrella contract and have control-plane-world-sync.spec.md implement it, and pick a single name/semantic for the resource-version primitive so the API contract isn't defined twice.

Related, softer — PR #179 (fix(control-plane): reconcile existing Keycloak clients on gated gateways): #179 is an implementation hotfix that depends on the current phase-gate early-return and already documents its interaction with #151. It doesn't touch the files #200 changes, but its correctness assumption (phase-gate return exists) is exactly what #151/#200 are redesigning. No direct conflict, but it belongs to the same decision cluster; whatever gate model maintainers pick should confirm where #179's lightweight Keycloak pass runs.

Data-model coordination (spans #151 + #185 + #200): Three PRs concurrently introduce resource-versioning vocabulary — generation/observed_generation (persisted, #151), resource_revision/watermark (transport metadata, #185), and UID + resourceVersion + generation + finalizers (contract-level, #200). These must converge into one coherent data model and one set of field names; merging any two without alignment will leave the data-model spec internally inconsistent.

No material conflicts were found with any other open PR.

Convention Checklist

Convention	Result
Conventional commit message	Pass
Spec internal links resolve	Pass
`git diff --check` (whitespace) clean	Pass
New canonical spec registered in Spec Registry	Fail
Spec consistent with shipped behavior / data model	Fail

Findings Summary (ordered by severity, highest first):

[Major] watch-delete-events.spec.md rewrite supersedes shipped GetUnscoped delete-enrichment without a migration note - Spec Consistency / Change Order (L12-L44)
[Major] Contract requires UID/resourceVersion/generation/finalizer primitives absent from the data model - Spec Completeness (reconciliation-contract L58, L70-L76)
[Minor] New canonical spec not registered in specs/index.spec.md Spec Registry - Spec Completeness
[Minor] e2e-testing.spec.md now asserts finalization-based 404 not yet producible - Spec Consistency / Change Order (L336-L383)

Confidence: High on the cross-PR conflicts (#151/#185 edit the same requirements/files, verified from their diffs) and on the GetUnscoped spec/code divergence (verified in the tree). Medium on severity of the data-model gap, since this is an intentionally forward-looking contract.

jsell-rh