From 6e80365c9f7920fbab9c320e489b0da12f25bec1 Mon Sep 17 00:00:00 2001 From: Sagar Muchhal Date: Wed, 27 Dec 2023 10:53:21 -0800 Subject: [PATCH] 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 | 2 + controllers/vspherecluster_reconciler.go | 4 +- controllers/vspheremachine_controller.go | 60 ++++++++------------- pkg/services/interfaces.go | 2 + pkg/services/vimmachine.go | 22 ++++++++ pkg/services/vmoperator/vmopmachine.go | 42 +++++++++++---- pkg/services/vmoperator/vmopmachine_test.go | 53 ++++++++++++------ pkg/util/machines.go | 46 ---------------- 8 files changed, 121 insertions(+), 110 deletions(-) diff --git a/controllers/vspherecluster_controller.go b/controllers/vspherecluster_controller.go index 0371f55c18..b2aff416c4 100644 --- a/controllers/vspherecluster_controller.go +++ b/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). diff --git a/controllers/vspherecluster_reconciler.go b/controllers/vspherecluster_reconciler.go index 9cf6331caa..dfe9f07146 100644 --- a/controllers/vspherecluster_reconciler.go +++ b/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) diff --git a/controllers/vspheremachine_controller.go b/controllers/vspheremachine_controller.go index b10cf04585..1ff3ce3d5b 100644 --- a/controllers/vspheremachine_controller.go +++ b/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 +} diff --git a/pkg/services/interfaces.go b/pkg/services/interfaces.go index e7169187d4..6404ee398d 100644 --- a/pkg/services/interfaces.go +++ b/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 diff --git a/pkg/services/vimmachine.go b/pkg/services/vimmachine.go index f5d3650984..d8e49234ed 100644 --- a/pkg/services/vimmachine.go +++ b/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{} diff --git a/pkg/services/vmoperator/vmopmachine.go b/pkg/services/vmoperator/vmopmachine.go index c9ebe5e38d..535ccfc812 100644 --- a/pkg/services/vmoperator/vmopmachine.go +++ b/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()) } diff --git a/pkg/services/vmoperator/vmopmachine_test.go b/pkg/services/vmoperator/vmopmachine_test.go index 2b85e8bcbd..2cfc70463d 100644 --- a/pkg/services/vmoperator/vmopmachine_test.go +++ b/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)) + }) +}) diff --git a/pkg/util/machines.go b/pkg/util/machines.go index 142a3c451d..124b6fbb85 100644 --- a/pkg/util/machines.go +++ b/pkg/util/machines.go @@ -36,52 +36,6 @@ import ( vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1" ) -// GetVSphereMachinesInCluster gets a cluster's VSphereMachine resources. -func GetVSphereMachinesInCluster( - ctx context.Context, - controllerClient client.Client, - namespace, clusterName string) ([]*infrav1.VSphereMachine, error) { - labels := map[string]string{clusterv1.ClusterNameLabel: clusterName} - machineList := &infrav1.VSphereMachineList{} - - if err := controllerClient.List( - ctx, machineList, - client.InNamespace(namespace), - client.MatchingLabels(labels)); err != nil { - return nil, err - } - - machines := make([]*infrav1.VSphereMachine, len(machineList.Items)) - for i := range machineList.Items { - machines[i] = &machineList.Items[i] - } - - return machines, nil -} - -// GetVMwareMachinesInCluster gets a cluster's vmware VSphereMachine resources. -func GetVMwareMachinesInCluster( - ctx context.Context, - controllerClient client.Client, - namespace, clusterName string) ([]*vmwarev1.VSphereMachine, error) { - labels := map[string]string{clusterv1.ClusterNameLabel: clusterName} - machineList := &vmwarev1.VSphereMachineList{} - - if err := controllerClient.List( - ctx, machineList, - client.InNamespace(namespace), - client.MatchingLabels(labels)); err != nil { - return nil, err - } - - machines := make([]*vmwarev1.VSphereMachine, len(machineList.Items)) - for i := range machineList.Items { - machines[i] = &machineList.Items[i] - } - - return machines, nil -} - // GetVSphereMachine gets a vmware.infrastructure.cluster.x-k8s.io.VSphereMachine resource for the given CAPI Machine. func GetVSphereMachine( ctx context.Context,