Merge pull request #537 from rccrdpccl/improve-machine-handling

ACM-30119: improve controlplane machine handling

Author: openshift-merge-bot[bot]

controlplane-components.yaml

@@ -1411,6 +1411,14 @@ rules:
   - list
   - update
   - watch
+- apiGroups:
+  - apiextensions.k8s.io
+  resources:
+  - customresourcedefinitions
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - bootstrap.cluster.x-k8s.io
   resources:

controlplane/config/rbac/role.yaml

@@ -27,6 +27,14 @@ rules:
   - list
   - update
   - watch
+- apiGroups:
+  - apiextensions.k8s.io
+  resources:
+  - customresourcedefinitions
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - bootstrap.cluster.x-k8s.io
   resources:

controlplane/internal/controller/openshiftassistedcontrolplane_controller.go

@@ -44,6 +44,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -78,6 +79,7 @@ type OpenshiftAssistedControlPlaneReconciler struct {
 
 var minVersion = semver.MustParse(minOpenShiftVersion)
 
+// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch
 // +kubebuilder:rbac:groups=bootstrap.cluster.x-k8s.io,resources=openshiftassistedconfigs,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=*,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinedeployments,verbs=get;list;watch
@@ -447,8 +449,8 @@ func (r *OpenshiftAssistedControlPlaneReconciler) computeDesiredMachine(oacp *co
 	desiredMachine.Spec.Deletion = oacp.Spec.MachineTemplate.Deletion
 	desiredMachine.Spec.FailureDomain = failureDomain
 
-	// Note: by setting the ownerRef on creation we signal to the Machine controller that this is not a stand-alone Machine.
-	_ = controllerutil.SetOwnerReference(oacp, desiredMachine, r.Scheme)
+	// Note: by setting the controller ownerRef on creation we signal to the Machine controller that this is not a stand-alone Machine.
+	_ = controllerutil.SetControllerReference(oacp, desiredMachine, r.Scheme)
 
 	// Set the in-place mutable fields.
 	// When we create a new Machine we will just create the Machine with those fields.
@@ -533,7 +535,6 @@ func (r *OpenshiftAssistedControlPlaneReconciler) reconcileReplicas(ctx context.
 	numMachines := machines.Len()
 	desiredReplicas := int(oacp.Spec.Replicas)
 	machinesToCreate := desiredReplicas - numMachines
-	var errs []error
 	if machinesToCreate > 0 {
 		fd, err := failuredomains.NextFailureDomainForScaleUp(ctx, cluster, machines)
 		if err != nil {
@@ -560,12 +561,12 @@ func (r *OpenshiftAssistedControlPlaneReconciler) reconcileReplicas(ctx context.
 	log.V(logutil.DebugLevel).Info("updating replica status", "oacp", oacp, "machines", machines)
 
 	r.updateReplicaStatus(ctx, oacp, machines)
-	return kerrors.NewAggregate(errs)
+	return nil
 }
 
 func (r *OpenshiftAssistedControlPlaneReconciler) scaleUpControlPlane(ctx context.Context, oacp *controlplanev1alpha3.OpenshiftAssistedControlPlane, cluster *clusterv1.Cluster, failureDomain string) (*clusterv1.Machine, error) {
 	name := names.SimpleNameGenerator.GenerateName(oacp.Name + "-")
-	machine, err := r.generateMachine(ctx, oacp, name, cluster, failureDomain)
+	machine, infraObj, err := r.generateMachine(ctx, oacp, name, cluster, failureDomain)
 	if err != nil {
 		return nil, err
 	}
@@ -574,6 +575,9 @@ func (r *OpenshiftAssistedControlPlaneReconciler) scaleUpControlPlane(ctx contex
 	if err := r.Create(ctx, bootstrapConfig); err != nil {
 		setConditionFalse(oacp, controlplanev1alpha3.MachinesCreatedCondition, controlplanev1alpha3.BootstrapTemplateCloningFailedReason,
 			"error creating bootstrap config: %v", err)
+		if deleteInfraErr := r.Delete(ctx, infraObj); deleteInfraErr != nil {
+			err = errors.Join(err, deleteInfraErr)
+		}
 		return nil, err
 	}
 	machine.Spec.Bootstrap.ConfigRef = clusterv1.ContractVersionedObjectReference{
@@ -587,9 +591,8 @@ func (r *OpenshiftAssistedControlPlaneReconciler) scaleUpControlPlane(ctx contex
 		if deleteBootstrapErr := r.Delete(ctx, bootstrapConfig); deleteBootstrapErr != nil {
 			err = errors.Join(err, deleteBootstrapErr)
 		}
-		infraRefKey := &corev1.ObjectReference{Name: machine.Spec.InfrastructureRef.Name, Namespace: machine.Namespace}
-		if deleteInfraRefErr := external.Delete(ctx, r.Client, infraRefKey); deleteInfraRefErr != nil {
-			err = errors.Join(err, deleteInfraRefErr)
+		if deleteInfraErr := r.Delete(ctx, infraObj); deleteInfraErr != nil {
+			err = errors.Join(err, deleteInfraErr)
 		}
 		return nil, err
 	}
@@ -697,18 +700,17 @@ func (r *OpenshiftAssistedControlPlaneReconciler) updateReplicaStatus(ctx contex
 	}
 }
 
-func (r *OpenshiftAssistedControlPlaneReconciler) generateMachine(ctx context.Context, oacp *controlplanev1alpha3.OpenshiftAssistedControlPlane, name string, cluster *clusterv1.Cluster, failureDomain string) (*clusterv1.Machine, error) {
-	// Compute desired Machine
+func (r *OpenshiftAssistedControlPlaneReconciler) generateMachine(ctx context.Context, oacp *controlplanev1alpha3.OpenshiftAssistedControlPlane, name string, cluster *clusterv1.Cluster, failureDomain string) (*clusterv1.Machine, *unstructured.Unstructured, error) {
 	machine := r.computeDesiredMachine(oacp, name, cluster, failureDomain)
-	infraRef, err := r.computeInfraRef(ctx, oacp, machine.Name, cluster.Name)
+	infraObj, infraRef, err := r.createInfraMachine(ctx, oacp, machine.Name, cluster.Name)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	machine.Spec.InfrastructureRef = infraRef
-	return machine, nil
+	return machine, infraObj, nil
 }
 
-func (r *OpenshiftAssistedControlPlaneReconciler) computeInfraRef(ctx context.Context, oacp *controlplanev1alpha3.OpenshiftAssistedControlPlane, machineName, clusterName string) (clusterv1.ContractVersionedObjectReference, error) {
+func (r *OpenshiftAssistedControlPlaneReconciler) createInfraMachine(ctx context.Context, oacp *controlplanev1alpha3.OpenshiftAssistedControlPlane, machineName, clusterName string) (*unstructured.Unstructured, clusterv1.ContractVersionedObjectReference, error) {
 	// Since the cloned resource should eventually have a controller ref for the Machine, we create an
 	// OwnerReference here without the Controller field set
 	infraCloneOwner := &metav1.OwnerReference{
@@ -718,13 +720,24 @@ func (r *OpenshiftAssistedControlPlaneReconciler) computeInfraRef(ctx context.Co
 		UID:        oacp.UID,
 	}
 
-	// Convert ContractVersionedObjectReference to corev1.ObjectReference for external.CreateFromTemplate
-	// The external package expects corev1.ObjectReference with APIVersion
-	templateRef := contractVersionedRefToObjectRef(&oacp.Spec.MachineTemplate.InfrastructureRef, oacp.Namespace)
+	// Fetch the infrastructure template using contract-based API version resolution
+	// instead of hardcoding an API version
+	template, err := external.GetObjectFromContractVersionedRef(ctx, r.Client, oacp.Spec.MachineTemplate.InfrastructureRef, oacp.Namespace)
+	if err != nil {
+		setConditionFalse(oacp, controlplanev1alpha3.MachinesCreatedCondition, controlplanev1alpha3.InfrastructureTemplateCloningFailedReason,
+			"error fetching infrastructure template: %v", err)
+		return nil, clusterv1.ContractVersionedObjectReference{}, err
+	}
+
+	templateRef := &corev1.ObjectReference{
+		APIVersion: template.GetAPIVersion(),
+		Kind:       template.GetKind(),
+		Name:       template.GetName(),
+		Namespace:  template.GetNamespace(),
+	}
 
-	// Clone the infrastructure template
-	_, infraRef, err := external.CreateFromTemplate(ctx, &external.CreateFromTemplateInput{
-		Client:      r.Client,
+	infraMachine, err := external.GenerateTemplate(&external.GenerateTemplateInput{
+		Template:    template,
 		TemplateRef: templateRef,
 		Namespace:   oacp.Namespace,
 		Name:        machineName,
@@ -734,12 +747,22 @@ func (r *OpenshiftAssistedControlPlaneReconciler) computeInfraRef(ctx context.Co
 		Annotations: oacp.Spec.MachineTemplate.ObjectMeta.Annotations,
 	})
 	if err != nil {
-		// Safe to return early here since no resources have been created yet.
 		setConditionFalse(oacp, controlplanev1alpha3.MachinesCreatedCondition, controlplanev1alpha3.InfrastructureTemplateCloningFailedReason,
-			"error creating infraenv: %v", err)
-		return clusterv1.ContractVersionedObjectReference{}, err
+			"error generating infrastructure clone: %v", err)
+		return nil, clusterv1.ContractVersionedObjectReference{}, err
+	}
+
+	if err := r.Create(ctx, infraMachine); err != nil {
+		setConditionFalse(oacp, controlplanev1alpha3.MachinesCreatedCondition, controlplanev1alpha3.InfrastructureTemplateCloningFailedReason,
+			"error creating infrastructure clone: %v", err)
+		return nil, clusterv1.ContractVersionedObjectReference{}, err
 	}
-	return infraRef, nil
+
+	return infraMachine, clusterv1.ContractVersionedObjectReference{
+		APIGroup: infraMachine.GroupVersionKind().Group,
+		Kind:     infraMachine.GetKind(),
+		Name:     infraMachine.GetName(),
+	}, nil
 }
 
 func (r *OpenshiftAssistedControlPlaneReconciler) generateOpenshiftAssistedConfig(oacp *controlplanev1alpha3.OpenshiftAssistedControlPlane, clusterName string, name string) *bootstrapv1alpha2.OpenshiftAssistedConfig {
@@ -828,24 +851,6 @@ func selectMachineForScaleDown(eligibleMachines collections.Machines, failureDom
 	return machineToScaleDown, nil
 }
 
-// contractVersionedRefToObjectRef converts ContractVersionedObjectReference (with APIGroup)
-// to corev1.ObjectReference (with APIVersion) for use with external.CreateFromTemplate.
-// Since ContractVersionedObjectReference only has APIGroup, we use the contract version convention
-// and default to v1beta1 for infrastructure providers.
-func contractVersionedRefToObjectRef(in *clusterv1.ContractVersionedObjectReference, namespace string) *corev1.ObjectReference {
-	apiVersion := ""
-	if in.APIGroup != "" {
-		// Default to v1beta1 for infrastructure group, as that's the common convention
-		apiVersion = in.APIGroup + "/v1beta1"
-	}
-	return &corev1.ObjectReference{
-		Kind:       in.Kind,
-		Name:       in.Name,
-		Namespace:  namespace,
-		APIVersion: apiVersion,
-	}
-}
-
 // Condition helper functions for setting metav1.Condition (new format).
 
 // setConditionTrue sets a condition to True.

controlplane/internal/controller/openshiftassistedcontrolplane_controller_test.go

@@ -48,6 +48,7 @@ import (
 
 	testutils "github.com/openshift-assisted/cluster-api-provider-openshift-assisted/test/utils"
 	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 )
@@ -597,6 +598,7 @@ var _ = Describe("Scale operations and machine updates", func() {
 		k8sVersion := "1.30.0"
 		mockKubernetesVersionDetector.EXPECT().GetKubernetesVersion(gomock.Any(), gomock.Any()).Return(&k8sVersion, nil).AnyTimes()
 
+		Expect(k8sClient.Create(ctx, getMetal3MachineTemplateCRD())).To(Succeed())
 		machineTemplate := getMachineTemplate("infratemplate", namespace)
 		Expect(k8sClient.Create(ctx, &machineTemplate)).To(Succeed())
 
@@ -1062,7 +1064,6 @@ var _ = Describe("Scale operations and machine updates", func() {
 	})
 })
 
-// Create dummy machine template
 func getMachineTemplate(name string, namespace string) metal3v1beta1.Metal3MachineTemplate {
 	return metal3v1beta1.Metal3MachineTemplate{
 		TypeMeta: metav1.TypeMeta{
@@ -1077,6 +1078,28 @@ func getMachineTemplate(name string, namespace string) metal3v1beta1.Metal3Machi
 	}
 }
 
+func getMetal3MachineTemplateCRD() *apiextensionsv1.CustomResourceDefinition {
+	return &apiextensionsv1.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "metal3machinetemplates.infrastructure.cluster.x-k8s.io",
+			Labels: map[string]string{
+				"cluster.x-k8s.io/v1beta1": "v1beta1",
+			},
+		},
+		Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+			Group: "infrastructure.cluster.x-k8s.io",
+			Names: apiextensionsv1.CustomResourceDefinitionNames{
+				Kind:   "Metal3MachineTemplate",
+				Plural: "metal3machinetemplates",
+			},
+			Scope: apiextensionsv1.NamespaceScoped,
+			Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
+				{Name: "v1beta1", Served: true, Storage: true},
+			},
+		},
+	}
+}
+
 func checkReadyConditions(expectedReadyConditions []clusterv1.ConditionType, openshiftAssistedControlPlane *controlplanev1alpha3.OpenshiftAssistedControlPlane) {
 	for _, conditionType := range expectedReadyConditions {
 		By(fmt.Sprintf("checking condition %s ready", conditionType))

controlplane/internal/controller/suite_test.go

@@ -22,6 +22,7 @@ import (
 	metal3v1beta1 "github.com/metal3-io/cluster-api-provider-metal3/api/v1beta1"
 	bootstrapv1alpha2 "github.com/openshift-assisted/cluster-api-provider-openshift-assisted/bootstrap/api/v1alpha2"
 
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 
@@ -63,5 +64,6 @@ var _ = BeforeSuite(func() {
 	utilruntime.Must(hiveext.AddToScheme(testScheme))
 	utilruntime.Must(metal3v1beta1.AddToScheme(testScheme))
 	utilruntime.Must(bootstrapv1alpha2.AddToScheme(testScheme))
+	utilruntime.Must(apiextensionsv1.AddToScheme(testScheme))
 
 })

test/e2e/manifests/capcoa/controlplane_install.yaml

@@ -1411,6 +1411,14 @@ rules:
   - list
   - update
   - watch
+- apiGroups:
+  - apiextensions.k8s.io
+  resources:
+  - customresourcedefinitions
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - bootstrap.cluster.x-k8s.io
   resources: