Skip updating VMOp immutable fields

Signed-off-by: Sagar Muchhal <[email protected]>
kubernetes-sigs · Dec 22, 2023 · b6bd18c · b6bd18c
1 parent d8bb2e3
commit b6bd18c
Show file tree

Hide file tree

Showing 13 changed files with 196 additions and 160 deletions.
diff --git 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
@@ -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, r.Client, 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
@@ -27,7 +27,6 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	apitypes "k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/tools/record"
@@ -108,7 +107,7 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 			).
 			Watches(
 				&clusterv1.Cluster{},
-				handler.EnqueueRequestsFromMapFunc(r.clusterToVMwareMachines),
+				handler.EnqueueRequestsFromMapFunc(r.VMService.EnqueueClusterToMachineRequests),
 				ctrlbldr.WithPredicates(
 					predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx)),
 				),
@@ -161,7 +160,7 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 		).
 		Watches(
 			&clusterv1.Cluster{},
-			handler.EnqueueRequestsFromMapFunc(r.clusterToVSphereMachines),
+			handler.EnqueueRequestsFromMapFunc(r.VMService.EnqueueClusterToMachineRequests),
 			ctrlbldr.WithPredicates(
 				predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx)),
 			),
@@ -408,42 +407,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)

diff --git a/pkg/context/vmware/machine_context.go b/pkg/context/vmware/machine_context.go
@@ -28,7 +28,7 @@ import (
 )
 
 // VMModifier allows a function to be passed to VM creation to modify its spec
-// The hook is loosely typed so as to allow for different VirtualMachine backends.
+// The hook is loosely typed to allow for different VirtualMachine backends.
 type VMModifier func(runtime.Object) (runtime.Object, error)
 
 // SupervisorMachineContext is a Go capvcontext used with a VSphereMachine.

