Merge pull request #365 from damdo/machine-status-conversion-and-syncing

OCPCLOUD-2995: Machine status conversion and syncing

Author: openshift-merge-bot[bot]

pkg/controllers/machinesetsync/machineset_sync_controller.go

@@ -1072,7 +1072,7 @@ func setChangedMAPIMachineSetStatusFields(existingMAPIMachineSet, convertedMAPIM
 	existingMAPIMachineSet.Status.ErrorMessage = convertedMAPIMachineSet.Status.ErrorMessage
 
 	for i := range convertedMAPIMachineSet.Status.Conditions {
-		existingMAPIMachineSet.Status.Conditions = util.SetMAPIMachineSetCondition(existingMAPIMachineSet.Status.Conditions, &convertedMAPIMachineSet.Status.Conditions[i])
+		existingMAPIMachineSet.Status.Conditions = util.SetMAPICondition(existingMAPIMachineSet.Status.Conditions, &convertedMAPIMachineSet.Status.Conditions[i])
 	}
 }
 

pkg/controllers/machinesync/machine_sync_controller.go

@@ -127,7 +127,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
 		// reference it on the CAPI Machine
 		capaMachineBuilder = awsv1resourcebuilder.AWSMachine().
 			WithNamespace(capiNamespace.GetName()).
-			WithName("machine-template")
+			WithName("foo")
 
 		mapiMachineSetBuilder = machinev1resourcebuilder.MachineSet().
 			WithNamespace(mapiNamespace.GetName()).
@@ -138,7 +138,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
 		// reference it on the CAPI MachineSet
 		capaMachineTemplateBuilder := awsv1resourcebuilder.AWSMachineTemplate().
 			WithNamespace(capiNamespace.GetName()).
-			WithName("machine-template")
+			WithName("foo")
 
 		capaMachineTemplate := capaMachineTemplateBuilder.Build()
 
@@ -168,7 +168,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
 
 		mapiMachineBuilder = machinev1resourcebuilder.Machine().
 			WithNamespace(mapiNamespace.GetName()).
-			WithGenerateName("foo").
+			WithName("foo").
 			WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil))
 
 		capiMachineBuilder = clusterv1resourcebuilder.Machine().
