Skip to content

Commit a68c2f7

Browse files
chrischdichrischdi
committed
Review fixes Damiano v1
1 parent 7d0c44c commit a68c2f7

File tree

7 files changed

+43
-40
lines changed

7 files changed

+43
-40
lines changed

‎pkg/controllers/machinesetsync/machineset_sync_controller_test.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -683,7 +683,6 @@ var _ = Describe("With a running MachineSetSync controller", func() {
683683
SatisfyAll(
684684
HaveField("Type", Equal(consts.SynchronizedCondition)),
685685
HaveField("Status", Equal(corev1.ConditionFalse)),
686-
HaveField("Severity", Equal(mapiv1beta1.ConditionSeverityError)),
687686
HaveField("Reason", Equal("FailedToConvertCAPIMachineSetToMAPI")),
688687
))),
689688
)
@@ -1124,7 +1123,6 @@ var _ = Describe("With a running MachineSetSync controller", func() {
11241123
SatisfyAll(
11251124
HaveField("Type", Equal(consts.SynchronizedCondition)),
11261125
HaveField("Status", Equal(corev1.ConditionFalse)),
1127-
HaveField("Severity", Equal(mapiv1beta1.ConditionSeverityError)),
11281126
HaveField("Reason", Equal("FailedToGetCAPIInfraResources")),
11291127
))),
11301128
)
@@ -1255,7 +1253,6 @@ var _ = Describe("applySynchronizedConditionWithPatch", func() {
12551253
HaveField("Status", Equal(corev1.ConditionFalse)),
12561254
HaveField("Reason", Equal("ErrorReason")),
12571255
HaveField("Message", Equal("Error message")),
1258-
HaveField("Severity", Equal(mapiv1beta1.ConditionSeverityError)),
12591256
))),
12601257
)
12611258
})

‎pkg/conversion/capi2mapi/aws.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"strings"
2222

2323
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
24+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
2425

2526
corev1 "k8s.io/api/core/v1"
2627
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -260,20 +261,20 @@ func (m machineAndAWSMachineAndAWSCluster) ToMachine() (*mapiv1beta1.Machine, []
260261

261262
warnings = append(warnings, warn...)
262263

263-
var additionalMachineAPILabels, additionalMachineAPIAnnotations map[string]string
264+
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string
264265
if !m.excludeMachineAPILabelsAndAnnotations {
265-
additionalMachineAPILabels = map[string]string{
266-
"machine.openshift.io/instance-type": m.awsMachine.Spec.InstanceType,
267-
"machine.openshift.io/region": m.awsCluster.Spec.Region,
268-
"machine.openshift.io/zone": ptr.Deref(m.machine.Spec.FailureDomain, ""),
266+
additionalMachineAPIMetadataLabels = map[string]string{
267+
consts.MAPIMachineMetadataLabelInstanceType: m.awsMachine.Spec.InstanceType,
268+
consts.MAPIMachineMetadataLabelRegion: m.awsCluster.Spec.Region,
269+
consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""),
269270
}
270271

271-
additionalMachineAPIAnnotations = map[string]string{
272-
"machine.openshift.io/instance-state": string(ptr.Deref(m.awsMachine.Status.InstanceState, "")),
272+
additionalMachineAPIMetadataAnnotations = map[string]string{
273+
consts.MAPIMachineMetadataAnnotationInstanceState: string(ptr.Deref(m.awsMachine.Status.InstanceState, "")),
273274
}
274275
}
275276

276-
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPILabels, additionalMachineAPIAnnotations)
277+
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations)
277278
if err != nil {
278279
errors = append(errors, err...)
279280
}

‎pkg/conversion/capi2mapi/machine.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ const (
3434
)
3535

3636
// fromCAPIMachineToMAPIMachine translates a core CAPI Machine to its MAPI Machine correspondent.
37-
func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine, additionalMachineAPILabels, additionalMachineAPIAnnotations map[string]string) (*mapiv1beta1.Machine, field.ErrorList) {
37+
func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string) (*mapiv1beta1.Machine, field.ErrorList) {
3838
errs := field.ErrorList{}
3939

4040
lifecycleHooks, capiMachineNonHookAnnotations := convertCAPILifecycleHookAnnotationsToMAPILifecycleHooksAndAnnotations(capiMachine.Annotations)
@@ -48,8 +48,8 @@ func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine, additionalMach
4848
ObjectMeta: metav1.ObjectMeta{
4949
Name: capiMachine.Name,
5050
Namespace: mapiNamespace,
51-
Labels: convertCAPILabelsToMAPILabels(capiMachine.Labels, additionalMachineAPILabels),
52-
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineNonHookAnnotations, additionalMachineAPIAnnotations),
51+
Labels: convertCAPILabelsToMAPILabels(capiMachine.Labels, additionalMachineAPIMetadataLabels),
52+
Annotations: convertCAPIAnnotationsToMAPIAnnotations(capiMachineNonHookAnnotations, additionalMachineAPIMetadataAnnotations),
5353
Finalizers: []string{mapiv1beta1.MachineFinalizer},
5454
OwnerReferences: nil, // OwnerReferences not populated here. They are added later by the machineSync controller.
5555
},

‎pkg/conversion/capi2mapi/openstack.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323

2424
mapiv1alpha1 "github.com/openshift/api/machine/v1alpha1"
2525
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
26+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
2627
corev1 "k8s.io/api/core/v1"
2728
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2829
"k8s.io/apimachinery/pkg/runtime"
@@ -204,22 +205,22 @@ func (m machineAndOpenStackMachineAndOpenStackCluster) ToMachine() (*mapiv1beta1
204205

205206
warnings = append(warnings, warns...)
206207

207-
var additionalMachineAPILabels, additionalMachineAPIAnnotations map[string]string
208+
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string
208209
if !m.excludeMachineAPILabelsAndAnnotations {
209-
additionalMachineAPILabels = map[string]string{
210+
additionalMachineAPIMetadataLabels = map[string]string{
210211
// Unable to determine the region without fetching the identity resources as done at:
211212
// https://github.com/openshift/machine-api-provider-openstack/blob/2defb131bd0836beba0a9790de7c9a63137a5cec/pkg/machine/actuator.go#L85-L89
212-
// "machine.openshift.io/region":
213-
"machine.openshift.io/instance-type": ptr.Deref(m.openstackMachine.Spec.Flavor, ""),
214-
"machine.openshift.io/zone": ptr.Deref(m.machine.Spec.FailureDomain, ""),
213+
// consts.MAPIMachineMetadataLabelRegion
214+
consts.MAPIMachineMetadataLabelInstanceType: ptr.Deref(m.openstackMachine.Spec.Flavor, ""),
215+
consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""),
215216
}
216217

217-
additionalMachineAPIAnnotations = map[string]string{
218-
"machine.openshift.io/instance-state": string(ptr.Deref(m.openstackMachine.Status.InstanceState, "")),
218+
additionalMachineAPIMetadataAnnotations = map[string]string{
219+
consts.MAPIMachineMetadataAnnotationInstanceState: string(ptr.Deref(m.openstackMachine.Status.InstanceState, "")),
219220
}
220221
}
221222

222-
mapiMachine, errs := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPILabels, additionalMachineAPIAnnotations)
223+
mapiMachine, errs := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations)
223224
if errs != nil {
224225
errors = append(errors, errs...)
225226
}

‎pkg/conversion/capi2mapi/powervs.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222

2323
mapiv1 "github.com/openshift/api/machine/v1"
2424
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
25+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
2526

2627
corev1 "k8s.io/api/core/v1"
2728
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -102,20 +103,20 @@ func (m machineAndPowerVSMachineAndPowerVSCluster) ToMachine() (*mapiv1beta1.Mac
102103
return nil, nil, fmt.Errorf("unable to convert PowerVS providerSpec to raw extension: %w", errRaw)
103104
}
104105

