feat: Added metrics for consistency checks

pkg/controllers/inflightchecks/controller.go → pkg/controllers/consistency/controller.go

@@ -12,13 +12,16 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package inflightchecks
+package consistency
 
 import (
 	"context"
+	"fmt"
+	"reflect"
 	"time"
 
 	"github.com/patrickmn/go-cache"
+	"github.com/prometheus/client_golang/prometheus"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/utils/clock"
 	"knative.dev/pkg/logging"
@@ -30,7 +33,6 @@ import (
 
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	"github.com/aws/karpenter-core/pkg/cloudprovider"
-	inflightchecksevents "github.com/aws/karpenter-core/pkg/controllers/inflightchecks/events"
 	"github.com/aws/karpenter-core/pkg/events"
 	corecontroller "github.com/aws/karpenter-core/pkg/operator/controller"
 )
@@ -48,7 +50,7 @@ type Controller struct {
 type Issue string
 
 type Check interface {
-	// Check performs the inflight check, this should return a list of slice discovered, or an empty
+	// Check performs the consistency check, this should return a list of slice discovered, or an empty
 	// slice if no issues were found
 	Check(context.Context, *v1.Node) ([]Issue, error)
 }
@@ -73,14 +75,14 @@ func NewController(clk clock.Clock, kubeClient client.Client, recorder events.Re
 }
 
 func (c *Controller) Name() string {
-	return "inflightchecks"
+	return "consistency"
 }
 
 func (c *Controller) Reconcile(ctx context.Context, node *v1.Node) (reconcile.Result, error) {
 	if node.Labels[v1alpha5.ProvisionerNameLabelKey] == "" {
 		return reconcile.Result{}, nil
 	}
-	// If we get an event before we should check for inflight checks, we ignore and wait
+	// If we get an event before we should check for consistency checks, we ignore and wait
 	if lastTime, ok := c.lastScanned.Get(client.ObjectKeyFromObject(node).String()); ok {
 		if lastTime, ok := lastTime.(time.Time); ok {
 			remaining := scanPeriod - c.clock.Since(lastTime)
@@ -91,17 +93,16 @@ func (c *Controller) Reconcile(ctx context.Context, node *v1.Node) (reconcile.Re
 	}
 	c.lastScanned.SetDefault(client.ObjectKeyFromObject(node).String(), c.clock.Now())
 
-	var allIssues []Issue
 	for _, check := range c.checks {
 		issues, err := check.Check(ctx, node)
 		if err != nil {
-			logging.FromContext(ctx).Errorf("checking node with %T, %s", check, err)
+			return reconcile.Result{}, fmt.Errorf("checking node with %T, %w", check, err)
+		}
+		for _, issue := range issues {
+			logging.FromContext(ctx).Errorf("check failed, %s", issue)
+			consistencyErrors.With(prometheus.Labels{checkLabel: reflect.TypeOf(check).Elem().Name()}).Inc()
+			c.recorder.Publish(CheckEvent(node, string(issue))...)
 		}
-		allIssues = append(allIssues, issues...)
-	}
-	for _, issue := range allIssues {
-		logging.FromContext(ctx).Infof("inflight check failed, %s", issue)
-		c.recorder.Publish(inflightchecksevents.InflightCheck(node, string(issue))...)
 	}
 	return reconcile.Result{RequeueAfter: scanPeriod}, nil
 }

pkg/controllers/inflightchecks/events/events.go → pkg/controllers/consistency/events.go

@@ -12,20 +12,20 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package events
+package consistency
 
 import (
 	v1 "k8s.io/api/core/v1"
 
 	"github.com/aws/karpenter-core/pkg/events"
 )
 
-func InflightCheck(node *v1.Node, message string) []events.Event {
+func CheckEvent(node *v1.Node, message string) []events.Event {
 	return []events.Event{
 		{
 			InvolvedObject: node,
 			Type:           v1.EventTypeWarning,
-			Reason:         "FailedInflightCheck",
+			Reason:         "FailedConsistencyCheck",
 			Message:        message,
 			DedupeValues:   []string{node.Name, message},
 		},

pkg/controllers/inflightchecks/failedinit.go → pkg/controllers/consistency/failedinit.go

@@ -12,7 +12,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package inflightchecks
+package consistency
 
 import (
 	"context"
@@ -68,18 +68,18 @@ func (f FailedInit) Check(ctx context.Context, n *v1.Node) ([]Issue, error) {
 	}
 	instanceType, ok := lo.Find(instanceTypes, func(it *cloudprovider.InstanceType) bool { return it.Name == n.Labels[v1.LabelInstanceTypeStable] })
 	if !ok {
-		return []Issue{Issue(fmt.Sprintf("Instance Type %q not found", n.Labels[v1.LabelInstanceTypeStable]))}, nil
+		return []Issue{Issue(fmt.Sprintf("instance type %q not found", n.Labels[v1.LabelInstanceTypeStable]))}, nil
 	}
 
 	// detect startup taints which should be removed
 	var result []Issue
 	if taint, ok := node.IsStartupTaintRemoved(n, provisioner); !ok {
-		result = append(result, Issue(fmt.Sprintf("Startup taint %q is still on the node", formatTaint(taint))))
+		result = append(result, Issue(fmt.Sprintf("startup taint %q is still on the node", formatTaint(taint))))
 	}
 
 	// and extended resources which never registered
 	if resource, ok := node.IsExtendedResourceRegistered(n, instanceType); !ok {
-		result = append(result, Issue(fmt.Sprintf("Expected resource %q didn't register on the node", resource)))
+		result = append(result, Issue(fmt.Sprintf("expected resource %q didn't register on the node", resource)))
 	}
 
 	return result, nil

pkg/controllers/consistency/metrics.go

@@ -0,0 +1,41 @@
+/*
+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 consistency
+
+import (
+	"github.com/prometheus/client_golang/prometheus"
+	crmetrics "sigs.k8s.io/controller-runtime/pkg/metrics"
+
+	"github.com/aws/karpenter-core/pkg/metrics"
+)
+
+func init() {
+	crmetrics.Registry.MustRegister(consistencyErrors)
+}
+
+const (
+	checkLabel = "check"
+	subsystem  = "consistency"
+)
+
+var consistencyErrors = prometheus.NewCounterVec(
+	prometheus.CounterOpts{
+		Namespace: metrics.Namespace,
+		Subsystem: subsystem,
+		Name:      "errors",
+		Help:      "Number of consistency checks that have failed.",
+	},
+	[]string{checkLabel},
+)

pkg/controllers/inflightchecks/nodeshape.go → pkg/controllers/consistency/nodeshape.go

@@ -12,7 +12,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package inflightchecks
+package consistency
 
 import (
 	"context"
@@ -60,19 +60,19 @@ func (n *NodeShape) Check(ctx context.Context, node *v1.Node) ([]Issue, error) {
 	}
 	instanceType, ok := lo.Find(instanceTypes, func(it *cloudprovider.InstanceType) bool { return it.Name == node.Labels[v1.LabelInstanceTypeStable] })
 	if !ok {
-		return []Issue{Issue(fmt.Sprintf("Instance Type %q not found", node.Labels[v1.LabelInstanceTypeStable]))}, nil
+		return []Issue{Issue(fmt.Sprintf("instance type %q not found", node.Labels[v1.LabelInstanceTypeStable]))}, nil
 	}
 	var issues []Issue
 	for resourceName, expectedQuantity := range instanceType.Capacity {
 		nodeQuantity, ok := node.Status.Capacity[resourceName]
 		if !ok && !expectedQuantity.IsZero() {
-			issues = append(issues, Issue(fmt.Sprintf("Expected resource %s not found", resourceName)))
+			issues = append(issues, Issue(fmt.Sprintf("expected resource %s not found", resourceName)))
 			continue
 		}
 
 		pct := nodeQuantity.AsApproximateFloat64() / expectedQuantity.AsApproximateFloat64()
 		if pct < 0.90 {
-			issues = append(issues, Issue(fmt.Sprintf("Expected %s of resource %s, but found %s (%0.1f%% of expected)", expectedQuantity.String(),
+			issues = append(issues, Issue(fmt.Sprintf("expected %s of resource %s, but found %s (%0.1f%% of expected)", expectedQuantity.String(),
 				resourceName, nodeQuantity.String(), pct*100)))
 		}
 	}

pkg/controllers/inflightchecks/suite_test.go → pkg/controllers/consistency/suite_test.go

@@ -12,7 +12,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package inflightchecks_test
+package consistency_test
 
 import (
 	"context"
@@ -35,15 +35,15 @@ import (
 	"github.com/aws/karpenter-core/pkg/apis/settings"
 	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	"github.com/aws/karpenter-core/pkg/cloudprovider/fake"
-	"github.com/aws/karpenter-core/pkg/controllers/inflightchecks"
+	"github.com/aws/karpenter-core/pkg/controllers/consistency"
 	"github.com/aws/karpenter-core/pkg/operator/controller"
 	"github.com/aws/karpenter-core/pkg/operator/scheme"
 	"github.com/aws/karpenter-core/pkg/test"
 	. "github.com/aws/karpenter-core/pkg/test/expectations"
 )
 
 var ctx context.Context
-var inflightController controller.Controller
+var consistencyController controller.Controller
 var env *test.Environment
 var fakeClock *clock.FakeClock
 var cp *fake.CloudProvider
@@ -65,7 +65,7 @@ var _ = BeforeSuite(func() {
 	ctx = settings.ToContext(ctx, test.Settings())
 	cp = &fake.CloudProvider{}
 	recorder = test.NewEventRecorder()
-	inflightController = inflightchecks.NewController(fakeClock, env.Client, recorder, cp)
+	consistencyController = consistency.NewController(fakeClock, env.Client, recorder, cp)
 })
 
 var _ = AfterSuite(func() {
@@ -124,8 +124,8 @@ var _ = Describe("Controller", func() {
 			}
 			ExpectApplied(ctx, env.Client, provisioner, machine, node)
 			fakeClock.Step(2 * time.Hour)
-			ExpectReconcileSucceeded(ctx, inflightController, client.ObjectKeyFromObject(node))
-			Expect(recorder.DetectedEvent("Expected resource \"fake.com/vendor-a\" didn't register on the node")).To(BeTrue())
+			ExpectReconcileSucceeded(ctx, consistencyController, client.ObjectKeyFromObject(node))
+			Expect(recorder.DetectedEvent("expected resource \"fake.com/vendor-a\" didn't register on the node")).To(BeTrue())
 		})
 		It("should detect issues with nodes that have a startup taint which isn't removed", func() {
 			provisioner.Spec.StartupTaints = []v1.Taint{
@@ -160,8 +160,8 @@ var _ = Describe("Controller", func() {
 			})
 			ExpectApplied(ctx, env.Client, provisioner, machine, node)
 			fakeClock.Step(2 * time.Hour)
-			ExpectReconcileSucceeded(ctx, inflightController, client.ObjectKeyFromObject(node))
-			Expect(recorder.DetectedEvent("Startup taint \"my.startup.taint:NoSchedule\" is still on the node")).To(BeTrue())
+			ExpectReconcileSucceeded(ctx, consistencyController, client.ObjectKeyFromObject(node))
+			Expect(recorder.DetectedEvent("startup taint \"my.startup.taint:NoSchedule\" is still on the node")).To(BeTrue())
 		})
 	})
 
@@ -193,8 +193,8 @@ var _ = Describe("Controller", func() {
 			ExpectApplied(ctx, env.Client, provisioner, machine, node, p, pdb)
 			ExpectManualBinding(ctx, env.Client, p, node)
 			_ = env.Client.Delete(ctx, node)
-			ExpectReconcileSucceeded(ctx, inflightController, client.ObjectKeyFromObject(node))
-			Expect(recorder.DetectedEvent(fmt.Sprintf("Can't drain node, PDB %s/%s is blocking evictions", pdb.Namespace, pdb.Name))).To(BeTrue())
+			ExpectReconcileSucceeded(ctx, consistencyController, client.ObjectKeyFromObject(node))
+			Expect(recorder.DetectedEvent(fmt.Sprintf("can't drain node, PDB %s/%s is blocking evictions", pdb.Namespace, pdb.Name))).To(BeTrue())
 		})
 	})
 
@@ -223,8 +223,8 @@ var _ = Describe("Controller", func() {
 				v1.ResourcePods:   resource.MustParse("10"),
 			}
 			ExpectApplied(ctx, env.Client, provisioner, machine, node)
-			ExpectReconcileSucceeded(ctx, inflightController, client.ObjectKeyFromObject(node))
-			Expect(recorder.DetectedEvent("Expected 128Gi of resource memory, but found 64Gi (50.0% of expected)")).To(BeTrue())
+			ExpectReconcileSucceeded(ctx, consistencyController, client.ObjectKeyFromObject(node))
+			Expect(recorder.DetectedEvent("expected 128Gi of resource memory, but found 64Gi (50.0% of expected)")).To(BeTrue())
 		})
 	})
 })

pkg/controllers/inflightchecks/termination.go → pkg/controllers/consistency/termination.go

@@ -12,7 +12,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package inflightchecks
+package consistency
 
 import (
 	"context"
@@ -51,7 +51,7 @@ func (t *Termination) Check(ctx context.Context, node *v1.Node) ([]Issue, error)
 	}
 	var issues []Issue
 	if pdb, ok := pdbs.CanEvictPods(pods); !ok {
-		issues = append(issues, Issue(fmt.Sprintf("Can't drain node, PDB %s is blocking evictions", pdb)))
+		issues = append(issues, Issue(fmt.Sprintf("can't drain node, PDB %s is blocking evictions", pdb)))
 	}
 	return issues, nil
 }

pkg/controllers/controllers.go

@@ -22,9 +22,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/aws/karpenter-core/pkg/cloudprovider"
+	"github.com/aws/karpenter-core/pkg/controllers/consistency"
 	"github.com/aws/karpenter-core/pkg/controllers/counter"
 	"github.com/aws/karpenter-core/pkg/controllers/deprovisioning"
-	"github.com/aws/karpenter-core/pkg/controllers/inflightchecks"
 	metricspod "github.com/aws/karpenter-core/pkg/controllers/metrics/pod"
 	metricsprovisioner "github.com/aws/karpenter-core/pkg/controllers/metrics/provisioner"
 	metricsstate "github.com/aws/karpenter-core/pkg/controllers/metrics/state"
@@ -69,6 +69,6 @@ func NewControllers(
 		metricspod.NewController(kubeClient),
 		metricsprovisioner.NewController(kubeClient),
 		counter.NewController(kubeClient, cluster),
-		inflightchecks.NewController(clock, kubeClient, recorder, cloudProvider),
+		consistency.NewController(clock, kubeClient, recorder, cloudProvider),
 	}
 }