Refactor get machine util functions

This patch refactors the VSphereMachine Service interface to expose a
new method to get the VSphereVM objects assoicated with a Cluster
object.
The idea is to reuse the VSphereMachine service without the need to
repeat similar code blocks in two places.

controllers/vspherecluster_controller.go

@@ -40,6 +40,7 @@ import (
 	"sigs.k8s.io/cluster-api-provider-vsphere/feature"
 	capvcontext "sigs.k8s.io/cluster-api-provider-vsphere/pkg/context"
 	inframanager "sigs.k8s.io/cluster-api-provider-vsphere/pkg/manager"
+	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/services"
 	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/services/vmoperator"
 )
 
@@ -86,6 +87,7 @@ func AddClusterControllerToManager(ctx context.Context, controllerManagerCtx *ca
 		ControllerManagerContext: controllerManagerCtx,
 		Client:                   controllerManagerCtx.Client,
 		clusterModuleReconciler:  NewReconciler(controllerManagerCtx),
+		vmService:                services.VimMachineService{Client: controllerManagerCtx.Client},
 	}
 	clusterToInfraFn := clusterToInfrastructureMapFunc(ctx, controllerManagerCtx)
 	c, err := ctrl.NewControllerManagedBy(mgr).

controllers/vspherecluster_reconciler.go

@@ -45,6 +45,7 @@ import (
 	"sigs.k8s.io/cluster-api-provider-vsphere/feature"
 	capvcontext "sigs.k8s.io/cluster-api-provider-vsphere/pkg/context"
 	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/identity"
+	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/services"
 	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/session"
 	infrautilv1 "sigs.k8s.io/cluster-api-provider-vsphere/pkg/util"
 )
@@ -53,6 +54,7 @@ type clusterReconciler struct {
 	ControllerManagerContext *capvcontext.ControllerManagerContext
 	Client                   client.Client
 
+	vmService               services.VimMachineService
 	clusterModuleReconciler Reconciler
 }
 
@@ -126,7 +128,7 @@ func (r *clusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 func (r *clusterReconciler) reconcileDelete(ctx context.Context, clusterCtx *capvcontext.ClusterContext) (reconcile.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	vsphereMachines, err := infrautilv1.GetVSphereMachinesInCluster(ctx, r.Client, clusterCtx.Cluster.Namespace, clusterCtx.Cluster.Name)
+	vsphereMachines, err := r.vmService.GetMachinesInCluster(ctx, clusterCtx.Cluster.Namespace, clusterCtx.Cluster.Name)
 	if err != nil {
 		return reconcile.Result{}, pkgerrors.Wrapf(err,
 			"unable to list VSphereMachines part of VSphereCluster %s/%s", clusterCtx.VSphereCluster.Namespace, clusterCtx.VSphereCluster.Name)

controllers/vspheremachine_controller.go

@@ -108,7 +108,7 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 			).
 			Watches(
 				&clusterv1.Cluster{},
-				handler.EnqueueRequestsFromMapFunc(r.clusterToVMwareMachines),
+				handler.EnqueueRequestsFromMapFunc(r.enqueueClusterToMachineRequests),
 				ctrlbldr.WithPredicates(
 					predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx)),
 				),
@@ -161,7 +161,7 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 		).
 		Watches(
 			&clusterv1.Cluster{},
-			handler.EnqueueRequestsFromMapFunc(r.clusterToVSphereMachines),
+			handler.EnqueueRequestsFromMapFunc(r.enqueueClusterToMachineRequests),
 			ctrlbldr.WithPredicates(
 				predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx)),
 			),
@@ -408,42 +408,6 @@ func (r *machineReconciler) patchMachineLabelsWithHostInfo(ctx context.Context,
 	return patchHelper.Patch(ctx, machine)
 }
 