105-
var additionalMachineAPILabels, additionalMachineAPIAnnotations map[string]string
106+
var additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations map[string]string
106107
if !m.excludeMachineAPILabelsAndAnnotations {
107-
additionalMachineAPILabels = map[string]string{
108-
"machine.openshift.io/instance-type": m.powerVSMachine.Spec.SystemType,
109-
"machine.openshift.io/region": ptr.Deref(m.powerVSMachine.Status.Region, ""),
110-
"machine.openshift.io/zone": ptr.Deref(m.machine.Spec.FailureDomain, ""),
108+
additionalMachineAPIMetadataLabels = map[string]string{
109+
consts.MAPIMachineMetadataLabelInstanceType: m.powerVSMachine.Spec.SystemType,
110+
consts.MAPIMachineMetadataLabelRegion: ptr.Deref(m.powerVSMachine.Status.Region, ""),
111+
consts.MAPIMachineMetadataLabelZone: ptr.Deref(m.machine.Spec.FailureDomain, ""),
111112
}
112113

113-
additionalMachineAPIAnnotations = map[string]string{
114-
"machine.openshift.io/instance-state": string(m.powerVSMachine.Status.InstanceState),
114+
additionalMachineAPIMetadataAnnotations = map[string]string{
115+
consts.MAPIMachineMetadataAnnotationInstanceState: string(m.powerVSMachine.Status.InstanceState),
115116
}
116117
}
117118

118-
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPILabels, additionalMachineAPIAnnotations)
119+
mapiMachine, err := fromCAPIMachineToMAPIMachine(m.machine, additionalMachineAPIMetadataLabels, additionalMachineAPIMetadataAnnotations)
119120
if err != nil {
120121
errors = append(errors, err...)
121122
}

‎pkg/conversion/mapi2capi/util.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,18 +20,14 @@ import (
2020
"fmt"
2121

2222
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
23+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
2324
"github.com/openshift/cluster-capi-operator/pkg/util"
2425
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2526
"k8s.io/apimachinery/pkg/util/sets"
2627
"k8s.io/apimachinery/pkg/util/validation/field"
2728
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
2829
)
2930

30-
var (
31-
skippedMAPILabels = sets.New("machine.openshift.io/instance-type", "machine.openshift.io/region", "machine.openshift.io/zone")
32-
skippedMAPIAnnotations = sets.New("machine.openshift.io/instance-state")
33-
)
34-
3531
func convertMAPIMachineSetSelectorToCAPI(mapiSelector metav1.LabelSelector) metav1.LabelSelector {
3632
capiSelector := mapiSelector.DeepCopy()
3733
capiSelector.MatchLabels = convertMAPILabelsToCAPI(mapiSelector.MatchLabels)
@@ -40,6 +36,9 @@ func convertMAPIMachineSetSelectorToCAPI(mapiSelector metav1.LabelSelector) meta
4036
}
4137

4238
func convertMAPILabelsToCAPI(mapiLabels map[string]string) map[string]string {
39+
// These labels should not be converted to CAPI labels as they are handled explicitly in the conversion logic.
40+
mapiMetadataLabelsToSkip := sets.New(consts.MAPIMachineMetadataLabelInstanceType, consts.MAPIMachineMetadataLabelRegion, consts.MAPIMachineMetadataLabelZone)
41+
4342
capiLabels := make(map[string]string)
4443

4544
toTransformLabels := map[string]func(string) (string, string){
@@ -60,7 +59,7 @@ func convertMAPILabelsToCAPI(mapiLabels map[string]string) map[string]string {
6059
}
6160

6261
// Ignore MAPI-specific labels that are explicitly handled.
63-
if skippedMAPILabels.Has(mapiLabelKey) {
62+
if mapiMetadataLabelsToSkip.Has(mapiLabelKey) {
6463
continue
6564
}
6665

@@ -76,6 +75,9 @@ func convertMAPIAnnotationsToCAPI(mapiAnnotations map[string]string) map[string]
7675
return nil
7776
}
7877

78+
// These annotations should not be converted to Cluster API annotations as they are handled explicitly in the conversion logic.
79+
mapiMetadataAnnotationsToSkip := sets.New(consts.MAPIMachineMetadataAnnotationInstanceState)
80+
7981
capiAnnotations := make(map[string]string)
8082

8183
for k, v := range mapiAnnotations {
@@ -85,7 +87,7 @@ func convertMAPIAnnotationsToCAPI(mapiAnnotations map[string]string) map[string]
8587
}
8688

8789
// Ignore MAPI-specific annotations that are explicitly handled.
88-
if skippedMAPIAnnotations.Has(k) {
90+
if mapiMetadataAnnotationsToSkip.Has(k) {
8991
continue
9092
}
9193

‎pkg/conversion/test/fuzz/fuzz.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
3030
"github.com/openshift/cluster-api-actuator-pkg/testutils"
3131
"github.com/openshift/cluster-capi-operator/pkg/conversion/capi2mapi"
32+
"github.com/openshift/cluster-capi-operator/pkg/conversion/consts"
3233
"github.com/openshift/cluster-capi-operator/pkg/conversion/mapi2capi"
3334
"github.com/openshift/cluster-capi-operator/pkg/util"
3435
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -636,15 +637,15 @@ func (f *MAPIMachineFuzzer) FuzzMachine(m *mapiv1beta1.Machine, c randfill.Conti
636637

637638
// Copy over from fuzzed struct to the designated labels to have the same value.
638639
if f.InstanceType != "" {
639-
m.Labels["machine.openshift.io/instance-type"] = f.InstanceType
640+
m.Labels[consts.MAPIMachineMetadataAnnotationInstanceState] = f.InstanceType
640641
}
641642

642643
if f.Region != "" {
643-
m.Labels["machine.openshift.io/region"] = f.Region
644+
m.Labels[consts.MAPIMachineMetadataLabelRegion] = f.Region
644645
}
645646

646647
if f.Zone != "" {
647-
m.Labels["machine.openshift.io/zone"] = f.Zone
648+
m.Labels[consts.MAPIMachineMetadataLabelZone] = f.Zone
648649
}
649650

650651
if len(m.Labels) == 0 {

0 commit comments

Comments
 (0)