OCPBUGS-84321: ClusterAPIMachineManagement: capi-controllers: excessive restarts of during cluster installation#614
Conversation
…during cluster installation Move capi-controllers Deployment from CVO to operator The capi-controllers Deployment was deployed by CVO as a static manifest, starting simultaneously with the installer. Since the installer needs time to install provider CRDs, capi-controllers would crash repeatedly until they became available. Move the Deployment to the operator's manifests-gen pipeline at installOrder 30, where probe-gated sequencing ensures all CRDs are Established before the Deployment is created. Add a tombstone so CVO deletes its old copy on upgrade. Assisted-by: Claude (Opus 4.6)
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-low
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-84321, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe default manifest generation path now uses a Kustomize input directory, embeds the CAPI controllers Deployment, records its image reference, and passes the controller image to generation. Admission policy test utilities now retain only recognized admission policy object types. ChangesCluster CAPI manifest generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
capi-operator-manifests/default/manifests.yaml (1)
1032-1032: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider setting
automountServiceAccountToken: falseif the API token isn't needed by this pod, or confirm it is required.
serviceAccountName: capi-controllersis set butautomountServiceAccountTokenisn't specified, defaulting to token mounting.As per path instructions, "
**/*.{yaml,yml}: ... automountServiceAccountToken: false unless needed".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@capi-operator-manifests/default/manifests.yaml` at line 1032, The pod spec using serviceAccountName capi-controllers is implicitly mounting a service account token, which should be disabled unless this workload needs API access. Update the manifest where the pod spec is defined to set automountServiceAccountToken to false if the token is not required, or otherwise confirm and keep the default behavior; use the serviceAccountName setting to locate the relevant pod template.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@capi-operator-manifests/default/manifests.yaml`:
- Around line 990-993: The container resource configuration only sets
resources.requests and is missing matching resources.limits for cpu and memory.
Update the container specs in manifests.yaml, including both affected resource
blocks, to add explicit limits alongside the existing requests so each container
has bounded resource usage.
- Around line 970-1027: Add livenessProbe and readinessProbe to both containers
in the manifest: the capi-controllers and machine-api-migration container specs
already expose dedicated healthz ports (healthz-o on 9440 and healthz-m on
9441), so wire the probes to those endpoints using the existing container names
and ports. Ensure each probe is defined alongside the current ports/args for the
corresponding container and points to the appropriate health check path on the
matching healthz port.
- Around line 970-1027: Both container specs in the manifest lack a
securityContext, so they still run with root-capable defaults; add a
securityContext to the capi-controllers and machine-api-migration container
definitions. Set runAsNonRoot and readOnlyRootFilesystem, disable
allowPrivilegeEscalation, and drop ALL capabilities (only add back anything
explicitly required). Use the existing container blocks with names
capi-controllers and machine-api-migration to place the same hardened settings
on both.
- Around line 948-1049: The capi-controllers Deployment manifest is missing
required hardening settings, so update the source manifest for the
capi-controllers Deployment to include a pod/container securityContext, add
resource limits alongside the existing requests, and define liveness and
readiness probes for both containers where appropriate. Make sure these
additions are placed in the same Deployment spec that defines the
capi-controllers and machine-api-migration containers so the rendered bundle
stays in sync.
---
Nitpick comments:
In `@capi-operator-manifests/default/manifests.yaml`:
- Line 1032: The pod spec using serviceAccountName capi-controllers is
implicitly mounting a service account token, which should be disabled unless
this workload needs API access. Update the manifest where the pod spec is
defined to set automountServiceAccountToken to false if the token is not
required, or otherwise confirm and keep the default behavior; use the
serviceAccountName setting to locate the relevant pod template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c6e49887-bf0f-40ca-be91-6bde2079827a
📒 Files selected for processing (6)
Makefileadmission-policies/default/capi-controllers-deployment.yamladmission-policies/default/kustomization.yamlcapi-operator-manifests/default/manifests.yamlcapi-operator-manifests/default/metadata.yamlmanifests/0000_30_cluster-api_00_tombstones-5.0.yaml
💤 Files with no reviewable changes (1)
- admission-policies/default/capi-controllers-deployment.yaml
| containers: | ||
| - args: | ||
| - --diagnostics-address=:8443 | ||
| command: | ||
| - /capi-controllers | ||
| env: | ||
| - name: RELEASE_VERSION | ||
| value: 0.0.1-snapshot | ||
| image: registry.ci.openshift.org/openshift:cluster-capi-operator | ||
| name: capi-controllers | ||
| ports: | ||
| - containerPort: 9443 | ||
| name: webhook-server | ||
| protocol: TCP | ||
| - containerPort: 8443 | ||
| name: diagnostics-o | ||
| protocol: TCP | ||
| - containerPort: 9440 | ||
| name: healthz-o | ||
| protocol: TCP | ||
| resources: | ||
| requests: | ||
| cpu: 10m | ||
| memory: 50Mi | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| volumeMounts: | ||
| - mountPath: /tmp/k8s-webhook-server/serving-certs | ||
| name: cert | ||
| readOnly: true | ||
| - mountPath: /tmp/k8s-metrics-server/serving-certs | ||
| name: metrics-cert | ||
| readOnly: true | ||
| - args: | ||
| - --diagnostics-address=:8442 | ||
| - --health-addr=:9441 | ||
| command: | ||
| - /machine-api-migration | ||
| env: | ||
| - name: RELEASE_VERSION | ||
| value: 0.0.1-snapshot | ||
| image: registry.ci.openshift.org/openshift:cluster-capi-operator | ||
| name: machine-api-migration | ||
| ports: | ||
| - containerPort: 8442 | ||
| name: diagnostics-m | ||
| protocol: TCP | ||
| - containerPort: 9441 | ||
| name: healthz-m | ||
| protocol: TCP | ||
| resources: | ||
| requests: | ||
| cpu: 10m | ||
| memory: 50Mi | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| volumeMounts: | ||
| - mountPath: /tmp/k8s-metrics-server/serving-certs | ||
| name: metrics-cert | ||
| readOnly: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add liveness and readiness probes.
Neither container defines livenessProbe/readinessProbe, despite both exposing dedicated healthz-* ports (9440, 9441) presumably intended for this purpose.
As per path instructions, "**/*.{yaml,yml}: ... Liveness + readiness probes defined".
🧰 Tools
🪛 Trivy (0.69.3)
[error] 978-1003: Root file system is not read-only
Container 'machine-api-migration' of Deployment 'capi-controllers' should set 'securityContext.readOnlyRootFilesystem' to true
Rule: KSV-0014
(IaC/Kubernetes)
[error] 978-1003: Default security context configured
container capi-controllers in openshift-cluster-api namespace is using the default security context
Rule: KSV-0118
(IaC/Kubernetes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@capi-operator-manifests/default/manifests.yaml` around lines 970 - 1027, Add
livenessProbe and readinessProbe to both containers in the manifest: the
capi-controllers and machine-api-migration container specs already expose
dedicated healthz ports (healthz-o on 9440 and healthz-m on 9441), so wire the
probes to those endpoints using the existing container names and ports. Ensure
each probe is defined alongside the current ports/args for the corresponding
container and points to the appropriate health check path on the matching
healthz port.
Source: Path instructions
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add securityContext (runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation, dropped capabilities) to both containers.
Neither capi-controllers nor machine-api-migration containers define a securityContext, so they run with the platform default (root-capable, writable root filesystem, privilege escalation allowed). Static analysis flags this (Checkov CKV_K8S_20/CKV_K8S_23, Trivy KSV-0014/KSV-0118).
🔒 Proposed securityContext additions
containers:
- args:
- --diagnostics-address=:8443
command:
- /capi-controllers
+ securityContext:
+ allowPrivilegeEscalation: false
+ readOnlyRootFilesystem: true
+ runAsNonRoot: true
+ capabilities:
+ drop:
+ - ALL
env:Apply the equivalent block to the machine-api-migration container as well.
As per path instructions, "**/*.{yaml,yml}: If this is a Kubernetes/OpenShift manifest or Helm template: securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false; Drop ALL capabilities, add only what is required".
🧰 Tools
🪛 Trivy (0.69.3)
[error] 978-1003: Root file system is not read-only
Container 'machine-api-migration' of Deployment 'capi-controllers' should set 'securityContext.readOnlyRootFilesystem' to true
Rule: KSV-0014
(IaC/Kubernetes)
[error] 978-1003: Default security context configured
container capi-controllers in openshift-cluster-api namespace is using the default security context
Rule: KSV-0118
(IaC/Kubernetes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@capi-operator-manifests/default/manifests.yaml` around lines 970 - 1027, Both
container specs in the manifest lack a securityContext, so they still run with
root-capable defaults; add a securityContext to the capi-controllers and
machine-api-migration container definitions. Set runAsNonRoot and
readOnlyRootFilesystem, disable allowPrivilegeEscalation, and drop ALL
capabilities (only add back anything explicitly required). Use the existing
container blocks with names capi-controllers and machine-api-migration to place
the same hardened settings on both.
Source: Path instructions
There was a problem hiding this comment.
Probes: The healthz ports (9440, 9441) are declared in the Deployment manifest but neither binary actually configures a health endpoint — there's no HealthProbeBindAddress in cmd/capi-controllers/main.go or cmd/machine-api-migration/main.go. Adding probes against those ports would cause the pods to fail health checks and crash-loop. The original Deployment on main has no probes for the same reason.
securityContext: Same as the other comment — the original Deployment on main has no securityContext. This PR moves the Deployment between deployment mechanisms without changing its spec. Hardening would be a separate effort.
There was a problem hiding this comment.
@sg00dwin, thanks for the clarification — that makes sense on both counts. Since the binaries don't wire up HealthProbeBindAddress for those healthz-* ports, adding probes now would indeed cause crash-looping, and since this PR is purely a deployment-mechanism migration (CVO static manifest → operator manifests-gen) without any spec changes, carrying over the existing (lack of) securityContext is consistent with keeping the diff scoped to ordering only. I'll withdraw both suggestions.
✏️ Learnings added
Learnt from: sg00dwin
Repo: openshift/cluster-capi-operator PR: 614
File: capi-operator-manifests/default/manifests.yaml:970-1027
Timestamp: 2026-07-02T21:04:22.725Z
Learning: In the openshift/cluster-capi-operator repository, the `capi-controllers` Deployment (in capi-operator-manifests/default/manifests.yaml) exposes `healthz-o` (port 9440) and `healthz-m` (port 9441) container ports, but neither `cmd/capi-controllers/main.go` nor `cmd/machine-api-migration/main.go` configures a `HealthProbeBindAddress`, so these ports do not serve actual health endpoints. Do not suggest adding livenessProbe/readinessProbe against these ports unless the binaries are updated to implement them. Additionally, this Deployment intentionally has no securityContext defined, matching the pre-existing static manifest deployed by CVO on main; PRs that migrate the Deployment between deployment mechanisms (e.g., from CVO static manifests to the operator's manifests-gen pipeline) without changing the Deployment spec should not be flagged for missing securityContext hardening, as that would be a separate effort.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Both binaries use commonoptions.go which registers:
- /healthz (ping) on the health-addr port
- /readyz (ping) on the same port
Default is :9440, and the machine-api-migration container overrides it to :9441 via --health-addr=:9441 in the Deployment args.
See pkg/commoncmdoptions/commonoptions.go
It's arguably out of scope for this, but may be worth adding in if you've got time :)
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-84321, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/test unit |
LoadAdmissionPolicyProfiles() deserialized every object from manifests.yaml without type filtering. Adding the capi-controllers Deployment to that file broke 68 admission policy tests. Restrict the loader to ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding types only. Assisted-by: Claude (Opus 4.6)
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-84321, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/pipeline auto |
|
Pipeline controller notification The |
|
|
||
| if co, ok := o.(client.Object); ok { | ||
| switch co := o.(type) { | ||
| case *admissionregistrationv1.ValidatingAdmissionPolicy: |
There was a problem hiding this comment.
What's the motivation here ? Missing any explanation in the PR description :)
There was a problem hiding this comment.
The Deployment is now part of the generated manifests (via kustomize), so LoadAdmissionPolicyProfiles() picks it up alongside the VAPs. This filter ensures only ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding objects get loaded — the Deployment is skipped so it doesn't interfere with admission policy tests.
Added an explanation to the PR description.
| - prevent-migration-when-machine-updating.yaml | ||
| - provide-warning-when-not-synchronized.yaml | ||
| - authoritative-api-transition-requires-capi-infrastructure-ready.yaml | ||
| - capi-controllers-deployment.yaml |
There was a problem hiding this comment.
Hmm - this isn't an admission policy? why are we in the admission policies folder / kustomization?
There was a problem hiding this comment.
Moved it out. The Deployment now lives in a new ocp-manifests-input/default/ directory with an aggregating kustomization that references both admission-policies/default and the Deployment. The Makefile points --kustomize-dir there instead. Admission policies directory is VAPs-only again.
| @@ -0,0 +1,16 @@ | |||
| # Tombstones for objects moved from CVO to operator-managed during 5.0. | |||
There was a problem hiding this comment.
Do we need/want a CVO tombstone here? I don't understand the motivation.
If we delete the CAPI controllers, we will have a period of time where there are no CAPI controllers running before they're recreated by the CAPI installer.
I think we need to ensure the ordering is right here - will having no CAPI controllers cause the CVO to stall?
We may be able to use the adopt-existing: always annotation on the Deployment so boxcutter adopts the existing one with no downtime?
There was a problem hiding this comment.
Dropped the tombstone entirely. The Deployment now has capi-operator.openshift.io/adopt-existing: "always", so Boxcutter adopts the existing CVO-managed Deployment in-place via SSA. No delete, no gap, no pod restarts during upgrade. On fresh install, the annotation is a no-op.
| --name cluster-capi-operator \ | ||
| --install-order 30 | ||
| --install-order 30 \ | ||
| --self-image-ref registry.ci.openshift.org/openshift:cluster-capi-operator |
There was a problem hiding this comment.
Why have we added this here? I'm not sure we need it, but I could be wrong.
I think we would end up doing a replace of registry.ci.openshift.org/openshift:cluster-capi-operator with registry.ci.openshift.org/openshift:cluster-capi-operator ?
The manifests that get shipped get rewritten by release tooling, but im not sure. One way to check would be running an e2e with this omitted.
There was a problem hiding this comment.
It's needed because release tooling rewrites images in CVO manifests (manifests/), but not in manifests embedded inside the operator image (capi-operator-manifests/). Without --self-image-ref, the Deployment would keep the build-time CI placeholder image, which doesn't exist in production. The operator uses the selfImageRef mechanism to replace it at runtime with the actual image digest from the pod spec.
|
@sg00dwin I think this is a good start, but think the approach needs refining. We shouldn't be putting our deployment in the admission policies folder. The CVO only installs annotated manifests, so we can probably keep the deployment in the |
- Move Deployment from admission-policies/ to ocp-manifests-input/default/ with an aggregating kustomization referencing both VAPs and the Deployment - Replace CVO tombstone with adopt-existing: always annotation for seamless ownership transfer on upgrade (SSA in-place patch, no gap, no restarts) - Keep --self-image-ref (embedded manifests aren't rewritten by release tooling) - Regenerate capi-operator-manifests/default/ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Updated per reviewChanges since last review:
All tests pass (27 suites), build and lint clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ocp-manifests-input/default/kustomization.yaml`:
- Line 6: Update the customization flow used by customizeDeployment in
manifests-gen/customizations.go so it preserves each container’s CPU and memory
resource limits instead of clearing container.Resources.Limits; ensure the
generated Deployment from capi-controllers-deployment.yaml retains limits for
every container.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9cfebbb3-992c-4863-af32-9759684c43e9
📒 Files selected for processing (4)
Makefilecapi-operator-manifests/default/manifests.yamlocp-manifests-input/default/capi-controllers-deployment.yamlocp-manifests-input/default/kustomization.yaml
✅ Files skipped from review due to trivial changes (1)
- ocp-manifests-input/default/capi-controllers-deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- capi-operator-manifests/default/manifests.yaml
|
|
||
| resources: | ||
| - ../../admission-policies/default | ||
| - capi-controllers-deployment.yaml |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Preserve CPU and memory limits in the generated Deployment.
Including capi-controllers-deployment.yaml routes it through manifests-gen/customizations.go (customizeDeployment), which clears container.Resources.Limits. The generated default profile therefore omits required CPU and memory limits.
As per path instructions, Kubernetes/OpenShift manifests must define resource limits on every container.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ocp-manifests-input/default/kustomization.yaml` at line 6, Update the
customization flow used by customizeDeployment in
manifests-gen/customizations.go so it preserves each container’s CPU and memory
resource limits instead of clearing container.Resources.Limits; ensure the
generated Deployment from capi-controllers-deployment.yaml retains limits for
every container.
Source: Path instructions
|
Scheduling tests matching the |
|
/retest-required |
|
/retest-required |
|
/retest |
|
@sg00dwin - CI looks sad, your changes look good. /approve Please ensure those two jobs pass once CI is happy again :) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: theobarberbany The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test e2e-azure-ovn-techpreview |
|
/test e2e-azure-ovn-techpreview |
|
@sg00dwin: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
|
/test e2e-azure-ovn-techpreview |
3 participants
Summary
adopt-existing: alwaysannotation for seamless ownership transfer on upgrade — no tombstone, no downtimeProblem
During cluster installation, CVO deploys the capi-controllers Deployment simultaneously with the capi-operator installer. The installer needs time to install provider CRDs (e.g.
AWSCluster,Cluster), but capi-controllers starts immediately and crashes because those CRDs don't exist yet. Both containers (capi-controllersandmachine-api-migration) exit withos.Exit(1)and restart ~10 times before the CRDs are eventually available.Solution
The operator's Boxcutter installer uses probe-gated sequencing — components at a higher installOrder only deploy after earlier components are confirmed ready. By moving the Deployment to installOrder 30 (after core CRDs at 10 and providers at 20), the controllers only start once all required CRDs exist.
No changes to controller logic. The fix is purely deployment ordering.
Upgrade path (adopt-existing)
The Deployment uses the
capi-operator.openshift.io/adopt-existing: "always"annotation, which tells Boxcutter to adopt the existing CVO-managed Deployment in-place via Server-Side Apply with ForceOwnership. No tombstone is needed — CVO simply stops managing the Deployment when the file is removed frommanifests/, and Boxcutter patches it in-place. No delete, no gap, no pod restarts during ownership transfer.On fresh install, the annotation is a no-op — Boxcutter creates the Deployment normally.
Self-image-ref
The
--self-image-refflag is needed because release tooling rewrites image references in CVO manifests (manifests/), but not in manifests embedded inside the operator image (capi-operator-manifests/). Without it, the Deployment would be created with the build-time CI placeholder image, which doesn't exist in production. At runtime, the operator replaces the placeholder with the actual image digest from the pod spec.Test utility change
The
LoadAdmissionPolicyProfiles()function now includes the Deployment in its kustomize output. A type filter was added to only keepValidatingAdmissionPolicyandValidatingAdmissionPolicyBindingobjects, so the Deployment doesn't interfere with admission policy tests.Changes
ocp-manifests-input/default/capi-controllers-deployment.yamlocp-manifests-input/default/kustomization.yamladmission-policies/default/kustomization.yamlMakefile--kustomize-diratocp-manifests-input/default; added--self-image-refcapi-operator-manifests/default/manifests.yamlcapi-operator-manifests/default/metadata.yamlmanifests/0000_30_cluster-api_17_deployment.yamlpkg/admissionpolicy/testutils/util.goAssisted-by: Claude (Opus 4.6)
Summary by CodeRabbit
New Features
Bug Fixes
Chores