@@ -333,11 +333,11 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
 			BeforeEach(func() {
 
 				By("Creating the MAPI machine")
-				mapiMachine = mapiMachineBuilder.WithName("test-machine").Build()
+				mapiMachine = mapiMachineBuilder.Build()
 				Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created")
 
 				By("Creating the CAPI Machine")
-				capiMachine = capiMachineBuilder.WithName("test-machine").Build()
+				capiMachine = capiMachineBuilder.Build()
 				Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created")
 
 				By("Setting the MAPI machine AuthoritativeAPI to Cluster API")
@@ -514,21 +514,21 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
 		Context("when the MAPI machine does not exist and the CAPI machine does", func() {
 			Context("and there is no CAPI machineSet owning the machine", func() {
 				BeforeEach(func() {
-					capiMachine = capiMachineBuilder.WithName("test-machine-no-machineset").Build()
+					By("Creating the CAPI machine")
+					capiMachine = capiMachineBuilder.Build()
 					Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created")
 
-					capaMachine = capaMachineBuilder.WithName("test-machine-no-machineset").WithOwnerReferences([]metav1.OwnerReference{
-						{
+					By("Updating the CAPA machine adding the CAPI machine as an owner")
+					Eventually(k.Update(capaMachine, func() {
+						capaMachine.OwnerReferences = append(capaMachine.OwnerReferences, metav1.OwnerReference{
 							Kind:               machineKind,
 							APIVersion:         clusterv1.GroupVersion.String(),
 							Name:               capiMachine.Name,
 							UID:                capiMachine.UID,
 							BlockOwnerDeletion: ptr.To(true),
 							Controller:         ptr.To(false),
-						},
-					}).Build()
-
-					Eventually(k8sClient.Create(ctx, capaMachine)).Should(Succeed(), "capa machine should be able to be created")
+						})
+					})).Should(Succeed(), "capa machine should be able to be updated")
 				})
 
 				It("should not make any changes to the CAPI machine", func() {

pkg/controllers/machinesync/machine_sync_controller_test.go

@@ -21,9 +21,12 @@ import (
 
 	mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	capierrors "sigs.k8s.io/cluster-api/errors"
 )
 
 const (
@@ -36,6 +39,11 @@ func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine) (*mapiv1beta1.
 
 	lifecycleHooks, capiMachineNonHookAnnotations := convertCAPILifecycleHookAnnotationsToMAPILifecycleHooksAndAnnotations(capiMachine.Annotations)
 
+	mapiMachineStatus, machineStatusErrs := convertCAPIMachineStatusToMAPI(capiMachine.Status)
+	if len(machineStatusErrs) > 0 {
+		errs = append(errs, machineStatusErrs...)
+	}
+
 	mapiMachine := &mapiv1beta1.Machine{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            capiMachine.Name,
@@ -55,6 +63,7 @@ func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine) (*mapiv1beta1.
 			// ProviderSpec: // ProviderSpec MUST NOT be populated here. It is added later by higher level fuctions.
 			// Taints: // TODO(OCPCLOUD-2861): Taint propagation from Machines to Nodes is not yet implemented in CAPI.
 		},
+		Status: mapiMachineStatus,
 	}
 
 	// Unused fields - Below this line are fields not used from the CAPI Machine.
@@ -99,6 +108,114 @@ func fromCAPIMachineToMAPIMachine(capiMachine *clusterv1.Machine) (*mapiv1beta1.
 	return mapiMachine, nil
 }
 
+// convertCAPIMachineStatusToMAPI converts a CAPI MachineStatus to MAPI format.
+func convertCAPIMachineStatusToMAPI(capiStatus clusterv1.MachineStatus) (mapiv1beta1.MachineStatus, field.ErrorList) {
+	errs := field.ErrorList{}
+
+	addresses, addressesErr := convertCAPIMachineAddressesToMAPI(capiStatus.Addresses)
+	if addressesErr != nil {
+		errs = append(errs, addressesErr...)
+	}
+
+	mapiStatus := mapiv1beta1.MachineStatus{
+		NodeRef:     capiStatus.NodeRef,
+		LastUpdated: capiStatus.LastUpdated,
+		// Conditions:   // TODO(OCPCLOUD-3193): Add MAPI conditions when they are implemented.
+		ErrorReason:  convertCAPIMachineFailureReasonToMAPIErrorReason(capiStatus.FailureReason),
+		ErrorMessage: convertCAPIMachineFailureMessageToMAPIErrorMessage(capiStatus.FailureMessage),
+		Phase:        convertCAPIMachinePhaseToMAPI(capiStatus.Phase),
+		Addresses:    addresses,
+
+		// LastOperation // this is MAPI-specific and not used in CAPI.
+		// ObservedGeneration: // We don't set the observed generation at this stage as it is handled by the machineSync controller.
+		// AuthoritativeAPI: // Ignore, this field as it is not present in CAPI.
+		// SynchronizedGeneration: // Ignore, this field as it is not present in CAPI.
+	}
+
+	// unused fields from CAPI MachineStatus
+	// - NodeInfo: not present on the MAPI Machine status.
+	// - CertificatesExpiryDate: not present on the MAPI Machine status.
+	// - BootstrapReady: this is derived and not stored directly in MAPI.
+	// - InfrastructureReady: this is derived and not stored directly in MAPI.
+	// - Deletion: not present on the MAPI Machine status.
+	// - V1Beta2: for now we use the V1Beta1 status fields to obtain the status of the MAPI Machine.
+
+	return mapiStatus, errs
+}
+
+// convertCAPIMachineAddressesToMAPI converts CAPI machine addresses to MAPI format.
+func convertCAPIMachineAddressesToMAPI(capiAddresses clusterv1.MachineAddresses) ([]corev1.NodeAddress, field.ErrorList) {
+	if capiAddresses == nil {
+		return nil, nil
+	}
+
+	errs := field.ErrorList{}
+	mapiAddresses := make([]corev1.NodeAddress, 0, len(capiAddresses))
+
+	// Addresses are slightly different between MAPI/CAPI.
+	for _, addr := range capiAddresses {
+		switch addr.Type {
+		case clusterv1.MachineHostName:
+			mapiAddresses = append(mapiAddresses, corev1.NodeAddress{Type: corev1.NodeHostName, Address: addr.Address})
+		case clusterv1.MachineExternalIP:
+			mapiAddresses = append(mapiAddresses, corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: addr.Address})
+		case clusterv1.MachineInternalIP:
+			mapiAddresses = append(mapiAddresses, corev1.NodeAddress{Type: corev1.NodeInternalIP, Address: addr.Address})
+		case clusterv1.MachineExternalDNS:
+			mapiAddresses = append(mapiAddresses, corev1.NodeAddress{Type: corev1.NodeExternalDNS, Address: addr.Address})
+		case clusterv1.MachineInternalDNS:
+			mapiAddresses = append(mapiAddresses, corev1.NodeAddress{Type: corev1.NodeInternalDNS, Address: addr.Address})
+		default:
+			errs = append(errs, field.Invalid(field.NewPath("status", "addresses"), string(addr.Type), string(addr.Type)+" unrecognized address type"))
+		}
+	}
+
+	return mapiAddresses, errs
+}
+
+// convertCAPIMachinePhaseToMAPI converts CAPI machine phase to MAPI format.
+func convertCAPIMachinePhaseToMAPI(capiPhase string) *string {
+	// Phase is slightly different between MAPI/CAPI.
+	// In CAPI can be one of: Pending;Provisioning;Provisioned;Running;Deleting;Deleted;Failed;Unknown
+	// In MAPI can be one of: Provisioning;Provisioned;Running;Deleting;Failed (missing Pending,Deleted,Unknown)
+	switch capiPhase {
+	case "":
+		return nil // Empty is equivalent to nil in MAPI.
+	case "Pending":
+		return nil // Pending is not supported in MAPI but is is a very early state so we don't need to represent it.
+	case "Deleted":
+		return ptr.To("Deleting") // Deleted is not supported in MAPI but we can stay in Deleting until the machine is fully removed.
+	case "Unknown":
+		return nil // Unknown is not supported in MAPI but we can set it to nil until we know more.
+	case "Provisioning", "Provisioned", "Running", "Deleting", "Failed":
+		return &capiPhase // This is a supported phase so we can represent it in MAPI.
+	default:
+		return nil // This is an unknown phase so we can't represent it in MAPI.
+	}
+}
+
+// convertCAPIMachineFailureReasonToMAPIErrorReason converts CAPI MachineStatusError to MAPI MachineStatusError.
+func convertCAPIMachineFailureReasonToMAPIErrorReason(capiFailureReason *capierrors.MachineStatusError) *mapiv1beta1.MachineStatusError {
+	if capiFailureReason == nil {
+		return nil
+	}
+
+	mapiErrorReason := mapiv1beta1.MachineStatusError(*capiFailureReason)
+
+	return &mapiErrorReason
+}
+
+// convertCAPIMachineFailureMessageToMAPIErrorMessage converts CAPI MachineStatusError to MAPI MachineStatusError.
+func convertCAPIMachineFailureMessageToMAPIErrorMessage(capiFailureMessage *string) *string {
+	if capiFailureMessage == nil {
+		return nil
+	}
+
+	mapiErrorMessage := *capiFailureMessage
+
+	return &mapiErrorMessage
+}
+
 const (
 	// Note the trailing slash here is important when we are trimming the prefix.
 	capiPreDrainAnnotationPrefix     = clusterv1.PreDrainDeleteHookAnnotationPrefix + "/"

pkg/conversion/capi2mapi/machine.go

@@ -25,10 +25,11 @@ import (
 	capabuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/cluster-api/infrastructure/v1beta2"
 	"github.com/openshift/cluster-capi-operator/pkg/conversion/test/matchers"
 	"github.com/openshift/cluster-capi-operator/pkg/util"
-	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	capierrors "sigs.k8s.io/cluster-api/errors"
 )
 
 var _ = Describe("capi2mapi Machine conversion", func() {
@@ -97,3 +98,72 @@ var _ = Describe("capi2mapi Machine conversion", func() {
 		}),
 	)
 })
+
+var _ = Describe("capi2mapi Machine Status Conversion", func() {
+	Context("when converting CAPI Machine status to MAPI", func() {
+		It("should set all MAPI Machine status fields and conditions to the expected values", func() {
+			// Set CAPI machine status fields
+			nodeRef := &corev1.ObjectReference{
+				Kind:      "Node",
+				Name:      "test-node",
+				Namespace: "",
+			}
+			lastUpdated := &metav1.Time{Time: time.Now()}
+			condition := clusterv1.Condition{
+				Type: "Available", Status: corev1.ConditionTrue,
+				Severity: clusterv1.ConditionSeverityNone,
+				Reason:   "MachineAvailable", Message: "Machine is available",
+			}
+
+			capiMachine := capibuilder.Machine().
+				WithName("test-machine").
+				WithNamespace("test-namespace").
+				WithNodeRef(nodeRef).
+				WithLastUpdated(lastUpdated).
+				WithAddresses(clusterv1.MachineAddresses{
+					{Type: clusterv1.MachineAddressType(corev1.NodeInternalIP), Address: "10.0.0.1"},
+					{Type: clusterv1.MachineAddressType(corev1.NodeExternalIP), Address: "203.0.113.1"},
+				}).
+				WithPhase("Running").
+				WithFailureReason(ptr.To(capierrors.MachineStatusError("InvalidConfiguration"))).
+				WithFailureMessage(ptr.To(string("Test failure message"))).
+				WithConditions([]clusterv1.Condition{condition}).
+				Build()
+
+			mapiStatus, errs := convertCAPIMachineStatusToMAPI(capiMachine.Status)
+			Expect(errs).To(BeEmpty())
+
+			Expect(mapiStatus.NodeRef).To(Equal(nodeRef))
+			Expect(mapiStatus.LastUpdated).To(Equal(lastUpdated))
+			Expect(mapiStatus.Addresses).To(ConsistOf(
+				SatisfyAll(HaveField("Type", corev1.NodeInternalIP), HaveField("Address", "10.0.0.1")),
+				SatisfyAll(HaveField("Type", corev1.NodeExternalIP), HaveField("Address", "203.0.113.1")),
+			))
+
+			Expect(mapiStatus.Phase).To(HaveValue(BeEquivalentTo(mapiv1beta1.PhaseRunning)))
+			Expect(mapiStatus.ErrorReason).To(HaveValue(BeEquivalentTo(mapiv1beta1.MachineStatusError("InvalidConfiguration"))))
+			Expect(mapiStatus.ErrorMessage).To(HaveValue(BeEquivalentTo("Test failure message")))
+
+			// We do not convert these conditions to MAPI conditions as they are not a 1:1 mapping conversion between CAPI and MAPI.
+			Expect(mapiStatus.Conditions).To(BeNil())
+		})
+
+		It("should set all MAPI Machine status fields to empty when CAPI MachineStatus is empty", func() {
+			capiMachine := capibuilder.Machine().
+				WithName("test-machine").
+				WithNamespace("test-namespace").
+				Build()
+
+			mapiStatus, errs := convertCAPIMachineStatusToMAPI(capiMachine.Status)
+			Expect(errs).To(BeEmpty())
+
+			Expect(mapiStatus.NodeRef).To(BeNil())
+			Expect(mapiStatus.LastUpdated).To(BeNil())
+			Expect(mapiStatus.Addresses).To(BeEmpty())
+			Expect(mapiStatus.Phase).To(BeNil())
+			Expect(mapiStatus.ErrorReason).To(BeNil())
+			Expect(mapiStatus.ErrorMessage).To(BeNil())
+			Expect(mapiStatus.Conditions).To(BeEmpty())
+		})
+	})
+})

pkg/conversion/capi2mapi/machine_test.go

@@ -76,7 +76,10 @@ func convertCAPIMachineSetStatusToMAPI(capiStatus clusterv1.MachineSetStatus) ma
 		// ObservedGeneration: // We don't set the observed generation at this stage as it is handled by the machineSetSync controller.
 		// AuthoritativeAPI: // Ignore, this field as it is not present in CAPI.
 		// SynchronizedGeneration: // Ignore, this field as it is not present in CAPI.
-		Conditions: convertCAPIConditionsToMAPI(capiStatus.Conditions),
+
+		// The only two conditions normally used for MAPI MachineSets are Paused and Synchronized.
+		// We do not convert these conditions to MAPI conditions as they are managed directly by the machineSet sync and migration controllers.
+		Conditions: nil,
 	}
 
 	// Convert FailureReason/FailureMessage to ErrorReason/ErrorMessage
@@ -100,10 +103,3 @@ func convertCAPIFailureReasonToMAPIErrorReason(capiFailureReason capierrors.Mach
 	mapiErrorReason := mapiv1beta1.MachineSetStatusError(capiFailureReason)
 	return &mapiErrorReason
 }
-
-// convertCAPIConditionsToMAPI converts CAPI conditions to MAPI conditions.
-func convertCAPIConditionsToMAPI(capiConditions clusterv1.Conditions) []mapiv1beta1.Condition {
-	// The only two conditions normally used for MAPI MachineSets are Paused and Synchronized.
-	// We do not convert these conditions to MAPI conditions as they are managed directly by the machineSet sync and migration controllers.
-	return nil
-}