diff --git 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 {
+	EnqueueClusterToMachineRequests(context.Context, client.Object) []reconcile.Request
 	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
@@ -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"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/integer"
@@ -36,6 +36,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
 	capvcontext "sigs.k8s.io/cluster-api-provider-vsphere/pkg/context"
@@ -47,8 +48,46 @@ type VimMachineService struct {
 	Client client.Client
 }
 
+// EnqueueClusterToMachineRequests returns a list of VSphereMachine reconcile requests
+// belonging to the cluster.
+func (v *VimMachineService) EnqueueClusterToMachineRequests(ctx context.Context, a client.Object) []reconcile.Request {
+	requests := []reconcile.Request{}
+	machines, err := v.GetMachinesInCluster(ctx, v.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
+}
+
+// GetMachinesInCluster returns a list of VSphereMachine objects belonging to the cluster.
+func (v *VimMachineService) GetMachinesInCluster(
+	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
+	}
+
+	return machineList.Items, nil
+}
+
 // FetchVSphereMachine returns a new MachineContext containing the vsphereMachine.
-func (v *VimMachineService) FetchVSphereMachine(ctx context.Context, name types.NamespacedName) (capvcontext.MachineContext, error) {
+func (v *VimMachineService) FetchVSphereMachine(ctx context.Context, name apitypes.NamespacedName) (capvcontext.MachineContext, error) {
 	vsphereMachine := &infrav1.VSphereMachine{}
 	err := v.Client.Get(ctx, name, vsphereMachine)
 
@@ -194,7 +233,7 @@ func (v *VimMachineService) GetHostInfo(ctx context.Context, machineCtx capvcont
 func (v *VimMachineService) findVSphereVM(ctx context.Context, vimMachineCtx *capvcontext.VIMMachineContext) (*infrav1.VSphereVM, error) {
 	// Get ready to find the associated VSphereVM resource.
 	vm := &infrav1.VSphereVM{}
-	vmKey := types.NamespacedName{
+	vmKey := apitypes.NamespacedName{
 		Namespace: vimMachineCtx.VSphereMachine.Namespace,
 		Name:      generateVMObjectName(vimMachineCtx, vimMachineCtx.Machine.Name),
 	}

diff --git a/pkg/services/vmoperator/constants.go b/pkg/services/vmoperator/constants.go
@@ -31,3 +31,30 @@ const (
 	// WorkerVMVMAntiAffinityTagValue is the value used for ProviderTagsAnnotationKey when the machine is a worker machine.
 	WorkerVMVMAntiAffinityTagValue = "WorkerVmVmAATag"
 )
+
+const (
+	// ClusterSelectorKey ...
+	ClusterSelectorKey = "capv.vmware.com/cluster.name"
+	// NodeSelectorKey ...
+	NodeSelectorKey = "capv.vmware.com/cluster.role"
+	// RoleNode ...
+	RoleNode = "node"
+	// RoleControlPlane is the constant used to indicate the node is a control plane node.
+	RoleControlPlane = "controlplane"
+
+	// LegacyClusterSelectorKey and LegacyNodeSelectorKey are added for backward compatibility.
+	// These will be removed in the future release.
+	// Please refer to the issue above for deprecation process.
+	// TODO(lubronzhan): Deprecated, will be removed in a future release.
+	// https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/issues/1483
+	//
+	// Deprecated: LegacyClusterSelectorKey will be removed in a future release.
+	LegacyClusterSelectorKey = "capw.vmware.com/cluster.name"
+
+	// LegacyNodeSelectorKey is added for backward compatibility.
+	// These will be removed in the future release.
+	// Please refer to the issue above for deprecation process.
+	//
+	// Deprecated: LegacyClusterSelectorKey will be removed in a future release.
+	LegacyNodeSelectorKey = "capw.vmware.com/cluster.role"
+)
diff --git a/pkg/services/vmoperator/control_plane_endpoint.go b/pkg/services/vmoperator/control_plane_endpoint.go
@@ -39,25 +39,6 @@ import (
 const (
 	defaultAPIBindPort                   = 6443
 	controlPlaneServiceAPIServerPortName = "apiserver"
-
-	clusterSelectorKey = "capv.vmware.com/cluster.name"
-	nodeSelectorKey    = "capv.vmware.com/cluster.role"
-	roleNode           = "node"
-	roleControlPlane   = "controlplane"
-
-	// TODO(lubronzhan): Deprecated, will be removed in a future release.
-	// https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/issues/1483
-	// legacyClusterSelectorKey and legacyNodeSelectorKey are added for backward compatibility.
-	// These will be removed in the future release.
-	// Please refer to the issue above for deprecation process.
-	//
-	// Deprecated: legacyClusterSelectorKey will be removed in a future release.
-	legacyClusterSelectorKey = "capw.vmware.com/cluster.name"
-
-	// Please refer to the issue above for deprecation process.
-	//
-	// Deprecated: legacyClusterSelectorKey will be removed in a future release.
-	legacyNodeSelectorKey = "capw.vmware.com/cluster.role"
 )
 
 // CPService represents the ability to reconcile a ControlPlaneEndpoint.
@@ -124,18 +105,18 @@ func controlPlaneVMServiceName(ctx *vmware.ClusterContext) string {
 
 // ClusterRoleVMLabels returns labels applied to a VirtualMachine in the cluster. The Control Plane
 // VM Service uses these labels to select VMs, as does the Cloud Provider.
-// Add the legacyNodeSelectorKey and legacyClusterSelectorKey to machines as well.
+// Add the LegacyNodeSelectorKey and LegacyClusterSelectorKey to machines as well.
 func clusterRoleVMLabels(ctx *vmware.ClusterContext, controlPlane bool) map[string]string {
 	result := map[string]string{
-		clusterSelectorKey:       ctx.Cluster.Name,
-		legacyClusterSelectorKey: ctx.Cluster.Name,
+		ClusterSelectorKey:       ctx.Cluster.Name,
+		LegacyClusterSelectorKey: ctx.Cluster.Name,
 	}
 	if controlPlane {
-		result[nodeSelectorKey] = roleControlPlane
-		result[legacyNodeSelectorKey] = roleControlPlane
+		result[NodeSelectorKey] = RoleControlPlane
+		result[LegacyNodeSelectorKey] = RoleControlPlane
 	} else {
-		result[nodeSelectorKey] = roleNode
-		result[legacyNodeSelectorKey] = roleNode
+		result[NodeSelectorKey] = RoleNode
+		result[LegacyNodeSelectorKey] = RoleNode
 	}
 	return result
 }

diff --git a/pkg/services/vmoperator/vmoperator_suite_test.go b/pkg/services/vmoperator/vmoperator_suite_test.go
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package vmoperator
+package vmoperator_test
 
 import (
 	"os"