OCPCLOUD-3557: split capi-controllers and machine-api-migration - #622
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@stefanonardo: This pull request references OCPCLOUD-3557 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change separates ChangesMachine API migration separation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant machine-api-migration
participant machine-api-migration-metrics
participant ServiceMonitor
machine-api-migration->>machine-api-migration-metrics: Expose HTTPS metrics on port 8443
ServiceMonitor->>machine-api-migration-metrics: Select migration metrics Service
ServiceMonitor->>machine-api-migration-metrics: Scrape metrics with TLS
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-aws-capi-techpreview |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/rbac.md`:
- Around line 41-43: The RBAC summary for the `machine-api-migration` Role in
`openshift-cluster-api` is mislabeled as “(read)” even though the manifest
grants both read and write verbs. Update the description in `docs/rbac.md` to
reflect the actual permissions from
`0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml`, using the
`machine-api-migration` Role entry and its Machine/MachineSet permissions as the
reference point.
In `@manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml`:
- Around line 26-69: The machine-api-migration Deployment container spec is
missing required hardening and health settings. Update the machine-api-migration
pod/container spec to add an explicit securityContext with
readOnlyRootFilesystem, allowPrivilegeEscalation disabled, and capabilities
dropping all, and set automountServiceAccountToken to false if the controller
does not need the token. Also add resource limits alongside the existing
requests, and define livenessProbe and readinessProbe for the healthz endpoint
exposed by the machine-api-migration container on port 9440.
🪄 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: af553a64-1e21-4c5f-92f4-9de4822110af
📒 Files selected for processing (12)
docs/rbac.mdmanifests/0000_30_cluster-api_02_machine-api-migration-service-account.yamlmanifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yamlmanifests/0000_30_cluster-api_03_rbac_roles.yamlmanifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yamlmanifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yamlmanifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yamlmanifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yamlmanifests/0000_30_cluster-api_10_metrics-service.yamlmanifests/0000_30_cluster-api_12_allow-ingress-to-metrics-operators.yamlmanifests/0000_30_cluster-api_17_deployment.yamlmanifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml
💤 Files with no reviewable changes (1)
- manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml (1)
12-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove the ineffective
targetPortmapping for this headless Service.With
clusterIP: None, Kubernetes ignorestargetPort; the Service port must already match the pod’s listening port. Keep the migration listener on8443and omittargetPort, or use a non-headless Service if named port remapping is required. (kubernetes.io)Suggested cleanup
- name: machine-api-migration-metrics port: 8443 - targetPort: diagnostics🤖 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 `@manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml` around lines 12 - 20, Update the Service definition for machine-api-migration-metrics by removing the targetPort mapping while retaining port 8443 and clusterIP: None, so the headless Service uses the pod’s listening port directly.
🤖 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 `@manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml`:
- Around line 54-55: Remove the control-plane nodeSelector entry from the
Deployment manifest, leaving only topology-neutral scheduling rules so the
workload can schedule on HyperShift.
In `@ocp-manifests-input/default/capi-controllers-deployment.yaml`:
- Around line 39-42: Update the generated capi-operator manifests.yaml to
replace the outdated diagnostics-o and healthz-o port references with the
renamed diagnostics and healthz names, matching the source deployment manifest
while preserving the existing port configuration.
---
Nitpick comments:
In `@manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml`:
- Around line 12-20: Update the Service definition for
machine-api-migration-metrics by removing the targetPort mapping while retaining
port 8443 and clusterIP: None, so the headless Service uses the pod’s listening
port directly.
🪄 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: 128a92f8-b856-4904-bd2c-21119a1479de
📒 Files selected for processing (12)
docs/rbac.mdmanifests/0000_30_cluster-api_02_machine-api-migration-service-account.yamlmanifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yamlmanifests/0000_30_cluster-api_03_rbac_roles.yamlmanifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yamlmanifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yamlmanifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yamlmanifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yamlmanifests/0000_30_cluster-api_10_metrics-service.yamlmanifests/0000_30_cluster-api_12_allow-ingress-to-metrics-operators.yamlmanifests/0000_30_cluster-api_17_machine-api-migration-deployment.yamlocp-manifests-input/default/capi-controllers-deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- manifests/0000_30_cluster-api_02_machine-api-migration-service-account.yaml
- manifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yaml
- manifests/0000_30_cluster-api_10_metrics-service.yaml
- manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml
- manifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yaml
- docs/rbac.md
- manifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml
- manifests/0000_30_cluster-api_03_rbac_roles.yaml
| nodeSelector: | ||
| node-role.kubernetes.io/control-plane: "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the control-plane node selector.
This hard requirement prevents the Deployment from scheduling on HyperShift, where these nodes are not available to this workload. Keep only topology-neutral scheduling rules unless this manifest is conditionally excluded for HyperShift.
Proposed fix
- nodeSelector:
- node-role.kubernetes.io/control-plane: ""As per coding guidelines: “nodeSelector/affinity targeting control-plane nodes (breaks on HyperShift).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nodeSelector: | |
| node-role.kubernetes.io/control-plane: "" |
🤖 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 `@manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml`
around lines 54 - 55, Remove the control-plane nodeSelector entry from the
Deployment manifest, leaving only topology-neutral scheduling rules so the
workload can schedule on HyperShift.
Source: Coding guidelines
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 `@manifests/0000_30_cluster-api_14_allow-egress-operators.yaml`:
- Line 29: Replace the unrestricted egress rule in the machine-api-migration
egress policy with a port-based rule that allows only Kubernetes API server
access. Find the `- {}` entry in the machine-api-migration egress rules and
replace it with the same API-server port rule used by the compatibility operator
elsewhere in this manifest to ensure consistent and restrictive egress control.
🪄 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: 3d068657-10d3-4b28-8fa1-d67934bd8400
📒 Files selected for processing (5)
cmd/capi-controllers/main.godocs/rbac.mdmanifests/0000_30_cluster-api_03_rbac_roles.yamlmanifests/0000_30_cluster-api_04_rbac_bindings.yamlmanifests/0000_30_cluster-api_14_allow-egress-operators.yaml
💤 Files with no reviewable changes (3)
- cmd/capi-controllers/main.go
- manifests/0000_30_cluster-api_04_rbac_bindings.yaml
- manifests/0000_30_cluster-api_03_rbac_roles.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/rbac.md
| | `0000_30_cluster-api_03_rbac_roles.yaml` | Role `capi-controllers` | `openshift-cluster-api` | CAPI Cluster + infra cluster resources, secrets, pod self-read, events, leases | | ||
| | `0000_30_cluster-api_03_rbac_roles.yaml` | Role `capi-controllers` | `openshift-machine-api` | MAPI machines (read-only for InfraCluster), controlplanemachinesets (InfraCluster), secrets (read-only) | | ||
| | `0000_30_cluster-api_03_rbac_roles.yaml` | Role `capi-controllers-kube-system` | `kube-system` | Secrets (vSphere credentials) | | ||
| | `0000_30_cluster-api_03_rbac_roles.yaml` | Role `cluster-capi-operator-pull-secret` | `openshift-config` | Pull-secret read | |
There was a problem hiding this comment.
cluster-capi-operator-pull-secret role and related resources removed. I can't find them used anywhere.
They were added in a61882d#diff-b259f954d26aaa43aa09f0fbc1c7af8f00d361b3122c70a2052b09ea91e5aaccR45
14d1bad to
0421858
Compare
|
/test ci/prow/unit /pipeline required |
|
Scheduling tests matching the |
|
/test unit |
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-aws-capi-techpreview e2e-openstack-capi-techpreview |
|
/test e2e-aws-capi-techpreview e2e-openstack-capi-techpreview |
|
/test unit |
|
/test e2e-gcp-capi-techpreview |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
/approve
Everything looks good but the feature-gate refecences need changing
/hold
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" |
There was a problem hiding this comment.
I think this should be MachineAPIMigration
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" |
There was a problem hiding this comment.
I think this should be MachineAPIMigration
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" |
There was a problem hiding this comment.
I think this should be MachineAPIMigration
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" |
There was a problem hiding this comment.
I think this should be MachineAPIMigration
| @@ -0,0 +1,74 @@ | |||
| --- | |||
There was a problem hiding this comment.
Same for this file, I think this should be MachineAPIMigration
| @@ -0,0 +1,21 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Same for this file, I think this should be MachineAPIMigration
| @@ -0,0 +1,23 @@ | |||
| apiVersion: monitoring.coreos.com/v1 | |||
There was a problem hiding this comment.
Same for this file, I think this should be MachineAPIMigration
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" |
There was a problem hiding this comment.
I think this should be MachineAPIMigration
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" |
There was a problem hiding this comment.
I think this should be MachineAPIMigration
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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
Separate the two binaries into independent Deployments with dedicated ServiceAccounts and least-privilege RBAC. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/pipeline required |
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest |
1 similar comment
|
/retest |
|
Disconnected job passed but failed deprovisioning (probably a flake), let's try once more /retest |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest Let's try once again, there has been (DNS) quota issue resolved recently which might help with disconnected DNS stack issue |
|
/override ci/prow/e2e-gcp-ovn-techpreview The failures there are unrelated to the PR itself (which works correctly also on GCP as shown by e2e-gcp-capi-techpreview), but are instead believed to be caused by:
|
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-gcp-ovn-techpreview 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 kubernetes-sigs/prow repository. |
|
@stefanonardo: The following test 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. |
|
Analyzing the CI runs here I can explicitly see the splitting has happened correctly: and that e2es are solidly passing /verified by @damdo |
openshift-ci-robot
added
the
verified
|
@damdo: This PR has been marked as verified by 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. |
5eb434a
into
openshift:main
6 participants
Summary
capi-controllersDeployment (two containers sharing one SA) into two independent Deployments with dedicated ServiceAccounts and least-privilege RBAC:8443(matching all other binaries)Test plan
make buildpassesmake lintpasses (0 issues)make unitpasses (pre-existing crdcompatibility failures excluded)audit2rbacon CI job audit logs to confirm zero 403s for both SAs🤖 Generated with Claude Code
Summary by CodeRabbit