OCPBUGS-63320: Fix MAPI->CAPI conversion not adding ControlPlane label #399
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@RadekManak: This pull request references Jira Issue OCPBUGS-63320, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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
|
| Cohort / File(s) | Change Summary |
|---|---|
Tests: authoritative API switching & LB preservation pkg/controllers/machinesync/aws_test.go |
Added encoding/json import; AWSCluster builder now uses region "us-east-1"; added test "should preserve load balancers when toggling authoritativeAPI MAPI → CAPI → MAPI" that creates an AWSCluster with two LBs and a MAPI master Machine (authority=MAPI) with LB providerSpec, verifies MAPI→CAPI sync, captures original providerSpec JSON, transitions authority to CAPI and back to MAPI via helper while verifying syncs, and asserts final providerSpec equals original. The test block appears duplicated. |
Tests: helper for AuthoritativeAPI transitions pkg/controllers/machinesync/test_helpers_test.go |
New internal test helper transitionToAuthoritativeAPI added. It updates a Machine's Spec.AuthoritativeAPI and drives/asserts Status.AuthoritativeAPI through Migrating → target using client Get/Update/Status().Update and Eventually assertions with timeouts; intended for machinesync tests. |
Conversion: MAPI → CAPI label mapping pkg/conversion/mapi2capi/util.go |
convertMAPILabelsToCAPI now injects clusterv1.MachineControlPlaneLabel (empty value) when MAPI label machine.openshift.io/cluster-api-machine-role == "master". Other label conversion behavior unchanged. |
Sequence Diagram(s)
sequenceDiagram
autonumber
actor Test as TestRunner
participant MAPI as Machine API controller
participant CAPI as Cluster API controller
participant AWS as AWSCluster / Provider
Note over Test,AWS: Setup AWSCluster (region us-east-1) with 2 LBs and MAPI master Machine
Test->>MAPI: create Machine (authority=MAPI) with LB providerSpec
MAPI->>CAPI: sync -> create/update CAPI Machine
CAPI->>AWS: reconcile provider resources (LBs)
Test->>Test: capture original providerSpec (JSON)
Test->>MAPI: transitionToAuthoritativeAPI(target=CAPI)
MAPI->>CAPI: status migrates -> CAPI authoritative
CAPI->>MAPI: sync (authority=CAPI)
Note over AWS: LBs unchanged
Test->>MAPI: transitionToAuthoritativeAPI(target=MAPI)
CAPI->>MAPI: status migrates -> MAPI authoritative
MAPI->>CAPI: sync (authority=MAPI)
Test->>Test: compare final providerSpec to original -> equal
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Inspect duplicated test block in
pkg/controllers/machinesync/aws_test.go— remove or justify duplication. - Validate
transitionToAuthoritativeAPIsequencing and eventual assertion timeouts for flakiness. - Verify JSON providerSpec capture/compare handles non-semantic differences (field ordering, defaults).
- Check added
MachineControlPlaneLabelinjection for any downstream assumptions.
Poem
🐰 I nudged the API to and fro,
Switched the master label just so,
LBs steady, not a blip,
Specs matched after every flip,
A happy hop — commit and go!
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title directly references the main change: adding the ControlPlane label during MAPI->CAPI conversion, which aligns with the key code modification in util.go that injects the clusterv1.MachineControlPlaneLabel. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
pkg/controllers/machinesync/aws_test.go(3 hunks)pkg/controllers/machinesync/test_helpers_test.go(1 hunks)pkg/conversion/mapi2capi/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controllers/machinesync/test_helpers_test.go
- pkg/conversion/mapi2capi/util.go
🔇 Additional comments (3)
pkg/controllers/machinesync/aws_test.go (3)
21-21: LGTM!The
encoding/jsonimport is necessary for unmarshaling provider specs in the new round-trip test.
95-95: LGTM!Explicitly setting the region makes the test setup more consistent with the reconciler's Infrastructure configuration (line 125) and improves test clarity.
296-374: No issues requiring changes.The verification confirms this test is appropriately written and placed. The
transitionToAuthoritativeAPIhelper is defined locally in the test file, the test logic correctly validates load balancer preservation across authority transitions, and there is no problematic duplication. While e2e helpers with similar names exist ine2e/machine_migration_helpers.go, they serve a different context (e2e testing) and do not conflict with this unit test. The AI summary's claim about a duplicate test block is incorrect—the test appears only once.
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 golangci-lint (2.5.0)
Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
Comment @coderabbitai help to get the list of available commands and usage tips.
openshift-ci-robot
added
jira/valid-bug
|
@RadekManak: This pull request references Jira Issue OCPBUGS-63320, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/conversion/mapi2capi/util.go (1)
39-46: Also treat machine type “master” as control-plane (optional).Some MAPI objects may set only the type label. Consider labeling control-plane when either role OR type is “master.”
Apply this focused tweak:
- if mapiLabelKey == "machine.openshift.io/cluster-api-machine-role" && mapiLabelVal == "master" { + if (mapiLabelKey == "machine.openshift.io/cluster-api-machine-role" || + mapiLabelKey == "machine.openshift.io/cluster-api-machine-type") && + mapiLabelVal == "master" { capiLabels[clusterv1.MachineControlPlaneLabel] = "" }If there are known cases where only the role label is guaranteed, feel free to skip this.
Also applies to: 53-56
pkg/controllers/machinesync/aws_test.go (1)
316-394: Harden the round‑trip test: assert control-plane label and compare LBs order‑insensitively.Add a check that the CAPI Machine carries the control-plane label (the core behavior this PR targets). Also make the LB comparison ignore order to prevent brittle failures if serialization order changes.
Apply this diff:
@@ By("Verifying MAPI -> CAPI sync is successful") Eventually(k.Object(mapiMachine), timeout).Should( @@ ) + By("Asserting the created CAPI Machine has the control-plane label") + capiMachine := clusterv1resourcebuilder.Machine(). + WithNamespace(capiNamespace.GetName()). + WithName(mapiMachine.GetName()). + Build() + Eventually(func() map[string]string { + _ = k8sClient.Get(ctx, client.ObjectKeyFromObject(capiMachine), capiMachine) + return capiMachine.GetLabels() + }, timeout).Should(HaveKeyWithValue(clusterv1.MachineControlPlaneLabel, "")) + By("Capturing original providerSpec") Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mapiMachine), mapiMachine)).To(Succeed()) originalProviderSpec := mapiMachine.Spec.ProviderSpec.Value.DeepCopy() @@ - var originalSpec, finalSpec mapiv1beta1.AWSMachineProviderConfig + var originalSpec, finalSpec mapiv1beta1.AWSMachineProviderConfig Expect(json.Unmarshal(originalProviderSpec.Raw, &originalSpec)).To(Succeed()) Expect(json.Unmarshal(mapiMachine.Spec.ProviderSpec.Value.Raw, &finalSpec)).To(Succeed()) - Expect(finalSpec.LoadBalancers).To(Equal(originalSpec.LoadBalancers), "load balancers should not change after round trip") + // Order-insensitive comparison to avoid brittle failures on serialization order. + Expect(finalSpec.LoadBalancers).To(ConsistOf(originalSpec.LoadBalancers), "load balancers should not change after round trip")If you prefer strict order equality, keep Equal; otherwise ConsistOf is safer for CI stability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/controllers/machinesync/aws_test.go(4 hunks)pkg/conversion/mapi2capi/util.go(1 hunks)
🔇 Additional comments (4)
pkg/conversion/mapi2capi/util.go (1)
53-56: Correctly adds CAPI control-plane label for MAPI master role.This ensures CAPI controllers recognize control-plane Machines. Looks good.
pkg/controllers/machinesync/aws_test.go (3)
21-21: Import usage is appropriate.encoding/json is used to safely compare provider specs.
86-104: Helper to toggle AuthoritativeAPI is clear and reliable.Spec/status updates wrapped in Eventually reduce flakiness. Nice.
115-116: Explicit AWS region improves determinism.Setting us-east-1 in the builder avoids implicit defaults differing across environments.
RadekManak
force-pushed
the
fix-missing-cp-label
branch
from
1b41171 to
0e90dd1
Compare
|
@RadekManak: This pull request references Jira Issue OCPBUGS-63320, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/conversion/mapi2capi/util.go (3)
53-56: Add a focused unit test for label injection.Please add a unit test asserting that a MAPI machine with role "master" yields a CAPI label set containing clusterv1.MachineControlPlaneLabel with value "" after conversion.
I can sketch the test if helpful.
39-46: De-duplicate the MAPI role label key and avoid magic strings.Extract the key into a local const and reuse it in both the transform map and the conditional.
Apply this diff:
func convertMAPILabelsToCAPI(mapiLabels map[string]string) map[string]string { capiLabels := make(map[string]string) + const mapiMachineRoleLabel = "machine.openshift.io/cluster-api-machine-role" + toTransformLabels := map[string]func(string) (string, string){ "machine.openshift.io/cluster-api-machine-type": func(mapiLabelValue string) (string, string) { return fmt.Sprintf("%s/%s", clusterv1.NodeRoleLabelPrefix, mapiLabelValue), "" }, - "machine.openshift.io/cluster-api-machine-role": func(mapiLabelValue string) (string, string) { + mapiMachineRoleLabel: func(mapiLabelValue string) (string, string) { return fmt.Sprintf("%s/%s", clusterv1.NodeRoleLabelPrefix, mapiLabelValue), "" }, } @@ - if mapiLabelKey == "machine.openshift.io/cluster-api-machine-role" && mapiLabelVal == "master" { + if mapiLabelKey == mapiMachineRoleLabel && mapiLabelVal == "master" { capiLabels[clusterv1.MachineControlPlaneLabel] = "" }Also applies to: 53-56
36-38: Optional: return nil for zero-length input for parity with annotations helper.convertMAPIAnnotationsToCAPI returns nil when input is empty; consider mirroring that here for consistency.
Apply this diff:
func convertMAPILabelsToCAPI(mapiLabels map[string]string) map[string]string { - capiLabels := make(map[string]string) + if len(mapiLabels) == 0 { + return nil + } + capiLabels := make(map[string]string)pkg/controllers/machinesync/aws_test.go (2)
86-107: Helper is fine; consider using komega UpdateStatus to reduce boilerplate.Not required, but switching to k.UpdateStatus for the two status transitions would make this more concise and consistent with other updates in the file.
319-397: Strengthen the round-trip test: assert control-plane label and avoid order-sensitivity in LB check.
- Add an assertion that the created CAPI Machine has clusterv1.MachineControlPlaneLabel set (this PR’s core fix).
- Compare load balancers ignoring order to avoid brittle failures.
Apply this diff:
@@ By("Verifying MAPI -> CAPI sync is successful") Eventually(k.Object(mapiMachine), timeout).Should( @@ ) + By("Asserting CAPI Machine has control-plane label") + capiMachine := clusterv1resourcebuilder.Machine(). + WithNamespace(capiNamespace.GetName()). + WithName(mapiMachine.GetName()). + Build() + Eventually(k8sClient.Get(ctx, client.ObjectKeyFromObject(capiMachine), capiMachine), timeout).Should(Succeed()) + Expect(capiMachine.Labels).To(HaveKeyWithValue(clusterv1.MachineControlPlaneLabel, "")) @@ - By("Verifying load balancers are unchanged after round trip") + By("Verifying load balancers are unchanged after round trip (order-insensitive)") Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mapiMachine), mapiMachine)).To(Succeed()) var originalSpec, finalSpec mapiv1beta1.AWSMachineProviderConfig Expect(json.Unmarshal(originalProviderSpec.Raw, &originalSpec)).To(Succeed()) Expect(json.Unmarshal(mapiMachine.Spec.ProviderSpec.Value.Raw, &finalSpec)).To(Succeed()) - Expect(finalSpec.LoadBalancers).To(Equal(originalSpec.LoadBalancers), "load balancers should not change after round trip") + Expect(finalSpec.LoadBalancers).To(ConsistOf(originalSpec.LoadBalancers), "load balancers should not change after round trip")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/controllers/machinesync/aws_test.go(4 hunks)pkg/conversion/mapi2capi/util.go(1 hunks)
🔇 Additional comments (1)
pkg/conversion/mapi2capi/util.go (1)
53-56: Fix is correct: adds CAPI control-plane label for master role.This implements the expected mapping so CAPI controllers classify CP machines correctly. Looks good.
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
/approve
Looks reasonable thanks!
I left a comment that I think we can address
| // switchAuthoritativeAPI simulates behavior of migration controller | ||
| switchAuthoritativeAPI := func(machine *mapiv1beta1.Machine, targetAPI mapiv1beta1.MachineAuthority) { | ||
| Eventually(k.Update(machine, func() { | ||
| machine.Spec.AuthoritativeAPI = targetAPI | ||
| }), timeout).Should(Succeed()) | ||
| Eventually(func() error { | ||
| if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { | ||
| return err | ||
| } | ||
| machine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMigrating | ||
|
|
||
| return k8sClient.Status().Update(ctx, machine) | ||
| }, timeout).Should(Succeed()) | ||
| Eventually(func() error { | ||
| if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { | ||
| return err | ||
| } | ||
| machine.Status.AuthoritativeAPI = targetAPI | ||
|
|
||
| return k8sClient.Status().Update(ctx, machine) | ||
| }, timeout).Should(Succeed()) | ||
| } | ||
|
|
There was a problem hiding this comment.
It may be worth putting this in the test framework. I know QE has also developed some of these utils, worth checking we aren't duping. And that if we are not, if we can put this close to theirs.
Also I'd call this transitionToAuthoritativeAPI() (as it transitions through Migrating before landing to the new API. Also I'd check in a pre-condition that the spec and status aren't already matching the targetAPI before calling the Update.
There was a problem hiding this comment.
FYI:
There was a problem hiding this comment.
Created the function for transitionToAuthoritativeAPI. The updateMachineAuthoritativeAPI function has use in e2e. The new transitionToAuthoritativeAPI also emulates the status update of migration controller for tests.
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
RadekManak
force-pushed
the
fix-missing-cp-label
branch
from
0e90dd1 to
fdc5965
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/machinesync/test_helpers_test.go (1)
30-62: Let the helper accept allEventuallyargumentsRight now the helper takes a single
interface{}for timing, so callers can’t pass both timeout and polling interval like they could when callingEventuallydirectly. Switching this parameter to a vararg slice keeps the helper just as flexible as native Gomega helpers.-func transitionToAuthoritativeAPI(ctx context.Context, k8sClient client.Client, k komega.Komega, machine *mapiv1beta1.Machine, targetAPI mapiv1beta1.MachineAuthority, timeout interface{}) { +func transitionToAuthoritativeAPI(ctx context.Context, k8sClient client.Client, k komega.Komega, machine *mapiv1beta1.Machine, targetAPI mapiv1beta1.MachineAuthority, eventuallyArgs ...interface{}) { @@ - Eventually(k.Update(machine, func() { + Eventually(k.Update(machine, func() { machine.Spec.AuthoritativeAPI = targetAPI - }), timeout).Should(Succeed()) + }), eventuallyArgs...).Should(Succeed()) @@ - Eventually(func() error { + Eventually(func() error { if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { return err } machine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMigrating return k8sClient.Status().Update(ctx, machine) - }, timeout).Should(Succeed()) + }, eventuallyArgs...).Should(Succeed()) @@ - Eventually(func() error { + Eventually(func() error { if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(machine), machine); err != nil { return err } machine.Status.AuthoritativeAPI = targetAPI return k8sClient.Status().Update(ctx, machine) - }, timeout).Should(Succeed()) + }, eventuallyArgs...).Should(Succeed())
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
pkg/controllers/machinesync/aws_test.go(3 hunks)pkg/controllers/machinesync/test_helpers_test.go(1 hunks)pkg/conversion/mapi2capi/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/conversion/mapi2capi/util.go
RadekManak
force-pushed
the
fix-missing-cp-label
branch
from
fdc5965 to
be0ddb6
Compare
|
@RadekManak: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
3 participants
When converting control plane machine to CAPI we need to add
"cluster.x-k8s.io/control-plane": ""label to have it correctly recognized as control plane machine by CAPI controllers and when converting back to MAPI.This PR updates the label conversion function to add the label when MAPI machine has master role label.
Test is added that converts AWS control plane MAPI->CAPI->CAPI. And verifies it still has load balancers.
Summary by CodeRabbit
Tests
Bug Fixes