From ee0d0b56ff16e7ce5142cebc2d74da903094bd1a Mon Sep 17 00:00:00 2001 From: Suket Sharma Date: Tue, 18 Jan 2022 17:17:22 -0800 Subject: [PATCH 1/5] Replace liveness and readiness with startup node controller --- pkg/controllers/node/controller.go | 8 ++-- pkg/controllers/node/liveness.go | 57 ----------------------- pkg/controllers/node/readiness.go | 42 ----------------- pkg/controllers/node/startup.go | 74 ++++++++++++++++++++++++++++++ pkg/controllers/node/suite_test.go | 35 +++----------- 5 files changed, 83 insertions(+), 133 deletions(-) delete mode 100644 pkg/controllers/node/liveness.go delete mode 100644 pkg/controllers/node/readiness.go create mode 100644 pkg/controllers/node/startup.go diff --git a/pkg/controllers/node/controller.go b/pkg/controllers/node/controller.go index 345773fec150..6e2dc0a78f28 100644 --- a/pkg/controllers/node/controller.go +++ b/pkg/controllers/node/controller.go @@ -42,7 +42,7 @@ const controllerName = "node" func NewController(kubeClient client.Client) *Controller { return &Controller{ kubeClient: kubeClient, - liveness: &Liveness{kubeClient: kubeClient}, + startup: &Startup{kubeClient: kubeClient}, emptiness: &Emptiness{kubeClient: kubeClient}, expiration: &Expiration{kubeClient: kubeClient}, } @@ -52,8 +52,7 @@ func NewController(kubeClient client.Client) *Controller { // taints, labels, finalizers. type Controller struct { kubeClient client.Client - readiness *Readiness - liveness *Liveness + startup *Startup emptiness *Emptiness expiration *Expiration finalizer *Finalizer @@ -93,8 +92,7 @@ func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reco for _, reconciler := range []interface { Reconcile(context.Context, *v1alpha5.Provisioner, *v1.Node) (reconcile.Result, error) }{ - c.readiness, - c.liveness, + c.startup, c.expiration, c.emptiness, c.finalizer, diff --git a/pkg/controllers/node/liveness.go b/pkg/controllers/node/liveness.go deleted file mode 100644 index 0b47e9f1972f..000000000000 --- a/pkg/controllers/node/liveness.go +++ /dev/null @@ -1,57 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package node - -import ( - "context" - "fmt" - "time" - - "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" - "github.com/aws/karpenter/pkg/utils/injectabletime" - "github.com/aws/karpenter/pkg/utils/node" - v1 "k8s.io/api/core/v1" - "knative.dev/pkg/logging" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -const LivenessTimeout = 15 * time.Minute - -// Liveness is a subreconciler that deletes nodes determined to be unrecoverable -type Liveness struct { - kubeClient client.Client -} - -// Reconcile reconciles the node -func (r *Liveness) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1.Node) (reconcile.Result, error) { - if timeSinceCreation := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); timeSinceCreation < LivenessTimeout { - return reconcile.Result{RequeueAfter: LivenessTimeout - timeSinceCreation}, nil - } - condition := node.GetCondition(n.Status.Conditions, v1.NodeReady) - // If the reason is "", then the condition has never been set. We expect - // either the kubelet to set this reason, or the kcm's - // node-lifecycle-controller to set the status to NodeStatusNeverUpdated if - // the kubelet cannot connect. Once the value is NodeStatusNeverUpdated and - // the node is beyond the liveness timeout, we will delete the node. - if condition.Reason != "" && condition.Reason != "NodeStatusNeverUpdated" { - return reconcile.Result{}, nil - } - logging.FromContext(ctx).Infof("Triggering termination for node that failed to join") - if err := r.kubeClient.Delete(ctx, n); err != nil { - return reconcile.Result{}, fmt.Errorf("deleting node, %w", err) - } - return reconcile.Result{}, nil -} diff --git a/pkg/controllers/node/readiness.go b/pkg/controllers/node/readiness.go deleted file mode 100644 index 50908284d173..000000000000 --- a/pkg/controllers/node/readiness.go +++ /dev/null @@ -1,42 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package node - -import ( - "context" - - "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" - "github.com/aws/karpenter/pkg/utils/node" - v1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -// Readiness is a subreconciler that removes the NotReady taint when the node is ready -type Readiness struct{} - -// Reconcile reconciles the node -func (r *Readiness) Reconcile(_ context.Context, _ *v1alpha5.Provisioner, n *v1.Node) (reconcile.Result, error) { - if !node.IsReady(n) { - return reconcile.Result{}, nil - } - taints := []v1.Taint{} - for _, taint := range n.Spec.Taints { - if taint.Key != v1alpha5.NotReadyTaintKey { - taints = append(taints, taint) - } - } - n.Spec.Taints = taints - return reconcile.Result{}, nil -} diff --git a/pkg/controllers/node/startup.go b/pkg/controllers/node/startup.go new file mode 100644 index 000000000000..6c15ba48d1e4 --- /dev/null +++ b/pkg/controllers/node/startup.go @@ -0,0 +1,74 @@ +/* +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package node + +import ( + "context" + "fmt" + "time" + + "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" + "github.com/aws/karpenter/pkg/utils/injectabletime" + "github.com/aws/karpenter/pkg/utils/node" + v1 "k8s.io/api/core/v1" + "knative.dev/pkg/logging" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +const StartupTimeout = 15 * time.Minute + +// Startup is a subreconciler that +// 1. Removes the NotReady taint when the node is ready. This taint is originally applied on node creation. +// 2. Terminates nodes that don't transition to ready within StartupTimeout +type Startup struct { + kubeClient client.Client +} + +// Reconcile reconciles the node +func (r *Startup) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1.Node) (reconcile.Result, error) { + if !notReadyTaintKeyExists(n) { + // At this point, the startup of the node is complete and no more evaluation is necessary. + return reconcile.Result{}, nil + } + + if !node.IsReady(n) { + if timeSinceCreation := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); timeSinceCreation < StartupTimeout { + return reconcile.Result{RequeueAfter: StartupTimeout - timeSinceCreation}, nil + } + logging.FromContext(ctx).Infof("Triggering termination for node that failed to transition to ready") + if err := r.kubeClient.Delete(ctx, n); err != nil { + return reconcile.Result{}, fmt.Errorf("deleting node, %w", err) + } + return reconcile.Result{}, nil + } + taints := []v1.Taint{} + for _, taint := range n.Spec.Taints { + if taint.Key != v1alpha5.NotReadyTaintKey { + taints = append(taints, taint) + } + } + n.Spec.Taints = taints + return reconcile.Result{}, nil +} + +func notReadyTaintKeyExists(n *v1.Node) bool { + for _, taint := range n.Spec.Taints { + if taint.Key == v1alpha5.NotReadyTaintKey { + return true + } + } + return false +} diff --git a/pkg/controllers/node/suite_test.go b/pkg/controllers/node/suite_test.go index 02ed0e4788c4..95b60a11a5ab 100644 --- a/pkg/controllers/node/suite_test.go +++ b/pkg/controllers/node/suite_test.go @@ -120,7 +120,7 @@ var _ = Describe("Controller", func() { }) }) - Context("Readiness", func() { + Context("Startup", func() { It("should not remove the readiness taint if not ready", func() { n := test.Node(test.NodeOptions{ ReadyStatus: v1.ConditionUnknown, @@ -181,9 +181,7 @@ var _ = Describe("Controller", func() { n = ExpectNodeExists(ctx, env.Client, n.Name) Expect(n.Spec.Taints).To(Equal(n.Spec.Taints)) }) - }) - Context("Liveness", func() { - It("should delete nodes if NodeStatusNeverUpdated after 5 minutes", func() { + It("should delete nodes if node not ready even after startup timeout", func() { n := test.Node(test.NodeOptions{ ObjectMeta: metav1.ObjectMeta{ Finalizers: []string{v1alpha5.TerminationFinalizer}, @@ -191,42 +189,21 @@ var _ = Describe("Controller", func() { }, ReadyStatus: v1.ConditionUnknown, ReadyReason: "NodeStatusNeverUpdated", - }) - ExpectCreated(ctx, env.Client, provisioner) - ExpectCreatedWithStatus(ctx, env.Client, n) - - ExpectReconcileSucceeded(ctx, controller, client.ObjectKeyFromObject(provisioner)) - - // Expect node not be deleted - n = ExpectNodeExists(ctx, env.Client, n.Name) - Expect(n.DeletionTimestamp.IsZero()).To(BeTrue()) - - // Simulate time passing and a n failing to join - injectabletime.Now = func() time.Time { return time.Now().Add(node.LivenessTimeout) } - ExpectReconcileSucceeded(ctx, controller, client.ObjectKeyFromObject(n)) - - n = ExpectNodeExists(ctx, env.Client, n.Name) - Expect(n.DeletionTimestamp.IsZero()).To(BeFalse()) - }) - It("should delete nodes if we never hear anything after 5 minutes", func() { - n := test.Node(test.NodeOptions{ - ObjectMeta: metav1.ObjectMeta{ - Finalizers: []string{v1alpha5.TerminationFinalizer}, - Labels: map[string]string{v1alpha5.ProvisionerNameLabelKey: provisioner.Name}, + Taints: []v1.Taint{ + {Key: v1alpha5.NotReadyTaintKey, Effect: v1.TaintEffectNoSchedule}, }, - ReadyStatus: v1.ConditionUnknown, - ReadyReason: "", }) ExpectCreated(ctx, env.Client, provisioner) ExpectCreatedWithStatus(ctx, env.Client, n) ExpectReconcileSucceeded(ctx, controller, client.ObjectKeyFromObject(provisioner)) + // Expect node not be deleted n = ExpectNodeExists(ctx, env.Client, n.Name) Expect(n.DeletionTimestamp.IsZero()).To(BeTrue()) // Simulate time passing and a n failing to join - injectabletime.Now = func() time.Time { return time.Now().Add(node.LivenessTimeout) } + injectabletime.Now = func() time.Time { return time.Now().Add(node.StartupTimeout) } ExpectReconcileSucceeded(ctx, controller, client.ObjectKeyFromObject(n)) n = ExpectNodeExists(ctx, env.Client, n.Name) From b99c11a0810b63376e88e5286d5c4eb13e547d8e Mon Sep 17 00:00:00 2001 From: Suket Sharma Date: Wed, 19 Jan 2022 15:45:55 -0800 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Ellis Tarn --- pkg/controllers/node/startup.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/node/startup.go b/pkg/controllers/node/startup.go index 6c15ba48d1e4..b91a908d63f5 100644 --- a/pkg/controllers/node/startup.go +++ b/pkg/controllers/node/startup.go @@ -45,10 +45,10 @@ func (r *Startup) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1. } if !node.IsReady(n) { - if timeSinceCreation := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); timeSinceCreation < StartupTimeout { + if age := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); age < StartupTimeout { return reconcile.Result{RequeueAfter: StartupTimeout - timeSinceCreation}, nil } - logging.FromContext(ctx).Infof("Triggering termination for node that failed to transition to ready") + logging.FromContext(ctx).Infof("Triggering termination for node that failed to become ready") if err := r.kubeClient.Delete(ctx, n); err != nil { return reconcile.Result{}, fmt.Errorf("deleting node, %w", err) } From 5df211c050b2c70b77609680c1745585531e651b Mon Sep 17 00:00:00 2001 From: Suket Sharma Date: Wed, 19 Jan 2022 15:47:22 -0800 Subject: [PATCH 3/5] add taints.HasKey --- pkg/apis/provisioning/v1alpha5/taints.go | 12 +++++++++++- pkg/controllers/node/startup.go | 11 ++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/pkg/apis/provisioning/v1alpha5/taints.go b/pkg/apis/provisioning/v1alpha5/taints.go index 56d73b6a902c..05c84d4a0b8d 100644 --- a/pkg/apis/provisioning/v1alpha5/taints.go +++ b/pkg/apis/provisioning/v1alpha5/taints.go @@ -52,7 +52,7 @@ func (ts Taints) WithPod(pod *v1.Pod) Taints { return ts } -// Has returns true if taints has a taint for the given key +// Has returns true if taints has a taint for the given key and effect func (ts Taints) Has(taint v1.Taint) bool { for _, t := range ts { if t.Key == taint.Key && t.Effect == taint.Effect { @@ -62,6 +62,16 @@ func (ts Taints) Has(taint v1.Taint) bool { return false } +// HasKey returns true if taints has a taint for the given key +func (ts Taints) HasKey(taintKey string) bool { + for _, t := range ts { + if t.Key == taintKey { + return true + } + } + return false +} + // Tolerates returns true if the pod tolerates all taints func (ts Taints) Tolerates(pod *v1.Pod) (errs error) { for i := range ts { diff --git a/pkg/controllers/node/startup.go b/pkg/controllers/node/startup.go index b91a908d63f5..f687815dea58 100644 --- a/pkg/controllers/node/startup.go +++ b/pkg/controllers/node/startup.go @@ -39,7 +39,7 @@ type Startup struct { // Reconcile reconciles the node func (r *Startup) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1.Node) (reconcile.Result, error) { - if !notReadyTaintKeyExists(n) { + if !v1alpha5.Taints(n.Spec.Taints).HasKey(v1alpha5.NotReadyTaintKey) { // At this point, the startup of the node is complete and no more evaluation is necessary. return reconcile.Result{}, nil } @@ -64,11 +64,4 @@ func (r *Startup) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1. return reconcile.Result{}, nil } -func notReadyTaintKeyExists(n *v1.Node) bool { - for _, taint := range n.Spec.Taints { - if taint.Key == v1alpha5.NotReadyTaintKey { - return true - } - } - return false -} + From 411c09102f6f6756b00a6ac7d12672d32c3aff5f Mon Sep 17 00:00:00 2001 From: Suket Sharma Date: Wed, 19 Jan 2022 15:51:03 -0800 Subject: [PATCH 4/5] rename startup to initialization --- pkg/controllers/node/controller.go | 20 +++++++++---------- .../node/{startup.go => initialization.go} | 12 +++++------ pkg/controllers/node/suite_test.go | 6 +++--- 3 files changed, 18 insertions(+), 20 deletions(-) rename pkg/controllers/node/{startup.go => initialization.go} (86%) diff --git a/pkg/controllers/node/controller.go b/pkg/controllers/node/controller.go index 6e2dc0a78f28..2879e87174c6 100644 --- a/pkg/controllers/node/controller.go +++ b/pkg/controllers/node/controller.go @@ -41,21 +41,21 @@ const controllerName = "node" // NewController constructs a controller instance func NewController(kubeClient client.Client) *Controller { return &Controller{ - kubeClient: kubeClient, - startup: &Startup{kubeClient: kubeClient}, - emptiness: &Emptiness{kubeClient: kubeClient}, - expiration: &Expiration{kubeClient: kubeClient}, + kubeClient: kubeClient, + initialization: &Initialization{kubeClient: kubeClient}, + emptiness: &Emptiness{kubeClient: kubeClient}, + expiration: &Expiration{kubeClient: kubeClient}, } } // Controller manages a set of properties on karpenter provisioned nodes, such as // taints, labels, finalizers. type Controller struct { - kubeClient client.Client - startup *Startup - emptiness *Emptiness - expiration *Expiration - finalizer *Finalizer + kubeClient client.Client + initialization *Initialization + emptiness *Emptiness + expiration *Expiration + finalizer *Finalizer } // Reconcile executes a reallocation control loop for the resource @@ -92,7 +92,7 @@ func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reco for _, reconciler := range []interface { Reconcile(context.Context, *v1alpha5.Provisioner, *v1.Node) (reconcile.Result, error) }{ - c.startup, + c.initialization, c.expiration, c.emptiness, c.finalizer, diff --git a/pkg/controllers/node/startup.go b/pkg/controllers/node/initialization.go similarity index 86% rename from pkg/controllers/node/startup.go rename to pkg/controllers/node/initialization.go index f687815dea58..b6d92071f205 100644 --- a/pkg/controllers/node/startup.go +++ b/pkg/controllers/node/initialization.go @@ -28,25 +28,25 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -const StartupTimeout = 15 * time.Minute +const InitializationTimeout = 15 * time.Minute // Startup is a subreconciler that // 1. Removes the NotReady taint when the node is ready. This taint is originally applied on node creation. // 2. Terminates nodes that don't transition to ready within StartupTimeout -type Startup struct { +type Initialization struct { kubeClient client.Client } // Reconcile reconciles the node -func (r *Startup) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1.Node) (reconcile.Result, error) { +func (r *Initialization) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1.Node) (reconcile.Result, error) { if !v1alpha5.Taints(n.Spec.Taints).HasKey(v1alpha5.NotReadyTaintKey) { // At this point, the startup of the node is complete and no more evaluation is necessary. return reconcile.Result{}, nil } if !node.IsReady(n) { - if age := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); age < StartupTimeout { - return reconcile.Result{RequeueAfter: StartupTimeout - timeSinceCreation}, nil + if age := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); age < InitializationTimeout { + return reconcile.Result{RequeueAfter: InitializationTimeout - age}, nil } logging.FromContext(ctx).Infof("Triggering termination for node that failed to become ready") if err := r.kubeClient.Delete(ctx, n); err != nil { @@ -63,5 +63,3 @@ func (r *Startup) Reconcile(ctx context.Context, _ *v1alpha5.Provisioner, n *v1. n.Spec.Taints = taints return reconcile.Result{}, nil } - - diff --git a/pkg/controllers/node/suite_test.go b/pkg/controllers/node/suite_test.go index 95b60a11a5ab..e30aa88ca491 100644 --- a/pkg/controllers/node/suite_test.go +++ b/pkg/controllers/node/suite_test.go @@ -120,7 +120,7 @@ var _ = Describe("Controller", func() { }) }) - Context("Startup", func() { + Context("Initialization", func() { It("should not remove the readiness taint if not ready", func() { n := test.Node(test.NodeOptions{ ReadyStatus: v1.ConditionUnknown, @@ -181,7 +181,7 @@ var _ = Describe("Controller", func() { n = ExpectNodeExists(ctx, env.Client, n.Name) Expect(n.Spec.Taints).To(Equal(n.Spec.Taints)) }) - It("should delete nodes if node not ready even after startup timeout", func() { + It("should delete nodes if node not ready even after Initialization timeout ", func() { n := test.Node(test.NodeOptions{ ObjectMeta: metav1.ObjectMeta{ Finalizers: []string{v1alpha5.TerminationFinalizer}, @@ -203,7 +203,7 @@ var _ = Describe("Controller", func() { Expect(n.DeletionTimestamp.IsZero()).To(BeTrue()) // Simulate time passing and a n failing to join - injectabletime.Now = func() time.Time { return time.Now().Add(node.StartupTimeout) } + injectabletime.Now = func() time.Time { return time.Now().Add(node.InitializationTimeout) } ExpectReconcileSucceeded(ctx, controller, client.ObjectKeyFromObject(n)) n = ExpectNodeExists(ctx, env.Client, n.Name) From 382440e7741e72125718eae84fcd7354e2634256 Mon Sep 17 00:00:00 2001 From: Suket Sharma Date: Wed, 19 Jan 2022 15:53:34 -0800 Subject: [PATCH 5/5] Missed some comments during the renaming --- pkg/controllers/node/initialization.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/node/initialization.go b/pkg/controllers/node/initialization.go index b6d92071f205..a15682bf269e 100644 --- a/pkg/controllers/node/initialization.go +++ b/pkg/controllers/node/initialization.go @@ -30,9 +30,9 @@ import ( const InitializationTimeout = 15 * time.Minute -// Startup is a subreconciler that +// Initialization is a subreconciler that // 1. Removes the NotReady taint when the node is ready. This taint is originally applied on node creation. -// 2. Terminates nodes that don't transition to ready within StartupTimeout +// 2. Terminates nodes that don't transition to ready within InitializationTimeout type Initialization struct { kubeClient client.Client }