Added SetDefault() for each Reconcile function

aws · Jun 21, 2021 · 61a6ef6 · 61a6ef6
1 parent 291e511
commit 61a6ef6
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 17 deletions.
diff --git a/pkg/controllers/controller.go b/pkg/controllers/controller.go
@@ -17,12 +17,12 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"reflect"
 
-	"github.com/awslabs/karpenter/pkg/apis/provisioning/v1alpha1"
+	"github.com/awslabs/karpenter/pkg/utils/conditions"
 
 	"go.uber.org/zap"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"knative.dev/pkg/apis"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -49,22 +49,24 @@ func (c *GenericController) Reconcile(ctx context.Context, req reconcile.Request
 	}
 	// 2. Copy object for merge patch base
 	persisted := resource.DeepCopyObject()
-	// 3. Set to true to remove race condition
+	// 3. Set Defaults
+	if _, ok := resource.(apis.Defaultable); ok {
+		resource.(apis.Defaultable).SetDefaults(ctx)
+	}
+	// 4. Set to true to remove race condition
 	// TODO: remove status conditions on provisioners
-	if provisioner, ok := resource.(*v1alpha1.Provisioner); ok {
-		provisioner.StatusConditions().MarkTrue(v1alpha1.Active)
+	if conditionsAccessor, ok := resource.(apis.ConditionsAccessor); ok {
+		apis.NewLivingConditionSet(conditions.Active).Manage(conditionsAccessor).MarkTrue(conditions.Active)
 	}
-	// 4. Reconcile
+	// 5. Reconcile
 	if _, err := c.Controller.Reconcile(ctx, resource); err != nil {
 		zap.S().Errorf("Controller failed to reconcile kind %s, %s",
 			resource.GetObjectKind().GroupVersionKind().Kind, err.Error())
 		return reconcile.Result{Requeue: true}, nil
 	}
-	// 5. Update Status using a merge patch
-	if !reflect.DeepEqual(resource, persisted) {
-		if err := c.Status().Patch(ctx, resource, client.MergeFrom(persisted)); err != nil {
-			return reconcile.Result{}, fmt.Errorf("Failed to persist changes to %s, %w", req.NamespacedName, err)
-		}
+	// 6. Update Status using a merge patch
+	if err := c.Status().Patch(ctx, resource, client.MergeFrom(persisted)); err != nil {
+		return reconcile.Result{}, fmt.Errorf("Failed to persist changes to %s, %w", req.NamespacedName, err)
 	}
 	return reconcile.Result{RequeueAfter: c.Interval()}, nil
 }

diff --git a/pkg/controllers/provisioning/v1alpha1/reallocation/controller.go b/pkg/controllers/provisioning/v1alpha1/reallocation/controller.go
@@ -67,7 +67,6 @@ func NewController(kubeClient client.Client, coreV1Client corev1.CoreV1Interface
 // Reconcile executes a reallocation control loop for the resource
 func (c *Controller) Reconcile(ctx context.Context, object client.Object) (reconcile.Result, error) {
 	provisioner := object.(*v1alpha1.Provisioner)
-
 	// 1. Set TTL on TTLable Nodes
 	if err := c.utilization.markUnderutilized(ctx, provisioner); err != nil {
 		return reconcile.Result{}, fmt.Errorf("adding ttl and underutilized label, %w", err)

diff --git a/pkg/controllers/provisioning/v1alpha1/reallocation/suite_test.go b/pkg/controllers/provisioning/v1alpha1/reallocation/suite_test.go
@@ -28,7 +28,6 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
-	"knative.dev/pkg/ptr"
 
 	. "github.com/awslabs/karpenter/pkg/test/expectations"
 	. "github.com/onsi/ginkgo"
@@ -72,7 +71,6 @@ var _ = Describe("Reallocation", func() {
 			},
 			Spec: v1alpha1.ProvisionerSpec{
 				Cluster:    &v1alpha1.ClusterSpec{Name: "test-cluster", Endpoint: "http://test-cluster", CABundle: "dGVzdC1jbHVzdGVyCg=="},
-				TTLSeconds: ptr.Int32(300),
 			},
 		}
 		ctx = context.Background()
@@ -83,8 +81,7 @@ var _ = Describe("Reallocation", func() {
 	})
 
 	Context("Reconciliation", func() {
-		It("should label nodes as underutilized", func() {
-
+		It("should label nodes as underutilized and add TTL", func() {
 			node := test.NodeWith(test.NodeOptions{
 				Labels: map[string]string{
 					v1alpha1.ProvisionerNameLabelKey:      provisioner.Name,

diff --git a/pkg/test/expectations/expectations.go b/pkg/test/expectations/expectations.go
@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/awslabs/karpenter/pkg/apis/provisioning/v1alpha1"
+	"github.com/awslabs/karpenter/pkg/utils/conditions"
 	"github.com/awslabs/karpenter/pkg/utils/log"
 	. "github.com/onsi/gomega"
 	v1 "k8s.io/api/core/v1"
@@ -93,7 +94,7 @@ func ExpectEventuallyReconciled(c client.Client, provisioner *v1alpha1.Provision
 	nn := types.NamespacedName{Name: provisioner.GetName(), Namespace: provisioner.GetNamespace()}
 	Eventually(func() bool {
 		Expect(c.Get(context.Background(), nn, provisioner)).To(Succeed())
-		return !provisioner.StatusConditions().GetCondition(v1alpha1.Active).IsUnknown()
+		return !provisioner.StatusConditions().GetCondition(conditions.Active).IsUnknown()
 	}, ReconcilerPropagationTime, RequestInterval).Should(BeTrue(), func() string {
 		return fmt.Sprintf("resources active condition was never updated\n%s", log.Pretty(provisioner))
 	})

diff --git a/pkg/utils/conditions/conditions.go b/pkg/utils/conditions/conditions.go
@@ -0,0 +1,7 @@
+package conditions
+
+import "knative.dev/pkg/apis"
+
+const (
+	Active apis.ConditionType = "Active"
+)