NO-JIRA: conversion: CAPI2MAPI set openshift labels for zone, region and instance-type #393
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chrischdi: This pull request explicitly references no jira issue. 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. |
WalkthroughAdds a constants package for Machine metadata keys; extends label/annotation conversion to accept and merge optional Machine-API metadata; provider converters (AWS, OpenStack, PowerVS) gained a flag to optionally populate and pass that metadata into machine conversion; provider fuzz tests refactored into per-provider fuzzer types. Changes
Sequence Diagram(s)sequenceDiagram
participant CAPI as CAPI Machine
participant ProviderConv as Provider Converter
participant MachineConv as fromCAPIMachineToMAPIMachine
participant LabelConv as convertCAPILabelsToMAPILabels
participant AnnConv as convertCAPIAnnotationsToMAPIAnnotations
participant MAPI as MAPI Machine
CAPI->>ProviderConv: From...TemplateAndCluster(...) -> ToMachine()
ProviderConv->>ProviderConv: excludeFlag = true (default)
alt excludeFlag == false
ProviderConv->>ProviderConv: extract instance-type/region/zone/state
ProviderConv->>ProviderConv: build additionalLabels/additionalAnnotations
end
ProviderConv->>MachineConv: fromCAPIMachineToMAPIMachine(capiMachine, additionalLabels, additionalAnnotations)
MachineConv->>LabelConv: convertCAPILabelsToMAPILabels(capiLabels, additionalLabels)
LabelConv-->>MachineConv: mergedLabels
MachineConv->>AnnConv: convertCAPIAnnotationsToMAPIAnnotations(capiAnns, additionalAnnotations)
AnnConv-->>MachineConv: mergedAnnotations
MachineConv->>MAPI: create MAPI Machine with mergedLabels/mergedAnnotations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration 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 (13)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (11)
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 Comment |
chrischdi
force-pushed
the
pr-machines-labels
branch
from
add1a98 to
e35c776
Compare
|
/retest-required |
|
Thanks @chrischdi this is great I noticed we don't set the |
pkg/conversion/capi2mapi/common.go
Outdated
| // TODO | ||
| // - machine.openshift.io/instance-state |
There was a problem hiding this comment.
Forgot to implement this todo!
Thanks, yeah looks like I added a TODO and did not implement it ... |
|
Have to take a look at the unit tests... |
![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 (1)
pkg/conversion/capi2mapi/machine.go (1)
37-37: Signature extension enables provider-specific label propagation.The updated signature and zone label injection correctly thread provider labels and instance state through the conversion flow. The
ptr.DerefonFailureDomainsafely handles nil.Optional: Defensive nil-map check.
Line 47 assigns to
additionalMachineAPILabelswithout verifying it's non-nil. While all current callers initialize the map, a defensive check would guard against future callers:+ if additionalMachineAPILabels == nil { + additionalMachineAPILabels = make(map[string]string) + } additionalMachineAPILabels["machine.openshift.io/zone"] = ptr.Deref(capiMachine.Spec.FailureDomain, "")Also applies to: 47-47, 53-54
📜 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 (6)
pkg/conversion/capi2mapi/aws.go(1 hunks)pkg/conversion/capi2mapi/common.go(4 hunks)pkg/conversion/capi2mapi/machine.go(2 hunks)pkg/conversion/capi2mapi/machineset.go(2 hunks)pkg/conversion/capi2mapi/openstack.go(2 hunks)pkg/conversion/capi2mapi/powervs.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/conversion/capi2mapi/openstack.go
🔇 Additional comments (5)
pkg/conversion/capi2mapi/powervs.go (1)
103-108: LGTM! Consistent label and state propagation.The implementation correctly constructs
additionalMachineAPILabelswith instance-type and region, and passes instance state to the conversion helper. The use ofptr.Dereffor the nullableStatus.Regionensures safe handling, with empty values filtered downstream by the logic incommon.go(lines 82-84).pkg/conversion/capi2mapi/machineset.go (1)
35-36: LGTM! Correct nil/empty values for MachineSet conversion.Passing
nilformachineAPILabelsand""forinstanceStateis appropriate here since MachineSet-level metadata lacks instance-specific provider details. The actual Machine template is enriched separately via the provider-specificToMachine()calls.Also applies to: 47-48
pkg/conversion/capi2mapi/aws.go (1)
224-229: LGTM! Proper label extraction and state handling.The AWS implementation correctly populates
additionalMachineAPILabelswith instance-type from the machine spec and region from the cluster spec, and safely extracts instance state withptr.Deref. This aligns with the PowerVS pattern and ensures consistent label propagation across providers.pkg/conversion/capi2mapi/common.go (2)
53-87: Correct label merge with proper empty-value filtering.The extended
convertCAPILabelsToMAPILabelsproperly merges provider-supplied labels while filtering empty values (lines 82-84) to avoid overwriting existing labels with blanks. The merge order (CAPI labels first, then provider labels) allows provider-specific labels to take precedence when keys overlap.
136-136: Instance-state annotation injection is correct.The
convertCAPIAnnotationsToMAPIAnnotationsextension conditionally injectsmachine.openshift.io/instance-stateonly wheninstanceStateis non-empty, preventing unnecessary annotation clutter.Also applies to: 167-169
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/conversion/capi2mapi/common.go (1)
136-183: Critical: provider annotations dropped when CAPI annotations are empty.convertCAPIAnnotationsToMAPIAnnotations returns nil if capiAnnotations is empty, ignoring non-empty machineAPIAnnotations. This prevents annotations like machine.openshift.io/instance-state from being set.
Apply this fix:
-func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string { - if len(capiAnnotations) == 0 { - return nil - } +func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string { + if len(capiAnnotations) == 0 && len(machineAPIAnnotations) == 0 { + return nil + } mapiAnnotations := make(map[string]string) @@ - for k, v := range machineAPIAnnotations { - // Ignore empty labels to ensure to not overwrite potentially existing labels with empty values. + for k, v := range machineAPIAnnotations { + // Ignore empty annotations to avoid overwriting existing values with empty strings. if v == "" { continue } mapiAnnotations[k] = v } // On the original MAPI object some label fields are nil rather than empty. // So return nil instead to avoid unnecessary diff being picked up by the diff checker. if len(mapiAnnotations) == 0 { return nil } return mapiAnnotations }
🧹 Nitpick comments (14)
pkg/conversion/mapi2capi/util.go (2)
57-61: Correctly excluding MAPI‑specific labels.Filtering out keys with the "machine.openshift.io/" prefix here is right; the special role/type transforms run before the filter. Consider extracting the prefix into a shared const to avoid duplication with the annotations path.
+const mapiMachinePrefix = "machine.openshift.io/" ... - if strings.HasPrefix(mapiLabelKey, "machine.openshift.io/") { + if strings.HasPrefix(mapiLabelKey, mapiMachinePrefix) { continue }
82-85: Nit: comment says “labels” in annotations function.Update the comment to “annotations” for clarity.
- // Ignore MAPI-specific labels that are not explicitly handled. + // Ignore MAPI-specific annotations that are not explicitly handled.pkg/conversion/capi2mapi/aws.go (3)
52-56: Document the new exclusion flag’s intent.Add a brief field comment explaining when provider-derived MAPI labels/annotations should be excluded (e.g., templates) vs included (concrete machines).
type machineAndAWSMachineAndAWSCluster struct { machine *clusterv1.Machine awsMachine *awsv1.AWSMachine awsCluster *awsv1.AWSCluster - excludeMachineAPILabelsAndAnnotations bool + // When true, omit machine.openshift.io/* labels/annotations produced by provider data + // (e.g., for templates); when false, include them (e.g., for concrete Machines). + excludeMachineAPILabelsAndAnnotations bool }
88-90: Confirm exclusion on MachineSet path is intentional.Setting excludeMachineAPILabelsAndAnnotations=true for MachineSet conversion means instance-type/region/zone labels and instance-state annotation won’t appear on the template. Please confirm this matches product requirements; if so, add a comment here to lock the behavior.
- awsCluster: ac, - excludeMachineAPILabelsAndAnnotations: true, + awsCluster: ac, + // Templates should not carry provider-derived runtime labels/annotations. + excludeMachineAPILabelsAndAnnotations: true,
226-237: Downstream merge functions already filter empty values—suggested refactor is optional.The concern about empty-valued labels/annotations is valid but already handled. The
convertCAPILabelsToMAPILabelsandconvertCAPIAnnotationsToMAPIAnnotationsfunctions both explicitly skip empty values withif v == "" { continue }when processing themachineAPILabelsandmachineAPIAnnotationsparameters. TheadditionalMachineAPILabelsandadditionalMachineAPIAnnotationsmaps (created at lines 226–237 in aws.go) are passed to these merge functions, where empty strings are filtered out before merging into the final ObjectMeta.The suggested refactor to filter at the source (in aws.go) would be more defensive and make the intent more explicit, but it is not required for correctness. The current implementation is safe because the downstream filtering is intentional (as documented in the code comments).
pkg/conversion/mapi2capi/openstack_fuzz_test.go (1)
169-257: Unify slice element type: use either []any or []interface{} consistently.Both are fine in Go ≥1.18, but mixing them is noisy. Pick one for both functions (recommend []any).
-func (f *openstackProviderFuzzer) FuzzerFuncsMachineSet(codecs runtimeserializer.CodecFactory) []any { +func (f *openstackProviderFuzzer) FuzzerFuncsMachineSet(codecs runtimeserializer.CodecFactory) []any { return []any{ ... } } -func (f *openstackProviderFuzzer) FuzzerFuncsMachine(codecs runtimeserializer.CodecFactory) []interface{} { - return append( - f.FuzzerFuncsMachineSet(codecs), - f.FuzzMachine, - ) -} +func (f *openstackProviderFuzzer) FuzzerFuncsMachine(codecs runtimeserializer.CodecFactory) []any { + return append(f.FuzzerFuncsMachineSet(codecs), f.FuzzMachine) +}Also applies to: 259-265
pkg/conversion/mapi2capi/powervs_fuzz_test.go (1)
141-161: Simplify duplicate switch cases.All three branches set the same ID type. Collapse to a single branch or vary intentionally if different types are supported.
- switch c.Int31n(3) { - case 0: - *serviceInstance = mapiv1.PowerVSResource{Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0))} - case 1: - *serviceInstance = mapiv1.PowerVSResource{Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0))} - case 2: - *serviceInstance = mapiv1.PowerVSResource{Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0))} - } + *serviceInstance = mapiv1.PowerVSResource{ + Type: mapiv1.PowerVSResourceTypeID, + ID: ptr.To(c.String(0)), + }pkg/conversion/capi2mapi/machine.go (1)
36-38: Add brief doc for new parameters.Explain that additionalMachineAPILabels/Annotations inject provider-derived machine.openshift.io/* into ObjectMeta, and are ignored when nil.
-// fromCAPIMachineToMAPIMachine translates a core CAPI Machine to its MAPI Machine correspondent. +// fromCAPIMachineToMAPIMachine translates a core CAPI Machine to its MAPI Machine correspondent. +// additionalMachineAPILabels/Annotations allow callers (providers) to merge extra +// machine.openshift.io/* labels/annotations into the MAPI ObjectMeta; pass nil to omit. func fromCAPIMachineToMAPIMachine( - capiMachine *clusterv1.Machine, additionalMachineAPILabels, additionalMachineAPIAnnotations map[string]string, + capiMachine *clusterv1.Machine, + additionalMachineAPILabels, additionalMachineAPIAnnotations map[string]string, ) (*mapiv1beta1.Machine, field.ErrorList) {pkg/conversion/capi2mapi/openstack.go (1)
79-81: MachineSet path currently skips provider labels/annotations; confirm intent.Setting excludeMachineAPILabelsAndAnnotations to true prevents instance-type/zone labels from flowing into MachineSet templates via ToMachine(). If labels should also be present on MachineSets, flip this to false (instance-state stays filtered as empty).
- openstackCluster: ac, - excludeMachineAPILabelsAndAnnotations: true, + openstackCluster: ac, + excludeMachineAPILabelsAndAnnotations: false,pkg/conversion/test/fuzz/fuzz.go (4)
52-55: Typo in comment.“azunction” → “a function”.
-// CAPI2MAPIMachineConverterConstructor is azunction that constructs a CAPI to MAPI Machine converter. +// CAPI2MAPIMachineConverterConstructor is a function that constructs a CAPI to MAPI Machine converter.
646-656: Minor comment typo.“machineer the roundtrip” → “machine after the roundtrip”.
-// so we get the same MAPI machineer the roundtrip. +// so we get the same MAPI machine after the roundtrip.
309-311: Fix mismatch in .spec failure message diff.Show a diff of the compared specs (with providerSpec ignored), not ObjectMeta.
-Expect(mapiMachine.Spec).To(WithTransform(ignoreMachineProviderSpec, testutils.MatchViaJSON(ignoreMachineProviderSpec(in.machine.Spec))), fmt.Sprintf("converted MAPI machine should have matching .spec: %s", cmp.Diff(mapiMachine.ObjectMeta, in.machine.ObjectMeta))) +Expect(mapiMachine.Spec).To( + WithTransform(ignoreMachineProviderSpec, testutils.MatchViaJSON(ignoreMachineProviderSpec(in.machine.Spec))), + fmt.Sprintf( + "converted MAPI machine should have matching .spec: %s", + cmp.Diff(ignoreMachineProviderSpec(mapiMachine.Spec), ignoreMachineProviderSpec(in.machine.Spec)), + ), +)
867-870: No-op nil guards in phase fuzzers.Assigning a new pointer to the local parameter does not affect the caller; either remove the nil guard or set via return/
**string(not necessary for this PR).- if phase == nil { - phase = ptr.To("") - } + // If phase is nil, skip fuzzing; the pointer is optional and set elsewhere.Also applies to: 899-902
pkg/conversion/mapi2capi/aws_fuzz_test.go (1)
131-135: Remove redundant CredentialsSecret assignment.The first assignment is overwritten immediately. Keep the hardcoded value if that’s required for migration.
- // Only one value here is valid in terms of fuzzing, so it is hardcoded. - ps.CredentialsSecret = &corev1.LocalObjectReference{ - Name: mapi2capi.DefaultCredentialsSecretName, - } @@ - // TODO(OCPCLOUD-2713): remove this, temporarily hardcoded for AWS to make the migration to work. - ps.CredentialsSecret = &corev1.LocalObjectReference{Name: "aws-cloud-credentials"} + // TODO(OCPCLOUD-2713): remove this, temporarily hardcoded for AWS to make the migration to work. + ps.CredentialsSecret = &corev1.LocalObjectReference{Name: "aws-cloud-credentials"}Also applies to: 141-142
📜 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 (12)
pkg/controllers/machinesync/machine_sync_controller_test.go(1 hunks)pkg/conversion/capi2mapi/aws.go(3 hunks)pkg/conversion/capi2mapi/common.go(4 hunks)pkg/conversion/capi2mapi/machine.go(2 hunks)pkg/conversion/capi2mapi/machineset.go(2 hunks)pkg/conversion/capi2mapi/openstack.go(4 hunks)pkg/conversion/capi2mapi/powervs.go(3 hunks)pkg/conversion/mapi2capi/aws_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/openstack_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/powervs_fuzz_test.go(5 hunks)pkg/conversion/mapi2capi/util.go(3 hunks)pkg/conversion/test/fuzz/fuzz.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/conversion/capi2mapi/machineset.go
- pkg/conversion/capi2mapi/powervs.go
🔇 Additional comments (11)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
962-965: LGTM! Safer label handling pattern.The change from directly assigning a new map to checking for nil and initializing before setting the key is a defensive coding improvement. This preserves any existing labels while safely adding the test sentinel, and aligns with the PR's broader goal of safer label/annotation handling during CAPI↔MAPI conversion.
pkg/conversion/mapi2capi/openstack_fuzz_test.go (2)
69-70: Good move to a provider fuzzer struct.Wiring f.FuzzerFuncsMachine and f.FuzzerFuncsMachineSet keeps tests consistent with other providers.
Also applies to: 79-80, 94-95, 105-106
118-166: Provider spec fuzzing looks solid.Clearing unsupported fields and propagating Flavor/AvailabilityZone into the MAPIMachineFuzzer enables label assertions downstream.
pkg/conversion/mapi2capi/powervs_fuzz_test.go (2)
71-72: Consistent provider fuzzer integration.Good alignment with AWS/OpenStack test patterns.
Also applies to: 81-82, 96-97, 107-108
116-134: Provider config fuzzing looks correct.TypeMeta normalization and nil-ing empty secrets are appropriate; copying SystemType into f.InstanceType enables label checks.
pkg/conversion/capi2mapi/openstack.go (2)
207-220: Label/annotation mapping LGTM.Correct keys; empty values are filtered downstream. Region intentionally omitted for OpenStack.
222-222: Updated conversion call wiring looks good.Passing additional labels/annotations through fromCAPIMachineToMAPIMachine aligns with the common merge logic.
pkg/conversion/capi2mapi/common.go (2)
49-49: Call site update OK.New signature usage is correct with nil for extra labels in selector path.
53-96: Label merging logic looks solid.CAPI label filtering retained; provider labels merged with empty-value guard; nil returned for empty result to minimize diffs.
pkg/conversion/test/fuzz/fuzz.go (1)
606-656: New MAPIMachineFuzzer is good.Consistent label sync into spec.metadata; fields reset each run; nil-map handling correct.
pkg/conversion/mapi2capi/aws_fuzz_test.go (1)
81-83: Two Machine fuzzers are registered; confirm order.Both MAPIMachineFuzzerFuncs and f.FuzzerFuncsMachine register a Machine fuzzer. Order should ensure provider FuzzMachine runs last so labels from provider config are copied into spec.metadata.
Also applies to: 106-109
|
/retest |
chrischdi
force-pushed
the
pr-machines-labels
branch
from
3530312 to
2523e12
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/conversion/capi2mapi/common.go (1)
136-139: Critical: Fix asymmetric nil-checking that discards machineAPIAnnotations.The early return on line 137-139 checks only
capiAnnotations, discarding anymachineAPIAnnotationseven when the latter contains values. This bug was previously flagged and remains unfixed.Apply this fix to match the correct pattern in
convertCAPILabelsToMAPILabels(line 54):-func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string { - if len(capiAnnotations) == 0 { - return nil - } +func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string { + if len(capiAnnotations) == 0 && len(machineAPIAnnotations) == 0 { + return nil + }Based on past review comments.
pkg/conversion/test/fuzz/fuzz.go (1)
143-144: Fix diff output to match compared fields.The assertion compares ObjectMeta but the diff prints TypeMeta. Use ObjectMeta in cmp.Diff.
-Expect(capiMachine.ObjectMeta).To(Equal(in.machine.ObjectMeta), fmt.Sprintf("diff: %s\nENDDIFF", cmp.Diff(capiMachine.TypeMeta, in.machine.TypeMeta))) +Expect(capiMachine.ObjectMeta).To(Equal(in.machine.ObjectMeta), fmt.Sprintf("diff: %s\nENDDIFF", cmp.Diff(capiMachine.ObjectMeta, in.machine.ObjectMeta)))
🧹 Nitpick comments (3)
pkg/conversion/test/fuzz/fuzz.go (2)
52-52: Typo in comment.“azunction” → “a function”.
-// CAPI2MAPIMachineConverterConstructor is azunction that constructs a CAPI to MAPI Machine converter. +// CAPI2MAPIMachineConverterConstructor is a function that constructs a CAPI to MAPI Machine converter.
655-666: Comment nits for clarity/typos.Minor grammar/typo fixes.
-// So these should match when we generate the initial MAPI machine -// so we get the same MAPI machineer the roundtrip. +// So these should match when we generate the initial MAPI Machine +// so we get the same MAPI Machine after the roundtrip.pkg/conversion/mapi2capi/aws_fuzz_test.go (1)
113-115: Optional: align providerID region with cluster region for readability.Not functionally required, but reducing cognitive dissonance in test data helps future readers.
-return "aws:///us-west-2a/i-" + strings.ReplaceAll(c.String(0), "/", "") +return "aws:///us-east-1a/i-" + strings.ReplaceAll(c.String(0), "/", "")
📜 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 (12)
pkg/controllers/machinesync/machine_sync_controller_test.go(1 hunks)pkg/conversion/capi2mapi/aws.go(3 hunks)pkg/conversion/capi2mapi/common.go(4 hunks)pkg/conversion/capi2mapi/machine.go(2 hunks)pkg/conversion/capi2mapi/machineset.go(2 hunks)pkg/conversion/capi2mapi/openstack.go(4 hunks)pkg/conversion/capi2mapi/powervs.go(3 hunks)pkg/conversion/mapi2capi/aws_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/openstack_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/powervs_fuzz_test.go(5 hunks)pkg/conversion/mapi2capi/util.go(3 hunks)pkg/conversion/test/fuzz/fuzz.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/conversion/mapi2capi/util.go
🔇 Additional comments (13)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
962-965: LGTM! Defensive nil-check prevents potential panic.This nil-check ensures safe map assignment and follows Go best practices. Since the test machine is built without explicit labels, this safeguard prevents a panic when assigning to a potentially nil map.
pkg/conversion/capi2mapi/machineset.go (1)
35-36: LGTM: Correct nil propagation for template-level conversions.Passing
nilfor the additional labels/annotations parameters is appropriate here since MachineSet conversions operate at the template level and should not include provider-specific machine labels (instance-type, region, zone) or runtime state annotations.Also applies to: 47-48
pkg/conversion/capi2mapi/aws.go (2)
55-55: LGTM: Correct flag initialization for template vs. machine conversions.The
excludeMachineAPILabelsAndAnnotationsfield correctly distinguishes between:
- Machine conversions (default
false): include provider labels/annotations- MachineSet template conversions (
true): exclude provider labels/annotationsThis ensures instance-specific metadata is only added to actual Machines, not templates.
Also applies to: 89-89
263-274: Verification confirms empty-value filtering is properly implemented.The code safely handles empty values from
ptr.Derefcalls. BothconvertCAPILabelsToMAPILabels(lines 80-84) andconvertCAPIAnnotationsToMAPIAnnotations(lines 167-174) filter empty strings withif v == ""before merging the additional maps. This pattern applies consistently across all call paths, ensuring empty values don't propagate.pkg/conversion/capi2mapi/machine.go (1)
37-37: LGTM: Signature extended correctly to support additional labels/annotations.The updated function signature and call sites properly thread provider-specific labels and annotations through the conversion pipeline.
Also applies to: 51-52
pkg/conversion/capi2mapi/openstack.go (1)
207-220: LGTM: Region omission properly documented.The inline comment (lines 210-212) clearly explains why
machine.openshift.io/regionis not set for OpenStack—determining the region requires fetching identity resources. This is a reasonable limitation and is well-documented.pkg/conversion/capi2mapi/common.go (1)
53-96: LGTM: Label merge logic correctly handles nil inputs and empty values.The function properly:
- Checks both input maps for emptiness before proceeding
- Filters CAPI-specific labels
- Skips empty values in machineAPILabels to avoid overwriting
- Returns nil for empty results to maintain consistency
pkg/conversion/capi2mapi/powervs.go (1)
105-116: No change required — missing Region is handled defensively.The conversion uses ptr.Deref(m.powerVSMachine.Status.Region, "") so an unset Status.Region yields an empty label rather than a crash; labels can also be omitted via excludeMachineAPILabelsAndAnnotations (used in MachineSet paths). To guarantee non-empty region labels, populate Status.Region earlier in the provider reconciler.
pkg/conversion/mapi2capi/powervs_fuzz_test.go (2)
112-135: Good: provider→label linkage via embedded MAPIMachineFuzzer.Embedding MAPIMachineFuzzer and copying SystemType into InstanceType keeps labels and provider config in sync. Looks solid.
141-209: Fuzzer composition reads well.Combining FuzzerFuncsMachineSet with FuzzMachine is clear and reusable.
pkg/conversion/mapi2capi/openstack_fuzz_test.go (2)
114-167: Good provider spec sanitization and label sync.Cleaning unsupported fields and wiring Flavor/AvailabilityZone into the embedded fuzzer’s InstanceType/Zone is appropriate.
169-264: Nice split between MachineSet vs Machine fuzzers.The separation and reuse via append keeps tests maintainable.
pkg/conversion/mapi2capi/aws_fuzz_test.go (1)
121-166: Provider config cleanup and constraints look right.Region alignment with AWSCluster, ensuring a nil DeviceName mapping, and clearing unsupported fields are all good. Keep the TODO visible for secret hardcoding removal.
Please confirm when OCPCLOUD-2713 is resolved so the duplicate CredentialsSecret assignment can be dropped.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/conversion/test/fuzz/fuzz.go (1)
627-630: Remove duplicate FuzzMachine registration and field resets that discard provider fuzzer state.This issue was already identified in a previous review. The reset code at lines 627-630 discards
InstanceType,Region, andZonevalues set by provider fuzzers, preventing label propagation. Additionally, line 675 creates a duplicate fuzzer registration since provider-specificFuzzerFuncsMachineimplementations already appendf.FuzzMachine.Remove the reset statements and the duplicate registration to preserve provider-set fields and eliminate nondeterministic behavior.
As per the previous review, apply these changes:
Remove the reset code:
func (f *MAPIMachineFuzzer) FuzzMachine(m *mapiv1beta1.Machine, c randfill.Continue) { - // Reset the fuzzer - f.InstanceType = "" - f.Region = "" - f.Zone = "" - c.FillNoCustom(m)Remove the duplicate registration:
return func(codecs runtimeserializer.CodecFactory) []interface{} { return []interface{}{ - // MAPI to CAPI conversion functions. - (&MAPIMachineFuzzer{}).FuzzMachine, + // MAPI to CAPI conversion functions.Also applies to: 675-675
🧹 Nitpick comments (2)
pkg/conversion/test/fuzz/fuzz.go (2)
308-309: Include actual diffs in failure messages to aid debugging.The error messages now use
fmt.Sprintfbut only wrap static strings. To help debug test failures, includecmp.Diffoutput showing the actual differences between compared objects.Apply this diff:
- Expect(mapiMachine.ObjectMeta).To(Equal(in.machine.ObjectMeta), fmt.Sprintf("converted MAPI machine should have matching .metadata")) - Expect(mapiMachine.Spec).To(WithTransform(ignoreMachineProviderSpec, testutils.MatchViaJSON(ignoreMachineProviderSpec(in.machine.Spec))), fmt.Sprintf("converted MAPI machine should have matching .spec")) + Expect(mapiMachine.ObjectMeta).To(Equal(in.machine.ObjectMeta), fmt.Sprintf("converted MAPI machine should have matching .metadata: %s", cmp.Diff(mapiMachine.ObjectMeta, in.machine.ObjectMeta))) + Expect(mapiMachine.Spec).To(WithTransform(ignoreMachineProviderSpec, testutils.MatchViaJSON(ignoreMachineProviderSpec(in.machine.Spec))), fmt.Sprintf("converted MAPI machine should have matching .spec: %s", cmp.Diff(ignoreMachineProviderSpec(mapiMachine.Spec), ignoreMachineProviderSpec(in.machine.Spec))))
364-364: Include actual diff in failure message.Same as lines 308-309, this error message should include the actual diff to aid debugging.
Apply this diff:
- Expect(mapiMachineSet.Spec).To(WithTransform(ignoreMachineSetProviderSpec, testutils.MatchViaJSON(ignoreMachineSetProviderSpec(in.machineSet.Spec))), fmt.Sprintf("converted MAPI machine set should have matching .spec")) + Expect(mapiMachineSet.Spec).To(WithTransform(ignoreMachineSetProviderSpec, testutils.MatchViaJSON(ignoreMachineSetProviderSpec(in.machineSet.Spec))), fmt.Sprintf("converted MAPI machine set should have matching .spec: %s", cmp.Diff(ignoreMachineSetProviderSpec(mapiMachineSet.Spec), ignoreMachineSetProviderSpec(in.machineSet.Spec))))
📜 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 (1)
pkg/conversion/test/fuzz/fuzz.go(5 hunks)
|
|
||
| Eventually(k.Update(testMachine, func() { | ||
| testMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} | ||
| if testMachine.ObjectMeta.Labels == nil { |
There was a problem hiding this comment.
Ah yep, this is now needed as otherwise you're removing a protected label by putting the sentinel in :)
chrischdi
force-pushed
the
pr-machines-labels
branch
2 times, most recently
from
b134097 to
ad801fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/conversion/capi2mapi/machine.go (1)
47-67: Apply the proposed changes to sync additional labels and annotations to Spec.ObjectMeta for round-trip consistency.The review comment is accurate. The current implementation applies
additionalMachineAPILabelsandadditionalMachineAPIAnnotationsonly toObjectMeta, not toSpec.ObjectMeta.Key findings from verification:
The conversion functions have mismatched signatures:
convertCAPILabelsToMAPILabelsaccepts both CAPI labels and additional Machine API labels, whileconvertCAPIMachineLabelsToMAPIMachineSpecObjectMetaLabelsaccepts only CAPI labels.Round-trip test comments explicitly document that MAPI->CAPI conversion merges both
machine.spec.metadata.labels/annotationsandmachine.metadata.labels/annotationsinto the CAPI location, and when converting back CAPI->MAPI, the library copies to both locations. The test fuzzer enforces this by syncingSpec.ObjectMetaannotations and labels to match top-levelObjectMeta.The current code violates this round-trip expectation by not applying the additional provider-derived metadata to
Spec.ObjectMeta.The suggested fix is necessary to maintain metadata consistency during round-trip conversions.
pkg/conversion/capi2mapi/common.go (1)
136-183: Fix inconsistent nil check and comment.Two issues found:
Critical: Line 137 only checks
capiAnnotationsfor emptiness, unlikeconvertCAPILabelsToMAPILabelswhich checks both inputs at line 54. This causes the function to returnnilearly whencapiAnnotationsis empty, ignoring any values inmachineAPIAnnotations.Minor: Line 176 says "label fields" but should reference "annotation fields" to match the context.
Apply this diff:
-func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string { - if len(capiAnnotations) == 0 { - return nil - } +func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string { + if len(capiAnnotations) == 0 && len(machineAPIAnnotations) == 0 { + return nil + } mapiAnnotations := make(map[string]string) toNotConvertAnnotations := sets.New( // We want to skip the CAPI paused annotation to be copied over to MAPI. clusterv1.PausedAnnotation, ) for k, v := range capiAnnotations { if toNotConvertAnnotations.Has(k) { // Skip this annotation. continue } if k == clusterv1.DeleteMachineAnnotation { mapiAnnotations[util.MapiDeleteMachineAnnotation] = v continue } // Ignore CAPI-specific annotations that are not explicitly handled. if strings.Contains(k, "cluster.x-k8s.io/") { continue } mapiAnnotations[k] = v } for k, v := range machineAPIAnnotations { // Ignore empty annotations to ensure to not overwrite potentially existing annotations with empty values. if v == "" { continue } mapiAnnotations[k] = v } - // On the original MAPI object some label fields are nil rather than empty. + // On the original MAPI object some annotation fields are nil rather than empty. // So return nil instead to avoid unnecessary diff being picked up by the diff checker. if len(mapiAnnotations) == 0 { return nil } return mapiAnnotations }
♻️ Duplicate comments (2)
pkg/conversion/test/fuzz/fuzz.go (2)
627-630: Don’t reset provider-propagated fields in MAPIMachineFuzzer.Resetting InstanceType/Region/Zone discards values set by provider fuzzers; labels won’t propagate.
Apply:
- // Reset the fuzzer - f.InstanceType = "" - f.Region = "" - f.Zone = ""
675-675: Remove duplicate FuzzMachine registration.Provider suites already append f.FuzzMachine; duplicate registration causes nondeterminism.
Apply:
- // MAPI to CAPI conversion functions. - (&MAPIMachineFuzzer{}).FuzzMachine, + // MAPI to CAPI conversion functions.
🧹 Nitpick comments (4)
pkg/conversion/capi2mapi/aws.go (2)
52-56: Add a brief doc on excludeMachineAPILabelsAndAnnotations.Clarify that false includes instance-type/region/zone labels and instance-state annotation; true excludes them (used for MachineSet path).
73-91: MachineSet path excludes provider labels/annos—confirm design.FromMachineSet... sets excludeMachineAPILabelsAndAnnotations=true, so ToMachine (used by ToMachineSet) won’t add extras; template inherits none. Confirm that we intentionally avoid provider labels/annos on templates.
Option: allow toggling via constructor arg or follow-up to include non-status labels on templates.
Also applies to: 291-327
pkg/conversion/capi2mapi/openstack.go (1)
43-47: Same template exclusion pattern as AWS—confirm intent.excludeMachineAPILabelsAndAnnotations=true on MachineSet wrapper means no provider labels/annos on templates. Confirm this is desired for OpenStack too.
Also applies to: 79-81, 222-223
pkg/conversion/mapi2capi/powervs_fuzz_test.go (1)
141-161: Broaden serviceInstance fuzzing types.All cases set Type=ID. Vary Types to improve coverage (e.g., ID, Name).
- switch c.Int31n(3) { - case 0: - *serviceInstance = mapiv1.PowerVSResource{ Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0)) } - case 1: - *serviceInstance = mapiv1.PowerVSResource{ Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0)) } - case 2: - *serviceInstance = mapiv1.PowerVSResource{ Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0)) } - } + switch c.Int31n(3) { + case 0: + *serviceInstance = mapiv1.PowerVSResource{ Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0)) } + case 1: + *serviceInstance = mapiv1.PowerVSResource{ Type: mapiv1.PowerVSResourceTypeName, Name: ptr.To(c.String(0)) } + case 2: + *serviceInstance = mapiv1.PowerVSResource{ Type: mapiv1.PowerVSResourceTypeID, ID: ptr.To(c.String(0)) } + }
📜 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 (12)
pkg/controllers/machinesync/machine_sync_controller_test.go(1 hunks)pkg/conversion/capi2mapi/aws.go(3 hunks)pkg/conversion/capi2mapi/common.go(4 hunks)pkg/conversion/capi2mapi/machine.go(2 hunks)pkg/conversion/capi2mapi/machineset.go(2 hunks)pkg/conversion/capi2mapi/openstack.go(4 hunks)pkg/conversion/capi2mapi/powervs.go(3 hunks)pkg/conversion/mapi2capi/aws_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/openstack_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/powervs_fuzz_test.go(5 hunks)pkg/conversion/mapi2capi/util.go(3 hunks)pkg/conversion/test/fuzz/fuzz.go(2 hunks)
🔇 Additional comments (13)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
962-965: LGTM! Proper nil check prevents potential panic.The defensive nil check before modifying the Labels map is correct and prevents a runtime panic when the builder doesn't initialize the map. This is idiomatic Go and aligns with the past review feedback.
pkg/conversion/capi2mapi/machineset.go (1)
35-36: Confirm intent: no provider labels/annos on MachineSet path.You now pass nil for extra maps. Given ToMachineSet later copies from a Machine produced without extras, provider labels/annos won’t land on the template. Verify this matches the PR’s scope (labels only on Machines, not MachineSets).
Also applies to: 47-48
pkg/conversion/mapi2capi/util.go (2)
58-60: LGTM: filter MAPI-only labels on MAPI→CAPI.Skipping machine.openshift.io/* avoids leaking MAPI-specific keys back into CAPI.
Consider adding a small unit test to lock this behavior (labels and annotations).
21-21: Import usage is correct.strings is now used in both filters. No action.
pkg/conversion/capi2mapi/aws.go (1)
263-276: Provider-derived labels/annos wiring looks right.
- instance-type from AWSMachine.Spec.InstanceType
- region from AWSCluster.Spec.Region
- zone from Machine.Spec.FailureDomain
- instance-state from AWSMachine.Status.InstanceState
Ensure empty values are pruned by downstream helpers.
pkg/conversion/capi2mapi/openstack.go (1)
207-220: LGTM: only instance-type and zone; no region (needs creds).Matches the PR note about OpenStack region. Annotation instance-state wiring OK.
pkg/conversion/mapi2capi/aws_fuzz_test.go (1)
162-166: Good: sync provider fields into MAPIMachineFuzzer.Ensures labels reflect providerSpec in fuzz.
pkg/conversion/mapi2capi/powervs_fuzz_test.go (1)
132-134: Good: sync SystemType into f.InstanceType.Keeps labels aligned in fuzz.
pkg/conversion/capi2mapi/common.go (2)
48-48: LGTM!Correctly passes
nilfor the newmachineAPILabelsparameter since selector conversion doesn't propagate provider-specific labels.
53-96: LGTM!The merge logic correctly:
- Checks both input maps for emptiness
- Skips empty values to avoid overwriting existing labels
- Returns nil when result is empty
pkg/conversion/mapi2capi/openstack_fuzz_test.go (1)
114-264: LGTM!The refactoring to a struct-based fuzzer pattern improves modularity and makes the fuzzing logic more maintainable. The implementation correctly:
- Encapsulates provider-specific fuzzing logic in
openstackProviderFuzzer- Sanitizes and seeds fields appropriately in
fuzzProviderSpec- Separates machine-set and machine-level fuzzing concerns
- Propagates instance type and zone to the embedded fuzzer (lines 164-165)
pkg/conversion/capi2mapi/powervs.go (2)
41-46: LGTM!The
excludeMachineAPILabelsAndAnnotationsflag correctly controls when provider-derived labels/annotations are propagated:
- Set to
truefor MachineSet template conversion (line 79) to avoid setting instance-specific metadata on templates- Defaults to
falsefor direct Machine conversion, enabling label/annotation propagationAlso applies to: 78-79
105-118: LGTM!The conditional label/annotation propagation is well-implemented:
- Safe nil handling using
ptr.Derefwith empty string defaults- Empty values are correctly filtered by the merge logic in
common.go(lines 82-84, 169-171)- Propagates the expected OpenShift labels: instance-type, region, and zone
- Adds the instance-state annotation as intended
The implementation aligns with the PR objectives.
pkg/conversion/mapi2capi/util.go
Outdated
| // if strings.HasPrefix(mapiLabelKey, "machine.openshift.io/") { | ||
| // continue | ||
| // } | ||
| if strings.HasPrefix(mapiLabelKey, "machine.openshift.io/") { |
There was a problem hiding this comment.
@damdo is this Change OK?
There was a problem hiding this comment.
Changed to explicitly list the ones we want to skip now.
chrischdi
force-pushed
the
pr-machines-labels
branch
from
ad801fd to
46c4075
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/conversion/capi2mapi/common.go (1)
136-183: Fix asymmetric nil-check that discards machineAPIAnnotations.The early return at lines 137-139 only checks
capiAnnotations, which means ifcapiAnnotationsis empty butmachineAPIAnnotationshas values, the function returnsniland discards the Machine API annotations entirely.This is inconsistent with the labels converter at line 54, which correctly checks both inputs:
if len(capiLabels) == 0 && len(machineAPILabels) == 0 { return nil }Apply this diff:
- if len(capiAnnotations) == 0 { + if len(capiAnnotations) == 0 && len(machineAPIAnnotations) == 0 { return nil }
📜 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 (12)
pkg/controllers/machinesync/machine_sync_controller_test.go(1 hunks)pkg/conversion/capi2mapi/aws.go(3 hunks)pkg/conversion/capi2mapi/common.go(4 hunks)pkg/conversion/capi2mapi/machine.go(2 hunks)pkg/conversion/capi2mapi/machineset.go(2 hunks)pkg/conversion/capi2mapi/openstack.go(4 hunks)pkg/conversion/capi2mapi/powervs.go(3 hunks)pkg/conversion/mapi2capi/aws_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/openstack_fuzz_test.go(6 hunks)pkg/conversion/mapi2capi/powervs_fuzz_test.go(5 hunks)pkg/conversion/mapi2capi/util.go(3 hunks)pkg/conversion/test/fuzz/fuzz.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/conversion/mapi2capi/aws_fuzz_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/conversion/capi2mapi/openstack.go
- pkg/conversion/mapi2capi/powervs_fuzz_test.go
- pkg/controllers/machinesync/machine_sync_controller_test.go
- pkg/conversion/mapi2capi/util.go
🔇 Additional comments (12)
pkg/conversion/capi2mapi/common.go (2)
46-51: LGTM - Selector conversion correctly passes nil for additional labels.The selector conversion appropriately passes
nilformachineAPILabelssince selectors should only contain the CAPI labels without merging additional Machine API labels.
53-96: LGTM - Label merging correctly implemented.The label converter properly:
- Checks both input maps for emptiness before returning nil
- Skips empty values when merging
machineAPILabelsto avoid overwriting existing labels- Returns nil when the result is empty to match MAPI conventions
pkg/conversion/capi2mapi/machineset.go (1)
28-67: LGTM - MachineSet conversion correctly updated.All four call sites to the label and annotation converters are properly updated to pass
nilfor the additional maps. This is correct since MachineSet conversion doesn't merge provider-derived labels/annotations—those are only added at the Machine level.pkg/conversion/capi2mapi/machine.go (1)
36-109: LGTM - Machine conversion correctly extended with additional maps.The function signature is properly updated to accept
additionalMachineAPILabelsandadditionalMachineAPIAnnotations, and these are correctly passed to the converter functions. This enables provider-specific converters to inject instance-type, region, zone labels and instance-state annotations.pkg/conversion/capi2mapi/aws.go (2)
50-92: LGTM - Exclude flag properly configured for MachineSets.The
excludeMachineAPILabelsAndAnnotationsfield is correctly initialized totrueinFromMachineSetAndAWSMachineTemplateAndAWSClusterat line 89. This ensures that instance-specific labels and annotations are not added to MachineSet templates, which is the correct behavior since those values are machine-specific.
236-289: LGTM - Additional labels and annotations correctly propagated.The conditional logic properly builds and passes provider-derived metadata:
- Labels: instance-type, region, and zone are sourced from the appropriate CAPI/CAPA resources
- Annotation: instance-state is sourced from AWSMachine status
- Empty values are safely handled with
ptr.Deref(..., "")- The maps are only constructed when
excludeMachineAPILabelsAndAnnotationsis falseThis aligns with OpenShift's Machine API label conventions.
pkg/conversion/mapi2capi/openstack_fuzz_test.go (4)
58-81: LGTM - Provider fuzzer properly instantiated for Machine tests.The
openstackProviderFuzzerinstance is correctly created and itsFuzzerFuncsMachineis passed to the fuzz test. This centralizes the OpenStack-specific fuzzing logic.
83-107: LGTM - Provider fuzzer properly instantiated for MachineSet tests.The
openstackProviderFuzzerinstance is correctly created and itsFuzzerFuncsMachineSetis passed to the fuzz test, consistent with the Machine test pattern.
114-166: LGTM - Provider spec fuzzing correctly propagates flavor and zone.The
fuzzProviderSpecmethod properly:
- Clears unsupported fields (FloatingIP, PrimarySubnet, SshUserName, etc.)
- Handles conditional fields (CloudsSecret, ServerGroup ID vs Name)
- Propagates
FlavortoInstanceTypeandAvailabilityZonetoZoneat lines 164-165Note: Region is intentionally not set, consistent with the PR description stating that determining the region for OpenStack would require fetching credentials.
169-264: LGTM - Fuzzer functions correctly composed.The
FuzzerFuncsMachineSetreturns fuzzers for OpenStack-specific types (BlockDeviceStorage, NetworkParam, PortOpts, etc.), andFuzzerFuncsMachineproperly extends this withFuzzMachineto handle machine-level fuzzing. This composition pattern is consistent with other provider fuzzers in the PR.pkg/conversion/capi2mapi/powervs.go (2)
40-82: LGTM - Exclude flag properly configured for MachineSets.The
excludeMachineAPILabelsAndAnnotationsfield is correctly initialized totrueat line 79, preventing instance-specific labels and annotations from being added to MachineSet templates. This mirrors the AWS implementation.
84-130: LGTM - Additional labels and annotations correctly propagated.The conditional logic properly builds and passes provider-derived metadata:
- Labels: instance-type (from SystemType), region (from Status.Region), and zone (from FailureDomain)
- Annotation: instance-state (from Status.InstanceState)
- Empty values are safely handled with
ptr.Deref(..., "")- The maps are only constructed when the exclude flag is false
This aligns with the AWS and OpenStack implementations and follows OpenShift's Machine API conventions.
There was a problem hiding this comment.
Thanks @chrischdi
A bunch of things but we are looking good here
pkg/conversion/capi2mapi/aws.go
Outdated
| additionalMachineAPILabels = map[string]string{ | ||
| "machine.openshift.io/instance-type": m.awsMachine.Spec.InstanceType, | ||
| "machine.openshift.io/region": m.awsCluster.Spec.Region, | ||
| "machine.openshift.io/zone": ptr.Deref(m.machine.Spec.FailureDomain, ""), | ||
| } | ||
|
|
||
| additionalMachineAPIAnnotations = map[string]string{ | ||
| "machine.openshift.io/instance-state": string(ptr.Deref(m.awsMachine.Status.InstanceState, "")), | ||
| } | ||
| } |
There was a problem hiding this comment.
So these are labels and annotations that will end up in the metadata.labels, rather than on the spec.metadata.labels of the MAPI Machine.
How can we make this extra clear with the naming?
maybe: additionalMAPIMachineMetadataLabels/Annotations?
So if in the future we need to add the spec ones we can have additionaMAPIMachineSpecMetadataLabels?
There was a problem hiding this comment.
adjusted. Added a new package to conversion, as we access these consts from capi2mapi and mapi2capi.
|
|
||
| func convertCAPILabelsToMAPILabels(capiLabels map[string]string) map[string]string { | ||
| if len(capiLabels) == 0 { | ||
| func convertCAPILabelsToMAPILabels(capiLabels map[string]string, machineAPILabels map[string]string) map[string]string { |
There was a problem hiding this comment.
We may want to change the param name based on what we decide on https://github.com/openshift/cluster-capi-operator/pull/393/files#r2487307586
|
|
||
| func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string) map[string]string { | ||
| if len(capiAnnotations) == 0 { | ||
| func convertCAPIAnnotationsToMAPIAnnotations(capiAnnotations map[string]string, machineAPIAnnotations map[string]string) map[string]string { |
There was a problem hiding this comment.
We may want to change the param name based on what we decide on https://github.com/openshift/cluster-capi-operator/pull/393/files#r2487307586
pkg/conversion/mapi2capi/util.go
Outdated
| skippedMAPILabels = sets.New("machine.openshift.io/instance-type", "machine.openshift.io/region", "machine.openshift.io/zone") | ||
| skippedMAPIAnnotations = sets.New("machine.openshift.io/instance-state") |
There was a problem hiding this comment.
Hm, could we dynamically pull these from a constant that we can use both here and in the implementation?
There was a problem hiding this comment.
These names will need changing if we change the param name
There was a problem hiding this comment.
using the consts now. Also adjusted the names.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/conversion/capi2mapi/powervs.go (1)
106-117: Consider filtering empty string values from metadata.Similar to aws.go, this code could set labels/annotations to empty strings when
Region,FailureDomain, orInstanceStateare nil or empty. Empty metadata values may not be meaningful.Apply the same filtering pattern as suggested for aws.go to avoid empty-string metadata entries.
pkg/conversion/capi2mapi/openstack.go (1)
208-221: Consider filtering empty string values from metadata.Similar to aws.go and powervs.go, this code could set labels/annotations to empty strings when
Flavor,FailureDomain, orInstanceStateare nil or empty.Apply the same filtering pattern as suggested for aws.go to avoid empty-string metadata entries.
🧹 Nitpick comments (2)
pkg/conversion/mapi2capi/util.go (1)
264-275: Consider filtering empty string values from metadata.The code uses
ptr.Derefwith""as the default, which could result in empty-string labels and annotations whenFailureDomainorInstanceStateare nil. Empty values may clutter metadata unnecessarily.Consider filtering out empty values:
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string if !m.excludeMachineAPILabelsAndAnnotations { additionalMachineAPIMetadataLabels = map[string]string{ consts.MAPIMachineMetadataLabelInstanceType: m.awsMachine.Spec.InstanceType, consts.MAPIMachineMetadataLabelRegion: m.awsCluster.Spec.Region, - consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""), } + if zone := ptr.Deref(m.machine.Spec.FailureDomain, ""); zone != "" { + additionalMachineAPIMetadataLabels[consts.MAPIMachineMetadataLabelZone] = zone + } - additionalMachineAPIMetadataAnnotations = map[string]string{ - consts.MAPIMachineMetadataAnnotationInstanceState: string(ptr.Deref(m.awsMachine.Status.InstanceState, "")), + if instanceState := string(ptr.Deref(m.awsMachine.Status.InstanceState, "")); instanceState != "" { + additionalMachineAPIMetadataAnnotations = map[string]string{ + consts.MAPIMachineMetadataAnnotationInstanceState: instanceState, + } } }Note: This same pattern appears in powervs.go and openstack.go and should be addressed consistently across all providers.
pkg/conversion/capi2mapi/aws.go (1)
264-276: Consider filtering empty values before map insertion.While the converter functions drop empty values (per past reviews), it's more defensive and clearer to check before adding entries to the map. Specifically:
- Line 269:
FailureDomaincould be nil, resulting in an empty zone label- Line 273:
InstanceStatecould be nil for newly created machines, resulting in an empty annotationApply this diff to filter empty values:
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string if !m.excludeMachineAPILabelsAndAnnotations { additionalMachineAPIMetadataLabels = map[string]string{ consts.MAPIMachineMetadataLabelInstanceType: m.awsMachine.Spec.InstanceType, consts.MAPIMachineMetadataLabelRegion: m.awsCluster.Spec.Region, - consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""), } + if zone := ptr.Deref(m.machine.Spec.FailureDomain, ""); zone != "" { + additionalMachineAPIMetadataLabels[consts.MAPIMachineMetadataLabelZone] = zone + } - additionalMachineAPIMetadataAnnotations = map[string]string{ - consts.MAPIMachineMetadataAnnotationInstanceState: string(ptr.Deref(m.awsMachine.Status.InstanceState, "")), + if instanceState := string(ptr.Deref(m.awsMachine.Status.InstanceState, "")); instanceState != "" { + additionalMachineAPIMetadataAnnotations = map[string]string{ + consts.MAPIMachineMetadataAnnotationInstanceState: instanceState, + } } }
📜 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 (7)
pkg/controllers/machinesetsync/machineset_sync_controller_test.go(0 hunks)pkg/conversion/capi2mapi/aws.go(4 hunks)pkg/conversion/capi2mapi/machine.go(2 hunks)pkg/conversion/capi2mapi/openstack.go(4 hunks)pkg/conversion/capi2mapi/powervs.go(4 hunks)pkg/conversion/mapi2capi/util.go(5 hunks)pkg/conversion/test/fuzz/fuzz.go(3 hunks)
💤 Files with no reviewable changes (1)
- pkg/controllers/machinesetsync/machineset_sync_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/conversion/test/fuzz/fuzz.go
🔇 Additional comments (8)
pkg/conversion/mapi2capi/util.go (2)
39-40: LGTM: Explicit skip sets improve clarity.The shift from prefix-based filtering to explicit skip sets makes the conversion logic more maintainable and easier to understand.
37-37: Signature extension enables metadata propagation.The additional parameters allow provider-specific metadata to be merged into the MAPI Machine's ObjectMeta during conversion.
pkg/conversion/capi2mapi/openstack.go (1)
211-213: Helpful context for OpenStack-specific behavior.The comment clearly explains why the region label is not set for OpenStack, which is valuable for future maintainers.
pkg/conversion/capi2mapi/machine.go (1)
37-37: LGTM: Clean signature extension for additional metadata.The function signature extension and parameter usage are correct. The additional metadata maps are passed directly to the converter functions, which (per past reviews) are nil-safe and properly handle empty values.
Also applies to: 51-52
pkg/conversion/capi2mapi/aws.go (4)
24-24: LGTM: Appropriate import for metadata constants.Adding the consts package import is correct for accessing the MAPI Machine metadata key constants used in the additional labels and annotations.
53-57: LGTM: Clear flag for conditional metadata behavior.The
excludeMachineAPILabelsAndAnnotationsfield is well-named and provides the necessary control to differentiate between Machine and MachineSet conversion paths.
89-90: LGTM: Correct exclusion for MachineSet template conversion.Setting
excludeMachineAPILabelsAndAnnotations: truefor MachineSet conversion is appropriate, as the additional provider-derived labels should only appear on actual Machine metadata, not in MachineSet template spec.
277-277: LGTM: Correct function call with additional metadata.The call to
fromCAPIMachineToMAPIMachinecorrectly passes the conditionally-built metadata maps, which are nil when excluded and properly populated otherwise.
chrischdi
force-pushed
the
pr-machines-labels
branch
from
a68c2f7 to
4ee8586
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/conversion/test/fuzz/fuzz.go (1)
627-630: Critical: Field reset discards provider-set values.The reset at lines 627-630 clears
InstanceType,Region, andZoneto empty strings at the start ofFuzzMachine. Provider fuzzers (AWS, PowerVS, OpenStack) populate these fields on their embeddedMAPIMachineFuzzerinstances before this method runs, but the reset discards those values. The conditional checks at lines 639-649 then fail because the fields are empty, preventing labels from being populated.This issue was flagged in a previous review as addressed, but the reset code remains.
Remove the reset lines:
func (f *MAPIMachineFuzzer) FuzzMachine(m *mapiv1beta1.Machine, c randfill.Continue) { - // Reset the fuzzer - f.InstanceType = "" - f.Region = "" - f.Zone = "" - c.FillNoCustom(m)
🧹 Nitpick comments (1)
pkg/conversion/consts/consts.go (1)
19-29: Polish comments for clarity.The comments on lines 19, 22, and 28 have minor grammatical inconsistencies. Consider adding "a" before "column" for better readability.
Apply this diff:
- // MAPIMachineMetadataLabelInstanceType is the label for the instance type of the machine is used as column for kubectl. + // MAPIMachineMetadataLabelInstanceType is the label for the instance type of the machine and is used as a column for kubectl. MAPIMachineMetadataLabelInstanceType = "machine.openshift.io/instance-type" - // MAPIMachineMetadataLabelRegion is the label for the region of the machine is used as column for kubectl. + // MAPIMachineMetadataLabelRegion is the label for the region of the machine and is used as a column for kubectl. MAPIMachineMetadataLabelRegion = "machine.openshift.io/region" // MAPIMachineMetadataLabelZone is the label for the zone of the machine and is used as column for kubectl. MAPIMachineMetadataLabelZone = "machine.openshift.io/zone" - // MAPIMachineMetadataAnnotationInstanceState is the annotation for the instance state of the machine is used as column for kubectl. + // MAPIMachineMetadataAnnotationInstanceState is the annotation for the instance state of the machine and is used as a column for kubectl. MAPIMachineMetadataAnnotationInstanceState = "machine.openshift.io/instance-state"
📜 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 (8)
pkg/controllers/machinesetsync/machineset_sync_controller_test.go(0 hunks)pkg/conversion/capi2mapi/aws.go(4 hunks)pkg/conversion/capi2mapi/machine.go(2 hunks)pkg/conversion/capi2mapi/openstack.go(4 hunks)pkg/conversion/capi2mapi/powervs.go(4 hunks)pkg/conversion/consts/consts.go(1 hunks)pkg/conversion/mapi2capi/util.go(5 hunks)pkg/conversion/test/fuzz/fuzz.go(3 hunks)
💤 Files with no reviewable changes (1)
- pkg/controllers/machinesetsync/machineset_sync_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/conversion/capi2mapi/openstack.go
- pkg/conversion/mapi2capi/util.go
🔇 Additional comments (3)
pkg/conversion/capi2mapi/machine.go (1)
37-52: LGTM: Additional metadata parameters correctly integrated.The function signature extension and the passing of additional metadata maps to the converter functions are correctly implemented. The converters handle nil-safety and merging appropriately.
pkg/conversion/capi2mapi/powervs.go (1)
106-119: LGTM: Conditional metadata propagation correctly implemented.The conditional logic properly gates metadata propagation based on
excludeMachineAPILabelsAndAnnotations, uses the correct constants from the consts package, and safely dereferences pointers with fallback values. The integration withfromCAPIMachineToMAPIMachineis correct.pkg/conversion/capi2mapi/aws.go (1)
264-277: LGTM: Consistent metadata propagation pattern.The conditional metadata building follows the same pattern as powervs.go, uses the correct constants, and safely handles pointer dereferences. The integration with
fromCAPIMachineToMAPIMachineis correct.
chrischdi
force-pushed
the
pr-machines-labels
branch
from
4ee8586 to
65251e6
Compare
|
[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
openshift-merge-robot
added
the
needs-rebase
…pe, region, zone and instance-state, ignore mapi labels when converting to CAPI
chrischdi
force-pushed
the
pr-machines-labels
branch
from
42bcf26 to
66eb47c
Compare
openshift-merge-robot
removed
the
needs-rebase
5 participants
Follow up, found during review from @damdo:
Configures the following labels in case we have the information:
Labels:
machine.openshift.io/instance-type: from InfraMachinemachine.openshift.io/region: from InfraMachine/Clustermachine.openshift.io/zone: From machine.spec.failureDomainAnnotation:
machine.openshift.io/instance-statefrom InfraMachineSummary by CodeRabbit
New Features
Tests
Chores
Bug Fixes