NutanixMachineReconiler SetupWithManager refactor (#436) (#441)

Refactor SetupWithManager function to call Build and Watch explicitly
instead of calling complete. Also add ClusterUnpausedAndInfrastructureReady
predicate to the watch call.

Makefile

@@ -361,10 +361,9 @@ mocks: ## Generate mocks for the project
 	mockgen -destination=mocks/ctlclient/client_mock.go -package=mockctlclient sigs.k8s.io/controller-runtime/pkg/client Client
 	mockgen -destination=mocks/ctlclient/manager_mock.go -package=mockctlclient sigs.k8s.io/controller-runtime/pkg/manager Manager
 	mockgen -destination=mocks/ctlclient/cache_mock.go -package=mockctlclient sigs.k8s.io/controller-runtime/pkg/cache Cache
-	mockgen -destination=mocks/k8sclient/cm_informer.go -package=mockk8sclient k8s.io/client-go/informers/core/v1 ConfigMapInformer
-	mockgen -destination=mocks/k8sclient/secret_informer.go -package=mockk8sclient k8s.io/client-go/informers/core/v1 SecretInformer
-	mockgen -destination=mocks/k8sclient/secret_lister.go -package=mockk8sclient k8s.io/client-go/listers/core/v1 SecretLister
-	mockgen -destination=mocks/k8sclient/secret_namespace_lister.go -package=mockk8sclient k8s.io/client-go/listers/core/v1 SecretNamespaceLister
+	mockgen -destination=mocks/k8sclient/informer.go -package=mockk8sclient k8s.io/client-go/informers/core/v1 ConfigMapInformer,SecretInformer
+	mockgen -destination=mocks/k8sclient/lister.go -package=mockk8sclient k8s.io/client-go/listers/core/v1 SecretLister,SecretNamespaceLister
+	mockgen -destination=mocks/k8sapimachinery/interfaces.go -package=mockmeta k8s.io/apimachinery/pkg/api/meta RESTMapper,RESTScope
 
 .PHONY: unit-test
 unit-test: setup-envtest ## Run unit tests.

controllers/nutanixcluster_controller.go

@@ -77,9 +77,14 @@ func NewNutanixClusterReconciler(client client.Client, secretInformer coreinform
 // SetupWithManager sets up the NutanixCluster controller with the Manager.
 func (r *NutanixClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
 	log := ctrl.LoggerFrom(ctx)
+	copts := controller.Options{
+		MaxConcurrentReconciles: r.controllerConfig.MaxConcurrentReconciles,
+		RateLimiter:             r.controllerConfig.RateLimiter,
+	}
+
 	c, err := ctrl.NewControllerManagedBy(mgr).
 		For(&infrav1.NutanixCluster{}). // Watch the controlled, infrastructure resource.
-		WithOptions(controller.Options{MaxConcurrentReconciles: r.controllerConfig.MaxConcurrentReconciles}).
+		WithOptions(copts).
 		Build(r)
 	if err != nil {
 		return err

controllers/nutanixcluster_controller_test.go

@@ -27,7 +27,9 @@ import (
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/gstruct"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/uuid"
@@ -36,10 +38,12 @@ import (
 	capiutil "sigs.k8s.io/cluster-api/util"
 	ctrl "sigs.k8s.io/controller-runtime"
 	ctlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
 	ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
 	mockctlclient "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/mocks/ctlclient"
+	mockmeta "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/mocks/k8sapimachinery"
 	nctx "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/pkg/context"
 )
 
@@ -823,3 +827,48 @@ func TestReconcileTrustBundleRefWithExistingOwner(t *testing.T) {
 	err := reconciler.reconcileTrustBundleRef(ctx, nutanixCluster)
 	assert.Error(t, err)
 }
+
+func TestNutanixClusterReconciler_SetupWithManager(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	ctx := context.Background()
+	log := ctrl.Log.WithName("controller")
+	scheme := runtime.NewScheme()
+	err := infrav1.AddToScheme(scheme)
+	require.NoError(t, err)
+	err = capiv1.AddToScheme(scheme)
+	require.NoError(t, err)
+
+	restScope := mockmeta.NewMockRESTScope(mockCtrl)
+	restScope.EXPECT().Name().Return(meta.RESTScopeNameNamespace).AnyTimes()
+
+	restMapper := mockmeta.NewMockRESTMapper(mockCtrl)
+	restMapper.EXPECT().RESTMapping(gomock.Any()).Return(&meta.RESTMapping{Scope: restScope}, nil).AnyTimes()
+
+	mockClient := mockctlclient.NewMockClient(mockCtrl)
+	mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+	mockClient.EXPECT().RESTMapper().Return(restMapper).AnyTimes()
+
+	cache := mockctlclient.NewMockCache(mockCtrl)
+
+	mgr := mockctlclient.NewMockManager(mockCtrl)
+	mgr.EXPECT().GetCache().Return(cache).AnyTimes()
+	mgr.EXPECT().GetScheme().Return(scheme).AnyTimes()
+	mgr.EXPECT().GetControllerOptions().Return(v1alpha1.ControllerConfigurationSpec{}).AnyTimes()
+	mgr.EXPECT().SetFields(gomock.Any()).Return(nil).AnyTimes()
+	mgr.EXPECT().GetLogger().Return(log).AnyTimes()
+	mgr.EXPECT().Add(gomock.Any()).Return(nil).AnyTimes()
+	mgr.EXPECT().GetClient().Return(mockClient).AnyTimes()
+
+	reconciler := &NutanixClusterReconciler{
+		Client: mockClient,
+		Scheme: scheme,
+		controllerConfig: &ControllerConfig{
+			MaxConcurrentReconciles: 1,
+		},
+	}
+
+	err = reconciler.SetupWithManager(ctx, mgr)
+	assert.NoError(t, err)
+}

controllers/nutanixmachine_controller.go

@@ -41,6 +41,7 @@ import (
 	"sigs.k8s.io/cluster-api/util/annotations"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
+	"sigs.k8s.io/cluster-api/util/predicates"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -101,22 +102,44 @@ func NewNutanixMachineReconciler(client client.Client, secretInformer coreinform
 }
 
 // SetupWithManager sets up the controller with the Manager.
-func (r *NutanixMachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, copts ...ControllerConfigOpts) error {
-	return ctrl.NewControllerManagedBy(mgr).
+func (r *NutanixMachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
+	copts := controller.Options{
+		MaxConcurrentReconciles: r.controllerConfig.MaxConcurrentReconciles,
+		RateLimiter:             r.controllerConfig.RateLimiter,
+	}
+
+	c, err := ctrl.NewControllerManagedBy(mgr).
 		For(&infrav1.NutanixMachine{}).
 		// Watch the CAPI resource that owns this infrastructure resource.
 		Watches(
 			&source.Kind{Type: &capiv1.Machine{}},
-			handler.EnqueueRequestsFromMapFunc(
-				capiutil.MachineToInfrastructureMapFunc(
-					infrav1.GroupVersion.WithKind("NutanixMachine"))),
+			handler.EnqueueRequestsFromMapFunc(capiutil.MachineToInfrastructureMapFunc(infrav1.GroupVersion.WithKind("NutanixMachine"))),
 		).
 		Watches(
 			&source.Kind{Type: &infrav1.NutanixCluster{}},
 			handler.EnqueueRequestsFromMapFunc(r.mapNutanixClusterToNutanixMachines(ctx)),
 		).
-		WithOptions(controller.Options{MaxConcurrentReconciles: r.controllerConfig.MaxConcurrentReconciles}).
-		Complete(r)
+		WithOptions(copts).
+		Build(r)
+	if err != nil {
+		return err
+	}
+
+	clusterToObjectFunc, err := capiutil.ClusterToObjectsMapper(r.Client, &infrav1.NutanixMachineList{}, mgr.GetScheme())
+	if err != nil {
+		return fmt.Errorf("failed to create mapper for Cluster to NutanixMachine: %s", err)
+	}
+
+	if err := c.Watch(
+		// Watch the CAPI resource that owns this infrastructure resource.
+		&source.Kind{Type: &capiv1.Cluster{}},
+		handler.EnqueueRequestsFromMapFunc(clusterToObjectFunc),
+		predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx)),
+	); err != nil {
+		return fmt.Errorf("failed adding a watch for ready clusters: %w", err)
+	}
+
+	return nil
 }
 
 func (r *NutanixMachineReconciler) mapNutanixClusterToNutanixMachines(ctx context.Context) handler.MapFunc {

controllers/nutanixmachine_controller_test.go

@@ -18,19 +18,27 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"testing"
 
+	"github.com/golang/mock/gomock"
 	credentialtypes "github.com/nutanix-cloud-native/prism-go-client/environment/credentials"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
 
 	infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
+	mockctlclient "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/mocks/ctlclient"
+	mockmeta "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/mocks/k8sapimachinery"
 	nctx "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/pkg/context"
 )
 
@@ -228,3 +236,136 @@ func TestNutanixMachineReconciler(t *testing.T) {
 		})
 	})
 }
+
+func TestNutanixMachineReconciler_SetupWithManager(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	ctx := context.Background()
+	log := ctrl.Log.WithName("controller")
+	scheme := runtime.NewScheme()
+	err := infrav1.AddToScheme(scheme)
+	require.NoError(t, err)
+	err = capiv1.AddToScheme(scheme)
+	require.NoError(t, err)
+
+	cache := mockctlclient.NewMockCache(mockCtrl)
+
+	mgr := mockctlclient.NewMockManager(mockCtrl)
+	mgr.EXPECT().GetCache().Return(cache).AnyTimes()
+	mgr.EXPECT().GetScheme().Return(scheme).AnyTimes()
+	mgr.EXPECT().GetControllerOptions().Return(v1alpha1.ControllerConfigurationSpec{}).AnyTimes()
+	mgr.EXPECT().SetFields(gomock.Any()).Return(nil).AnyTimes()
+	mgr.EXPECT().GetLogger().Return(log).AnyTimes()
+	mgr.EXPECT().Add(gomock.Any()).Return(nil).AnyTimes()
+
+	restScope := mockmeta.NewMockRESTScope(mockCtrl)
+	restScope.EXPECT().Name().Return(meta.RESTScopeNameNamespace).AnyTimes()
+
+	restMapper := mockmeta.NewMockRESTMapper(mockCtrl)
+	restMapper.EXPECT().RESTMapping(gomock.Any()).Return(&meta.RESTMapping{Scope: restScope}, nil).AnyTimes()
+
+	mockClient := mockctlclient.NewMockClient(mockCtrl)
+	mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+	mockClient.EXPECT().RESTMapper().Return(restMapper).AnyTimes()
+
+	reconciler := &NutanixMachineReconciler{
+		Client: mockClient,
+		Scheme: scheme,
+		controllerConfig: &ControllerConfig{
+			MaxConcurrentReconciles: 1,
+		},
+	}
+
+	err = reconciler.SetupWithManager(ctx, mgr)
+	assert.NoError(t, err)
+}
+
+func TestNutanixMachineReconciler_SetupWithManager_BuildError(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	ctx := context.Background()
+	log := ctrl.Log.WithName("controller")
+	scheme := runtime.NewScheme()
+	err := infrav1.AddToScheme(scheme)
+	require.NoError(t, err)
+	err = capiv1.AddToScheme(scheme)
+	require.NoError(t, err)
+
+	cache := mockctlclient.NewMockCache(mockCtrl)
+
+	mgr := mockctlclient.NewMockManager(mockCtrl)
+	mgr.EXPECT().GetCache().Return(cache).AnyTimes()
+	mgr.EXPECT().GetScheme().Return(scheme).AnyTimes()
+	mgr.EXPECT().GetControllerOptions().Return(v1alpha1.ControllerConfigurationSpec{}).AnyTimes()
+	mgr.EXPECT().SetFields(gomock.Any()).Return(nil).AnyTimes()
+	mgr.EXPECT().GetLogger().Return(log).AnyTimes()
+	mgr.EXPECT().Add(gomock.Any()).Return(errors.New("error")).AnyTimes()
+
+	restScope := mockmeta.NewMockRESTScope(mockCtrl)
+	restScope.EXPECT().Name().Return(meta.RESTScopeNameNamespace).AnyTimes()
+
+	restMapper := mockmeta.NewMockRESTMapper(mockCtrl)
+	restMapper.EXPECT().RESTMapping(gomock.Any()).Return(&meta.RESTMapping{Scope: restScope}, nil).AnyTimes()
+
+	mockClient := mockctlclient.NewMockClient(mockCtrl)
+	mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+	mockClient.EXPECT().RESTMapper().Return(restMapper).AnyTimes()
+
+	reconciler := &NutanixMachineReconciler{
+		Client: mockClient,
+		Scheme: scheme,
+		controllerConfig: &ControllerConfig{
+			MaxConcurrentReconciles: 1,
+		},
+	}
+
+	err = reconciler.SetupWithManager(ctx, mgr)
+	assert.Error(t, err)
+}
+
+func TestNutanixMachineReconciler_SetupWithManager_ClusterToTypedObjectsMapperError(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	ctx := context.Background()
+	log := ctrl.Log.WithName("controller")
+	scheme := runtime.NewScheme()
+	err := infrav1.AddToScheme(scheme)
+	require.NoError(t, err)
+	err = capiv1.AddToScheme(scheme)
+	require.NoError(t, err)
+
+	cache := mockctlclient.NewMockCache(mockCtrl)
+
+	mgr := mockctlclient.NewMockManager(mockCtrl)
+	mgr.EXPECT().GetCache().Return(cache).AnyTimes()
+	mgr.EXPECT().GetScheme().Return(scheme).AnyTimes()
+	mgr.EXPECT().GetControllerOptions().Return(v1alpha1.ControllerConfigurationSpec{}).AnyTimes()
+	mgr.EXPECT().SetFields(gomock.Any()).Return(nil).AnyTimes()
+	mgr.EXPECT().GetLogger().Return(log).AnyTimes()
+	mgr.EXPECT().Add(gomock.Any()).Return(nil).AnyTimes()
+
+	restScope := mockmeta.NewMockRESTScope(mockCtrl)
+	restScope.EXPECT().Name().Return(meta.RESTScopeName("")).AnyTimes()
+
+	restMapper := mockmeta.NewMockRESTMapper(mockCtrl)
+	restMapper.EXPECT().RESTMapping(gomock.Any()).Return(&meta.RESTMapping{Scope: restScope}, nil).AnyTimes()
+
+	mockClient := mockctlclient.NewMockClient(mockCtrl)
+	mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+	mockClient.EXPECT().RESTMapper().Return(restMapper).AnyTimes()
+
+	reconciler := &NutanixMachineReconciler{
+		Client: mockClient,
+		Scheme: scheme,
+		controllerConfig: &ControllerConfig{
+			MaxConcurrentReconciles: 1,
+		},
+	}
+
+	err = reconciler.SetupWithManager(ctx, mgr)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to create mapper for Cluster to NutanixMachine")
+}

go.mod

@@ -5,14 +5,16 @@ go 1.22.0
 require (
 	github.com/blang/semver v3.5.1+incompatible
 	github.com/blang/semver/v4 v4.0.0
+	github.com/go-logr/logr v1.3.0
 	github.com/golang/mock v1.6.0
 	github.com/google/uuid v1.3.0
 	github.com/nutanix-cloud-native/prism-go-client v0.4.0
 	github.com/onsi/ginkgo/v2 v2.9.2
 	github.com/onsi/gomega v1.27.5
 	github.com/pkg/errors v0.9.1
-	github.com/stretchr/testify v1.8.1
+	github.com/stretchr/testify v1.8.4
 	go.uber.org/zap v1.24.0
+	golang.org/x/time v0.3.0
 	k8s.io/api v0.26.10
 	k8s.io/apiextensions-apiserver v0.26.10
 	k8s.io/apimachinery v0.26.10
@@ -52,7 +54,6 @@ require (
 	github.com/evanphx/json-patch/v5 v5.6.0 // indirect
 	github.com/fsnotify/fsnotify v1.6.0 // indirect
 	github.com/go-errors/errors v1.0.1 // indirect
-	github.com/go-logr/logr v1.2.3 // indirect
 	github.com/go-logr/zapr v1.2.3 // indirect
 	github.com/go-openapi/jsonpointer v0.19.5 // indirect
 	github.com/go-openapi/jsonreference v0.20.0 // indirect
@@ -112,13 +113,12 @@ require (
 	go.starlark.net v0.0.0-20200306205701-8dd3e2ee1dd5 // indirect
 	go.uber.org/atomic v1.9.0 // indirect
 	go.uber.org/multierr v1.8.0 // indirect
-	golang.org/x/crypto v0.15.0 // indirect
-	golang.org/x/net v0.18.0 // indirect
+	golang.org/x/crypto v0.14.0 // indirect
+	golang.org/x/net v0.17.0 // indirect
 	golang.org/x/oauth2 v0.6.0 // indirect
-	golang.org/x/sys v0.14.0 // indirect
-	golang.org/x/term v0.14.0 // indirect
-	golang.org/x/text v0.14.0 // indirect
-	golang.org/x/time v0.3.0 // indirect
+	golang.org/x/sys v0.13.0 // indirect
+	golang.org/x/term v0.13.0 // indirect
+	golang.org/x/text v0.13.0 // indirect
 	golang.org/x/tools v0.7.0 // indirect
 	gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect