cleanup client commands (#179)

the design I have seen more often is to use the controller client directly on the reconciler. We cannot do that directly for Create since we have events (that require then name) but we can wrap the command and use r.New instead (and all others, r.Patch, r.Status, R.Get appear to work). Signed-off-by: vsoch <vsoch@users.noreply.github.com> Co-authored-by: vsoch <vsoch@users.noreply.github.com>
flux-framework · Jun 3, 2023 · e495c66 · e495c66
1 parent d7baba5
commit e495c66
Show file tree

Hide file tree

Showing 7 changed files with 31 additions and 25 deletions.
diff --git a/controllers/flux/extra.go b/controllers/flux/extra.go
@@ -250,7 +250,7 @@ func (r *MiniClusterReconciler) createMiniClusterIngress(
 		r.log.Error(err, "🔴 Create ingress", "Service", restfulServiceName)
 		return err
 	}
-	err = r.Client.Create(ctx, ingress)
+	err = r.New(ctx, ingress)
 	if err != nil {
 		r.log.Error(err, "🔴 Create ingress", "Service", restfulServiceName)
 		return err

diff --git a/controllers/flux/minicluster.go b/controllers/flux/minicluster.go
@@ -142,7 +142,7 @@ func (r *MiniClusterReconciler) ensureMiniCluster(
 	if status != jobctrl.ConditionJobReady {
 		clusterCopy := cluster.DeepCopy()
 		jobctrl.FlagConditionReady(clusterCopy)
-		r.Client.Status().Update(ctx, clusterCopy)
+		r.Status().Update(ctx, clusterCopy)
 	}
 
 	// And we re-queue so the Ready condition triggers next steps!
@@ -214,7 +214,7 @@ func (r *MiniClusterReconciler) getExistingJob(
 ) (*batchv1.Job, error) {
 
 	existing := &batchv1.Job{}
-	err := r.Client.Get(
+	err := r.Get(
 		ctx,
 		types.NamespacedName{
 			Name:      cluster.Name,
@@ -254,7 +254,7 @@ func (r *MiniClusterReconciler) getMiniCluster(
 				"Name:", job.Name,
 			)
 
-			err = r.Client.Create(ctx, job)
+			err = r.New(ctx, job)
 			if err != nil {
 				r.log.Error(
 					err,
@@ -292,7 +292,7 @@ func (r *MiniClusterReconciler) getConfigMap(
 
 	// Look for the config map by name
 	existing := &corev1.ConfigMap{}
-	err := r.Client.Get(
+	err := r.Get(
 		ctx,
 		types.NamespacedName{
 			Name:      configFullName,
@@ -347,7 +347,7 @@ func (r *MiniClusterReconciler) getConfigMap(
 				"Namespace", dep.Namespace,
 				"Name", dep.Name,
 			)
-			err = r.Client.Create(ctx, dep)
+			err = r.New(ctx, dep)
 			if err != nil {
 				r.log.Error(
 					err, "❌ Failed to create MiniCluster ConfigMap",

diff --git a/controllers/flux/minicluster_controller.go b/controllers/flux/minicluster_controller.go
@@ -21,6 +21,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
+
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -35,7 +36,7 @@ type MiniClusterUpdateWatcher interface {
 
 // MiniClusterReconciler reconciles a MiniCluster object
 type MiniClusterReconciler struct {
-	Client     client.Client
+	client.Client
 	Scheme     *runtime.Scheme
 	Manager    ctrl.Manager
 	log        logr.Logger
@@ -113,7 +114,7 @@ func (r *MiniClusterReconciler) Reconcile(
 	r.log.Info("Request: ", "req", req)
 
 	// Does the Flux Job exist yet (based on name and namespace)
-	err := r.Client.Get(ctx, req.NamespacedName, &cluster)
+	err := r.Get(ctx, req.NamespacedName, &cluster)
 	if err != nil {
 
 		// Create it, doesn't exist yet
@@ -158,6 +159,11 @@ func (r *MiniClusterReconciler) Reconcile(
 	return result, nil
 }
 
+// Wrapper to Client.Create (New) for easier interaction
+func (r *MiniClusterReconciler) New(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
+	return r.Client.Create(ctx, obj, opts...)
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *MiniClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).

diff --git a/controllers/flux/pods.go b/controllers/flux/pods.go
@@ -54,7 +54,7 @@ func (r *MiniClusterReconciler) ensureServicePod(
 				"Name:", pod.Name,
 			)
 
-			err = r.Client.Create(ctx, pod)
+			err = r.New(ctx, pod)
 			if err != nil {
 				r.log.Error(
 					err,
@@ -89,7 +89,7 @@ func (r *MiniClusterReconciler) getExistingPod(
 ) (*corev1.Pod, error) {
 
 	existing := &corev1.Pod{}
-	err := r.Client.Get(
+	err := r.Get(
 		ctx,
 		types.NamespacedName{
 			Name:      cluster.Name + "-services",

diff --git a/controllers/flux/scale.go b/controllers/flux/scale.go
@@ -36,7 +36,7 @@ func (r *MiniClusterReconciler) addScaleSelector(
 		return ctrl.Result{}, nil
 	}
 	cluster.Status.Selector = selector
-	err := r.Client.Status().Update(ctx, cluster)
+	err := r.Status().Update(ctx, cluster)
 	r.log.Info("MiniCluster", "ScaleSelector", selector, "Status", "Updating")
 	return ctrl.Result{Requeue: true}, err
 }
@@ -54,10 +54,10 @@ func (r *MiniClusterReconciler) disallowScale(
 	cluster.Status.Size = cluster.Status.MaximumSize
 
 	// Apply the patch to restore to the original size
-	err := r.Client.Patch(ctx, cluster, patch)
+	err := r.Patch(ctx, cluster, patch)
 
 	// First update fixes the status
-	r.Client.Status().Update(ctx, cluster)
+	r.Status().Update(ctx, cluster)
 	return ctrl.Result{Requeue: true}, err
 }
 
@@ -74,9 +74,9 @@ func (r *MiniClusterReconciler) allowScale(
 	job.Spec.Completions = &cluster.Spec.Size
 	cluster.Status.Size = cluster.Spec.Size
 
-	err := r.Client.Patch(ctx, job, patch)
+	err := r.Patch(ctx, job, patch)
 	// I don't check for error because I want both changes to go in at once
-	r.Client.Status().Update(ctx, cluster)
+	r.Status().Update(ctx, cluster)
 	return ctrl.Result{Requeue: true}, err
 }
 
@@ -93,8 +93,8 @@ func (r *MiniClusterReconciler) restoreOriginalSize(
 	cluster.Status.Size = cluster.Spec.Size
 
 	// Apply the patch to restore to the original size
-	r.Client.Status().Update(ctx, cluster)
-	err := r.Client.Patch(ctx, cluster, patch)
+	r.Status().Update(ctx, cluster)
+	err := r.Patch(ctx, cluster, patch)
 	return ctrl.Result{Requeue: true}, err
 }
 

diff --git a/controllers/flux/service.go b/controllers/flux/service.go
@@ -39,7 +39,7 @@ func (r *MiniClusterReconciler) exposeServices(
 
 	// This service is for the restful API
 	existing := &corev1.Service{}
-	err := r.Client.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: cluster.Namespace}, existing)
+	err := r.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: cluster.Namespace}, existing)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			_, err = r.createHeadlessService(ctx, cluster, serviceName, selector)
@@ -67,7 +67,7 @@ func (r *MiniClusterReconciler) createHeadlessService(
 		},
 	}
 	ctrl.SetControllerReference(cluster, service, r.Scheme)
-	err := r.Client.Create(ctx, service)
+	err := r.New(ctx, service)
 	if err != nil {
 		r.log.Error(err, "🔴 Create service", "Service", service.Name)
 	}
@@ -85,7 +85,7 @@ func (r *MiniClusterReconciler) exposeService(
 
 	// This service is for the restful API
 	existing := &corev1.Service{}
-	err := r.Client.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: cluster.Namespace}, existing)
+	err := r.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: cluster.Namespace}, existing)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			r.log.Info("Creating service with: ", serviceName, cluster.Namespace)
@@ -110,7 +110,7 @@ func (r *MiniClusterReconciler) exposeService(
 				},
 			}
 			ctrl.SetControllerReference(cluster, service, r.Scheme)
-			err := r.Client.Create(ctx, service)
+			err := r.New(ctx, service)
 			if err != nil {
 				r.log.Error(err, "🔴 Create service", "Service", service.Name)
 			}

diff --git a/controllers/flux/volumes.go b/controllers/flux/volumes.go
@@ -266,7 +266,7 @@ func (r *MiniClusterReconciler) getExistingPersistentVolume(
 
 	// First look for an existing persistent volume
 	existing := &corev1.PersistentVolume{}
-	err := r.Client.Get(
+	err := r.Get(
 		ctx,
 		types.NamespacedName{
 			Name:      volumeName,
@@ -300,7 +300,7 @@ func (r *MiniClusterReconciler) getPersistentVolume(
 				"Name", volumeName,
 			)
 
-			err = r.Client.Create(ctx, volume)
+			err = r.New(ctx, volume)
 			if err != nil {
 				r.log.Error(
 					err, "Failed to create MiniCluster Mounted Volume",
@@ -341,7 +341,7 @@ func (r *MiniClusterReconciler) getExistingPersistentVolumeClaim(
 ) (*corev1.PersistentVolumeClaim, error) {
 
 	existing := &corev1.PersistentVolumeClaim{}
-	err := r.Client.Get(
+	err := r.Get(
 		ctx,
 		types.NamespacedName{
 			Name:      claimName,
@@ -373,7 +373,7 @@ func (r *MiniClusterReconciler) getPersistentVolumeClaim(
 				"Namespace", cluster.Namespace,
 				"Name", claimName,
 			)
-			err = r.Client.Create(ctx, volume)
+			err = r.New(ctx, volume)
 
 			// This is a creation error we need to report back
 			if err != nil {