OCPCLOUD-2899: Support converting taints between MAPI and CAPI#567
Conversation
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@nrb: This pull request references OCPCLOUD-2899 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 task 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. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughThis PR implements symmetric taint conversion between CAPI and MAPI Machine representations. It removes a module replace directive, adds bidirectional conversion helpers, removes spec.taints from unsupported validation, implements CAPI↔MAPI machine conversion with tests, extends MachineSet conversion, and normalizes the fuzzer to align with conversion expectations. ChangesBidirectional Taint Conversion
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@pkg/conversion/capi2mapi/machine.go`:
- Around line 66-67: Detect and reject any CAPI taint with Propagation ==
OnInitialization instead of silently converting it to an Always taint: add
validation before or inside convertCAPITaintsToMAPITaints (or create
validateCAPITaintPropagation called from the caller that currently passes
capiMachine.Spec.Taints) to iterate capiMachine.Spec.Taints, if any
taint.Propagation == OnInitialization return an explicit error naming the
offending taint (key/effect/propagation) so the caller can fail conversion; do
not map OnInitialization to an Always taint or drop propagation silently. Ensure
the conversion call site propagates this error up instead of continuing.
🪄 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: 96e16cba-e3d1-43a4-99c5-0355bcc1384f
⛔ Files ignored due to path filters (24)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta1/cluster_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta1/conversion.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta1/machine_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta1/zz_generated.conversion.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/sigs.k8s.io/cluster-api/api/core/v1beta1/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/cluster_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/clusterclass_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/common_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/machine_phase_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/machine_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/machinehealthcheck_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/machineset_types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/v1beta1_condition_consts.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/sigs.k8s.io/cluster-api/feature/feature.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/internal/contract/controlplane.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/util/conditions/sort.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/util/util.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api/util/version/version.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (9)
go.modpkg/conversion/capi2mapi/common.gopkg/conversion/capi2mapi/machine.gopkg/conversion/capi2mapi/machine_test.gopkg/conversion/mapi2capi/machine.gopkg/conversion/mapi2capi/machine_test.gopkg/conversion/mapi2capi/machineset_test.gopkg/conversion/mapi2capi/util.gopkg/conversion/test/fuzz/fuzz.go
💤 Files with no reviewable changes (1)
- go.mod
| Taints: convertCAPITaintsToMAPITaints(capiMachine.Spec.Taints), | ||
| // ProviderSpec: // ProviderSpec MUST NOT be populated here. It is added later by higher level fuctions. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cluster API v1.12.3, is MachineTaintPropagationOnInitialization semantically distinct from MachineTaintPropagationAlways for Machine.spec.taints reconciliation?
💡 Result:
Yes, in Cluster API v1.12.3, MachineTaintPropagationOnInitialization and MachineTaintPropagationAlways are semantically distinct regarding the reconciliation of taints on a node [1]. The distinction lies in how the Cluster API controller manages the lifecycle of the taint after its initial application [1]: - MachineTaintPropagationAlways: The controller continuously reconciles the taint [1]. This ensures that the taint remains on the node if it is currently defined on the Machine [1]. If the taint is manually removed from the node, the controller will re-add it during the next reconciliation loop [1]. Similarly, if the taint is removed from the Machine spec, the controller will remove it from the node [1]. - MachineTaintPropagationOnInitialization: The controller ensures the taint is applied to the node only once during the initialization phase [1]. After this initial application, the controller ceases to manage the taint [1]. Consequently, if the taint is removed from the node, it will not be re-added by the controller, and if the taint is removed from the Machine spec, no action is taken on the node [1]. This behavior allows for different operational patterns, such as using OnInitialization for taints that are intended to be manually managed or removed after a one-time setup task (e.g., preparing a node with specific drivers) [1].
Citations:
- 1: https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20250513-propogate-taints.md
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
fd 'machine.go' | grep 'conversion/capi2mapi'Repository: openshift/cluster-capi-operator
Length of output: 111
🏁 Script executed:
# Once we find the file, let's view the lines in context
rg -A 15 -B 10 'convertCAPITaintsToMAPITaints' pkg/conversion/capi2mapi/machine.goRepository: openshift/cluster-capi-operator
Length of output: 1542
🏁 Script executed:
# Let's find and examine the convertCAPITaintsToMAPITaints function definition
rg -A 20 'func convertCAPITaintsToMAPITaints' --type goRepository: openshift/cluster-capi-operator
Length of output: 1371
🏁 Script executed:
# Check the MAPI Machine and Taint type definitions
fd 'types\.go|machine\.go' | xargs grep -l 'type Machine struct' | head -3Repository: openshift/cluster-capi-operator
Length of output: 270
🏁 Script executed:
# Check if MAPI taint type has a Propagation field
rg 'type.*Taint.*struct' --type go -A 10 | head -50Repository: openshift/cluster-capi-operator
Length of output: 5339
🏁 Script executed:
# View the full MachineTaint struct definition
rg -A 30 'type MachineTaint struct' vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/common_types.goRepository: openshift/cluster-capi-operator
Length of output: 1974
🏁 Script executed:
# Also check what values MachineTaintPropagation can have
rg 'MachineTaintPropagation' vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/ -A 2 -B 2Repository: openshift/cluster-capi-operator
Length of output: 3154
🏁 Script executed:
# Verify the location of the conversion call and its error handling context
cat -n pkg/conversion/capi2mapi/machine.go | sed -n '50,85p'Repository: openshift/cluster-capi-operator
Length of output: 2370
Reject CAPI taints with OnInitialization propagation that MAPI cannot represent.
The conversion at line 66 silently drops taint propagation information, transforming OnInitialization taints into continuously reconciled Always taints. MAPI/Kubernetes taints lack propagation semantics; CAPI's OnInitialization (apply once, then stop managing) and Always (continuously reconcile) are fundamentally different behaviors.
Validate taint propagation early and surface an error for unsupported values:
Proposed fix
func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string) (*mapiv1beta1.Machine, field.ErrorList) {
errs := field.ErrorList{}
+ for i, t := range capiMachine.Spec.Taints {
+ if t.Propagation != "" && t.Propagation != clusterv1.MachineTaintPropagationAlways {
+ errs = append(errs, field.Invalid(
+ field.NewPath("spec", "taints").Index(i).Child("propagation"),
+ t.Propagation,
+ "only Always taint propagation is supported for MAPI conversion",
+ ))
+ }
+ }
lifecycleHooks, capiMachineNonHookAnnotations := convertCAPILifecycleHookAnnotationsToMAPILifecycleHooksAndAnnotations(capiMachine.Annotations)🤖 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 `@pkg/conversion/capi2mapi/machine.go` around lines 66 - 67, Detect and reject
any CAPI taint with Propagation == OnInitialization instead of silently
converting it to an Always taint: add validation before or inside
convertCAPITaintsToMAPITaints (or create validateCAPITaintPropagation called
from the caller that currently passes capiMachine.Spec.Taints) to iterate
capiMachine.Spec.Taints, if any taint.Propagation == OnInitialization return an
explicit error naming the offending taint (key/effect/propagation) so the caller
can fail conversion; do not map OnInitialization to an Always taint or drop
propagation silently. Ensure the conversion call site propagates this error up
instead of continuing.
|
@nrb: all tests passed! 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. |
👋 May I ask some questions:
I'm asking because the installer still uses |
Yeah, we'll need to turn that on.
It looks like that backport was made against the 1.13 series, not 1.12? I don't see any mention of a git backport in kubernetes-sigs/cluster-api#13455. IMO we should try to move to v1beta2 if it's not too painful...but given the delay on CAPA, it might be. |
Arghh, sorry, I didn't look at the PR date... 🤦 Anywayy, +1 to use v1beta2! On the installer side, we did move to generate CAPI manifests with |
3 participants
This work relies on having CAPI v1.12, though it doesn't look like bumped yet, possibly because of the upstream CAPA bump (kubernetes-sigs/cluster-api-provider-aws#5857) being in progress.
Summary by CodeRabbit
New Features
Tests
Chores