-func (r *machineReconciler) clusterToVSphereMachines(ctx context.Context, a client.Object) []reconcile.Request {
-	requests := []reconcile.Request{}
-	machines, err := util.GetVSphereMachinesInCluster(ctx, r.Client, a.GetNamespace(), a.GetName())
-	if err != nil {
-		return requests
-	}
-	for _, m := range machines {
-		r := reconcile.Request{
-			NamespacedName: apitypes.NamespacedName{
-				Name:      m.Name,
-				Namespace: m.Namespace,
-			},
-		}
-		requests = append(requests, r)
-	}
-	return requests
-}
-
-func (r *machineReconciler) clusterToVMwareMachines(ctx context.Context, a client.Object) []reconcile.Request {
-	requests := []reconcile.Request{}
-	machines, err := util.GetVMwareMachinesInCluster(ctx, r.Client, a.GetNamespace(), a.GetName())
-	if err != nil {
-		return requests
-	}
-	for _, m := range machines {
-		r := reconcile.Request{
-			NamespacedName: apitypes.NamespacedName{
-				Name:      m.Name,
-				Namespace: m.Namespace,
-			},
-		}
-		requests = append(requests, r)
-	}
-	return requests
-}
-
 // Return hooks that will be invoked when a VirtualMachine is created.
 func (r *machineReconciler) setVMModifiers(ctx context.Context, machineCtx capvcontext.MachineContext) error {
 	log := ctrl.LoggerFrom(ctx)
@@ -465,3 +429,23 @@ func (r *machineReconciler) setVMModifiers(ctx context.Context, machineCtx capvc
 	supervisorMachineCtx.VMModifiers = []vmware.VMModifier{networkModifier}
 	return nil
 }
+
+// enqueueClusterToMachineRequests returns a list of VSphereMachine reconcile requests
+// belonging to the cluster.
+func (r *machineReconciler) enqueueClusterToMachineRequests(ctx context.Context, a client.Object) []reconcile.Request {
+	requests := []reconcile.Request{}
+	machines, err := r.VMService.GetMachinesInCluster(ctx, a.GetNamespace(), a.GetName())
+	if err != nil {
+		return requests
+	}
+	for _, m := range machines {
+		r := reconcile.Request{
+			NamespacedName: apitypes.NamespacedName{
+				Name:      m.GetName(),
+				Namespace: m.GetNamespace(),
+			},
+		}
+		requests = append(requests, r)
+	}
+	return requests
+}

pkg/services/interfaces.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
@@ -32,6 +33,7 @@ import (
 
 // VSphereMachineService is used for vsphere VM lifecycle and syncing with VSphereMachine types.
 type VSphereMachineService interface {
+	GetMachinesInCluster(ctx context.Context, namespace, clusterName string) ([]client.Object, error)
 	FetchVSphereMachine(ctx context.Context, name types.NamespacedName) (capvcontext.MachineContext, error)
 	FetchVSphereCluster(ctx context.Context, cluster *clusterv1.Cluster, machineContext capvcontext.MachineContext) (capvcontext.MachineContext, error)
 	ReconcileDelete(ctx context.Context, machineCtx capvcontext.MachineContext) error

pkg/services/vimmachine.go

@@ -47,6 +47,28 @@ type VimMachineService struct {
 	Client client.Client
 }
 
+// GetMachinesInCluster returns a list of VSphereMachine objects belonging to the cluster.
+func (v *VimMachineService) GetMachinesInCluster(
+	ctx context.Context,
+	namespace, clusterName string) ([]client.Object, error) {
+	labels := map[string]string{clusterv1.ClusterNameLabel: clusterName}
+	machineList := &infrav1.VSphereMachineList{}
+
+	if err := v.Client.List(
+		ctx, machineList,
+		client.InNamespace(namespace),
+		client.MatchingLabels(labels)); err != nil {
+		return nil, err
+	}
+
+	objects := []client.Object{}
+	for _, machine := range machineList.Items {
+		m := machine
+		objects = append(objects, &m)
+	}
+	return objects, nil
+}
+
 // FetchVSphereMachine returns a new MachineContext containing the vsphereMachine.
 func (v *VimMachineService) FetchVSphereMachine(ctx context.Context, name types.NamespacedName) (capvcontext.MachineContext, error) {
 	vsphereMachine := &infrav1.VSphereMachine{}

pkg/services/vmoperator/vmopmachine.go

@@ -26,7 +26,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
+	apitypes "k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/conditions"
@@ -46,8 +46,30 @@ type VmopMachineService struct {
 	Client client.Client
 }
 
+// GetMachinesInCluster returns a list of VSphereMachine objects belonging to the cluster.
+func (v *VmopMachineService) GetMachinesInCluster(
+	ctx context.Context,
+	namespace, clusterName string) ([]client.Object, error) {
+	labels := map[string]string{clusterv1.ClusterNameLabel: clusterName}
+	machineList := &vmwarev1.VSphereMachineList{}
+
+	if err := v.Client.List(
+		ctx, machineList,
+		client.InNamespace(namespace),
+		client.MatchingLabels(labels)); err != nil {
+		return nil, err
+	}
+
+	objects := []client.Object{}
+	for _, machine := range machineList.Items {
+		m := machine
+		objects = append(objects, &m)
+	}
+	return objects, nil
+}
+
 // FetchVSphereMachine returns a MachineContext with a VSphereMachine for the passed NamespacedName.
-func (v *VmopMachineService) FetchVSphereMachine(ctx context.Context, name types.NamespacedName) (capvcontext.MachineContext, error) {
+func (v *VmopMachineService) FetchVSphereMachine(ctx context.Context, name apitypes.NamespacedName) (capvcontext.MachineContext, error) {
 	vsphereMachine := &vmwarev1.VSphereMachine{}
 	err := v.Client.Get(ctx, name, vsphereMachine)
 	return &vmware.SupervisorMachineContext{VSphereMachine: vsphereMachine}, err
@@ -82,17 +104,17 @@ func (v *VmopMachineService) ReconcileDelete(ctx context.Context, machineCtx cap
 
 	// If debug logging is enabled, report the number of vms in the cluster before and after the reconcile
 	if log.V(5).Enabled() {
-		vms, err := v.getVirtualMachinesInCluster(ctx, supervisorMachineCtx)
+		vms, err := v.GetVirtualMachinesInCluster(ctx, supervisorMachineCtx)
 		log.V(5).Info("Trace Destroy PRE: VirtualMachines", "vmcount", len(vms), "err", err)
 		defer func() {
-			vms, err := v.getVirtualMachinesInCluster(ctx, supervisorMachineCtx)
+			vms, err := v.GetVirtualMachinesInCluster(ctx, supervisorMachineCtx)
 			log.V(5).Info("Trace Destroy POST: VirtualMachines", "vmcount", len(vms), "err", err)
 		}()
 	}
 
 	// First, check to see if it's already deleted
 	vmopVM := vmoprv1.VirtualMachine{}
-	if err := v.Client.Get(ctx, types.NamespacedName{Namespace: supervisorMachineCtx.Machine.Namespace, Name: supervisorMachineCtx.Machine.Name}, &vmopVM); err != nil {
+	if err := v.Client.Get(ctx, apitypes.NamespacedName{Namespace: supervisorMachineCtx.Machine.Namespace, Name: supervisorMachineCtx.Machine.Name}, &vmopVM); err != nil {
 		// If debug logging is enabled, report the number of vms in the cluster before and after the reconcile
 		if apierrors.IsNotFound(err) {
 			supervisorMachineCtx.VSphereMachine.Status.VMStatus = vmwarev1.VirtualMachineStateNotFound
@@ -143,10 +165,10 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 
 	// If debug logging is enabled, report the number of vms in the cluster before and after the reconcile
 	if log.V(5).Enabled() {
-		vms, err := v.getVirtualMachinesInCluster(ctx, supervisorMachineCtx)
+		vms, err := v.GetVirtualMachinesInCluster(ctx, supervisorMachineCtx)
 		log.V(5).Info("Trace ReconcileVM PRE: VirtualMachines", "vmcount", len(vms), "err", err)
 		defer func() {
-			vms, err = v.getVirtualMachinesInCluster(ctx, supervisorMachineCtx)
+			vms, err = v.GetVirtualMachinesInCluster(ctx, supervisorMachineCtx)
 			log.V(5).Info("Trace ReconcileVM POST: VirtualMachines", "vmcount", len(vms), "err", err)
 		}()
 	}
@@ -393,9 +415,9 @@ func (v *VmopMachineService) reconcileProviderID(ctx context.Context, supervisor
 	}
 }
 
-// getVirtualMachinesInCluster returns all VMOperator VirtualMachine objects in the current cluster.
-// First filter by clusterSelectorKey. If the result is empty, they fall back to legacyClusterSelectorKey.
-func (v *VmopMachineService) getVirtualMachinesInCluster(ctx context.Context, supervisorMachineCtx *vmware.SupervisorMachineContext) ([]*vmoprv1.VirtualMachine, error) {
+// GetVirtualMachinesInCluster returns all VMOperator VirtualMachine objects in the current cluster.
+// First filter by ClusterSelectorKey. If the result is empty, they fall back to LegacyClusterSelectorKey.
+func (v *VmopMachineService) GetVirtualMachinesInCluster(ctx context.Context, supervisorMachineCtx *vmware.SupervisorMachineContext) ([]*vmoprv1.VirtualMachine, error) {
 	if supervisorMachineCtx.Cluster == nil {
 		return []*vmoprv1.VirtualMachine{}, errors.Errorf("No cluster is set for machine %s in namespace %s", supervisorMachineCtx.GetVSphereMachine().GetName(), supervisorMachineCtx.GetVSphereMachine().GetNamespace())
 	}

pkg/services/vmoperator/vmopmachine_test.go

@@ -31,9 +31,11 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/conditions"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
 	vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/context/fake"
 	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/context/vmware"
 	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/util"
 )
@@ -57,20 +59,22 @@ func updateReconciledVMStatus(ctx context.Context, vmService VmopMachineService,
 	Expect(err).ShouldNot(HaveOccurred())
 }
 
+const (
+	machineName              = "test-machine"
+	clusterName              = "test-cluster"
+	controlPlaneLabelTrue    = true
+	k8sVersion               = "test-k8sVersion"
+	className                = "test-className"
+	imageName                = "test-imageName"
+	storageClass             = "test-storageClass"
+	minHardwareVersion       = int32(17)
+	vmIP                     = "127.0.0.1"
+	biosUUID                 = "test-biosUuid"
+	missingK8SVersionFailure = "missing kubernetes version"
+)
+
 var _ = Describe("VirtualMachine tests", func() {
-	const (
-		machineName              = "test-machine"
-		clusterName              = "test-cluster"
-		controlPlaneLabelTrue    = true
-		k8sVersion               = "test-k8sVersion"
-		className                = "test-className"
-		imageName                = "test-imageName"
-		storageClass             = "test-storageClass"
-		minHardwareVersion       = int32(17)
-		vmIP                     = "127.0.0.1"
-		biosUUID                 = "test-biosUuid"
-		missingK8SVersionFailure = "missing kubernetes version"
-	)
+
 	var (
 		bootstrapData = "test-bootstrap-data"
 
@@ -91,7 +95,6 @@ var _ = Describe("VirtualMachine tests", func() {
 		vsphereMachine           *vmwarev1.VSphereMachine
 		supervisorMachineContext *vmware.SupervisorMachineContext
 
-		// vm     vmwarev1.VirtualMachine
 		vmopVM    *vmoprv1.VirtualMachine
 		vmService VmopMachineService
 	)
@@ -135,7 +138,7 @@ var _ = Describe("VirtualMachine tests", func() {
 			Expect(vmopVM != nil).Should(Equal(expectVMOpVM))
 
 			if vmopVM != nil {
-				vms, _ := vmService.getVirtualMachinesInCluster(ctx, machineContext)
+				vms, _ := vmService.GetVirtualMachinesInCluster(ctx, machineContext)
 				Expect(vms).Should(HaveLen(1))
 				Expect(vmopVM.Spec.ImageName).To(Equal(expectedImageName))
 				Expect(vmopVM.Spec.ClassName).To(Equal(className))
@@ -595,3 +598,23 @@ var _ = Describe("VirtualMachine tests", func() {
 		})
 	})
 })
+
+var _ = Describe("GetMachinesInCluster", func() {
+
+	initObjs := []client.Object{
+		util.CreateVSphereMachine(machineName, clusterName, className, imageName, storageClass, controlPlaneLabelTrue),
+	}
+
+	controllerManagerContext := fake.NewControllerManagerContext(initObjs...)
+	vmService := VmopMachineService{Client: controllerManagerContext.Client}
+
+	It("returns a list of VMs belonging to the cluster", func() {
+		objs, err := vmService.GetMachinesInCluster(context.TODO(),
+			corev1.NamespaceDefault,
+			clusterName)
+
+		Expect(err).ToNot(HaveOccurred())
+		Expect(objs).To(HaveLen(1))
+		Expect(objs[0].GetName()).To(Equal(machineName))
+	})
+})