From 330b98b1ebca2c8d4c5bfca5886b0ffa58927bb6 Mon Sep 17 00:00:00 2001 From: Jason Deal Date: Sat, 23 Nov 2024 20:48:54 -0800 Subject: [PATCH] feat: only operate on cloudprovider managed resources (#1818) --- kwok/charts/crds/karpenter.sh_nodeclaims.yaml | 9 + kwok/charts/crds/karpenter.sh_nodepools.yaml | 14 ++ pkg/apis/crds/karpenter.sh_nodeclaims.yaml | 9 + pkg/apis/crds/karpenter.sh_nodepools.yaml | 14 ++ pkg/apis/v1/labels.go | 5 + pkg/apis/v1/nodeclaim.go | 11 ++ pkg/apis/v1/nodeclaim_validation_cel_test.go | 5 +- pkg/apis/v1/nodepool.go | 20 +- pkg/apis/v1/nodepool_default_test.go | 5 +- pkg/apis/v1/nodepool_validation_cel_test.go | 29 ++- pkg/apis/v1/suite_test.go | 50 ----- pkg/cloudprovider/fake/cloudprovider.go | 4 +- pkg/controllers/controllers.go | 21 +- .../disruption/consolidation_test.go | 16 +- pkg/controllers/disruption/controller.go | 9 +- pkg/controllers/disruption/helpers.go | 20 +- .../disruption/orchestration/suite_test.go | 4 +- pkg/controllers/disruption/suite_test.go | 16 +- pkg/controllers/disruption/validation.go | 2 +- pkg/controllers/metrics/node/suite_test.go | 2 +- .../metrics/nodepool/controller.go | 20 +- .../metrics/nodepool/suite_test.go | 61 ++++-- pkg/controllers/metrics/pod/suite_test.go | 5 +- pkg/controllers/node/health/suite_test.go | 13 +- .../node/termination/controller.go | 11 +- .../node/termination/suite_test.go | 39 ++-- .../node/termination/terminator/terminator.go | 4 +- .../nodeclaim/consistency/controller.go | 39 ++-- .../nodeclaim/consistency/suite_test.go | 185 +++++++++++------- .../disruption/consolidation_test.go | 33 ++++ .../nodeclaim/disruption/controller.go | 33 ++-- .../nodeclaim/disruption/drift_test.go | 40 ++-- .../nodeclaim/disruption/suite_test.go | 8 +- .../nodeclaim/expiration/controller.go | 20 +- .../nodeclaim/expiration/suite_test.go | 52 +++-- .../nodeclaim/garbagecollection/controller.go | 12 +- .../nodeclaim/garbagecollection/suite_test.go | 7 +- .../nodeclaim/lifecycle/controller.go | 15 +- .../nodeclaim/lifecycle/hydration.go | 69 +++++++ .../nodeclaim/lifecycle/hydration_test.go | 76 +++++++ .../nodeclaim/lifecycle/initialization.go | 8 +- .../lifecycle/initialization_test.go | 122 +++++++----- .../nodeclaim/lifecycle/launch_test.go | 50 +++-- .../nodeclaim/lifecycle/liveness_test.go | 77 +++++--- .../nodeclaim/lifecycle/registration.go | 10 +- .../nodeclaim/lifecycle/registration_test.go | 56 ++++-- .../nodeclaim/lifecycle/suite_test.go | 61 ++++-- .../nodeclaim/lifecycle/termination_test.go | 94 +++++---- .../nodeclaim/podevents/controller.go | 18 +- .../nodeclaim/podevents/suite_test.go | 14 +- .../nodepool/counter/controller.go | 42 ++-- .../nodepool/counter/suite_test.go | 38 +++- pkg/controllers/nodepool/hash/controller.go | 28 ++- pkg/controllers/nodepool/hash/suite_test.go | 17 +- .../nodepool/readiness/controller.go | 57 +++--- .../nodepool/readiness/suite_test.go | 11 +- .../nodepool/validation/controller.go | 16 +- .../nodepool/validation/suite_test.go | 16 +- pkg/controllers/provisioning/provisioner.go | 56 +++--- .../scheduling/nodeclaimtemplate.go | 5 +- .../scheduling/scheduling_benchmark_test.go | 2 +- .../provisioning/scheduling/suite_test.go | 4 +- pkg/controllers/provisioning/suite_test.go | 14 +- pkg/controllers/state/cluster.go | 12 +- pkg/controllers/state/informer/nodeclaim.go | 20 +- pkg/controllers/state/informer/nodepool.go | 20 +- pkg/controllers/state/suite_test.go | 25 ++- pkg/test/environment.go | 32 ++- pkg/test/nodeclaim.go | 9 +- pkg/test/nodeclass.go | 10 + pkg/test/nodepool.go | 5 +- pkg/test/nodes.go | 5 +- pkg/utils/node/node.go | 31 ++- pkg/utils/node/suite_test.go | 18 +- pkg/utils/nodeclaim/nodeclaim.go | 104 ++++++---- pkg/utils/nodeclaim/suite_test.go | 101 +++++++++- pkg/utils/nodepool/nodepool.go | 88 ++++++++- pkg/utils/nodepool/suite_test.go | 97 +++++++++ test/pkg/debug/nodeclaim.go | 4 +- 79 files changed, 1643 insertions(+), 761 deletions(-) create mode 100644 pkg/controllers/nodeclaim/lifecycle/hydration.go create mode 100644 pkg/controllers/nodeclaim/lifecycle/hydration_test.go create mode 100644 pkg/utils/nodepool/suite_test.go diff --git a/kwok/charts/crds/karpenter.sh_nodeclaims.yaml b/kwok/charts/crds/karpenter.sh_nodeclaims.yaml index 5116d68851..dbb84c33a0 100644 --- a/kwok/charts/crds/karpenter.sh_nodeclaims.yaml +++ b/kwok/charts/crds/karpenter.sh_nodeclaims.yaml @@ -88,12 +88,21 @@ spec: description: API version of the referent pattern: ^[^/]*$ type: string + x-kubernetes-validations: + - message: group may not be empty + rule: self != '' kind: description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"' type: string + x-kubernetes-validations: + - message: kind may not be empty + rule: self != '' name: description: 'Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names' type: string + x-kubernetes-validations: + - message: name may not be empty + rule: self != '' required: - group - kind diff --git a/kwok/charts/crds/karpenter.sh_nodepools.yaml b/kwok/charts/crds/karpenter.sh_nodepools.yaml index 35def943c8..ca885fa0e7 100644 --- a/kwok/charts/crds/karpenter.sh_nodepools.yaml +++ b/kwok/charts/crds/karpenter.sh_nodepools.yaml @@ -229,17 +229,31 @@ spec: description: API version of the referent pattern: ^[^/]*$ type: string + x-kubernetes-validations: + - message: group may not be empty + rule: self != '' kind: description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"' type: string + x-kubernetes-validations: + - message: kind may not be empty + rule: self != '' name: description: 'Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names' type: string + x-kubernetes-validations: + - message: name may not be empty + rule: self != '' required: - group - kind - name type: object + x-kubernetes-validations: + - message: nodeClassRef.group is immutable + rule: self.group == oldSelf.group + - message: nodeClassRef.kind is immutable + rule: self.kind == oldSelf.kind requirements: description: Requirements are layered with GetLabels and applied to every node. items: diff --git a/pkg/apis/crds/karpenter.sh_nodeclaims.yaml b/pkg/apis/crds/karpenter.sh_nodeclaims.yaml index 2bdda91931..517088614f 100644 --- a/pkg/apis/crds/karpenter.sh_nodeclaims.yaml +++ b/pkg/apis/crds/karpenter.sh_nodeclaims.yaml @@ -88,12 +88,21 @@ spec: description: API version of the referent pattern: ^[^/]*$ type: string + x-kubernetes-validations: + - message: group may not be empty + rule: self != '' kind: description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"' type: string + x-kubernetes-validations: + - message: kind may not be empty + rule: self != '' name: description: 'Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names' type: string + x-kubernetes-validations: + - message: name may not be empty + rule: self != '' required: - group - kind diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml index 68da49b076..67d7d2d728 100644 --- a/pkg/apis/crds/karpenter.sh_nodepools.yaml +++ b/pkg/apis/crds/karpenter.sh_nodepools.yaml @@ -229,17 +229,31 @@ spec: description: API version of the referent pattern: ^[^/]*$ type: string + x-kubernetes-validations: + - message: group may not be empty + rule: self != '' kind: description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"' type: string + x-kubernetes-validations: + - message: kind may not be empty + rule: self != '' name: description: 'Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names' type: string + x-kubernetes-validations: + - message: name may not be empty + rule: self != '' required: - group - kind - name type: object + x-kubernetes-validations: + - message: nodeClassRef.group is immutable + rule: self.group == oldSelf.group + - message: nodeClassRef.kind is immutable + rule: self.kind == oldSelf.kind requirements: description: Requirements are layered with GetLabels and applied to every node. items: diff --git a/pkg/apis/v1/labels.go b/pkg/apis/v1/labels.go index 722508c9ee..9364936dfe 100644 --- a/pkg/apis/v1/labels.go +++ b/pkg/apis/v1/labels.go @@ -21,6 +21,7 @@ import ( "strings" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/karpenter/pkg/apis" @@ -141,3 +142,7 @@ func GetLabelDomain(key string) string { } return "" } + +func NodeClassLabelKey(gk schema.GroupKind) string { + return fmt.Sprintf("%s/%s", gk.Group, strings.ToLower(gk.Kind)) +} diff --git a/pkg/apis/v1/nodeclaim.go b/pkg/apis/v1/nodeclaim.go index 1c163858ba..ccaf4df97e 100644 --- a/pkg/apis/v1/nodeclaim.go +++ b/pkg/apis/v1/nodeclaim.go @@ -20,6 +20,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" ) // NodeClaimSpec describes the desired state of the NodeClaim @@ -97,17 +98,27 @@ type ResourceRequirements struct { type NodeClassReference struct { // Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds" + // +kubebuilder:validation:XValidation:rule="self != ''",message="kind may not be empty" // +required Kind string `json:"kind"` // Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names + // +kubebuilder:validation:XValidation:rule="self != ''",message="name may not be empty" // +required Name string `json:"name"` // API version of the referent + // +kubebuilder:validation:XValidation:rule="self != ''",message="group may not be empty" // +kubebuilder:validation:Pattern=`^[^/]*$` // +required Group string `json:"group"` } +func (ncr *NodeClassReference) GroupKind() schema.GroupKind { + return schema.GroupKind{ + Group: ncr.Group, + Kind: ncr.Kind, + } +} + // +kubebuilder:object:generate=false type Provider = runtime.RawExtension diff --git a/pkg/apis/v1/nodeclaim_validation_cel_test.go b/pkg/apis/v1/nodeclaim_validation_cel_test.go index 0d5e426098..c0d3e59b72 100644 --- a/pkg/apis/v1/nodeclaim_validation_cel_test.go +++ b/pkg/apis/v1/nodeclaim_validation_cel_test.go @@ -46,8 +46,9 @@ var _ = Describe("Validation", func() { ObjectMeta: metav1.ObjectMeta{Name: strings.ToLower(randomdata.SillyName())}, Spec: NodeClaimSpec{ NodeClassRef: &NodeClassReference{ - Kind: "NodeClaim", - Name: "default", + Group: "karpenter.test.sh", + Kind: "TestNodeClaim", + Name: "default", }, Requirements: []NodeSelectorRequirementWithMinValues{ { diff --git a/pkg/apis/v1/nodepool.go b/pkg/apis/v1/nodepool.go index 094b7b8852..8c0c451f50 100644 --- a/pkg/apis/v1/nodepool.go +++ b/pkg/apis/v1/nodepool.go @@ -19,7 +19,6 @@ package v1 import ( "fmt" "math" - "sort" "strconv" "github.com/mitchellh/hashstructure/v2" @@ -181,6 +180,8 @@ type NodeClaimTemplateSpec struct { // +required Requirements []NodeSelectorRequirementWithMinValues `json:"requirements" hash:"ignore"` // NodeClassRef is a reference to an object that defines provider specific configuration + // +kubebuilder:validation:XValidation:rule="self.group == oldSelf.group",message="nodeClassRef.group is immutable" + // +kubebuilder:validation:XValidation:rule="self.kind == oldSelf.kind",message="nodeClassRef.kind is immutable" // +required NodeClassRef *NodeClassReference `json:"nodeClassRef"` // TerminationGracePeriod is the maximum duration the controller will wait before forcefully deleting the pods on a node, measured from when deletion is first initiated. @@ -289,23 +290,6 @@ type NodePoolList struct { Items []NodePool `json:"items"` } -// OrderByWeight orders the NodePools in the NodePoolList by their priority weight in-place. -// This priority evaluates the following things in precedence order: -// 1. NodePools that have a larger weight are ordered first -// 2. If two NodePools have the same weight, then the NodePool with the name later in the alphabet will come first -func (nl *NodePoolList) OrderByWeight() { - sort.Slice(nl.Items, func(a, b int) bool { - weightA := lo.FromPtr(nl.Items[a].Spec.Weight) - weightB := lo.FromPtr(nl.Items[b].Spec.Weight) - - if weightA == weightB { - // Order NodePools by name for a consistent ordering when sorting equal weight - return nl.Items[a].Name > nl.Items[b].Name - } - return weightA > weightB - }) -} - // MustGetAllowedDisruptions calls GetAllowedDisruptionsByReason if the error is not nil. This reduces the // amount of state that the disruption controller must reconcile, while allowing the GetAllowedDisruptionsByReason() // to bubble up any errors in validation. diff --git a/pkg/apis/v1/nodepool_default_test.go b/pkg/apis/v1/nodepool_default_test.go index 96d6954713..538ba7e507 100644 --- a/pkg/apis/v1/nodepool_default_test.go +++ b/pkg/apis/v1/nodepool_default_test.go @@ -42,8 +42,9 @@ var _ = Describe("CEL/Default", func() { Template: NodeClaimTemplate{ Spec: NodeClaimTemplateSpec{ NodeClassRef: &NodeClassReference{ - Kind: "NodeClaim", - Name: "default", + Group: "karpenter.test.sh", + Kind: "TestNodeClaim", + Name: "default", }, Requirements: []NodeSelectorRequirementWithMinValues{ { diff --git a/pkg/apis/v1/nodepool_validation_cel_test.go b/pkg/apis/v1/nodepool_validation_cel_test.go index e5e62cb2ef..a6754ea182 100644 --- a/pkg/apis/v1/nodepool_validation_cel_test.go +++ b/pkg/apis/v1/nodepool_validation_cel_test.go @@ -46,8 +46,9 @@ var _ = Describe("CEL/Validation", func() { Template: NodeClaimTemplate{ Spec: NodeClaimTemplateSpec{ NodeClassRef: &NodeClassReference{ - Kind: "NodeClaim", - Name: "default", + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", }, Requirements: []NodeSelectorRequirementWithMinValues{ { @@ -619,4 +620,28 @@ var _ = Describe("CEL/Validation", func() { Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed()) }) }) + Context("NodeClassRef", func() { + It("should fail to mutate group", func() { + Expect(env.Client.Create(ctx, nodePool)).To(Succeed()) + nodePool.Spec.Template.Spec.NodeClassRef.Group = "karpenter.test.mutated.sh" + Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed()) + }) + It("should fail to mutate kind", func() { + Expect(env.Client.Create(ctx, nodePool)).To(Succeed()) + nodePool.Spec.Template.Spec.NodeClassRef.Group = "TestNodeClass2" + Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed()) + }) + It("should fail if group is unset", func() { + nodePool.Spec.Template.Spec.NodeClassRef.Group = "" + Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed()) + }) + It("should fail if kind is unset", func() { + nodePool.Spec.Template.Spec.NodeClassRef.Kind = "" + Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed()) + }) + It("should fail if name is unset", func() { + nodePool.Spec.Template.Spec.NodeClassRef.Name = "" + Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed()) + }) + }) }) diff --git a/pkg/apis/v1/suite_test.go b/pkg/apis/v1/suite_test.go index 84a63d540e..c47999e248 100644 --- a/pkg/apis/v1/suite_test.go +++ b/pkg/apis/v1/suite_test.go @@ -18,15 +18,12 @@ package v1_test import ( "context" - "math/rand" "testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/samber/lo" "sigs.k8s.io/karpenter/pkg/apis" - v1 "sigs.k8s.io/karpenter/pkg/apis/v1" "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" "sigs.k8s.io/karpenter/pkg/test/v1alpha1" @@ -53,50 +50,3 @@ var _ = AfterEach(func() { var _ = AfterSuite(func() { Expect(env.Stop()).To(Succeed(), "Failed to stop environment") }) - -var _ = Describe("OrderByWeight", func() { - It("should order the NodePools by weight", func() { - // Generate 10 NodePools that have random weights, some might have the same weights - var nodePools []v1.NodePool - for i := 0; i < 10; i++ { - np := test.NodePool(v1.NodePool{ - Spec: v1.NodePoolSpec{ - Weight: lo.ToPtr[int32](int32(rand.Intn(100) + 1)), //nolint:gosec - }, - }) - nodePools = append(nodePools, *np) - } - - nodePools = lo.Shuffle(nodePools) - nodePoolList := v1.NodePoolList{Items: nodePools} - nodePoolList.OrderByWeight() - - lastWeight := 101 // This is above the allowed weight values - for _, np := range nodePoolList.Items { - Expect(lo.FromPtr(np.Spec.Weight)).To(BeNumerically("<=", lastWeight)) - lastWeight = int(lo.FromPtr(np.Spec.Weight)) - } - }) - It("should order the NodePools by name when the weights are the same", func() { - // Generate 10 NodePools with the same weight - var nodePools []v1.NodePool - for i := 0; i < 10; i++ { - np := test.NodePool(v1.NodePool{ - Spec: v1.NodePoolSpec{ - Weight: lo.ToPtr[int32](10), - }, - }) - nodePools = append(nodePools, *np) - } - - nodePools = lo.Shuffle(nodePools) - nodePoolList := v1.NodePoolList{Items: nodePools} - nodePoolList.OrderByWeight() - - lastName := "zzzzzzzzzzzzzzzzzzzzzzzz" // large string value - for _, np := range nodePoolList.Items { - Expect(np.Name < lastName).To(BeTrue()) - lastName = np.Name - } - }) -}) diff --git a/pkg/cloudprovider/fake/cloudprovider.go b/pkg/cloudprovider/fake/cloudprovider.go index 01c9b196f5..946a56ac3f 100644 --- a/pkg/cloudprovider/fake/cloudprovider.go +++ b/pkg/cloudprovider/fake/cloudprovider.go @@ -165,8 +165,8 @@ func (c *CloudProvider) Create(ctx context.Context, nodeClaim *v1.NodeClaim) (*v } func (c *CloudProvider) Get(_ context.Context, id string) (*v1.NodeClaim, error) { - c.mu.RLock() - defer c.mu.RUnlock() + c.mu.Lock() + defer c.mu.Unlock() if c.NextGetErr != nil { tempError := c.NextGetErr diff --git a/pkg/controllers/controllers.go b/pkg/controllers/controllers.go index 1669f1c746..f7caafc463 100644 --- a/pkg/controllers/controllers.go +++ b/pkg/controllers/controllers.go @@ -62,8 +62,7 @@ func NewControllers( recorder events.Recorder, cloudProvider cloudprovider.CloudProvider, ) []controller.Controller { - - cluster := state.NewCluster(clock, kubeClient) + cluster := state.NewCluster(clock, kubeClient, cloudProvider) p := provisioning.NewProvisioner(kubeClient, recorder, cloudProvider, cluster, clock) evictionQueue := terminator.NewQueue(kubeClient, recorder) disruptionQueue := orchestration.NewQueue(kubeClient, recorder, cluster, clock, p) @@ -73,22 +72,22 @@ func NewControllers( disruption.NewController(clock, kubeClient, p, cloudProvider, recorder, cluster, disruptionQueue), provisioning.NewPodController(kubeClient, p), provisioning.NewNodeController(kubeClient, p), - nodepoolhash.NewController(kubeClient), - expiration.NewController(clock, kubeClient), + nodepoolhash.NewController(kubeClient, cloudProvider), + expiration.NewController(clock, kubeClient, cloudProvider), informer.NewDaemonSetController(kubeClient, cluster), informer.NewNodeController(kubeClient, cluster), informer.NewPodController(kubeClient, cluster), - informer.NewNodePoolController(kubeClient, cluster), - informer.NewNodeClaimController(kubeClient, cluster), + informer.NewNodePoolController(kubeClient, cloudProvider, cluster), + informer.NewNodeClaimController(kubeClient, cloudProvider, cluster), termination.NewController(clock, kubeClient, cloudProvider, terminator.NewTerminator(clock, kubeClient, evictionQueue, recorder), recorder), metricspod.NewController(kubeClient, cluster), - metricsnodepool.NewController(kubeClient), + metricsnodepool.NewController(kubeClient, cloudProvider), metricsnode.NewController(cluster), nodepoolreadiness.NewController(kubeClient, cloudProvider), - nodepoolcounter.NewController(kubeClient, cluster), - nodepoolvalidation.NewController(kubeClient), - podevents.NewController(clock, kubeClient), - nodeclaimconsistency.NewController(clock, kubeClient, recorder), + nodepoolcounter.NewController(kubeClient, cloudProvider, cluster), + nodepoolvalidation.NewController(kubeClient, cloudProvider), + podevents.NewController(clock, kubeClient, cloudProvider), + nodeclaimconsistency.NewController(clock, kubeClient, cloudProvider, recorder), nodeclaimlifecycle.NewController(clock, kubeClient, cloudProvider, recorder), nodeclaimgarbagecollection.NewController(clock, kubeClient, cloudProvider), nodeclaimdisruption.NewController(clock, kubeClient, cloudProvider), diff --git a/pkg/controllers/disruption/consolidation_test.go b/pkg/controllers/disruption/consolidation_test.go index d342a6136f..5777967c1c 100644 --- a/pkg/controllers/disruption/consolidation_test.go +++ b/pkg/controllers/disruption/consolidation_test.go @@ -616,7 +616,7 @@ var _ = Describe("Consolidation", func() { ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, nodes, nodeClaims) emptyConsolidation := disruption.NewEmptiness(disruption.MakeConsolidation(fakeClock, cluster, env.Client, prov, cloudProvider, recorder, queue)) - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, emptyConsolidation.Reason()) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, emptyConsolidation.Reason()) Expect(err).To(Succeed()) candidates, err := disruption.GetCandidates(ctx, cluster, env.Client, recorder, fakeClock, cloudProvider, emptyConsolidation.ShouldDisrupt, emptyConsolidation.Class(), queue) @@ -679,7 +679,7 @@ var _ = Describe("Consolidation", func() { ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, nodes, nodeClaims) emptyConsolidation := disruption.NewEmptiness(disruption.MakeConsolidation(fakeClock, cluster, env.Client, prov, cloudProvider, recorder, queue)) - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, emptyConsolidation.Reason()) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, emptyConsolidation.Reason()) Expect(err).To(Succeed()) candidates, err := disruption.GetCandidates(ctx, cluster, env.Client, recorder, fakeClock, cloudProvider, emptyConsolidation.ShouldDisrupt, emptyConsolidation.Class(), queue) @@ -703,7 +703,7 @@ var _ = Describe("Consolidation", func() { ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, nodes, nodeClaims) multiConsolidation := disruption.NewMultiNodeConsolidation(disruption.MakeConsolidation(fakeClock, cluster, env.Client, prov, cloudProvider, recorder, queue)) - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, multiConsolidation.Reason()) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, multiConsolidation.Reason()) Expect(err).To(Succeed()) candidates, err := disruption.GetCandidates(ctx, cluster, env.Client, recorder, fakeClock, cloudProvider, multiConsolidation.ShouldDisrupt, multiConsolidation.Class(), queue) @@ -766,7 +766,7 @@ var _ = Describe("Consolidation", func() { ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, nodes, nodeClaims) multiConsolidation := disruption.NewMultiNodeConsolidation(disruption.MakeConsolidation(fakeClock, cluster, env.Client, prov, cloudProvider, recorder, queue)) - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, multiConsolidation.Reason()) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, multiConsolidation.Reason()) Expect(err).To(Succeed()) candidates, err := disruption.GetCandidates(ctx, cluster, env.Client, recorder, fakeClock, cloudProvider, multiConsolidation.ShouldDisrupt, multiConsolidation.Class(), queue) @@ -790,7 +790,7 @@ var _ = Describe("Consolidation", func() { ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, nodes, nodeClaims) singleConsolidation := disruption.NewSingleNodeConsolidation(disruption.MakeConsolidation(fakeClock, cluster, env.Client, prov, cloudProvider, recorder, queue)) - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, singleConsolidation.Reason()) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, singleConsolidation.Reason()) Expect(err).To(Succeed()) candidates, err := disruption.GetCandidates(ctx, cluster, env.Client, recorder, fakeClock, cloudProvider, singleConsolidation.ShouldDisrupt, singleConsolidation.Class(), queue) @@ -853,7 +853,7 @@ var _ = Describe("Consolidation", func() { ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, nodes, nodeClaims) singleConsolidation := disruption.NewSingleNodeConsolidation(disruption.MakeConsolidation(fakeClock, cluster, env.Client, prov, cloudProvider, recorder, queue)) - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, singleConsolidation.Reason()) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, singleConsolidation.Reason()) Expect(err).To(Succeed()) candidates, err := disruption.GetCandidates(ctx, cluster, env.Client, recorder, fakeClock, cloudProvider, singleConsolidation.ShouldDisrupt, singleConsolidation.Class(), queue) @@ -3014,7 +3014,9 @@ var _ = Describe("Consolidation", func() { Spec: v1.NodeClaimTemplateSpec{ Requirements: []v1.NodeSelectorRequirementWithMinValues{}, NodeClassRef: &v1.NodeClassReference{ - Name: "non-existent", + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "non-existent", }, }, }, diff --git a/pkg/controllers/disruption/controller.go b/pkg/controllers/disruption/controller.go index e43e51f680..ff34302f0f 100644 --- a/pkg/controllers/disruption/controller.go +++ b/pkg/controllers/disruption/controller.go @@ -36,6 +36,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" "sigs.k8s.io/karpenter/pkg/utils/pretty" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -168,7 +169,7 @@ func (c *Controller) disrupt(ctx context.Context, disruption Method) (bool, erro if len(candidates) == 0 { return false, nil } - disruptionBudgetMapping, err := BuildDisruptionBudgetMapping(ctx, c.cluster, c.clock, c.kubeClient, c.recorder, disruption.Reason()) + disruptionBudgetMapping, err := BuildDisruptionBudgetMapping(ctx, c.cluster, c.clock, c.kubeClient, c.cloudProvider, c.recorder, disruption.Reason()) if err != nil { return false, fmt.Errorf("building disruption budgets, %w", err) } @@ -276,13 +277,13 @@ func (c *Controller) logAbnormalRuns(ctx context.Context) { // logInvalidBudgets will log if there are any invalid schedules detected func (c *Controller) logInvalidBudgets(ctx context.Context) { - nodePoolList := &v1.NodePoolList{} - if err := c.kubeClient.List(ctx, nodePoolList); err != nil { + nps, err := nodepoolutils.ListManaged(ctx, c.kubeClient, c.cloudProvider) + if err != nil { log.FromContext(ctx).Error(err, "failed listing nodepools") return } var buf bytes.Buffer - for _, np := range nodePoolList.Items { + for _, np := range nps { // Use a dummy value of 100 since we only care if this errors. for _, method := range c.methods { if _, err := np.GetAllowedDisruptionsByReason(c.clock, 100, method.Reason()); err != nil { diff --git a/pkg/controllers/disruption/helpers.go b/pkg/controllers/disruption/helpers.go index d6fe1243ef..a89c7d23d5 100644 --- a/pkg/controllers/disruption/helpers.go +++ b/pkg/controllers/disruption/helpers.go @@ -39,6 +39,7 @@ import ( "sigs.k8s.io/karpenter/pkg/metrics" operatorlogging "sigs.k8s.io/karpenter/pkg/operator/logging" nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" "sigs.k8s.io/karpenter/pkg/utils/pdb" ) @@ -162,13 +163,13 @@ func GetCandidates(ctx context.Context, cluster *state.Cluster, kubeClient clien // BuildNodePoolMap builds a provName -> nodePool map and a provName -> instanceName -> instance type map func BuildNodePoolMap(ctx context.Context, kubeClient client.Client, cloudProvider cloudprovider.CloudProvider) (map[string]*v1.NodePool, map[string]map[string]*cloudprovider.InstanceType, error) { nodePoolMap := map[string]*v1.NodePool{} - nodePoolList := &v1.NodePoolList{} - if err := kubeClient.List(ctx, nodePoolList); err != nil { + nodePools, err := nodepoolutils.ListManaged(ctx, kubeClient, cloudProvider) + if err != nil { return nil, nil, fmt.Errorf("listing node pools, %w", err) } + nodePoolToInstanceTypesMap := map[string]map[string]*cloudprovider.InstanceType{} - for i := range nodePoolList.Items { - np := &nodePoolList.Items[i] + for _, np := range nodePools { nodePoolMap[np.Name] = np nodePoolInstanceTypes, err := cloudProvider.GetInstanceTypes(ctx, np) @@ -193,7 +194,7 @@ func BuildNodePoolMap(ctx context.Context, kubeClient client.Client, cloudProvid // We calculate allowed disruptions by taking the max disruptions allowed by disruption reason and subtracting the number of nodes that are NotReady and already being deleted by that disruption reason. // //nolint:gocyclo -func BuildDisruptionBudgetMapping(ctx context.Context, cluster *state.Cluster, clk clock.Clock, kubeClient client.Client, recorder events.Recorder, reason v1.DisruptionReason) (map[string]int, error) { +func BuildDisruptionBudgetMapping(ctx context.Context, cluster *state.Cluster, clk clock.Clock, kubeClient client.Client, cloudProvider cloudprovider.CloudProvider, recorder events.Recorder, reason v1.DisruptionReason) (map[string]int, error) { disruptionBudgetMapping := map[string]int{} numNodes := map[string]int{} // map[nodepool] -> node count in nodepool disrupting := map[string]int{} // map[nodepool] -> nodes undergoing disruption @@ -226,19 +227,18 @@ func BuildDisruptionBudgetMapping(ctx context.Context, cluster *state.Cluster, c disrupting[nodePool]++ } } - nodePoolList := &v1.NodePoolList{} - if err := kubeClient.List(ctx, nodePoolList); err != nil { + nodePools, err := nodepoolutils.ListManaged(ctx, kubeClient, cloudProvider) + if err != nil { return disruptionBudgetMapping, fmt.Errorf("listing node pools, %w", err) } - for _, nodePool := range nodePoolList.Items { + for _, nodePool := range nodePools { allowedDisruptions := nodePool.MustGetAllowedDisruptions(clk, numNodes[nodePool.Name], reason) disruptionBudgetMapping[nodePool.Name] = lo.Max([]int{allowedDisruptions - disrupting[nodePool.Name], 0}) - NodePoolAllowedDisruptions.Set(float64(allowedDisruptions), map[string]string{ metrics.NodePoolLabel: nodePool.Name, metrics.ReasonLabel: string(reason), }) if allowedDisruptions == 0 { - recorder.Publish(disruptionevents.NodePoolBlockedForDisruptionReason(lo.ToPtr(nodePool), reason)) + recorder.Publish(disruptionevents.NodePoolBlockedForDisruptionReason(nodePool, reason)) } } return disruptionBudgetMapping, nil diff --git a/pkg/controllers/disruption/orchestration/suite_test.go b/pkg/controllers/disruption/orchestration/suite_test.go index dcb5054420..78a3c1260c 100644 --- a/pkg/controllers/disruption/orchestration/suite_test.go +++ b/pkg/controllers/disruption/orchestration/suite_test.go @@ -81,9 +81,9 @@ var _ = BeforeSuite(func() { ctx = options.ToContext(ctx, test.Options()) fakeClock = clock.NewFakeClock(time.Now()) cloudProvider = fake.NewCloudProvider() - cluster = state.NewCluster(fakeClock, env.Client) + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) nodeStateController = informer.NewNodeController(env.Client, cluster) - nodeClaimStateController = informer.NewNodeClaimController(env.Client, cluster) + nodeClaimStateController = informer.NewNodeClaimController(env.Client, cloudProvider, cluster) recorder = test.NewEventRecorder() prov = provisioning.NewProvisioner(env.Client, recorder, cloudProvider, cluster, fakeClock) queue = NewTestingQueue(env.Client, recorder, cluster, fakeClock, prov) diff --git a/pkg/controllers/disruption/suite_test.go b/pkg/controllers/disruption/suite_test.go index 4ee24f5bc8..8deea114de 100644 --- a/pkg/controllers/disruption/suite_test.go +++ b/pkg/controllers/disruption/suite_test.go @@ -92,9 +92,9 @@ var _ = BeforeSuite(func() { ctx = options.ToContext(ctx, test.Options()) cloudProvider = fake.NewCloudProvider() fakeClock = clock.NewFakeClock(time.Now()) - cluster = state.NewCluster(fakeClock, env.Client) + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) nodeStateController = informer.NewNodeController(env.Client, cluster) - nodeClaimStateController = informer.NewNodeClaimController(env.Client, cluster) + nodeClaimStateController = informer.NewNodeClaimController(env.Client, cloudProvider, cluster) recorder = test.NewEventRecorder() prov = provisioning.NewProvisioner(env.Client, recorder, cloudProvider, cluster, fakeClock) queue = NewTestingQueue(env.Client, recorder, cluster, fakeClock, prov) @@ -639,7 +639,7 @@ var _ = Describe("BuildDisruptionBudgetMapping", func() { ExpectApplied(ctx, env.Client, unmanaged) ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, []*corev1.Node{unmanaged}, []*v1.NodeClaim{}) for _, reason := range allKnownDisruptionReasons { - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, reason) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, reason) Expect(err).To(Succeed()) // This should not bring in the unmanaged node. Expect(budgets[nodePool.Name]).To(Equal(10)) @@ -670,7 +670,7 @@ var _ = Describe("BuildDisruptionBudgetMapping", func() { ExpectReconcileSucceeded(ctx, nodeClaimStateController, client.ObjectKeyFromObject(nodeClaim)) for _, reason := range allKnownDisruptionReasons { - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, reason) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, reason) Expect(err).To(Succeed()) // This should not bring in the uninitialized node. Expect(budgets[nodePool.Name]).To(Equal(10)) @@ -702,7 +702,7 @@ var _ = Describe("BuildDisruptionBudgetMapping", func() { ExpectReconcileSucceeded(ctx, nodeClaimStateController, client.ObjectKeyFromObject(nodeClaim)) for _, reason := range allKnownDisruptionReasons { - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, reason) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, reason) Expect(err).To(Succeed()) // This should not bring in the terminating node. Expect(budgets[nodePool.Name]).To(Equal(10)) @@ -724,7 +724,7 @@ var _ = Describe("BuildDisruptionBudgetMapping", func() { } for _, reason := range allKnownDisruptionReasons { - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, reason) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, reason) Expect(err).To(Succeed()) Expect(budgets[nodePool.Name]).To(Equal(0)) } @@ -748,7 +748,7 @@ var _ = Describe("BuildDisruptionBudgetMapping", func() { } for _, reason := range allKnownDisruptionReasons { - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, reason) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, reason) Expect(err).To(Succeed()) Expect(budgets[nodePool.Name]).To(Equal(8)) } @@ -769,7 +769,7 @@ var _ = Describe("BuildDisruptionBudgetMapping", func() { } for _, reason := range allKnownDisruptionReasons { - budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, recorder, reason) + budgets, err := disruption.BuildDisruptionBudgetMapping(ctx, cluster, fakeClock, env.Client, cloudProvider, recorder, reason) Expect(err).To(Succeed()) Expect(budgets[nodePool.Name]).To(Equal(8)) } diff --git a/pkg/controllers/disruption/validation.go b/pkg/controllers/disruption/validation.go index f3514c8a5a..c177a24245 100644 --- a/pkg/controllers/disruption/validation.go +++ b/pkg/controllers/disruption/validation.go @@ -128,7 +128,7 @@ func (v *Validation) ValidateCandidates(ctx context.Context, candidates ...*Cand if len(validatedCandidates) != len(candidates) { return nil, NewValidationError(fmt.Errorf("%d candidates are no longer valid", len(candidates)-len(validatedCandidates))) } - disruptionBudgetMapping, err := BuildDisruptionBudgetMapping(ctx, v.cluster, v.clock, v.kubeClient, v.recorder, v.reason) + disruptionBudgetMapping, err := BuildDisruptionBudgetMapping(ctx, v.cluster, v.clock, v.kubeClient, v.cloudProvider, v.recorder, v.reason) if err != nil { return nil, fmt.Errorf("building disruption budgets, %w", err) } diff --git a/pkg/controllers/metrics/node/suite_test.go b/pkg/controllers/metrics/node/suite_test.go index 19f4985800..1d654b49f4 100644 --- a/pkg/controllers/metrics/node/suite_test.go +++ b/pkg/controllers/metrics/node/suite_test.go @@ -61,7 +61,7 @@ var _ = BeforeSuite(func() { cloudProvider = fake.NewCloudProvider() cloudProvider.InstanceTypes = fake.InstanceTypesAssorted() fakeClock = clock.NewFakeClock(time.Now()) - cluster = state.NewCluster(fakeClock, env.Client) + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) nodeController = informer.NewNodeController(env.Client, cluster) metricsStateController = node.NewController(cluster) }) diff --git a/pkg/controllers/metrics/nodepool/controller.go b/pkg/controllers/metrics/nodepool/controller.go index 63e14ee59f..853f5a966c 100644 --- a/pkg/controllers/metrics/nodepool/controller.go +++ b/pkg/controllers/metrics/nodepool/controller.go @@ -27,14 +27,17 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" crmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/metrics" "sigs.k8s.io/karpenter/pkg/operator/injection" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" ) const ( @@ -72,15 +75,17 @@ var ( ) type Controller struct { - kubeClient client.Client - metricStore *metrics.Store + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider + metricStore *metrics.Store } // NewController constructs a controller instance -func NewController(kubeClient client.Client) *Controller { +func NewController(kubeClient client.Client, cloudProvider cloudprovider.CloudProvider) *Controller { return &Controller{ - kubeClient: kubeClient, - metricStore: metrics.NewStore(), + kubeClient: kubeClient, + cloudProvider: cloudProvider, + metricStore: metrics.NewStore(), } } @@ -95,6 +100,9 @@ func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reco } return reconcile.Result{}, client.IgnoreNotFound(err) } + if !nodepoolutils.IsManaged(nodePool, c.cloudProvider) { + return reconcile.Result{}, nil + } c.metricStore.Update(req.NamespacedName.String(), buildMetrics(nodePool)) // periodically update our metrics per nodepool even if nothing has changed return reconcile.Result{RequeueAfter: 5 * time.Minute}, nil @@ -133,6 +141,6 @@ func makeLabels(nodePool *v1.NodePool, resourceTypeName string) prometheus.Label func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("metrics.nodepool"). - For(&v1.NodePool{}). + For(&v1.NodePool{}, builder.WithPredicates(nodepoolutils.IsManagedPredicateFuncs(c.cloudProvider))). Complete(c) } diff --git a/pkg/controllers/metrics/nodepool/suite_test.go b/pkg/controllers/metrics/nodepool/suite_test.go index a3eeb6faeb..0253025c2c 100644 --- a/pkg/controllers/metrics/nodepool/suite_test.go +++ b/pkg/controllers/metrics/nodepool/suite_test.go @@ -29,6 +29,7 @@ import ( "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/controllers/metrics/nodepool" "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" @@ -39,6 +40,7 @@ import ( var nodePoolController *nodepool.Controller var ctx context.Context var env *test.Environment +var cp *fake.CloudProvider func TestAPIs(t *testing.T) { ctx = TestContextWithLogger(t) @@ -48,7 +50,8 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...)) - nodePoolController = nodepool.NewController(env.Client) + cp = fake.NewCloudProvider() + nodePoolController = nodepool.NewController(env.Client, cp) }) var _ = AfterSuite(func() { @@ -63,32 +66,48 @@ var _ = Describe("Metrics", func() { Template: v1.NodeClaimTemplate{ Spec: v1.NodeClaimTemplateSpec{ NodeClassRef: &v1.NodeClassReference{ - Name: "default", + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", }, }, }, }, }) }) - It("should update the nodepool limit metrics", func() { - limits := v1.Limits{ - corev1.ResourceCPU: resource.MustParse("10"), - corev1.ResourceMemory: resource.MustParse("10Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), - } - nodePool.Spec.Limits = limits - ExpectApplied(ctx, env.Client, nodePool) - ExpectReconcileSucceeded(ctx, nodePoolController, client.ObjectKeyFromObject(nodePool)) - - for k, v := range limits { - m, found := FindMetricWithLabelValues("karpenter_nodepools_limit", map[string]string{ - "nodepool": nodePool.GetName(), - "resource_type": strings.ReplaceAll(k.String(), "-", "_"), - }) - Expect(found).To(BeTrue()) - Expect(m.GetGauge().GetValue()).To(BeNumerically("~", v.AsApproximateFloat64())) - } - }) + DescribeTable( + "should update the nodepool limit metrics", + func(isNodePoolManaged bool) { + limits := v1.Limits{ + corev1.ResourceCPU: resource.MustParse("10"), + corev1.ResourceMemory: resource.MustParse("10Mi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + } + nodePool.Spec.Limits = limits + if !isNodePoolManaged { + nodePool.Spec.Template.Spec.NodeClassRef = &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + } + } + ExpectApplied(ctx, env.Client, nodePool) + ExpectReconcileSucceeded(ctx, nodePoolController, client.ObjectKeyFromObject(nodePool)) + + for k, v := range limits { + m, found := FindMetricWithLabelValues("karpenter_nodepools_limit", map[string]string{ + "nodepool": nodePool.GetName(), + "resource_type": strings.ReplaceAll(k.String(), "-", "_"), + }) + Expect(found).To(Equal(isNodePoolManaged)) + if isNodePoolManaged { + Expect(m.GetGauge().GetValue()).To(BeNumerically("~", v.AsApproximateFloat64())) + } + } + }, + Entry("should update the nodepool limit metrics", true), + Entry("should ignore nodepools not managed by this instance of Karpenter", false), + ) It("should update the nodepool usage metrics", func() { resources := corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("10"), diff --git a/pkg/controllers/metrics/pod/suite_test.go b/pkg/controllers/metrics/pod/suite_test.go index f649df6a4a..a69794e22f 100644 --- a/pkg/controllers/metrics/pod/suite_test.go +++ b/pkg/controllers/metrics/pod/suite_test.go @@ -29,6 +29,7 @@ import ( clock "k8s.io/utils/clock/testing" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/controllers/metrics/pod" "sigs.k8s.io/karpenter/pkg/controllers/state" "sigs.k8s.io/karpenter/pkg/test" @@ -40,6 +41,7 @@ var podController *pod.Controller var ctx context.Context var env *test.Environment var cluster *state.Cluster +var cloudProvider *fake.CloudProvider var fakeClock *clock.FakeClock func TestAPIs(t *testing.T) { @@ -51,7 +53,8 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { env = test.NewEnvironment() fakeClock = clock.NewFakeClock(time.Now()) - cluster = state.NewCluster(fakeClock, env.Client) + cloudProvider = fake.NewCloudProvider() + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) podController = pod.NewController(env.Client, cluster) }) diff --git a/pkg/controllers/node/health/suite_test.go b/pkg/controllers/node/health/suite_test.go index 4d9d8a9338..ee8d46b01d 100644 --- a/pkg/controllers/node/health/suite_test.go +++ b/pkg/controllers/node/health/suite_test.go @@ -27,9 +27,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clock "k8s.io/utils/clock/testing" - "sigs.k8s.io/controller-runtime/pkg/cache" - - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -60,11 +57,11 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeClaimFieldIndexer(ctx), test.VolumeAttachmentFieldIndexer(ctx), func(c cache.Cache) error { - return c.IndexField(ctx, &corev1.Node{}, "spec.providerID", func(obj client.Object) []string { - return []string{obj.(*corev1.Node).Spec.ProviderID} - }) - })) + env = test.NewEnvironment( + test.WithCRDs(apis.CRDs...), + test.WithCRDs(v1alpha1.CRDs...), + test.WithFieldIndexers(test.NodeClaimProviderIDFieldIndexer(ctx), test.VolumeAttachmentFieldIndexer(ctx), test.NodeProviderIDFieldIndexer(ctx)), + ) cloudProvider = fake.NewCloudProvider() cloudProvider = fake.NewCloudProvider() recorder = test.NewEventRecorder() diff --git a/pkg/controllers/node/termination/controller.go b/pkg/controllers/node/termination/controller.go index 6249d1b27e..67e12c6ebf 100644 --- a/pkg/controllers/node/termination/controller.go +++ b/pkg/controllers/node/termination/controller.go @@ -31,6 +31,7 @@ import ( "k8s.io/client-go/util/workqueue" "k8s.io/utils/clock" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -87,7 +88,11 @@ func (c *Controller) finalize(ctx context.Context, node *corev1.Node) (reconcile if !controllerutil.ContainsFinalizer(node, v1.TerminationFinalizer) { return reconcile.Result{}, nil } - nodeClaims, err := nodeutils.GetNodeClaims(ctx, node, c.kubeClient) + if !nodeutils.IsManaged(node, c.cloudProvider) { + return reconcile.Result{}, nil + } + + nodeClaims, err := nodeutils.GetNodeClaims(ctx, c.kubeClient, node) if err != nil { return reconcile.Result{}, fmt.Errorf("listing nodeclaims, %w", err) } @@ -143,7 +148,7 @@ func (c *Controller) finalize(ctx context.Context, node *corev1.Node) (reconcile return reconcile.Result{RequeueAfter: 1 * time.Second}, nil } } - nodeClaims, err = nodeutils.GetNodeClaims(ctx, node, c.kubeClient) + nodeClaims, err = nodeutils.GetNodeClaims(ctx, c.kubeClient, node) if err != nil { return reconcile.Result{}, fmt.Errorf("deleting nodeclaims, %w", err) } @@ -283,7 +288,7 @@ func (c *Controller) nodeTerminationTime(node *corev1.Node, nodeClaims ...*v1.No func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("node.termination"). - For(&corev1.Node{}). + For(&corev1.Node{}, builder.WithPredicates(nodeutils.IsManagedPredicateFuncs(c.cloudProvider))). WithOptions( controller.Options{ RateLimiter: workqueue.NewTypedMaxOfRateLimiter[reconcile.Request]( diff --git a/pkg/controllers/node/termination/suite_test.go b/pkg/controllers/node/termination/suite_test.go index d7727c48d9..5758579227 100644 --- a/pkg/controllers/node/termination/suite_test.go +++ b/pkg/controllers/node/termination/suite_test.go @@ -60,7 +60,11 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeClaimFieldIndexer(ctx), test.VolumeAttachmentFieldIndexer(ctx))) + env = test.NewEnvironment( + test.WithCRDs(apis.CRDs...), + test.WithCRDs(v1alpha1.CRDs...), + test.WithFieldIndexers(test.NodeClaimProviderIDFieldIndexer(ctx), test.VolumeAttachmentFieldIndexer(ctx)), + ) cloudProvider = fake.NewCloudProvider() recorder = test.NewEventRecorder() @@ -108,6 +112,18 @@ var _ = Describe("Termination", func() { ExpectObjectReconciled(ctx, env.Client, terminationController, node) ExpectNotFound(ctx, env.Client, node) }) + It("should ignore nodes not managed by this Karpenter instance", func() { + delete(node.Labels, "karpenter.test.sh/testnodeclass") + node.Labels = lo.Assign(node.Labels, map[string]string{"karpenter.test.sh/unmanagednodeclass": "default"}) + ExpectApplied(ctx, env.Client, node) + Expect(env.Client.Delete(ctx, node)).To(Succeed()) + node = ExpectNodeExists(ctx, env.Client, node.Name) + + // Reconcile twice, once to set the NodeClaim to terminating, another to check the instance termination status (and delete the node). + ExpectObjectReconciled(ctx, env.Client, terminationController, node) + ExpectObjectReconciled(ctx, env.Client, terminationController, node) + ExpectExists(ctx, env.Client, node) + }) It("should delete nodeclaims associated with nodes", func() { ExpectApplied(ctx, env.Client, node, nodeClaim, nodeClaim) Expect(env.Client.Delete(ctx, node)).To(Succeed()) @@ -126,25 +142,20 @@ var _ = Describe("Termination", func() { ExpectNotFound(ctx, env.Client, node, nodeClaim) }) It("should not race if deleting nodes in parallel", func() { - const numNodes = 10 - var nodes []*corev1.Node - for i := 0; i < numNodes; i++ { - node = test.Node(test.NodeOptions{ - ObjectMeta: metav1.ObjectMeta{ - Finalizers: []string{v1.TerminationFinalizer}, - }, - }) + nodes := lo.Times(10, func(_ int) *corev1.Node { + return test.NodeClaimLinkedNode(nodeClaim) + }) + for _, node := range nodes { ExpectApplied(ctx, env.Client, node, nodeClaim) Expect(env.Client.Delete(ctx, node)).To(Succeed()) - node = ExpectNodeExists(ctx, env.Client, node.Name) - nodes = append(nodes, node) + *node = *ExpectNodeExists(ctx, env.Client, node.Name) } // Reconcile twice, once to set the NodeClaim to terminating, another to check the instance termination status (and delete the node). for range 2 { var wg sync.WaitGroup // this is enough to trip the race detector - for i := 0; i < numNodes; i++ { + for i := range nodes { wg.Add(1) go func(node *corev1.Node) { defer GinkgoRecover() @@ -154,7 +165,9 @@ var _ = Describe("Termination", func() { } wg.Wait() } - ExpectNotFound(ctx, env.Client, lo.Map(nodes, func(n *corev1.Node, _ int) client.Object { return n })...) + for _, node := range nodes { + ExpectNotFound(ctx, env.Client, node) + } }) It("should exclude nodes from load balancers when terminating", func() { labels := map[string]string{"foo": "bar"} diff --git a/pkg/controllers/node/termination/terminator/terminator.go b/pkg/controllers/node/termination/terminator/terminator.go index 3310f50542..82940aa8ef 100644 --- a/pkg/controllers/node/termination/terminator/terminator.go +++ b/pkg/controllers/node/termination/terminator/terminator.go @@ -31,7 +31,7 @@ import ( terminatorevents "sigs.k8s.io/karpenter/pkg/controllers/node/termination/terminator/events" "sigs.k8s.io/karpenter/pkg/events" - nodeutil "sigs.k8s.io/karpenter/pkg/utils/node" + nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" podutil "sigs.k8s.io/karpenter/pkg/utils/pod" ) @@ -94,7 +94,7 @@ func (t *Terminator) Taint(ctx context.Context, node *corev1.Node, taint corev1. // Drain evicts pods from the node and returns true when all pods are evicted // https://kubernetes.io/docs/concepts/architecture/nodes/#graceful-node-shutdown func (t *Terminator) Drain(ctx context.Context, node *corev1.Node, nodeGracePeriodExpirationTime *time.Time) error { - pods, err := nodeutil.GetPods(ctx, t.kubeClient, node) + pods, err := nodeutils.GetPods(ctx, t.kubeClient, node) if err != nil { return fmt.Errorf("listing pods on node, %w", err) } diff --git a/pkg/controllers/nodeclaim/consistency/controller.go b/pkg/controllers/nodeclaim/consistency/controller.go index 832d10809d..6480c0bcb1 100644 --- a/pkg/controllers/nodeclaim/consistency/controller.go +++ b/pkg/controllers/nodeclaim/consistency/controller.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" "k8s.io/utils/clock" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/log" @@ -35,17 +36,19 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/events" "sigs.k8s.io/karpenter/pkg/operator/injection" - nodeclaimutil "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" ) type Controller struct { - clock clock.Clock - kubeClient client.Client - checks []Check - recorder events.Recorder - lastScanned *cache.Cache + clock clock.Clock + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider + checks []Check + recorder events.Recorder + lastScanned *cache.Cache } type Issue string @@ -59,12 +62,13 @@ type Check interface { // scanPeriod is how often we inspect and report issues that are found. const scanPeriod = 10 * time.Minute -func NewController(clk clock.Clock, kubeClient client.Client, recorder events.Recorder) *Controller { +func NewController(clk clock.Clock, kubeClient client.Client, cloudProvider cloudprovider.CloudProvider, recorder events.Recorder) *Controller { return &Controller{ - clock: clk, - kubeClient: kubeClient, - recorder: recorder, - lastScanned: cache.New(scanPeriod, 1*time.Minute), + clock: clk, + kubeClient: kubeClient, + cloudProvider: cloudProvider, + recorder: recorder, + lastScanned: cache.New(scanPeriod, 1*time.Minute), checks: []Check{ NewTermination(clk, kubeClient), NewNodeShape(), @@ -74,10 +78,13 @@ func NewController(clk clock.Clock, kubeClient client.Client, recorder events.Re func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "nodeclaim.consistency") - + if !nodeclaimutils.IsManaged(nodeClaim, c.cloudProvider) { + return reconcile.Result{}, nil + } if nodeClaim.Status.ProviderID == "" { return reconcile.Result{}, nil } + stored := nodeClaim.DeepCopy() // If we get an event before we should check for consistency checks, we ignore and wait if lastTime, ok := c.lastScanned.Get(string(nodeClaim.UID)); ok { @@ -92,9 +99,9 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (re // We assume the invariant that there is a single node for a single nodeClaim. If this invariant is violated, // then we assume this is bubbled up through the nodeClaim lifecycle controller and don't perform consistency checks - node, err := nodeclaimutil.NodeForNodeClaim(ctx, c.kubeClient, nodeClaim) + node, err := nodeclaimutils.NodeForNodeClaim(ctx, c.kubeClient, nodeClaim) if err != nil { - return reconcile.Result{}, nodeclaimutil.IgnoreDuplicateNodeError(nodeclaimutil.IgnoreNodeNotFoundError(err)) + return reconcile.Result{}, nodeclaimutils.IgnoreDuplicateNodeError(nodeclaimutils.IgnoreNodeNotFoundError(err)) } if err = c.checkConsistency(ctx, nodeClaim, node); err != nil { return reconcile.Result{}, err @@ -140,10 +147,10 @@ func (c *Controller) checkConsistency(ctx context.Context, nodeClaim *v1.NodeCla func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("nodeclaim.consistency"). - For(&v1.NodeClaim{}). + For(&v1.NodeClaim{}, builder.WithPredicates(nodeclaimutils.IsManagedPredicateFuncs(c.cloudProvider))). Watches( &corev1.Node{}, - nodeclaimutil.NodeEventHandler(c.kubeClient), + nodeclaimutils.NodeEventHandler(c.kubeClient, c.cloudProvider), ). WithOptions(controller.Options{MaxConcurrentReconciles: 10}). Complete(reconcile.AsReconciler(m.GetClient(), c)) diff --git a/pkg/controllers/nodeclaim/consistency/suite_test.go b/pkg/controllers/nodeclaim/consistency/suite_test.go index 24a6b24117..a5c318ac86 100644 --- a/pkg/controllers/nodeclaim/consistency/suite_test.go +++ b/pkg/controllers/nodeclaim/consistency/suite_test.go @@ -29,7 +29,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" clock "k8s.io/utils/clock/testing" - "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/karpenter/pkg/apis" @@ -58,15 +57,15 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(func(c cache.Cache) error { - return c.IndexField(ctx, &corev1.Node{}, "spec.providerID", func(obj client.Object) []string { - return []string{obj.(*corev1.Node).Spec.ProviderID} - }) - })) + env = test.NewEnvironment( + test.WithCRDs(apis.CRDs...), + test.WithCRDs(v1alpha1.CRDs...), + test.WithFieldIndexers(test.NodeClaimProviderIDFieldIndexer(ctx), test.NodeProviderIDFieldIndexer(ctx)), + ) ctx = options.ToContext(ctx, test.Options()) cp = &fake.CloudProvider{} recorder = test.NewEventRecorder() - nodeClaimConsistencyController = consistency.NewController(fakeClock, env.Client, recorder) + nodeClaimConsistencyController = consistency.NewController(fakeClock, env.Client, cp, recorder) }) var _ = AfterSuite(func() { @@ -89,42 +88,118 @@ var _ = Describe("NodeClaimController", func() { BeforeEach(func() { nodePool = test.NodePool() }) - Context("Termination failure", func() { - It("should detect issues with a node that is stuck deleting due to a PDB", func() { - nodeClaim, node := test.NodeClaimAndNode(v1.NodeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, - corev1.LabelInstanceTypeStable: "default-instance-type", + Context("Termination", func() { + DescribeTable( + "Termination", + func(isNodeClaimManaged bool) { + nodeClaimOpts := []v1.NodeClaim{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + corev1.LabelInstanceTypeStable: "default-instance-type", + }, }, - }, - Status: v1.NodeClaimStatus{ - ProviderID: test.RandomProviderID(), - Capacity: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("1Gi"), - corev1.ResourcePods: resource.MustParse("10"), + Status: v1.NodeClaimStatus{ + ProviderID: test.RandomProviderID(), + Capacity: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + corev1.ResourcePods: resource.MustParse("10"), + }, }, - }, - }) - podsLabels := map[string]string{"myapp": "deleteme"} - pdb := test.PodDisruptionBudget(test.PDBOptions{ - Labels: podsLabels, - MaxUnavailable: &intstr.IntOrString{IntVal: 0, Type: intstr.Int}, - }) - nodeClaim.Finalizers = []string{"prevent.deletion/now"} - p := test.Pod(test.PodOptions{ObjectMeta: metav1.ObjectMeta{Labels: podsLabels}}) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node, p, pdb) - ExpectManualBinding(ctx, env.Client, p, node) - _ = env.Client.Delete(ctx, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimConsistencyController, nodeClaim) - Expect(recorder.DetectedEvent(fmt.Sprintf("can't drain node, PDB %q is blocking evictions", client.ObjectKeyFromObject(pdb)))).To(BeTrue()) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeConsistentStateFound).IsFalse()).To(BeTrue()) - }) + }} + if !isNodeClaimManaged { + nodeClaimOpts = append(nodeClaimOpts, v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + } + nodeClaim, node := test.NodeClaimAndNode(nodeClaimOpts...) + podsLabels := map[string]string{"myapp": "deleteme"} + pdb := test.PodDisruptionBudget(test.PDBOptions{ + Labels: podsLabels, + MaxUnavailable: &intstr.IntOrString{IntVal: 0, Type: intstr.Int}, + }) + nodeClaim.Finalizers = []string{"prevent.deletion/now"} + p := test.Pod(test.PodOptions{ObjectMeta: metav1.ObjectMeta{Labels: podsLabels}}) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node, p, pdb) + ExpectManualBinding(ctx, env.Client, p, node) + _ = env.Client.Delete(ctx, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimConsistencyController, nodeClaim) + Expect(recorder.DetectedEvent(fmt.Sprintf("can't drain node, PDB %q is blocking evictions", client.ObjectKeyFromObject(pdb)))).To(Equal(isNodeClaimManaged)) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + if isNodeClaimManaged { + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeConsistentStateFound).IsFalse()).To(BeTrue()) + } else { + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeConsistentStateFound).IsUnknown()).To(BeTrue()) + } + }, + Entry("should detect issues with a node that is stuck deleting due to a PDB", true), + Entry("should ignore NodeClaims which aren't managed by this instance of Karpenter", false), + ) }) Context("Node Shape", func() { + DescribeTable( + "Node Shape", + func(isNodeClaimManaged bool) { + nodeClaimOpts := []v1.NodeClaim{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + corev1.LabelInstanceTypeStable: "arm-instance-type", + v1.NodeInitializedLabelKey: "true", + }, + }, + Spec: v1.NodeClaimSpec{ + Resources: v1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("64Gi"), + corev1.ResourcePods: resource.MustParse("5"), + }, + }, + }, + Status: v1.NodeClaimStatus{ + ProviderID: test.RandomProviderID(), + Capacity: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("16"), + corev1.ResourceMemory: resource.MustParse("128Gi"), + corev1.ResourcePods: resource.MustParse("10"), + }, + }, + }} + if !isNodeClaimManaged { + nodeClaimOpts = append(nodeClaimOpts, v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + } + nodeClaim, node := test.NodeClaimAndNode(nodeClaimOpts...) + nodeClaim.StatusConditions().SetUnknown(v1.ConditionTypeConsistentStateFound) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node) + ExpectMakeNodeClaimsInitialized(ctx, env.Client, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimConsistencyController, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + if isNodeClaimManaged { + Expect(nodeClaim.StatusConditions().IsTrue(v1.ConditionTypeConsistentStateFound)).To(BeTrue()) + } else { + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeConsistentStateFound).IsUnknown()).To(BeTrue()) + } + }, + Entry("should set consistent state found condition to true if there are no consistency issues", true), + Entry("should ignore NodeClaims which aren't managed by this instance of Karpenter", false), + ) It("should detect issues that launch with much fewer resources than expected", func() { nodeClaim, node := test.NodeClaimAndNode(v1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{ @@ -164,39 +239,5 @@ var _ = Describe("NodeClaimController", func() { nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeConsistentStateFound).IsFalse()).To(BeTrue()) }) - It("should set consistent state found condition to true if there are no consistency issues", func() { - nodeClaim, node := test.NodeClaimAndNode(v1.NodeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, - corev1.LabelInstanceTypeStable: "arm-instance-type", - v1.NodeInitializedLabelKey: "true", - }, - }, - Spec: v1.NodeClaimSpec{ - Resources: v1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("8"), - corev1.ResourceMemory: resource.MustParse("64Gi"), - corev1.ResourcePods: resource.MustParse("5"), - }, - }, - }, - Status: v1.NodeClaimStatus{ - ProviderID: test.RandomProviderID(), - Capacity: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("16"), - corev1.ResourceMemory: resource.MustParse("128Gi"), - corev1.ResourcePods: resource.MustParse("10"), - }, - }, - }) - nodeClaim.StatusConditions().SetUnknown(v1.ConditionTypeConsistentStateFound) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node) - ExpectMakeNodeClaimsInitialized(ctx, env.Client, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimConsistencyController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().IsTrue(v1.ConditionTypeConsistentStateFound)).To(BeTrue()) - }) }) }) diff --git a/pkg/controllers/nodeclaim/disruption/consolidation_test.go b/pkg/controllers/nodeclaim/disruption/consolidation_test.go index 821874b11e..d2cb27ffed 100644 --- a/pkg/controllers/nodeclaim/disruption/consolidation_test.go +++ b/pkg/controllers/nodeclaim/disruption/consolidation_test.go @@ -49,6 +49,39 @@ var _ = Describe("Underutilized", func() { nodeClaim.StatusConditions().SetTrue(v1.ConditionTypeInitialized) ExpectApplied(ctx, env.Client, nodeClaim, nodePool) }) + It("should ignore NodeClaims not managed by this instance of Karpenter", func() { + unmanagedNodeClaim, _ := test.NodeClaimAndNode(v1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + corev1.LabelInstanceTypeStable: it.Name, + }, + }, + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + unmanagedNodeClaim.Status.LastPodEventTime.Time = fakeClock.Now().Add(-5 * time.Minute) + unmanagedNodeClaim.StatusConditions().SetTrue(v1.ConditionTypeInitialized) + ExpectApplied(ctx, env.Client, unmanagedNodeClaim, nodePool) + + // set the lastPodEvent as now, so it's first marked as not consolidatable + unmanagedNodeClaim.Status.LastPodEventTime.Time = fakeClock.Now() + ExpectApplied(ctx, env.Client, unmanagedNodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, unmanagedNodeClaim) + unmanagedNodeClaim = ExpectExists(ctx, env.Client, unmanagedNodeClaim) + Expect(unmanagedNodeClaim.StatusConditions().Get(v1.ConditionTypeConsolidatable).IsUnknown()).To(BeTrue()) + + fakeClock.Step(1 * time.Minute) + + ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, unmanagedNodeClaim) + unmanagedNodeClaim = ExpectExists(ctx, env.Client, unmanagedNodeClaim) + Expect(unmanagedNodeClaim.StatusConditions().Get(v1.ConditionTypeConsolidatable).IsUnknown()).To(BeTrue()) + }) It("should mark NodeClaims as consolidatable", func() { // set the lastPodEvent as now, so it's first marked as not consolidatable nodeClaim.Status.LastPodEventTime.Time = fakeClock.Now() diff --git a/pkg/controllers/nodeclaim/disruption/controller.go b/pkg/controllers/nodeclaim/disruption/controller.go index e5cef47117..9aadb36a04 100644 --- a/pkg/controllers/nodeclaim/disruption/controller.go +++ b/pkg/controllers/nodeclaim/disruption/controller.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/utils/clock" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -34,7 +35,7 @@ import ( v1 "sigs.k8s.io/karpenter/pkg/apis/v1" "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/operator/injection" - nodeclaimutil "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" "sigs.k8s.io/karpenter/pkg/utils/result" ) @@ -67,6 +68,9 @@ func NewController(clk clock.Clock, kubeClient client.Client, cloudProvider clou func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "nodeclaim.disruption") + if !nodeclaimutils.IsManaged(nodeClaim, c.cloudProvider) { + return reconcile.Result{}, nil + } if !nodeClaim.DeletionTimestamp.IsZero() { return reconcile.Result{}, nil } @@ -109,24 +113,15 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (re } func (c *Controller) Register(_ context.Context, m manager.Manager) error { - builder := controllerruntime.NewControllerManagedBy(m) - for _, nodeClass := range c.cloudProvider.GetSupportedNodeClasses() { - builder = builder.Watches( - nodeClass, - nodeclaimutil.NodeClassEventHandler(c.kubeClient), - ) - } - return builder. + b := controllerruntime.NewControllerManagedBy(m). Named("nodeclaim.disruption"). - For(&v1.NodeClaim{}). + For(&v1.NodeClaim{}, builder.WithPredicates(nodeclaimutils.IsManagedPredicateFuncs(c.cloudProvider))). WithOptions(controller.Options{MaxConcurrentReconciles: 10}). - Watches( - &v1.NodePool{}, - nodeclaimutil.NodePoolEventHandler(c.kubeClient), - ). - Watches( - &corev1.Pod{}, - nodeclaimutil.PodEventHandler(c.kubeClient), - ). - Complete(reconcile.AsReconciler(m.GetClient(), c)) + Watches(&v1.NodePool{}, nodeclaimutils.NodePoolEventHandler(c.kubeClient, c.cloudProvider)). + Watches(&corev1.Pod{}, nodeclaimutils.PodEventHandler(c.kubeClient, c.cloudProvider)) + + for _, nodeClass := range c.cloudProvider.GetSupportedNodeClasses() { + b.Watches(nodeClass, nodeclaimutils.NodeClassEventHandler(c.kubeClient)) + } + return b.Complete(reconcile.AsReconciler(m.GetClient(), c)) } diff --git a/pkg/controllers/nodeclaim/disruption/drift_test.go b/pkg/controllers/nodeclaim/disruption/drift_test.go index 0137607ce3..7ba3dd13f0 100644 --- a/pkg/controllers/nodeclaim/disruption/drift_test.go +++ b/pkg/controllers/nodeclaim/disruption/drift_test.go @@ -58,14 +58,30 @@ var _ = Describe("Drift", func() { nodeClaim.StatusConditions().SetTrue(v1.ConditionTypeLaunched) Expect(nodeClaim.StatusConditions().Clear(v1.ConditionTypeDrifted)).To(Succeed()) }) - It("should detect drift", func() { - cp.Drifted = "drifted" - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) + DescribeTable( + "Drift", + func(isNodeClaimManaged bool) { + cp.Drifted = "drifted" + if !isNodeClaimManaged { + nodeClaim.Spec.NodeClassRef = &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + } + } + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeTrue()) - }) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + if isNodeClaimManaged { + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeTrue()) + } else { + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsUnknown()).To(BeTrue()) + } + }, + Entry("should detect drift", true), + Entry("should ignore drift for NodeClaims not managed by this instance of Karpenter", false), + ) It("should detect stale instance type drift if the instance type label doesn't exist", func() { delete(nodeClaim.Labels, corev1.LabelInstanceTypeStable) ExpectApplied(ctx, env.Client, nodePool, nodeClaim) @@ -393,7 +409,7 @@ var _ = Describe("Drift", func() { var nodePoolController *hash.Controller BeforeEach(func() { cp.Drifted = "" - nodePoolController = hash.NewController(env.Client) + nodePoolController = hash.NewController(env.Client, cp) nodePool = &v1.NodePool{ ObjectMeta: nodePool.ObjectMeta, Spec: v1.NodePoolSpec{ @@ -411,9 +427,9 @@ var _ = Describe("Drift", func() { Spec: v1.NodeClaimTemplateSpec{ Requirements: nodePool.Spec.Template.Spec.Requirements, NodeClassRef: &v1.NodeClassReference{ - Kind: "fakeKind", - Name: "fakeName", - Group: "fakeGroup", + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", }, Taints: []corev1.Taint{ { @@ -458,9 +474,7 @@ var _ = Describe("Drift", func() { Entry("Labels", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{ObjectMeta: v1.ObjectMeta{Labels: map[string]string{"keyLabelTest": "valueLabelTest"}}}}}), Entry("Taints", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{Taints: []corev1.Taint{{Key: "keytest2taint", Effect: corev1.TaintEffectNoExecute}}}}}}), Entry("StartupTaints", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{StartupTaints: []corev1.Taint{{Key: "keytest2taint", Effect: corev1.TaintEffectNoExecute}}}}}}), - Entry("NodeClassRef APIVersion", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{NodeClassRef: &v1.NodeClassReference{Group: "testVersion"}}}}}), Entry("NodeClassRef Name", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{NodeClassRef: &v1.NodeClassReference{Name: "testName"}}}}}), - Entry("NodeClassRef Kind", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{NodeClassRef: &v1.NodeClassReference{Kind: "testKind"}}}}}), Entry("ExpireAfter", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{ExpireAfter: v1.MustParseNillableDuration("100m")}}}}), Entry("TerminationGracePeriod", v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{TerminationGracePeriod: &metav1.Duration{Duration: 100 * time.Minute}}}}}), ) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index 0e4fe6d19e..23fbc1641b 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -28,8 +28,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" clock "k8s.io/utils/clock/testing" - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -59,11 +57,7 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(func(c cache.Cache) error { - return c.IndexField(ctx, &corev1.Node{}, "spec.providerID", func(obj client.Object) []string { - return []string{obj.(*corev1.Node).Spec.ProviderID} - }) - })) + env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeProviderIDFieldIndexer(ctx))) ctx = options.ToContext(ctx, test.Options()) cp = fake.NewCloudProvider() nodeClaimDisruptionController = nodeclaimdisruption.NewController(fakeClock, env.Client, cp) diff --git a/pkg/controllers/nodeclaim/expiration/controller.go b/pkg/controllers/nodeclaim/expiration/controller.go index 13c7cfd1bb..479f30d6a0 100644 --- a/pkg/controllers/nodeclaim/expiration/controller.go +++ b/pkg/controllers/nodeclaim/expiration/controller.go @@ -22,30 +22,38 @@ import ( "k8s.io/utils/clock" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/metrics" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" ) // Expiration is a nodeclaim controller that deletes expired nodeclaims based on expireAfter type Controller struct { - clock clock.Clock - kubeClient client.Client + clock clock.Clock + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider } // NewController constructs a nodeclaim disruption controller -func NewController(clk clock.Clock, kubeClient client.Client) *Controller { +func NewController(clk clock.Clock, kubeClient client.Client, cloudProvider cloudprovider.CloudProvider) *Controller { return &Controller{ - kubeClient: kubeClient, - clock: clk, + clock: clk, + kubeClient: kubeClient, + cloudProvider: cloudProvider, } } func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { + if !nodeclaimutils.IsManaged(nodeClaim, c.cloudProvider) { + return reconcile.Result{}, nil + } if !nodeClaim.DeletionTimestamp.IsZero() { return reconcile.Result{}, nil } @@ -81,6 +89,6 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (re func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("nodeclaim.expiration"). - For(&v1.NodeClaim{}). + For(&v1.NodeClaim{}, builder.WithPredicates(nodeclaimutils.IsManagedPredicateFuncs(c.cloudProvider))). Complete(reconcile.AsReconciler(m.GetClient(), c)) } diff --git a/pkg/controllers/nodeclaim/expiration/suite_test.go b/pkg/controllers/nodeclaim/expiration/suite_test.go index fc152533ca..3ab3cc6592 100644 --- a/pkg/controllers/nodeclaim/expiration/suite_test.go +++ b/pkg/controllers/nodeclaim/expiration/suite_test.go @@ -26,11 +26,10 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clock "k8s.io/utils/clock/testing" - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/controllers/nodeclaim/expiration" "sigs.k8s.io/karpenter/pkg/metrics" "sigs.k8s.io/karpenter/pkg/operator/options" @@ -43,6 +42,7 @@ import ( var ctx context.Context var expirationController *expiration.Controller var env *test.Environment +var cp *fake.CloudProvider var fakeClock *clock.FakeClock func TestAPIs(t *testing.T) { @@ -53,13 +53,10 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(func(c cache.Cache) error { - return c.IndexField(ctx, &corev1.Node{}, "spec.providerID", func(obj client.Object) []string { - return []string{obj.(*corev1.Node).Spec.ProviderID} - }) - })) + env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeProviderIDFieldIndexer(ctx))) ctx = options.ToContext(ctx, test.Options()) - expirationController = expiration.NewController(fakeClock, env.Client) + cp = fake.NewCloudProvider() + expirationController = expiration.NewController(fakeClock, env.Client, cp) }) var _ = AfterSuite(func() { @@ -121,23 +118,40 @@ var _ = Describe("Expiration", func() { }) }) }) + DescribeTable( + "Expiration", + func(isNodeClaimManaged bool) { + nodeClaim.Spec.ExpireAfter = v1.MustParseNillableDuration("30s") + if !isNodeClaimManaged { + nodeClaim.Spec.NodeClassRef = &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + } + } + ExpectApplied(ctx, env.Client, nodeClaim) + + // step forward to make the node expired + fakeClock.Step(60 * time.Second) + ExpectObjectReconciled(ctx, env.Client, expirationController, nodeClaim) + if isNodeClaimManaged { + // with forceful termination, when we see a nodeclaim meets the conditions for expiration + // we should remove it + ExpectNotFound(ctx, env.Client, nodeClaim) + } else { + ExpectExists(ctx, env.Client, nodeClaim) + } + }, + Entry("should remove nodeclaims that are expired", true), + Entry("should ignore expired NodeClaims that are not managed by this Karpenter instance", false), + ) + It("should not remove the NodeClaims when expiration is disabled", func() { nodeClaim.Spec.ExpireAfter = v1.MustParseNillableDuration("Never") ExpectApplied(ctx, env.Client, nodeClaim) ExpectObjectReconciled(ctx, env.Client, expirationController, nodeClaim) nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) }) - It("should remove nodeclaims that are expired", func() { - nodeClaim.Spec.ExpireAfter = v1.MustParseNillableDuration("30s") - ExpectApplied(ctx, env.Client, nodeClaim) - - // step forward to make the node expired - fakeClock.Step(60 * time.Second) - ExpectObjectReconciled(ctx, env.Client, expirationController, nodeClaim) - // with forceful termination, when we see a nodeclaim meets the conditions for expiration - // we should remove it - ExpectNotFound(ctx, env.Client, nodeClaim) - }) It("should not remove non-expired NodeClaims", func() { nodeClaim.Spec.ExpireAfter = v1.MustParseNillableDuration("200s") ExpectApplied(ctx, env.Client, nodeClaim) diff --git a/pkg/controllers/nodeclaim/garbagecollection/controller.go b/pkg/controllers/nodeclaim/garbagecollection/controller.go index 5a4d510682..bfaaa28bf0 100644 --- a/pkg/controllers/nodeclaim/garbagecollection/controller.go +++ b/pkg/controllers/nodeclaim/garbagecollection/controller.go @@ -39,7 +39,7 @@ import ( "sigs.k8s.io/karpenter/pkg/metrics" "sigs.k8s.io/karpenter/pkg/operator/injection" nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" - nodeclaimutil "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" ) type Controller struct { @@ -59,8 +59,8 @@ func NewController(c clock.Clock, kubeClient client.Client, cloudProvider cloudp func (c *Controller) Reconcile(ctx context.Context) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "nodeclaim.garbagecollection") - nodeClaimList := &v1.NodeClaimList{} - if err := c.kubeClient.List(ctx, nodeClaimList); err != nil { + nodeClaims, err := nodeclaimutils.ListManaged(ctx, c.kubeClient, c.cloudProvider) + if err != nil { return reconcile.Result{}, err } cloudProviderNodeClaims, err := c.cloudProvider.List(ctx) @@ -75,7 +75,7 @@ func (c *Controller) Reconcile(ctx context.Context) (reconcile.Result, error) { })...) // Only consider NodeClaims that are Registered since we don't want to fully rely on the CloudProvider // API to trigger deletion of the Node. Instead, we'll wait for our registration timeout to trigger - nodeClaims := lo.Filter(lo.ToSlicePtr(nodeClaimList.Items), func(n *v1.NodeClaim, _ int) bool { + nodeClaims = lo.Filter(nodeClaims, func(n *v1.NodeClaim, _ int) bool { return n.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue() && n.DeletionTimestamp.IsZero() && !cloudProviderProviderIDs.Has(n.Status.ProviderID) @@ -83,10 +83,10 @@ func (c *Controller) Reconcile(ctx context.Context) (reconcile.Result, error) { errs := make([]error, len(nodeClaims)) workqueue.ParallelizeUntil(ctx, 20, len(nodeClaims), func(i int) { - node, err := nodeclaimutil.NodeForNodeClaim(ctx, c.kubeClient, nodeClaims[i]) + node, err := nodeclaimutils.NodeForNodeClaim(ctx, c.kubeClient, nodeClaims[i]) // Ignore these errors since a registered NodeClaim should only have a NotFound node when // the Node was deleted out from under us and a Duplicate Node is an invalid state - if nodeclaimutil.IgnoreDuplicateNodeError(nodeclaimutil.IgnoreNodeNotFoundError(err)) != nil { + if nodeclaimutils.IgnoreDuplicateNodeError(nodeclaimutils.IgnoreNodeNotFoundError(err)) != nil { errs[i] = err } // We do a check on the Ready condition of the node since, even though the CloudProvider says the instance diff --git a/pkg/controllers/nodeclaim/garbagecollection/suite_test.go b/pkg/controllers/nodeclaim/garbagecollection/suite_test.go index 80666e9168..65b13682b0 100644 --- a/pkg/controllers/nodeclaim/garbagecollection/suite_test.go +++ b/pkg/controllers/nodeclaim/garbagecollection/suite_test.go @@ -29,7 +29,6 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" clock "k8s.io/utils/clock/testing" - "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/karpenter/pkg/apis" @@ -60,11 +59,7 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(func(c cache.Cache) error { - return c.IndexField(ctx, &corev1.Node{}, "spec.providerID", func(obj client.Object) []string { - return []string{obj.(*corev1.Node).Spec.ProviderID} - }) - })) + env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeProviderIDFieldIndexer(ctx))) ctx = options.ToContext(ctx, test.Options()) cloudProvider = fake.NewCloudProvider() garbageCollectionController = nodeclaimgarbagecollection.NewController(fakeClock, env.Client, cloudProvider) diff --git a/pkg/controllers/nodeclaim/lifecycle/controller.go b/pkg/controllers/nodeclaim/lifecycle/controller.go index 35b149f409..89401dfdae 100644 --- a/pkg/controllers/nodeclaim/lifecycle/controller.go +++ b/pkg/controllers/nodeclaim/lifecycle/controller.go @@ -32,6 +32,7 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/clock" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -46,7 +47,7 @@ import ( "sigs.k8s.io/karpenter/pkg/events" "sigs.k8s.io/karpenter/pkg/metrics" "sigs.k8s.io/karpenter/pkg/operator/injection" - nodeclaimutil "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" "sigs.k8s.io/karpenter/pkg/utils/result" terminationutil "sigs.k8s.io/karpenter/pkg/utils/termination" ) @@ -68,6 +69,7 @@ type Controller struct { registration *Registration initialization *Initialization liveness *Liveness + hydration *Hydration } func NewController(clk clock.Clock, kubeClient client.Client, cloudProvider cloudprovider.CloudProvider, recorder events.Recorder) *Controller { @@ -80,16 +82,17 @@ func NewController(clk clock.Clock, kubeClient client.Client, cloudProvider clou registration: &Registration{kubeClient: kubeClient}, initialization: &Initialization{kubeClient: kubeClient}, liveness: &Liveness{clock: clk, kubeClient: kubeClient}, + hydration: &Hydration{kubeClient: kubeClient}, } } func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named(c.Name()). - For(&v1.NodeClaim{}). + For(&v1.NodeClaim{}, builder.WithPredicates(nodeclaimutils.IsManagedPredicateFuncs(c.cloudProvider))). Watches( &corev1.Node{}, - nodeclaimutil.NodeEventHandler(c.kubeClient), + nodeclaimutils.NodeEventHandler(c.kubeClient, c.cloudProvider), ). WithOptions(controller.Options{ RateLimiter: workqueue.NewTypedMaxOfRateLimiter[reconcile.Request]( @@ -110,6 +113,9 @@ func (c *Controller) Name() string { func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, c.Name()) + if !nodeclaimutils.IsManaged(nodeClaim, c.cloudProvider) { + return reconcile.Result{}, nil + } if !nodeClaim.DeletionTimestamp.IsZero() { return c.finalize(ctx, nodeClaim) } @@ -138,6 +144,7 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (re c.registration, c.initialization, c.liveness, + c.hydration, } { res, err := reconciler.Reconcile(ctx, nodeClaim) errs = multierr.Append(errs, err) @@ -181,7 +188,7 @@ func (c *Controller) finalize(ctx context.Context, nodeClaim *v1.NodeClaim) (rec // instance is terminated by CCM. // Upstream Kubelet Fix: https://github.com/kubernetes/kubernetes/pull/119661 if nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue() { - nodes, err := nodeclaimutil.AllNodesForNodeClaim(ctx, c.kubeClient, nodeClaim) + nodes, err := nodeclaimutils.AllNodesForNodeClaim(ctx, c.kubeClient, nodeClaim) if err != nil { return reconcile.Result{}, err } diff --git a/pkg/controllers/nodeclaim/lifecycle/hydration.go b/pkg/controllers/nodeclaim/lifecycle/hydration.go new file mode 100644 index 0000000000..832f8480f5 --- /dev/null +++ b/pkg/controllers/nodeclaim/lifecycle/hydration.go @@ -0,0 +1,69 @@ +/* +Copyright The Kubernetes Authors. + +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 lifecycle + +import ( + "context" + "fmt" + + "github.com/samber/lo" + "k8s.io/apimachinery/pkg/api/equality" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" +) + +// The Hydration sub-reconciler is used to hydrate Nodes / NodeClaims with metadata added in new versions of Karpenter. +// TODO: Remove after a sufficiently long time from the v1.1 release +type Hydration struct { + kubeClient client.Client +} + +func (h *Hydration) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { + nodeClaim.Labels = lo.Assign(nodeClaim.Labels, map[string]string{ + v1.NodeClassLabelKey(nodeClaim.Spec.NodeClassRef.GroupKind()): nodeClaim.Spec.NodeClassRef.Name, + }) + if err := h.hydrateNode(ctx, nodeClaim); err != nil { + return reconcile.Result{}, client.IgnoreNotFound(fmt.Errorf("hydrating node, %w", err)) + } + return reconcile.Result{}, nil +} + +func (h *Hydration) hydrateNode(ctx context.Context, nodeClaim *v1.NodeClaim) error { + node, err := nodeclaimutils.NodeForNodeClaim(ctx, h.kubeClient, nodeClaim) + if err != nil { + if nodeclaimutils.IsNodeNotFoundError(err) { + return nil + } + return err + } + stored := node.DeepCopy() + node.Labels = lo.Assign(node.Labels, map[string]string{ + v1.NodeClassLabelKey(nodeClaim.Spec.NodeClassRef.GroupKind()): nodeClaim.Spec.NodeClassRef.Name, + }) + if !equality.Semantic.DeepEqual(stored, node) { + // We use client.MergeFromWithOptimisticLock because patching a list with a JSON merge patch + // can cause races due to the fact that it fully replaces the list on a change + // Here, we are updating the taint list + if err := h.kubeClient.Patch(ctx, node, client.MergeFrom(stored)); err != nil { + return err + } + } + return nil +} diff --git a/pkg/controllers/nodeclaim/lifecycle/hydration_test.go b/pkg/controllers/nodeclaim/lifecycle/hydration_test.go new file mode 100644 index 0000000000..9ada851a31 --- /dev/null +++ b/pkg/controllers/nodeclaim/lifecycle/hydration_test.go @@ -0,0 +1,76 @@ +/* +Copyright The Kubernetes Authors. + +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 lifecycle_test + +import ( + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/samber/lo" + + v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/test" + . "sigs.k8s.io/karpenter/pkg/test/expectations" +) + +var _ = Describe("Hydration", func() { + DescribeTable( + "Hydration", + func(isNodeClaimManaged bool) { + nodeClassRef := lo.Ternary(isNodeClaimManaged, &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", + }, &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }) + nodeClaim, node := test.NodeClaimAndNode(v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: nodeClassRef, + }, + }) + delete(nodeClaim.Labels, v1.NodeClassLabelKey(nodeClassRef.GroupKind())) + delete(node.Labels, v1.NodeClassLabelKey(nodeClassRef.GroupKind())) + // Launch the NodeClaim to ensure the lifecycle controller doesn't override the provider-id and break the + // link between the Node and NodeClaim. + nodeClaim.StatusConditions().SetTrue(v1.ConditionTypeLaunched) + ExpectApplied(ctx, env.Client, nodeClaim, node) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + + // The missing NodeClass label should have been propagated to both the Node and NodeClaim + node = ExpectExists(ctx, env.Client, node) + fmt.Printf("provider id: %s\n", node.Spec.ProviderID) + value, ok := node.Labels[v1.NodeClassLabelKey(nodeClassRef.GroupKind())] + Expect(ok).To(Equal(isNodeClaimManaged)) + if isNodeClaimManaged { + Expect(value).To(Equal(nodeClassRef.Name)) + } + + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + value, ok = nodeClaim.Labels[v1.NodeClassLabelKey(nodeClassRef.GroupKind())] + Expect(ok).To(Equal(isNodeClaimManaged)) + if isNodeClaimManaged { + Expect(value).To(Equal(nodeClassRef.Name)) + } + }, + Entry("should hydrate missing metadata onto the NodeClaim and Node", true), + Entry("should ignore NodeClaims which aren't managed by this Karpenter instance", false), + ) +}) diff --git a/pkg/controllers/nodeclaim/lifecycle/initialization.go b/pkg/controllers/nodeclaim/lifecycle/initialization.go index 962758dbc0..de22b0188a 100644 --- a/pkg/controllers/nodeclaim/lifecycle/initialization.go +++ b/pkg/controllers/nodeclaim/lifecycle/initialization.go @@ -30,8 +30,8 @@ import ( v1 "sigs.k8s.io/karpenter/pkg/apis/v1" "sigs.k8s.io/karpenter/pkg/scheduling" - nodeutil "sigs.k8s.io/karpenter/pkg/utils/node" - nodeclaimutil "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" "sigs.k8s.io/karpenter/pkg/utils/resources" ) @@ -52,13 +52,13 @@ func (i *Initialization) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) return reconcile.Result{}, nil } ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("provider-id", nodeClaim.Status.ProviderID)) - node, err := nodeclaimutil.NodeForNodeClaim(ctx, i.kubeClient, nodeClaim) + node, err := nodeclaimutils.NodeForNodeClaim(ctx, i.kubeClient, nodeClaim) if err != nil { nodeClaim.StatusConditions().SetUnknownWithReason(v1.ConditionTypeInitialized, "NodeNotFound", "Node not registered with cluster") return reconcile.Result{}, nil //nolint:nilerr } ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("Node", klog.KRef("", node.Name))) - if nodeutil.GetCondition(node, corev1.NodeReady).Status != corev1.ConditionTrue { + if nodeutils.GetCondition(node, corev1.NodeReady).Status != corev1.ConditionTrue { nodeClaim.StatusConditions().SetUnknownWithReason(v1.ConditionTypeInitialized, "NodeNotReady", "Node status is NotReady") return reconcile.Result{}, nil } diff --git a/pkg/controllers/nodeclaim/lifecycle/initialization_test.go b/pkg/controllers/nodeclaim/lifecycle/initialization_test.go index 1eb1719257..ade6be99f5 100644 --- a/pkg/controllers/nodeclaim/lifecycle/initialization_test.go +++ b/pkg/controllers/nodeclaim/lifecycle/initialization_test.go @@ -36,58 +36,82 @@ var _ = Describe("Initialization", func() { BeforeEach(func() { nodePool = test.NodePool() }) - It("should consider the nodeClaim initialized when all initialization conditions are met", func() { - nodeClaim := test.NodeClaim(v1.NodeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, + DescribeTable( + "Initialization", + func(isNodeClaimManaged bool) { + nodeClaimOpts := []v1.NodeClaim{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }, }, - }, - Spec: v1.NodeClaimSpec{ - Resources: v1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2"), - corev1.ResourceMemory: resource.MustParse("50Mi"), - corev1.ResourcePods: resource.MustParse("5"), + Spec: v1.NodeClaimSpec{ + Resources: v1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2"), + corev1.ResourceMemory: resource.MustParse("50Mi"), + corev1.ResourcePods: resource.MustParse("5"), + }, }, }, - }, - }) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - - node := test.Node(test.NodeOptions{ - ProviderID: nodeClaim.Status.ProviderID, - Taints: []corev1.Taint{v1.UnregisteredNoExecuteTaint}, - }) - ExpectApplied(ctx, env.Client, node) - - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint - - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeRegistered).Status).To(Equal(metav1.ConditionTrue)) - Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeInitialized).Status).To(Equal(metav1.ConditionUnknown)) - - node = ExpectExists(ctx, env.Client, node) - node.Status.Capacity = corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("10"), - corev1.ResourceMemory: resource.MustParse("100Mi"), - corev1.ResourcePods: resource.MustParse("110"), - } - node.Status.Allocatable = corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("8"), - corev1.ResourceMemory: resource.MustParse("80Mi"), - corev1.ResourcePods: resource.MustParse("110"), - } - ExpectApplied(ctx, env.Client, node) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeRegistered).Status).To(Equal(metav1.ConditionTrue)) - Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeInitialized).Status).To(Equal(metav1.ConditionTrue)) - }) + }} + if !isNodeClaimManaged { + nodeClaimOpts = append(nodeClaimOpts, v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + } + nodeClaim := test.NodeClaim(nodeClaimOpts...) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + + node := test.Node(test.NodeOptions{ + ProviderID: nodeClaim.Status.ProviderID, + Taints: []corev1.Taint{v1.UnregisteredNoExecuteTaint}, + }) + ExpectApplied(ctx, env.Client, node) + + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint + + // If we're testing that Karpenter correctly ignores unmanaged NodeClaims, we must set the registered + // status condition manually since the registration sub-reconciler should also ignore it. + if !isNodeClaimManaged { + nodeClaim.StatusConditions().SetTrue(v1.ConditionTypeRegistered) + ExpectApplied(ctx, env.Client, nodeClaim) + } + + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).IsUnknown()).To(BeTrue()) + + node = ExpectExists(ctx, env.Client, node) + node.Status.Capacity = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + corev1.ResourceMemory: resource.MustParse("100Mi"), + corev1.ResourcePods: resource.MustParse("110"), + } + node.Status.Allocatable = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("80Mi"), + corev1.ResourcePods: resource.MustParse("110"), + } + ExpectApplied(ctx, env.Client, node) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).IsTrue()).To(Equal(isNodeClaimManaged)) + }, + Entry("should consider the NodeClaim initialized when all initialization conditions are met", true), + Entry("should ignore NodeClaims which aren't managed by this Karpenter instance", false), + ) It("shouldn't consider the nodeClaim initialized when it has not registered", func() { nodeClaim := test.NodeClaim(v1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/controllers/nodeclaim/lifecycle/launch_test.go b/pkg/controllers/nodeclaim/lifecycle/launch_test.go index 2e69ceca87..d25e04c6fe 100644 --- a/pkg/controllers/nodeclaim/lifecycle/launch_test.go +++ b/pkg/controllers/nodeclaim/lifecycle/launch_test.go @@ -21,6 +21,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/samber/lo" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -34,24 +35,43 @@ var _ = Describe("Launch", func() { BeforeEach(func() { nodePool = test.NodePool() }) - It("should launch an instance when a new NodeClaim is created", func() { - nodeClaim := test.NodeClaim(v1.NodeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, + DescribeTable( + "Launch", + func(isNodeClaimManaged bool) { + nodeClaimOpts := []v1.NodeClaim{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }, }, - }, - }) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + }} + if !isNodeClaimManaged { + nodeClaimOpts = append(nodeClaimOpts, v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + } + nodeClaim := test.NodeClaim(nodeClaimOpts...) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(cloudProvider.CreateCalls).To(HaveLen(1)) - Expect(cloudProvider.CreatedNodeClaims).To(HaveLen(1)) - _, err := cloudProvider.Get(ctx, nodeClaim.Status.ProviderID) - Expect(err).ToNot(HaveOccurred()) - }) + Expect(cloudProvider.CreateCalls).To(HaveLen(lo.Ternary(isNodeClaimManaged, 1, 0))) + Expect(cloudProvider.CreatedNodeClaims).To(HaveLen(lo.Ternary(isNodeClaimManaged, 1, 0))) + if isNodeClaimManaged { + _, err := cloudProvider.Get(ctx, nodeClaim.Status.ProviderID) + Expect(err).ToNot(HaveOccurred()) + } + }, + Entry("should launch an instance when a new NodeClaim is created", true), + Entry("should ignore NodeClaims which aren't managed by this Karpenter instance", false), + ) It("should add the Launched status condition after creating the NodeClaim", func() { nodeClaim := test.NodeClaim(v1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/controllers/nodeclaim/lifecycle/liveness_test.go b/pkg/controllers/nodeclaim/lifecycle/liveness_test.go index 0202f9eadc..8fe3421782 100644 --- a/pkg/controllers/nodeclaim/lifecycle/liveness_test.go +++ b/pkg/controllers/nodeclaim/lifecycle/liveness_test.go @@ -36,6 +36,55 @@ var _ = Describe("Liveness", func() { BeforeEach(func() { nodePool = test.NodePool() }) + DescribeTable( + "Liveness", + func(isManagedNodeClaim bool) { + nodeClaimOpts := []v1.NodeClaim{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }, + }, + Spec: v1.NodeClaimSpec{ + Resources: v1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2"), + corev1.ResourceMemory: resource.MustParse("50Mi"), + corev1.ResourcePods: resource.MustParse("5"), + fake.ResourceGPUVendorA: resource.MustParse("1"), + }, + }, + }, + }} + if !isManagedNodeClaim { + nodeClaimOpts = append(nodeClaimOpts, v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + } + nodeClaim := test.NodeClaim(nodeClaimOpts...) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + + // If the node hasn't registered in the registration timeframe, then we deprovision the NodeClaim + fakeClock.Step(time.Minute * 20) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + ExpectFinalizersRemoved(ctx, env.Client, nodeClaim) + if isManagedNodeClaim { + ExpectNotFound(ctx, env.Client, nodeClaim) + } else { + ExpectExists(ctx, env.Client, nodeClaim) + } + }, + Entry("should delete the nodeClaim when the Node hasn't registered past the registration ttl", true), + Entry("should ignore NodeClaims not managed by this Karpenter instance", false), + ) It("shouldn't delete the nodeClaim when the node has registered past the registration ttl", func() { nodeClaim := test.NodeClaim(v1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{ @@ -66,34 +115,6 @@ var _ = Describe("Liveness", func() { ExpectExists(ctx, env.Client, nodeClaim) ExpectExists(ctx, env.Client, node) }) - It("should delete the nodeClaim when the Node hasn't registered past the registration ttl", func() { - nodeClaim := test.NodeClaim(v1.NodeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, - }, - }, - Spec: v1.NodeClaimSpec{ - Resources: v1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2"), - corev1.ResourceMemory: resource.MustParse("50Mi"), - corev1.ResourcePods: resource.MustParse("5"), - fake.ResourceGPUVendorA: resource.MustParse("1"), - }, - }, - }, - }) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - - // If the node hasn't registered in the registration timeframe, then we deprovision the NodeClaim - fakeClock.Step(time.Minute * 20) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - ExpectFinalizersRemoved(ctx, env.Client, nodeClaim) - ExpectNotFound(ctx, env.Client, nodeClaim) - }) It("should delete the NodeClaim when the NodeClaim hasn't launched past the registration ttl", func() { nodeClaim := test.NodeClaim(v1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/controllers/nodeclaim/lifecycle/registration.go b/pkg/controllers/nodeclaim/lifecycle/registration.go index 3f29c64a58..c23cfe7799 100644 --- a/pkg/controllers/nodeclaim/lifecycle/registration.go +++ b/pkg/controllers/nodeclaim/lifecycle/registration.go @@ -33,7 +33,7 @@ import ( v1 "sigs.k8s.io/karpenter/pkg/apis/v1" "sigs.k8s.io/karpenter/pkg/metrics" "sigs.k8s.io/karpenter/pkg/scheduling" - nodeclaimutil "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" ) type Registration struct { @@ -45,13 +45,13 @@ func (r *Registration) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) ( return reconcile.Result{}, nil } ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("provider-id", nodeClaim.Status.ProviderID)) - node, err := nodeclaimutil.NodeForNodeClaim(ctx, r.kubeClient, nodeClaim) + node, err := nodeclaimutils.NodeForNodeClaim(ctx, r.kubeClient, nodeClaim) if err != nil { - if nodeclaimutil.IsNodeNotFoundError(err) { + if nodeclaimutils.IsNodeNotFoundError(err) { nodeClaim.StatusConditions().SetUnknownWithReason(v1.ConditionTypeRegistered, "NodeNotFound", "Node not registered with cluster") return reconcile.Result{}, nil } - if nodeclaimutil.IsDuplicateNodeError(err) { + if nodeclaimutils.IsDuplicateNodeError(err) { nodeClaim.StatusConditions().SetFalse(v1.ConditionTypeRegistered, "MultipleNodesFound", "Invariant violated, matched multiple nodes") return reconcile.Result{}, nil } @@ -87,7 +87,7 @@ func (r *Registration) syncNode(ctx context.Context, nodeClaim *v1.NodeClaim, no stored := node.DeepCopy() controllerutil.AddFinalizer(node, v1.TerminationFinalizer) - node = nodeclaimutil.UpdateNodeOwnerReferences(nodeClaim, node) + node = nodeclaimutils.UpdateNodeOwnerReferences(nodeClaim, node) node.Labels = lo.Assign(node.Labels, nodeClaim.Labels) node.Annotations = lo.Assign(node.Annotations, nodeClaim.Annotations) // Sync all taints inside NodeClaim into the Node taints diff --git a/pkg/controllers/nodeclaim/lifecycle/registration_test.go b/pkg/controllers/nodeclaim/lifecycle/registration_test.go index 0b23f47d61..9f9b93459e 100644 --- a/pkg/controllers/nodeclaim/lifecycle/registration_test.go +++ b/pkg/controllers/nodeclaim/lifecycle/registration_test.go @@ -32,26 +32,48 @@ var _ = Describe("Registration", func() { BeforeEach(func() { nodePool = test.NodePool() }) - It("should match the nodeClaim to the Node when the Node comes online", func() { - nodeClaim := test.NodeClaim(v1.NodeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, + DescribeTable( + "Registration", + func(isManagedNodeClaim bool) { + nodeClaimOpts := []v1.NodeClaim{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }, }, - }, - }) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + }} + if !isManagedNodeClaim { + nodeClaimOpts = append(nodeClaimOpts, v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + } + nodeClaim := test.NodeClaim(nodeClaimOpts...) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []corev1.Taint{v1.UnregisteredNoExecuteTaint}}) - ExpectApplied(ctx, env.Client, node) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []corev1.Taint{v1.UnregisteredNoExecuteTaint}}) + ExpectApplied(ctx, env.Client, node) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeRegistered).Status).To(Equal(metav1.ConditionTrue)) - Expect(nodeClaim.Status.NodeName).To(Equal(node.Name)) - }) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + if isManagedNodeClaim { + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue()) + Expect(nodeClaim.Status.NodeName).To(Equal(node.Name)) + } else { + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsUnknown()).To(BeTrue()) + Expect(nodeClaim.Status.NodeName).To(Equal("")) + } + }, + Entry("should match the nodeClaim to the Node when the Node comes online", true), + Entry("should ignore NodeClaims not managed by this Karpenter instance", false), + ) It("should add the owner reference to the Node when the Node comes online", func() { nodeClaim := test.NodeClaim(v1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/controllers/nodeclaim/lifecycle/suite_test.go b/pkg/controllers/nodeclaim/lifecycle/suite_test.go index 15b8201fa6..ffba573a5f 100644 --- a/pkg/controllers/nodeclaim/lifecycle/suite_test.go +++ b/pkg/controllers/nodeclaim/lifecycle/suite_test.go @@ -24,12 +24,9 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/samber/lo" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" clock "k8s.io/utils/clock/testing" - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -57,11 +54,7 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(func(c cache.Cache) error { - return c.IndexField(ctx, &corev1.Node{}, "spec.providerID", func(obj client.Object) []string { - return []string{obj.(*corev1.Node).Spec.ProviderID} - }) - })) + env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeProviderIDFieldIndexer(ctx))) ctx = options.ToContext(ctx, test.Options()) cloudProvider = fake.NewCloudProvider() @@ -84,21 +77,47 @@ var _ = Describe("Finalizer", func() { BeforeEach(func() { nodePool = test.NodePool() }) - It("should add the finalizer if it doesn't exist", func() { - nodeClaim := test.NodeClaim(v1.NodeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, + Context("TerminationFinalizer", func() { + It("should add the finalizer if it doesn't exist", func() { + nodeClaim := test.NodeClaim(v1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }, }, - }, + }) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + _, ok := lo.Find(nodeClaim.Finalizers, func(f string) bool { + return f == v1.TerminationFinalizer + }) + Expect(ok).To(BeTrue()) }) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - _, ok := lo.Find(nodeClaim.Finalizers, func(f string) bool { - return f == v1.TerminationFinalizer + It("shouldn't add the finalizer to NodeClaims not managed by this instance of Karpenter", func() { + nodeClaim := test.NodeClaim(v1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }, + }, + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + _, ok := lo.Find(nodeClaim.Finalizers, func(f string) bool { + return f == v1.TerminationFinalizer + }) + Expect(ok).To(BeFalse()) }) - Expect(ok).To(BeTrue()) }) }) diff --git a/pkg/controllers/nodeclaim/lifecycle/termination_test.go b/pkg/controllers/nodeclaim/lifecycle/termination_test.go index 3780108c6f..6d2d4d9238 100644 --- a/pkg/controllers/nodeclaim/lifecycle/termination_test.go +++ b/pkg/controllers/nodeclaim/lifecycle/termination_test.go @@ -67,41 +67,65 @@ var _ = Describe("Termination", func() { }) lifecycle.InstanceTerminationDurationSeconds.Reset() }) - It("should delete the node and the CloudProvider NodeClaim when NodeClaim deletion is triggered", func() { - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - _, err := cloudProvider.Get(ctx, nodeClaim.Status.ProviderID) - Expect(err).ToNot(HaveOccurred()) - - node := test.NodeClaimLinkedNode(nodeClaim) - ExpectApplied(ctx, env.Client, node) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue()) - - // Expect the node and the nodeClaim to both be gone - Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed()) - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // triggers the node deletion - ExpectFinalizersRemoved(ctx, env.Client, node) - ExpectNotFound(ctx, env.Client, node) - - result := ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // now all the nodes are gone so nodeClaim deletion continues - Expect(result.RequeueAfter).To(BeEquivalentTo(5 * time.Second)) - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInstanceTerminating).IsTrue()).To(BeTrue()) - - ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // this will call cloudProvider Get to check if the instance is still around - - ExpectMetricHistogramSampleCountValue("karpenter_nodeclaims_instance_termination_duration_seconds", 1, map[string]string{"nodepool": nodePool.Name}) - ExpectMetricHistogramSampleCountValue("karpenter_nodeclaims_termination_duration_seconds", 1, map[string]string{"nodepool": nodePool.Name}) - ExpectNotFound(ctx, env.Client, nodeClaim, node) - - // Expect the nodeClaim to be gone from the cloudprovider - _, err = cloudProvider.Get(ctx, nodeClaim.Status.ProviderID) - Expect(cloudprovider.IsNodeClaimNotFoundError(err)).To(BeTrue()) - }) + DescribeTable( + "Termination", + func(isNodeClaimManaged bool) { + if !isNodeClaimManaged { + nodeClaim.Spec.NodeClassRef = &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + } + } + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + + if isNodeClaimManaged { + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + _, err := cloudProvider.Get(ctx, nodeClaim.Status.ProviderID) + Expect(err).ToNot(HaveOccurred()) + } + + node := test.NodeClaimLinkedNode(nodeClaim) + ExpectApplied(ctx, env.Client, node) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(Equal(isNodeClaimManaged)) + + if !isNodeClaimManaged { + nodeClaim.StatusConditions().SetTrue(v1.ConditionTypeRegistered) + ExpectApplied(ctx, env.Client, nodeClaim) + } + + // Expect the node and the nodeClaim to both be gone + Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed()) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // triggers the node deletion + ExpectFinalizersRemoved(ctx, env.Client, node) + if isNodeClaimManaged { + ExpectNotFound(ctx, env.Client, node) + result := ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // now all the nodes are gone so nodeClaim deletion continues + Expect(result.RequeueAfter).To(BeEquivalentTo(5 * time.Second)) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInstanceTerminating).IsTrue()).To(BeTrue()) + + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // this will call cloudProvider Get to check if the instance is still around + + ExpectMetricHistogramSampleCountValue("karpenter_nodeclaims_instance_termination_duration_seconds", 1, map[string]string{"nodepool": nodePool.Name}) + ExpectMetricHistogramSampleCountValue("karpenter_nodeclaims_termination_duration_seconds", 1, map[string]string{"nodepool": nodePool.Name}) + ExpectNotFound(ctx, env.Client, nodeClaim, node) + + // Expect the nodeClaim to be gone from the cloudprovider + _, err := cloudProvider.Get(ctx, nodeClaim.Status.ProviderID) + Expect(cloudprovider.IsNodeClaimNotFoundError(err)).To(BeTrue()) + } else { + + ExpectExists(ctx, env.Client, node) + ExpectExists(ctx, env.Client, nodeClaim) + } + }, + Entry("should delete the node and the CloudProvider NodeClaim when NodeClaim deletion is triggered", true), + Entry("should ignore NodeClaims which aren't managed by this Karpenter instance", false), + ) It("should delete the NodeClaim when the spec resource.Quantity values will change during deserialization", func() { nodeClaim.SetGroupVersionKind(object.GVK(nodeClaim)) // This is needed so that the GVK is set on the unstructured object u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(nodeClaim) diff --git a/pkg/controllers/nodeclaim/podevents/controller.go b/pkg/controllers/nodeclaim/podevents/controller.go index 8313e08c40..803195505d 100644 --- a/pkg/controllers/nodeclaim/podevents/controller.go +++ b/pkg/controllers/nodeclaim/podevents/controller.go @@ -32,7 +32,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/karpenter/pkg/cloudprovider" nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" podutils "sigs.k8s.io/karpenter/pkg/utils/pod" ) @@ -43,15 +45,17 @@ const dedupeTimeout = 10 * time.Second // Podevents is a nodeclaim controller that deletes adds the lastPodEvent status onto the nodeclaim type Controller struct { - clock clock.Clock - kubeClient client.Client + clock clock.Clock + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider } // NewController constructs a nodeclaim disruption controller -func NewController(clk clock.Clock, kubeClient client.Client) *Controller { +func NewController(clk clock.Clock, kubeClient client.Client, cloudProvider cloudprovider.CloudProvider) *Controller { return &Controller{ - kubeClient: kubeClient, - clock: clk, + clock: clk, + kubeClient: kubeClient, + cloudProvider: cloudProvider, } } @@ -67,13 +71,15 @@ func (c *Controller) Reconcile(ctx context.Context, pod *corev1.Pod) (reconcile. if err := c.kubeClient.Get(ctx, types.NamespacedName{Name: pod.Spec.NodeName}, node); err != nil { return reconcile.Result{}, client.IgnoreNotFound(fmt.Errorf("getting node, %w", err)) } - // If there's no associated node claim, it's not a karpenter owned node. nc, err := nodeutils.NodeClaimForNode(ctx, c.kubeClient, node) if err != nil { // if the nodeclaim doesn't exist, or has duplicates, ignore. return reconcile.Result{}, nodeutils.IgnoreDuplicateNodeClaimError(nodeutils.IgnoreNodeClaimNotFoundError(fmt.Errorf("getting nodeclaims for node, %w", err))) } + if !nodeclaimutils.IsManaged(nc, c.cloudProvider) { + return reconcile.Result{}, nil + } stored := nc.DeepCopy() // If we've set the lastPodEvent before and it hasn't been before the timeout, don't do anything diff --git a/pkg/controllers/nodeclaim/podevents/suite_test.go b/pkg/controllers/nodeclaim/podevents/suite_test.go index e637644828..7ac206c4dd 100644 --- a/pkg/controllers/nodeclaim/podevents/suite_test.go +++ b/pkg/controllers/nodeclaim/podevents/suite_test.go @@ -26,8 +26,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clock "k8s.io/utils/clock/testing" - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -54,14 +52,14 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(func(c cache.Cache) error { - return c.IndexField(ctx, &v1.NodeClaim{}, "status.providerID", func(obj client.Object) []string { - return []string{obj.(*v1.NodeClaim).Status.ProviderID} - }) - })) + env = test.NewEnvironment( + test.WithCRDs(apis.CRDs...), + test.WithCRDs(v1alpha1.CRDs...), + test.WithFieldIndexers(test.NodeClaimProviderIDFieldIndexer(ctx), test.NodeProviderIDFieldIndexer(ctx)), + ) ctx = options.ToContext(ctx, test.Options()) cp = fake.NewCloudProvider() - podEventsController = podevents.NewController(fakeClock, env.Client) + podEventsController = podevents.NewController(fakeClock, env.Client, cp) }) var _ = AfterSuite(func() { diff --git a/pkg/controllers/nodepool/counter/controller.go b/pkg/controllers/nodepool/counter/controller.go index c67df79171..e369030c2e 100644 --- a/pkg/controllers/nodepool/counter/controller.go +++ b/pkg/controllers/nodepool/counter/controller.go @@ -24,24 +24,26 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/resource" - "k8s.io/apimachinery/pkg/types" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/controllers/state" "sigs.k8s.io/karpenter/pkg/operator/injection" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" "sigs.k8s.io/karpenter/pkg/utils/resources" ) // Controller for the resource type Controller struct { - kubeClient client.Client - cluster *state.Cluster + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider + cluster *state.Cluster } var ResourceNode = corev1.ResourceName("nodes") @@ -55,16 +57,20 @@ var BaseResources = corev1.ResourceList{ } // NewController is a constructor -func NewController(kubeClient client.Client, cluster *state.Cluster) *Controller { +func NewController(kubeClient client.Client, cloudProvider cloudprovider.CloudProvider, cluster *state.Cluster) *Controller { return &Controller{ - kubeClient: kubeClient, - cluster: cluster, + kubeClient: kubeClient, + cloudProvider: cloudProvider, + cluster: cluster, } } // Reconcile a control loop for the resource func (c *Controller) Reconcile(ctx context.Context, nodePool *v1.NodePool) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "nodepool.counter") + if !nodepoolutils.IsManaged(nodePool, c.cloudProvider) { + return reconcile.Result{}, nil + } // We need to ensure that our internal cluster state mechanism is synced before we proceed // Otherwise, we have the potential to patch over the status with a lower value for the nodepool resource @@ -108,25 +114,9 @@ func (c *Controller) resourceCountsFor(ownerLabel string, ownerName string) core func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("nodepool.counter"). - For(&v1.NodePool{}). - Watches( - &v1.NodeClaim{}, - handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []reconcile.Request { - if name, ok := o.GetLabels()[v1.NodePoolLabelKey]; ok { - return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: name}}} - } - return nil - }), - ). - Watches( - &corev1.Node{}, - handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []reconcile.Request { - if name, ok := o.GetLabels()[v1.NodePoolLabelKey]; ok { - return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: name}}} - } - return nil - }), - ). + For(&v1.NodePool{}, builder.WithPredicates(nodepoolutils.IsManagedPredicateFuncs(c.cloudProvider))). + Watches(&v1.NodeClaim{}, nodepoolutils.NodeClaimEventHandler()). + Watches(&corev1.Node{}, nodepoolutils.NodeEventHandler()). WithOptions(controller.Options{MaxConcurrentReconciles: 10}). Complete(reconcile.AsReconciler(m.GetClient(), c)) diff --git a/pkg/controllers/nodepool/counter/suite_test.go b/pkg/controllers/nodepool/counter/suite_test.go index d2e0490e1d..af21dde4af 100644 --- a/pkg/controllers/nodepool/counter/suite_test.go +++ b/pkg/controllers/nodepool/counter/suite_test.go @@ -63,11 +63,11 @@ var _ = BeforeSuite(func() { cloudProvider = fake.NewCloudProvider() env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...)) fakeClock = clock.NewFakeClock(time.Now()) - cluster = state.NewCluster(fakeClock, env.Client) - nodeClaimController = informer.NewNodeClaimController(env.Client, cluster) + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) + nodeClaimController = informer.NewNodeClaimController(env.Client, cloudProvider, cluster) nodeController = informer.NewNodeController(env.Client, cluster) - nodePoolInformerController = informer.NewNodePoolController(env.Client, cluster) - nodePoolController = counter.NewController(env.Client, cluster) + nodePoolInformerController = informer.NewNodePoolController(env.Client, cloudProvider, cluster) + nodePoolController = counter.NewController(env.Client, cloudProvider, cluster) }) var _ = AfterSuite(func() { @@ -117,6 +117,36 @@ var _ = Describe("Counter", func() { ExpectObjectReconciled(ctx, env.Client, nodePoolController, nodePool) nodePool = ExpectExists(ctx, env.Client, nodePool) }) + It("should ignore NodePools which aren't managed by this instance of Karpenter", func() { + nodePool = test.NodePool(v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }}}}) + nodeClaim, node = test.NodeClaimAndNode(v1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }}, + Status: v1.NodeClaimStatus{ + ProviderID: test.RandomProviderID(), + Capacity: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourcePods: resource.MustParse("256"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + }, + Spec: v1.NodeClaimSpec{ + NodeClassRef: nodePool.Spec.Template.Spec.NodeClassRef, + }, + }) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodePoolInformerController, nodePool) + ExpectObjectReconciled(ctx, env.Client, nodePoolController, nodePool) + nodePool = ExpectExists(ctx, env.Client, nodePool) + Expect(nodePool.Status.Resources).To(BeNil()) + }) It("should set well-known resource to zero when no nodes exist in the cluster", func() { ExpectObjectReconciled(ctx, env.Client, nodePoolController, nodePool) nodePool = ExpectExists(ctx, env.Client, nodePool) diff --git a/pkg/controllers/nodepool/hash/controller.go b/pkg/controllers/nodepool/hash/controller.go index dc13613fe9..8dc294bfa5 100644 --- a/pkg/controllers/nodepool/hash/controller.go +++ b/pkg/controllers/nodepool/hash/controller.go @@ -23,30 +23,39 @@ import ( "go.uber.org/multierr" "k8s.io/apimachinery/pkg/api/equality" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/operator/injection" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" ) // Controller is hash controller that constructs a hash based on the fields that are considered for static drift. // The hash is placed in the metadata for increased observability and should be found on each object. type Controller struct { - kubeClient client.Client + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider } -func NewController(kubeClient client.Client) *Controller { +func NewController(kubeClient client.Client, cloudProvider cloudprovider.CloudProvider) *Controller { return &Controller{ - kubeClient: kubeClient, + kubeClient: kubeClient, + cloudProvider: cloudProvider, } } // Reconcile the resource func (c *Controller) Reconcile(ctx context.Context, np *v1.NodePool) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "nodepool.hash") + if !nodepoolutils.IsManaged(np, c.cloudProvider) { + return reconcile.Result{}, nil + } stored := np.DeepCopy() @@ -71,7 +80,7 @@ func (c *Controller) Reconcile(ctx context.Context, np *v1.NodePool) (reconcile. func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("nodepool.hash"). - For(&v1.NodePool{}). + For(&v1.NodePool{}, builder.WithPredicates(nodepoolutils.IsManagedPredicateFuncs(c.cloudProvider))). WithOptions(controller.Options{MaxConcurrentReconciles: 10}). Complete(reconcile.AsReconciler(m.GetClient(), c)) } @@ -81,14 +90,13 @@ func (c *Controller) Register(_ context.Context, m manager.Manager) error { // NodePool. Since, we cannot rely on the `nodepool-hash` on the NodeClaims, due to the breaking change, we will need to re-calculate the hash and update the annotation. // For more information on the Drift Hash Versioning: https://github.com/kubernetes-sigs/karpenter/blob/main/designs/drift-hash-versioning.md func (c *Controller) updateNodeClaimHash(ctx context.Context, np *v1.NodePool) error { - ncList := &v1.NodeClaimList{} - if err := c.kubeClient.List(ctx, ncList, client.MatchingLabels(map[string]string{v1.NodePoolLabelKey: np.Name})); err != nil { + nodeClaims, err := nodeclaimutils.ListManaged(ctx, c.kubeClient, c.cloudProvider, nodeclaimutils.ForNodePool(np.Name)) + if err != nil { return err } - errs := make([]error, len(ncList.Items)) - for i := range ncList.Items { - nc := ncList.Items[i] + errs := make([]error, len(nodeClaims)) + for i, nc := range nodeClaims { stored := nc.DeepCopy() if nc.Annotations[v1.NodePoolHashVersionAnnotationKey] != v1.NodePoolHashVersion { @@ -105,7 +113,7 @@ func (c *Controller) updateNodeClaimHash(ctx context.Context, np *v1.NodePool) e } if !equality.Semantic.DeepEqual(stored, nc) { - if err := c.kubeClient.Patch(ctx, &nc, client.MergeFrom(stored)); err != nil { + if err := c.kubeClient.Patch(ctx, nc, client.MergeFrom(stored)); err != nil { errs[i] = client.IgnoreNotFound(err) } } diff --git a/pkg/controllers/nodepool/hash/suite_test.go b/pkg/controllers/nodepool/hash/suite_test.go index af026807d6..5e7b06a1d5 100644 --- a/pkg/controllers/nodepool/hash/suite_test.go +++ b/pkg/controllers/nodepool/hash/suite_test.go @@ -30,6 +30,7 @@ import ( "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/controllers/nodepool/hash" "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" @@ -40,6 +41,7 @@ import ( var nodePoolController *hash.Controller var ctx context.Context var env *test.Environment +var cp *fake.CloudProvider func TestAPIs(t *testing.T) { ctx = TestContextWithLogger(t) @@ -49,7 +51,8 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...)) - nodePoolController = hash.NewController(env.Client) + cp = fake.NewCloudProvider() + nodePoolController = hash.NewController(env.Client, cp) }) var _ = AfterSuite(func() { @@ -91,6 +94,18 @@ var _ = Describe("Static Drift Hash", func() { }, }) }) + It("should ignore NodePools which aren't managed by this instance of Karpenter", func() { + nodePool.Spec.Template.Spec.NodeClassRef = &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + } + ExpectApplied(ctx, env.Client, nodePool) + ExpectObjectReconciled(ctx, env.Client, nodePoolController, nodePool) + nodePool = ExpectExists(ctx, env.Client, nodePool) + _, ok := nodePool.Annotations[v1.NodePoolHashAnnotationKey] + Expect(ok).To(BeFalse()) + }) // TODO we should split this out into a DescribeTable It("should update the drift hash when NodePool static field is updated", func() { ExpectApplied(ctx, env.Client, nodePool) diff --git a/pkg/controllers/nodepool/readiness/controller.go b/pkg/controllers/nodepool/readiness/controller.go index 1488599a6e..7ec99f3d07 100644 --- a/pkg/controllers/nodepool/readiness/controller.go +++ b/pkg/controllers/nodepool/readiness/controller.go @@ -18,7 +18,6 @@ package readiness import ( "context" - logger "log" "github.com/awslabs/operatorpkg/object" "github.com/awslabs/operatorpkg/status" @@ -26,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -34,7 +34,7 @@ import ( v1 "sigs.k8s.io/karpenter/pkg/apis/v1" "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/operator/injection" - "sigs.k8s.io/karpenter/pkg/utils/nodepool" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" ) // Controller for the resource @@ -54,22 +54,28 @@ func NewController(kubeClient client.Client, cloudProvider cloudprovider.CloudPr func (c *Controller) Reconcile(ctx context.Context, nodePool *v1.NodePool) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "nodepool.readiness") stored := nodePool.DeepCopy() - supportedNC := c.cloudProvider.GetSupportedNodeClasses() - if len(supportedNC) == 0 { - logger.Fatal("no supported node classes found for the cloud provider") + nodeClass, ok := lo.Find(c.cloudProvider.GetSupportedNodeClasses(), func(nc status.Object) bool { + return object.GVK(nc).GroupKind() == nodePool.Spec.Template.Spec.NodeClassRef.GroupKind() + }) + if !ok { + // Ignore NodePools which aren't using a supported NodeClass. + return reconcile.Result{}, nil } - nodeClass, err := c.getNodeClass(ctx, nodePool, supportedNC) - if err != nil { + + err := c.kubeClient.Get(ctx, client.ObjectKey{Name: nodePool.Spec.Template.Spec.NodeClassRef.Name}, nodeClass) + if client.IgnoreNotFound(err) != nil { return reconcile.Result{}, err } - if nodeClass == nil { + switch { + case errors.IsNotFound(err): nodePool.StatusConditions().SetFalse(v1.ConditionTypeNodeClassReady, "NodeClassNotFound", "NodeClass not found on cluster") - } else if !nodeClass.GetDeletionTimestamp().IsZero() { + case !nodeClass.GetDeletionTimestamp().IsZero(): nodePool.StatusConditions().SetFalse(v1.ConditionTypeNodeClassReady, "NodeClassTerminating", "NodeClass is Terminating") - } else { + default: c.setReadyCondition(nodePool, nodeClass) } + if !equality.Semantic.DeepEqual(stored, nodePool) { // We use client.MergeFromWithOptimisticLock because patching a list with a JSON merge patch // can cause races due to the fact that it fully replaces the list on a change @@ -83,21 +89,6 @@ func (c *Controller) Reconcile(ctx context.Context, nodePool *v1.NodePool) (reco } return reconcile.Result{}, nil } -func (c *Controller) getNodeClass(ctx context.Context, nodePool *v1.NodePool, supportedNC []status.Object) (status.Object, error) { - nodeClass, ok := lo.Find(supportedNC, func(nc status.Object) bool { - return object.GVK(nc).Group == nodePool.Spec.Template.Spec.NodeClassRef.Group && object.GVK(nc).Kind == nodePool.Spec.Template.Spec.NodeClassRef.Kind - }) - if !ok { - return nodeClass, nil - } - if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: nodePool.Spec.Template.Spec.NodeClassRef.Name}, nodeClass); err != nil { - if errors.IsNotFound(err) { - return nil, nil - } - return nil, err - } - return nodeClass, nil -} func (c *Controller) setReadyCondition(nodePool *v1.NodePool, nodeClass status.Object) { ready := nodeClass.StatusConditions().Get(status.ConditionReady) @@ -111,16 +102,12 @@ func (c *Controller) setReadyCondition(nodePool *v1.NodePool, nodeClass status.O } func (c *Controller) Register(_ context.Context, m manager.Manager) error { - builder := controllerruntime.NewControllerManagedBy(m) + b := controllerruntime.NewControllerManagedBy(m). + Named("nodepool.readiness"). + For(&v1.NodePool{}, builder.WithPredicates(nodepoolutils.IsManagedPredicateFuncs(c.cloudProvider))). + WithOptions(controller.Options{MaxConcurrentReconciles: 10}) for _, nodeClass := range c.cloudProvider.GetSupportedNodeClasses() { - builder = builder.Watches( - nodeClass, - nodepool.NodeClassEventHandler(c.kubeClient), - ) + b.Watches(nodeClass, nodepoolutils.NodeClassEventHandler(c.kubeClient)) } - return builder. - Named("nodepool.readiness"). - For(&v1.NodePool{}). - WithOptions(controller.Options{MaxConcurrentReconciles: 10}). - Complete(reconcile.AsReconciler(m.GetClient(), c)) + return b.Complete(reconcile.AsReconciler(m.GetClient(), c)) } diff --git a/pkg/controllers/nodepool/readiness/suite_test.go b/pkg/controllers/nodepool/readiness/suite_test.go index 6e3755421d..d3c276705c 100644 --- a/pkg/controllers/nodepool/readiness/suite_test.go +++ b/pkg/controllers/nodepool/readiness/suite_test.go @@ -74,13 +74,16 @@ var _ = Describe("Readiness", func() { nodePool.Spec.Template.Spec.NodeClassRef.Group = object.GVK(nodeClass).Group nodePool.Spec.Template.Spec.NodeClassRef.Kind = object.GVK(nodeClass).Kind }) - It("should have status condition on nodePool as not ready if nodeClass referenced in nodePool is not in supported nodeClasses", func() { - nodePool.Spec.Template.Spec.NodeClassRef.Group = "group" - nodePool.Spec.Template.Spec.NodeClassRef.Kind = "kind" + It("should ignore NodePools which aren't managed by this instance of Karpenter", func() { + nodePool.Spec.Template.Spec.NodeClassRef = &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + } ExpectApplied(ctx, env.Client, nodePool, nodeClass) _ = ExpectObjectReconciled(ctx, env.Client, controller, nodePool) nodePool = ExpectExists(ctx, env.Client, nodePool) - Expect(nodePool.StatusConditions().Get(status.ConditionReady).IsFalse()).To(BeTrue()) + Expect(nodePool.StatusConditions().Get(status.ConditionReady).IsUnknown()).To(BeFalse()) }) It("should have status condition on nodePool as not ready when nodeClass does not exist", func() { ExpectApplied(ctx, env.Client, nodePool) diff --git a/pkg/controllers/nodepool/validation/controller.go b/pkg/controllers/nodepool/validation/controller.go index 986952ca50..6ebc67054b 100644 --- a/pkg/controllers/nodepool/validation/controller.go +++ b/pkg/controllers/nodepool/validation/controller.go @@ -22,29 +22,37 @@ import ( "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/operator/injection" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" ) // Controller for the resource type Controller struct { - kubeClient client.Client + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider } // NewController is a constructor -func NewController(kubeClient client.Client) *Controller { +func NewController(kubeClient client.Client, cloudProvider cloudprovider.CloudProvider) *Controller { return &Controller{ - kubeClient: kubeClient, + kubeClient: kubeClient, + cloudProvider: cloudProvider, } } func (c *Controller) Reconcile(ctx context.Context, nodePool *v1.NodePool) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "nodepool.validation") + if !nodepoolutils.IsManaged(nodePool, c.cloudProvider) { + return reconcile.Result{}, nil + } stored := nodePool.DeepCopy() err := nodePool.RuntimeValidate() if err != nil { @@ -69,7 +77,7 @@ func (c *Controller) Reconcile(ctx context.Context, nodePool *v1.NodePool) (reco func (c *Controller) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("nodepool.validation"). - For(&v1.NodePool{}). + For(&v1.NodePool{}, builder.WithPredicates(nodepoolutils.IsManagedPredicateFuncs(c.cloudProvider))). WithOptions(controller.Options{MaxConcurrentReconciles: 10}). Complete(reconcile.AsReconciler(m.GetClient(), c)) } diff --git a/pkg/controllers/nodepool/validation/suite_test.go b/pkg/controllers/nodepool/validation/suite_test.go index c66aa14199..3b6b5a785d 100644 --- a/pkg/controllers/nodepool/validation/suite_test.go +++ b/pkg/controllers/nodepool/validation/suite_test.go @@ -30,6 +30,7 @@ import ( "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" "sigs.k8s.io/karpenter/pkg/test/v1alpha1" @@ -41,6 +42,7 @@ var ( ctx context.Context env *test.Environment nodePool *v1.NodePool + cp *fake.CloudProvider ) func TestAPIs(t *testing.T) { @@ -51,7 +53,8 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...)) - nodePoolValidationController = NewController(env.Client) + cp = fake.NewCloudProvider() + nodePoolValidationController = NewController(env.Client, cp) }) var _ = AfterEach(func() { ExpectCleanedUp(ctx, env.Client) @@ -65,6 +68,17 @@ var _ = Describe("Counter", func() { nodePool = test.NodePool() nodePool.StatusConditions().SetUnknown(v1.ConditionTypeValidationSucceeded) }) + It("should ignore NodePools which aren't managed by this instance of Karpenter", func() { + nodePool.Spec.Template.Spec.NodeClassRef = &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + } + ExpectApplied(ctx, env.Client, nodePool) + ExpectObjectReconciled(ctx, env.Client, nodePoolValidationController, nodePool) + nodePool = ExpectExists(ctx, env.Client, nodePool) + Expect(nodePool.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsUnknown()).To(BeTrue()) + }) It("should set the NodePoolValidationSucceeded status condition to true if nodePool healthy checks succeed", func() { ExpectApplied(ctx, env.Client, nodePool) ExpectObjectReconciled(ctx, env.Client, nodePoolValidationController, nodePool) diff --git a/pkg/controllers/provisioning/provisioner.go b/pkg/controllers/provisioning/provisioner.go index 4817e5321d..1517180a54 100644 --- a/pkg/controllers/provisioning/provisioner.go +++ b/pkg/controllers/provisioning/provisioner.go @@ -49,7 +49,8 @@ import ( "sigs.k8s.io/karpenter/pkg/metrics" "sigs.k8s.io/karpenter/pkg/operator/injection" "sigs.k8s.io/karpenter/pkg/scheduling" - nodeutil "sigs.k8s.io/karpenter/pkg/utils/node" + nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" "sigs.k8s.io/karpenter/pkg/utils/pretty" ) @@ -158,7 +159,7 @@ func (p *Provisioner) CreateNodeClaims(ctx context.Context, nodeClaims []*schedu func (p *Provisioner) GetPendingPods(ctx context.Context) ([]*corev1.Pod, error) { // filter for provisionable pods first, so we don't check for validity/PVCs on pods we won't provision anyway // (e.g. those owned by daemonsets) - pods, err := nodeutil.GetProvisionablePods(ctx, p.kubeClient) + pods, err := nodeutils.GetProvisionablePods(ctx, p.kubeClient) if err != nil { return nil, fmt.Errorf("listing pods, %w", err) } @@ -212,53 +213,48 @@ var ErrNodePoolsNotFound = errors.New("no nodepools found") //nolint:gocyclo func (p *Provisioner) NewScheduler(ctx context.Context, pods []*corev1.Pod, stateNodes []*state.StateNode) (*scheduler.Scheduler, error) { - nodePoolList := &v1.NodePoolList{} - err := p.kubeClient.List(ctx, nodePoolList) + nodePools, err := nodepoolutils.ListManaged(ctx, p.kubeClient, p.cloudProvider) if err != nil { - return nil, fmt.Errorf("listing node pools, %w", err) + return nil, fmt.Errorf("listing nodepools, %w", err) } - nodePoolList.Items = lo.Filter(nodePoolList.Items, func(n v1.NodePool, _ int) bool { - if !n.StatusConditions().IsTrue(status.ConditionReady) { - log.FromContext(ctx).WithValues("NodePool", klog.KRef("", n.Name)).Error(err, "nodePool not ready") + nodePools = lo.Filter(nodePools, func(np *v1.NodePool, _ int) bool { + if !np.StatusConditions().IsTrue(status.ConditionReady) { + log.FromContext(ctx).WithValues("NodePool", klog.KRef("", np.Name)).Error(err, "ignoring nodepool, not ready") return false } - return n.DeletionTimestamp.IsZero() + return np.DeletionTimestamp.IsZero() }) - if len(nodePoolList.Items) == 0 { + if len(nodePools) == 0 { return nil, ErrNodePoolsNotFound } // nodeTemplates generated from NodePools are ordered by weight // since they are stored within a slice and scheduling // will always attempt to schedule on the first nodeTemplate - nodePoolList.OrderByWeight() + nodepoolutils.OrderByWeight(nodePools) instanceTypes := map[string][]*cloudprovider.InstanceType{} domains := map[string]sets.Set[string]{} - var notReadyNodePools []string - for _, nodePool := range nodePoolList.Items { - // Get instance type options - instanceTypeOptions, err := p.cloudProvider.GetInstanceTypes(ctx, lo.ToPtr(nodePool)) + for _, np := range nodePools { + its, err := p.cloudProvider.GetInstanceTypes(ctx, np) if err != nil { - // we just log an error and skip the provisioner to prevent a single mis-configured provisioner from stopping - // all scheduling - log.FromContext(ctx).WithValues("NodePool", klog.KRef("", nodePool.Name)).Error(err, "skipping, unable to resolve instance types") + log.FromContext(ctx).WithValues("NodePool", klog.KRef("", np.Name)).Error(err, "skipping, unable to resolve instance types") continue } - if len(instanceTypeOptions) == 0 { - log.FromContext(ctx).WithValues("NodePool", klog.KRef("", nodePool.Name)).Info("skipping, no resolved instance types found") + if len(its) == 0 { + log.FromContext(ctx).WithValues("NodePool", klog.KRef("", np.Name)).Info("skipping, no resolved instance types found") continue } - instanceTypes[nodePool.Name] = instanceTypeOptions + instanceTypes[np.Name] = its // Construct Topology Domains - for _, instanceType := range instanceTypeOptions { + for _, it := range its { // We need to intersect the instance type requirements with the current nodePool requirements. This // ensures that something like zones from an instance type don't expand the universe of valid domains. - requirements := scheduling.NewNodeSelectorRequirementsWithMinValues(nodePool.Spec.Template.Spec.Requirements...) - requirements.Add(scheduling.NewLabelRequirements(nodePool.Spec.Template.Labels).Values()...) - requirements.Add(instanceType.Requirements.Values()...) + requirements := scheduling.NewNodeSelectorRequirementsWithMinValues(np.Spec.Template.Spec.Requirements...) + requirements.Add(scheduling.NewLabelRequirements(np.Spec.Template.Labels).Values()...) + requirements.Add(it.Requirements.Values()...) for key, requirement := range requirements { // This code used to execute a Union between domains[key] and requirement.Values(). @@ -273,8 +269,8 @@ func (p *Provisioner) NewScheduler(ctx context.Context, pods []*corev1.Pod, stat } } - requirements := scheduling.NewNodeSelectorRequirementsWithMinValues(nodePool.Spec.Template.Spec.Requirements...) - requirements.Add(scheduling.NewLabelRequirements(nodePool.Spec.Template.Labels).Values()...) + requirements := scheduling.NewNodeSelectorRequirementsWithMinValues(np.Spec.Template.Spec.Requirements...) + requirements.Add(scheduling.NewLabelRequirements(np.Spec.Template.Labels).Values()...) for key, requirement := range requirements { if requirement.Operator() == corev1.NodeSelectorOpIn { // The following is a performance optimisation, for the explanation see the comment above @@ -286,9 +282,7 @@ func (p *Provisioner) NewScheduler(ctx context.Context, pods []*corev1.Pod, stat } } } - if len(notReadyNodePools) > 0 { - log.FromContext(ctx).WithValues("nodePools", nodePoolList).Info("skipped nodePools, not ready") - } + // inject topology constraints pods = p.injectVolumeTopologyRequirements(ctx, pods) @@ -301,7 +295,7 @@ func (p *Provisioner) NewScheduler(ctx context.Context, pods []*corev1.Pod, stat if err != nil { return nil, fmt.Errorf("getting daemon pods, %w", err) } - return scheduler.NewScheduler(ctx, p.kubeClient, lo.ToSlicePtr(nodePoolList.Items), p.cluster, stateNodes, topology, instanceTypes, daemonSetPods, p.recorder, p.clock), nil + return scheduler.NewScheduler(ctx, p.kubeClient, nodePools, p.cluster, stateNodes, topology, instanceTypes, daemonSetPods, p.recorder, p.clock), nil } func (p *Provisioner) Schedule(ctx context.Context) (scheduler.Results, error) { diff --git a/pkg/controllers/provisioning/scheduling/nodeclaimtemplate.go b/pkg/controllers/provisioning/scheduling/nodeclaimtemplate.go index 379bec28ce..0de33b527e 100644 --- a/pkg/controllers/provisioning/scheduling/nodeclaimtemplate.go +++ b/pkg/controllers/provisioning/scheduling/nodeclaimtemplate.go @@ -57,7 +57,10 @@ func NewNodeClaimTemplate(nodePool *v1.NodePool) *NodeClaimTemplate { v1.NodePoolHashAnnotationKey: nodePool.Hash(), v1.NodePoolHashVersionAnnotationKey: v1.NodePoolHashVersion, }) - nct.Labels = lo.Assign(nct.Labels, map[string]string{v1.NodePoolLabelKey: nodePool.Name}) + nct.Labels = lo.Assign(nct.Labels, map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + v1.NodeClassLabelKey(nodePool.Spec.Template.Spec.NodeClassRef.GroupKind()): nodePool.Spec.Template.Spec.NodeClassRef.Name, + }) nct.Requirements.Add(scheduling.NewNodeSelectorRequirementsWithMinValues(nct.Spec.Requirements...).Values()...) nct.Requirements.Add(scheduling.NewLabelRequirements(nct.Labels).Values()...) return nct diff --git a/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go b/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go index c8562398d6..2844b95362 100644 --- a/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go +++ b/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go @@ -166,7 +166,7 @@ func benchmarkScheduler(b *testing.B, instanceCount, podCount int) { client := fakecr.NewFakeClient() pods := makeDiversePods(podCount) clock := &clock.RealClock{} - cluster = state.NewCluster(clock, client) + cluster = state.NewCluster(clock, client, cloudProvider) domains := map[string]sets.Set[string]{} topology, err := scheduling.NewTopology(ctx, client, cluster, domains, pods) if err != nil { diff --git a/pkg/controllers/provisioning/scheduling/suite_test.go b/pkg/controllers/provisioning/scheduling/suite_test.go index 1f0b19037f..5a8e496d75 100644 --- a/pkg/controllers/provisioning/scheduling/suite_test.go +++ b/pkg/controllers/provisioning/scheduling/suite_test.go @@ -88,9 +88,9 @@ var _ = BeforeSuite(func() { // set these on the cloud provider, so we can manipulate them if needed cloudProvider.InstanceTypes = instanceTypes fakeClock = clock.NewFakeClock(time.Now()) - cluster = state.NewCluster(fakeClock, env.Client) + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) nodeStateController = informer.NewNodeController(env.Client, cluster) - nodeClaimStateController = informer.NewNodeClaimController(env.Client, cluster) + nodeClaimStateController = informer.NewNodeClaimController(env.Client, cloudProvider, cluster) podStateController = informer.NewPodController(env.Client, cluster) prov = provisioning.NewProvisioner(env.Client, events.NewRecorder(&record.FakeRecorder{}), cloudProvider, cluster, fakeClock) }) diff --git a/pkg/controllers/provisioning/suite_test.go b/pkg/controllers/provisioning/suite_test.go index acee66e8f3..d7b16f6013 100644 --- a/pkg/controllers/provisioning/suite_test.go +++ b/pkg/controllers/provisioning/suite_test.go @@ -76,7 +76,7 @@ var _ = BeforeSuite(func() { ctx = options.ToContext(ctx, test.Options()) cloudProvider = fake.NewCloudProvider() fakeClock = clock.NewFakeClock(time.Now()) - cluster = state.NewCluster(fakeClock, env.Client) + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) nodeController = informer.NewNodeController(env.Client, cluster) prov = provisioning.NewProvisioner(env.Client, events.NewRecorder(&record.FakeRecorder{}), cloudProvider, cluster, fakeClock) daemonsetController = informer.NewDaemonSetController(env.Client, cluster) @@ -1293,9 +1293,9 @@ var _ = Describe("Provisioning", func() { Template: v1.NodeClaimTemplate{ Spec: v1.NodeClaimTemplateSpec{ NodeClassRef: &v1.NodeClassReference{ - Group: "cloudprovider.karpenter.sh", - Kind: "CloudProvider", - Name: "default", + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "test", }, }, }, @@ -1308,9 +1308,9 @@ var _ = Describe("Provisioning", func() { Expect(cloudProvider.CreateCalls).To(HaveLen(1)) Expect(cloudProvider.CreateCalls[0].Spec.NodeClassRef).To(Equal( &v1.NodeClassReference{ - Group: "cloudprovider.karpenter.sh", - Kind: "CloudProvider", - Name: "default", + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "test", }, )) ExpectScheduled(ctx, env.Client, pod) diff --git a/pkg/controllers/state/cluster.go b/pkg/controllers/state/cluster.go index e008d18778..feb222e00a 100644 --- a/pkg/controllers/state/cluster.go +++ b/pkg/controllers/state/cluster.go @@ -38,13 +38,16 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/scheduling" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" podutils "sigs.k8s.io/karpenter/pkg/utils/pod" ) // Cluster maintains cluster state that is often needed but expensive to compute. type Cluster struct { kubeClient client.Client + cloudProvider cloudprovider.CloudProvider clock clock.Clock mu sync.RWMutex nodes map[string]*StateNode // provider id -> cached node @@ -68,10 +71,11 @@ type Cluster struct { antiAffinityPods sync.Map // pod namespaced name -> *corev1.Pod of pods that have required anti affinities } -func NewCluster(clk clock.Clock, client client.Client) *Cluster { +func NewCluster(clk clock.Clock, client client.Client, cloudProvider cloudprovider.CloudProvider) *Cluster { return &Cluster{ clock: clk, kubeClient: client, + cloudProvider: cloudProvider, nodes: map[string]*StateNode{}, bindings: map[types.NamespacedName]string{}, daemonSetPods: sync.Map{}, @@ -106,8 +110,8 @@ func (c *Cluster) Synced(ctx context.Context) (synced bool) { defer func() { ClusterStateSynced.Set(lo.Ternary[float64](synced, 1, 0), nil) }() - nodeClaimList := &v1.NodeClaimList{} - if err := c.kubeClient.List(ctx, nodeClaimList); err != nil { + nodeClaims, err := nodeclaimutils.ListManaged(ctx, c.kubeClient, c.cloudProvider) + if err != nil { log.FromContext(ctx).Error(err, "failed checking cluster state sync") return false } @@ -131,7 +135,7 @@ func (c *Cluster) Synced(ctx context.Context) (synced bool) { c.mu.RUnlock() nodeClaimNames := sets.New[string]() - for _, nodeClaim := range nodeClaimList.Items { + for _, nodeClaim := range nodeClaims { nodeClaimNames.Insert(nodeClaim.Name) } nodeNames := sets.New[string]() diff --git a/pkg/controllers/state/informer/nodeclaim.go b/pkg/controllers/state/informer/nodeclaim.go index 386770695b..a153e62dfb 100644 --- a/pkg/controllers/state/informer/nodeclaim.go +++ b/pkg/controllers/state/informer/nodeclaim.go @@ -21,27 +21,32 @@ import ( "k8s.io/apimachinery/pkg/api/errors" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/controllers/state" "sigs.k8s.io/karpenter/pkg/operator/injection" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" ) // NodeClaimController reconciles nodeclaim for the purpose of maintaining state. type NodeClaimController struct { - kubeClient client.Client - cluster *state.Cluster + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider + cluster *state.Cluster } // NewNodeClaimController constructs a controller instance -func NewNodeClaimController(kubeClient client.Client, cluster *state.Cluster) *NodeClaimController { +func NewNodeClaimController(kubeClient client.Client, cloudProvider cloudprovider.CloudProvider, cluster *state.Cluster) *NodeClaimController { return &NodeClaimController{ - kubeClient: kubeClient, - cluster: cluster, + kubeClient: kubeClient, + cloudProvider: cloudProvider, + cluster: cluster, } } @@ -56,6 +61,9 @@ func (c *NodeClaimController) Reconcile(ctx context.Context, req reconcile.Reque } return reconcile.Result{}, client.IgnoreNotFound(err) } + if !nodeclaimutils.IsManaged(nodeClaim, c.cloudProvider) { + return reconcile.Result{}, nil + } c.cluster.UpdateNodeClaim(nodeClaim) // ensure it's aware of any nodes we discover, this is a no-op if the node is already known to our cluster state return reconcile.Result{RequeueAfter: stateRetryPeriod}, nil @@ -64,7 +72,7 @@ func (c *NodeClaimController) Reconcile(ctx context.Context, req reconcile.Reque func (c *NodeClaimController) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("state.nodeclaim"). - For(&v1.NodeClaim{}). + For(&v1.NodeClaim{}, builder.WithPredicates(nodeclaimutils.IsManagedPredicateFuncs(c.cloudProvider))). WithOptions(controller.Options{MaxConcurrentReconciles: 10}). Complete(c) } diff --git a/pkg/controllers/state/informer/nodepool.go b/pkg/controllers/state/informer/nodepool.go index e08018515f..aa93a4ef6a 100644 --- a/pkg/controllers/state/informer/nodepool.go +++ b/pkg/controllers/state/informer/nodepool.go @@ -20,6 +20,7 @@ import ( "context" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/event" @@ -28,25 +29,32 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/controllers/state" "sigs.k8s.io/karpenter/pkg/operator/injection" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" ) // NodePoolController reconciles NodePools to re-trigger consolidation on change. type NodePoolController struct { - kubeClient client.Client - cluster *state.Cluster + kubeClient client.Client + cloudProvider cloudprovider.CloudProvider + cluster *state.Cluster } -func NewNodePoolController(kubeClient client.Client, cluster *state.Cluster) *NodePoolController { +func NewNodePoolController(kubeClient client.Client, cloudProvider cloudprovider.CloudProvider, cluster *state.Cluster) *NodePoolController { return &NodePoolController{ - kubeClient: kubeClient, - cluster: cluster, + kubeClient: kubeClient, + cloudProvider: cloudProvider, + cluster: cluster, } } func (c *NodePoolController) Reconcile(ctx context.Context, np *v1.NodePool) (reconcile.Result, error) { ctx = injection.WithControllerName(ctx, "state.nodepool") //nolint:ineffassign,staticcheck + if !nodepoolutils.IsManaged(np, c.cloudProvider) { + return reconcile.Result{}, nil + } // Something changed in the NodePool so we should re-consider consolidation c.cluster.MarkUnconsolidated() @@ -56,7 +64,7 @@ func (c *NodePoolController) Reconcile(ctx context.Context, np *v1.NodePool) (re func (c *NodePoolController) Register(_ context.Context, m manager.Manager) error { return controllerruntime.NewControllerManagedBy(m). Named("state.nodepool"). - For(&v1.NodePool{}). + For(&v1.NodePool{}, builder.WithPredicates(nodepoolutils.IsManagedPredicateFuncs(c.cloudProvider))). WithOptions(controller.Options{MaxConcurrentReconciles: 10}). WithEventFilter(predicate.GenerationChangedPredicate{}). WithEventFilter(predicate.Funcs{DeleteFunc: func(event event.DeleteEvent) bool { return false }}). diff --git a/pkg/controllers/state/suite_test.go b/pkg/controllers/state/suite_test.go index 391e001607..bc310a1a24 100644 --- a/pkg/controllers/state/suite_test.go +++ b/pkg/controllers/state/suite_test.go @@ -74,11 +74,11 @@ var _ = BeforeSuite(func() { ctx = options.ToContext(ctx, test.Options()) cloudProvider = fake.NewCloudProvider() fakeClock = clock.NewFakeClock(time.Now()) - cluster = state.NewCluster(fakeClock, env.Client) - nodeClaimController = informer.NewNodeClaimController(env.Client, cluster) + cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) + nodeClaimController = informer.NewNodeClaimController(env.Client, cloudProvider, cluster) nodeController = informer.NewNodeController(env.Client, cluster) podController = informer.NewPodController(env.Client, cluster) - nodePoolController = informer.NewNodePoolController(env.Client, cluster) + nodePoolController = informer.NewNodePoolController(env.Client, cloudProvider, cluster) daemonsetController = informer.NewDaemonSetController(env.Client, cluster) }) @@ -844,6 +844,10 @@ var _ = Describe("Node Resource Level", func() { nodeClaim := test.NodeClaim(v1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{ Finalizers: []string{v1.TerminationFinalizer}, + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + corev1.LabelInstanceTypeStable: cloudProvider.InstanceTypes[0].Name, + }, }, Spec: v1.NodeClaimSpec{ Requirements: []v1.NodeSelectorRequirementWithMinValues{ @@ -863,7 +867,9 @@ var _ = Describe("Node Resource Level", func() { }, }, NodeClassRef: &v1.NodeClassReference{ - Name: "default", + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", }, }, Status: v1.NodeClaimStatus{ @@ -880,16 +886,7 @@ var _ = Describe("Node Resource Level", func() { }, }, }) - node := test.Node(test.NodeOptions{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1.NodePoolLabelKey: nodePool.Name, - corev1.LabelInstanceTypeStable: cloudProvider.InstanceTypes[0].Name, - }, - Finalizers: []string{v1.TerminationFinalizer}, - }, - ProviderID: nodeClaim.Status.ProviderID, - }) + node := test.NodeClaimLinkedNode(nodeClaim) ExpectApplied(ctx, env.Client, nodeClaim, node) ExpectReconcileSucceeded(ctx, nodeClaimController, client.ObjectKeyFromObject(nodeClaim)) ExpectReconcileSucceeded(ctx, nodeController, client.ObjectKeyFromObject(node)) diff --git a/pkg/test/environment.go b/pkg/test/environment.go index 0d66529a71..fc7a1dae7c 100644 --- a/pkg/test/environment.go +++ b/pkg/test/environment.go @@ -23,6 +23,7 @@ import ( "github.com/awslabs/operatorpkg/option" "github.com/samber/lo" + "go.uber.org/multierr" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -59,14 +60,25 @@ func WithCRDs(crds ...*apiextensionsv1.CustomResourceDefinition) option.Function } } -// WithFieldIndexers expects a function that indexes fields against the cache such as cache.IndexField(...) +// WithFieldIndexers expects a function that indexes fields against the cache such as cache.IndexField(...). +// +// Note: Only use when necessary, the use of field indexers in functional tests requires the use of the cache syncing +// client, which can have significant drawbacks for test performance. func WithFieldIndexers(fieldIndexers ...func(cache.Cache) error) option.Function[EnvironmentOptions] { return func(o *EnvironmentOptions) { o.fieldIndexers = append(o.fieldIndexers, fieldIndexers...) } } -func NodeClaimFieldIndexer(ctx context.Context) func(cache.Cache) error { +func NodeProviderIDFieldIndexer(ctx context.Context) func(cache.Cache) error { + return func(c cache.Cache) error { + return c.IndexField(ctx, &corev1.Node{}, "spec.providerID", func(obj client.Object) []string { + return []string{obj.(*corev1.Node).Spec.ProviderID} + }) + } +} + +func NodeClaimProviderIDFieldIndexer(ctx context.Context) func(cache.Cache) error { return func(c cache.Cache) error { return c.IndexField(ctx, &v1.NodeClaim{}, "status.providerID", func(obj client.Object) []string { return []string{obj.(*v1.NodeClaim).Status.ProviderID} @@ -74,6 +86,22 @@ func NodeClaimFieldIndexer(ctx context.Context) func(cache.Cache) error { } } +func NodeClaimNodeClassRefFieldIndexer(ctx context.Context) func(cache.Cache) error { + return func(c cache.Cache) error { + var err error + err = multierr.Append(err, c.IndexField(ctx, &v1.NodeClaim{}, "spec.nodeClassRef.group", func(obj client.Object) []string { + return []string{obj.(*v1.NodeClaim).Spec.NodeClassRef.Group} + })) + err = multierr.Append(err, c.IndexField(ctx, &v1.NodeClaim{}, "spec.nodeClassRef.kind", func(obj client.Object) []string { + return []string{obj.(*v1.NodeClaim).Spec.NodeClassRef.Kind} + })) + err = multierr.Append(err, c.IndexField(ctx, &v1.NodeClaim{}, "spec.nodeClassRef.name", func(obj client.Object) []string { + return []string{obj.(*v1.NodeClaim).Spec.NodeClassRef.Name} + })) + return err + } +} + func VolumeAttachmentFieldIndexer(ctx context.Context) func(cache.Cache) error { return func(c cache.Cache) error { return c.IndexField(ctx, &storagev1.VolumeAttachment{}, "spec.nodeName", func(obj client.Object) []string { diff --git a/pkg/test/nodeclaim.go b/pkg/test/nodeclaim.go index 924c7fa1dc..467297b66f 100644 --- a/pkg/test/nodeclaim.go +++ b/pkg/test/nodeclaim.go @@ -19,7 +19,9 @@ package test import ( "fmt" + "github.com/awslabs/operatorpkg/object" "github.com/imdario/mergo" + "github.com/samber/lo" corev1 "k8s.io/api/core/v1" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -42,9 +44,14 @@ func NodeClaim(overrides ...v1.NodeClaim) *v1.NodeClaim { } if override.Spec.NodeClassRef == nil { override.Spec.NodeClassRef = &v1.NodeClassReference{ - Name: "default", + Group: object.GVK(defaultNodeClass).Group, + Kind: object.GVK(defaultNodeClass).Kind, + Name: "default", } } + override.Labels = lo.Assign(map[string]string{ + v1.NodeClassLabelKey(override.Spec.NodeClassRef.GroupKind()): override.Spec.NodeClassRef.Name, + }, override.Labels) if override.Spec.Requirements == nil { override.Spec.Requirements = []v1.NodeSelectorRequirementWithMinValues{} } diff --git a/pkg/test/nodeclass.go b/pkg/test/nodeclass.go index 6587ef1b3b..ee6b43b923 100644 --- a/pkg/test/nodeclass.go +++ b/pkg/test/nodeclass.go @@ -25,6 +25,16 @@ import ( "sigs.k8s.io/karpenter/pkg/test/v1alpha1" ) +var ( + // defaultNodeClass is the default NodeClass type used when creating NodeClassRefs for NodePools and NodeClaims + defaultNodeClass status.Object = &v1alpha1.TestNodeClass{} +) + +// SetDefaultNodeClassType configures the default NodeClass type used when generating NodeClassRefs for test NodePools and NodeClaims. +func SetDefaultNodeClassType(nc status.Object) { + defaultNodeClass = nc +} + // NodeClass creates a test NodeClass with defaults that can be overridden by overrides. // Overrides are applied in order, with a last write wins semantic. func NodeClass(overrides ...v1alpha1.TestNodeClass) *v1alpha1.TestNodeClass { diff --git a/pkg/test/nodepool.go b/pkg/test/nodepool.go index 75f8b7a013..1cca0af673 100644 --- a/pkg/test/nodepool.go +++ b/pkg/test/nodepool.go @@ -19,6 +19,7 @@ package test import ( "fmt" + "github.com/awslabs/operatorpkg/object" "github.com/imdario/mergo" "github.com/samber/lo" corev1 "k8s.io/api/core/v1" @@ -45,7 +46,9 @@ func NodePool(overrides ...v1.NodePool) *v1.NodePool { } if override.Spec.Template.Spec.NodeClassRef == nil { override.Spec.Template.Spec.NodeClassRef = &v1.NodeClassReference{ - Name: "default", + Group: object.GVK(defaultNodeClass).Group, + Kind: object.GVK(defaultNodeClass).Kind, + Name: "default", } } if override.Spec.Template.Spec.Requirements == nil { diff --git a/pkg/test/nodes.go b/pkg/test/nodes.go index 11489ac468..4e8c9c5bb8 100644 --- a/pkg/test/nodes.go +++ b/pkg/test/nodes.go @@ -20,6 +20,7 @@ import ( "fmt" "github.com/imdario/mergo" + "github.com/samber/lo" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -71,7 +72,9 @@ func NodeClaimLinkedNode(nodeClaim *v1.NodeClaim) *corev1.Node { n := Node( NodeOptions{ ObjectMeta: metav1.ObjectMeta{ - Labels: nodeClaim.Labels, + Labels: lo.Assign(map[string]string{ + v1.NodeClassLabelKey(nodeClaim.Spec.NodeClassRef.GroupKind()): nodeClaim.Spec.NodeClassRef.Name, + }, nodeClaim.Labels), Annotations: nodeClaim.Annotations, Finalizers: nodeClaim.Finalizers, }, diff --git a/pkg/utils/node/node.go b/pkg/utils/node/node.go index 0dad5718dd..0587c2fe1c 100644 --- a/pkg/utils/node/node.go +++ b/pkg/utils/node/node.go @@ -21,12 +21,17 @@ import ( "errors" "fmt" + "github.com/awslabs/operatorpkg/object" + "github.com/awslabs/operatorpkg/status" "github.com/samber/lo" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/predicate" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" "sigs.k8s.io/karpenter/pkg/utils/pod" ) @@ -94,12 +99,12 @@ func GetPods(ctx context.Context, kubeClient client.Client, nodes ...*corev1.Nod } // GetNodeClaims grabs all NodeClaims with a providerID that matches the provided Node -func GetNodeClaims(ctx context.Context, node *corev1.Node, kubeClient client.Client) ([]*v1.NodeClaim, error) { - nodeClaimList := &v1.NodeClaimList{} - if err := kubeClient.List(ctx, nodeClaimList, client.MatchingFields{"status.providerID": node.Spec.ProviderID}); err != nil { - return nil, fmt.Errorf("listing nodeClaims, %w", err) +func GetNodeClaims(ctx context.Context, kubeClient client.Client, node *corev1.Node) ([]*v1.NodeClaim, error) { + ncs := &v1.NodeClaimList{} + if err := kubeClient.List(ctx, ncs, nodeclaimutils.ForProviderID(node.Spec.ProviderID)); err != nil { + return nil, fmt.Errorf("listing nodeclaims, %w", err) } - return lo.ToSlicePtr(nodeClaimList.Items), nil + return lo.ToSlicePtr(ncs.Items), nil } // NodeClaimForNode is a helper function that takes a corev1.Node and attempts to find the matching v1.NodeClaim by its providerID @@ -107,7 +112,7 @@ func GetNodeClaims(ctx context.Context, node *corev1.Node, kubeClient client.Cli // 1. No v1.NodeClaims match the corev1.Node's providerID // 2. Multiple v1.NodeClaims match the corev1.Node's providerID func NodeClaimForNode(ctx context.Context, c client.Client, node *corev1.Node) (*v1.NodeClaim, error) { - nodeClaims, err := GetNodeClaims(ctx, node, c) + nodeClaims, err := GetNodeClaims(ctx, c, node) if err != nil { return nil, err } @@ -159,3 +164,17 @@ func GetCondition(n *corev1.Node, match corev1.NodeConditionType) corev1.NodeCon } return corev1.NodeCondition{} } + +func IsManaged(node *corev1.Node, cp cloudprovider.CloudProvider) bool { + return lo.ContainsBy(cp.GetSupportedNodeClasses(), func(nodeClass status.Object) bool { + _, ok := node.Labels[v1.NodeClassLabelKey(object.GVK(nodeClass).GroupKind())] + return ok + }) +} + +// IsManagedPredicateFuncs is used to filter controller-runtime NodeClaim watches to NodeClaims managed by the given cloudprovider. +func IsManagedPredicateFuncs(cp cloudprovider.CloudProvider) predicate.Funcs { + return predicate.NewPredicateFuncs(func(o client.Object) bool { + return IsManaged(o.(*corev1.Node), cp) + }) +} diff --git a/pkg/utils/node/suite_test.go b/pkg/utils/node/suite_test.go index 80acbc909f..6ef71e78ec 100644 --- a/pkg/utils/node/suite_test.go +++ b/pkg/utils/node/suite_test.go @@ -29,7 +29,7 @@ import ( "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" "sigs.k8s.io/karpenter/pkg/test/v1alpha1" - "sigs.k8s.io/karpenter/pkg/utils/node" + nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" . "sigs.k8s.io/karpenter/pkg/utils/testing" ) @@ -45,7 +45,7 @@ func TestAPIs(t *testing.T) { } var _ = BeforeSuite(func() { - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeClaimFieldIndexer(ctx))) + env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeClaimProviderIDFieldIndexer(ctx))) }) var _ = AfterSuite(func() { @@ -60,21 +60,13 @@ var _ = Describe("NodeUtils", func() { var testNode *corev1.Node var nodeClaim *v1.NodeClaim BeforeEach(func() { - nodeClaim = test.NodeClaim(v1.NodeClaim{ - Spec: v1.NodeClaimSpec{ - NodeClassRef: &v1.NodeClassReference{ - Kind: "NodeClassRef", - Group: "test.cloudprovider", - Name: "default", - }, - }, - }) + nodeClaim = test.NodeClaim() }) It("should return nodeClaim for node which has the same provider ID", func() { testNode = test.NodeClaimLinkedNode(nodeClaim) ExpectApplied(ctx, env.Client, testNode, nodeClaim) - nodeClaims, err := node.GetNodeClaims(ctx, testNode, env.Client) + nodeClaims, err := nodeutils.GetNodeClaims(ctx, env.Client, testNode) Expect(err).NotTo(HaveOccurred()) Expect(nodeClaims).To(HaveLen(1)) for _, nc := range nodeClaims { @@ -87,7 +79,7 @@ var _ = Describe("NodeUtils", func() { }) ExpectApplied(ctx, env.Client, testNode, nodeClaim) - nodeClaims, err := node.GetNodeClaims(ctx, testNode, env.Client) + nodeClaims, err := nodeutils.GetNodeClaims(ctx, env.Client, testNode) Expect(err).NotTo(HaveOccurred()) Expect(nodeClaims).To(HaveLen(0)) }) diff --git a/pkg/utils/nodeclaim/nodeclaim.go b/pkg/utils/nodeclaim/nodeclaim.go index 02d326df7b..b229b6404d 100644 --- a/pkg/utils/nodeclaim/nodeclaim.go +++ b/pkg/utils/nodeclaim/nodeclaim.go @@ -22,69 +22,105 @@ import ( "fmt" "github.com/awslabs/operatorpkg/object" + "github.com/awslabs/operatorpkg/status" "github.com/samber/lo" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" ) +func IsManaged(nodeClaim *v1.NodeClaim, cp cloudprovider.CloudProvider) bool { + return lo.ContainsBy(cp.GetSupportedNodeClasses(), func(nodeClass status.Object) bool { + return object.GVK(nodeClass).GroupKind() == nodeClaim.Spec.NodeClassRef.GroupKind() + }) +} + +// IsManagedPredicateFuncs is used to filter controller-runtime NodeClaim watches to NodeClaims managed by the given cloudprovider. +func IsManagedPredicateFuncs(cp cloudprovider.CloudProvider) predicate.Funcs { + return predicate.NewPredicateFuncs(func(o client.Object) bool { + return IsManaged(o.(*v1.NodeClaim), cp) + }) +} + +func ForProviderID(providerID string) client.ListOption { + return client.MatchingFields{"status.providerID": providerID} +} + +func ForNodePool(nodePoolName string) client.ListOption { + return client.MatchingLabels(map[string]string{v1.NodePoolLabelKey: nodePoolName}) +} + +func ForNodeClass(nodeClass status.Object) client.ListOption { + return client.MatchingFields{ + "spec.nodeClassRef.group": object.GVK(nodeClass).Group, + "spec.nodeClassRef.kind": object.GVK(nodeClass).Kind, + "spec.nodeClassRef.name": nodeClass.GetName(), + } +} + +func ListManaged(ctx context.Context, c client.Client, cloudProvider cloudprovider.CloudProvider, opts ...client.ListOption) ([]*v1.NodeClaim, error) { + nodeClaimList := &v1.NodeClaimList{} + if err := c.List(ctx, nodeClaimList, opts...); err != nil { + return nil, err + } + return lo.FilterMap(nodeClaimList.Items, func(nc v1.NodeClaim, _ int) (*v1.NodeClaim, bool) { + return &nc, IsManaged(&nc, cloudProvider) + }), nil +} + // PodEventHandler is a watcher on corev1.Pods that maps Pods to NodeClaim based on the node names // and enqueues reconcile.Requests for the NodeClaims -func PodEventHandler(c client.Client) handler.EventHandler { - return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) (requests []reconcile.Request) { - if name := o.(*corev1.Pod).Spec.NodeName; name != "" { - node := &corev1.Node{} - if err := c.Get(ctx, types.NamespacedName{Name: name}, node); err != nil { - return []reconcile.Request{} - } - nodeClaimList := &v1.NodeClaimList{} - if err := c.List(ctx, nodeClaimList, client.MatchingFields{"status.providerID": node.Spec.ProviderID}); err != nil { - return []reconcile.Request{} - } - return lo.Map(nodeClaimList.Items, func(n v1.NodeClaim, _ int) reconcile.Request { - return reconcile.Request{ - NamespacedName: client.ObjectKeyFromObject(&n), - } - }) +func PodEventHandler(c client.Client, cloudProvider cloudprovider.CloudProvider) handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) []reconcile.Request { + nodeName := o.(*corev1.Pod).Spec.NodeName + if nodeName == "" { + return nil + } + node := &corev1.Node{} + if err := c.Get(ctx, types.NamespacedName{Name: nodeName}, node); err != nil { + return nil + } + ncs, err := ListManaged(ctx, c, cloudProvider, ForProviderID(node.Spec.ProviderID)) + if err != nil { + return nil } - return requests + return lo.Map(ncs, func(nc *v1.NodeClaim, _ int) reconcile.Request { + return reconcile.Request{NamespacedName: client.ObjectKeyFromObject(nc)} + }) }) } // NodeEventHandler is a watcher on corev1.Node that maps Nodes to NodeClaims based on provider ids // and enqueues reconcile.Requests for the NodeClaims -func NodeEventHandler(c client.Client) handler.EventHandler { +func NodeEventHandler(c client.Client, cloudProvider cloudprovider.CloudProvider) handler.EventHandler { return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) []reconcile.Request { - node := o.(*corev1.Node) - nodeClaimList := &v1.NodeClaimList{} - if err := c.List(ctx, nodeClaimList, client.MatchingFields{"status.providerID": node.Spec.ProviderID}); err != nil { - return []reconcile.Request{} + ncs, err := ListManaged(ctx, c, cloudProvider, ForProviderID(o.(*corev1.Node).Spec.ProviderID)) + if err != nil { + return nil } - return lo.Map(nodeClaimList.Items, func(n v1.NodeClaim, _ int) reconcile.Request { - return reconcile.Request{ - NamespacedName: client.ObjectKeyFromObject(&n), - } + return lo.Map(ncs, func(nc *v1.NodeClaim, _ int) reconcile.Request { + return reconcile.Request{NamespacedName: client.ObjectKeyFromObject(nc)} }) }) } // NodePoolEventHandler is a watcher on v1.NodeClaim that maps NodePool to NodeClaims based // on the v1.NodePoolLabelKey and enqueues reconcile.Requests for the NodeClaim -func NodePoolEventHandler(c client.Client) handler.EventHandler { +func NodePoolEventHandler(c client.Client, cloudProvider cloudprovider.CloudProvider) handler.EventHandler { return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) (requests []reconcile.Request) { - nodeClaimList := &v1.NodeClaimList{} - if err := c.List(ctx, nodeClaimList, client.MatchingLabels(map[string]string{v1.NodePoolLabelKey: o.GetName()})); err != nil { - return requests + ncs, err := ListManaged(ctx, c, cloudProvider, ForNodePool(o.GetName())) + if err != nil { + return nil } - return lo.Map(nodeClaimList.Items, func(n v1.NodeClaim, _ int) reconcile.Request { - return reconcile.Request{ - NamespacedName: client.ObjectKeyFromObject(&n), - } + return lo.Map(ncs, func(nc *v1.NodeClaim, _ int) reconcile.Request { + return reconcile.Request{NamespacedName: client.ObjectKeyFromObject(nc)} }) }) } diff --git a/pkg/utils/nodeclaim/suite_test.go b/pkg/utils/nodeclaim/suite_test.go index 412059c833..5175ffcef0 100644 --- a/pkg/utils/nodeclaim/suite_test.go +++ b/pkg/utils/nodeclaim/suite_test.go @@ -18,6 +18,7 @@ package nodeclaim_test import ( "context" + "fmt" "testing" . "github.com/onsi/ginkgo/v2" @@ -31,16 +32,19 @@ import ( "sigs.k8s.io/karpenter/pkg/apis" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" + "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" "sigs.k8s.io/karpenter/pkg/test/v1alpha1" - nodeclaimutil "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" . "sigs.k8s.io/karpenter/pkg/utils/testing" ) var ( - ctx context.Context - env *test.Environment + ctx context.Context + env *test.Environment + cloudProvider cloudprovider.CloudProvider ) func TestAPIs(t *testing.T) { @@ -50,7 +54,8 @@ func TestAPIs(t *testing.T) { } var _ = BeforeSuite(func() { - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...)) + env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeClaimProviderIDFieldIndexer(ctx))) + cloudProvider = fake.NewCloudProvider() }) var _ = AfterSuite(func() { @@ -121,7 +126,7 @@ var _ = Describe("NodeClaimUtils", func() { }, }) node = test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID}) - node = nodeclaimutil.UpdateNodeOwnerReferences(nodeClaim, node) + node = nodeclaimutils.UpdateNodeOwnerReferences(nodeClaim, node) Expect(lo.Contains(node.OwnerReferences, metav1.OwnerReference{ APIVersion: lo.Must(apiutil.GVKForObject(nodeClaim, scheme.Scheme)).GroupVersion().String(), @@ -131,4 +136,90 @@ var _ = Describe("NodeClaimUtils", func() { BlockOwnerDeletion: lo.ToPtr(true), })) }) + Context("List", func() { + It("should filter by provider ID", func() { + ncs := lo.Times(2, func(_ int) *v1.NodeClaim { + return test.NodeClaim(v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", + }, + }, + }) + }) + ExpectApplied(ctx, env.Client, ncs[0], ncs[1]) + res, err := nodeclaimutils.ListManaged(ctx, env.Client, cloudProvider, nodeclaimutils.ForProviderID(ncs[0].Status.ProviderID)) + Expect(err).To(BeNil()) + Expect(len(res)).To(Equal(1)) + Expect(res[0].Name).To(Equal(ncs[0].Name)) + }) + It("should filter by NodePool", func() { + ncs := lo.Times(2, func(i int) *v1.NodeClaim { + return test.NodeClaim(v1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: fmt.Sprintf("nodepool-%d", i), + }, + }, + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", + }, + }, + }) + }) + ExpectApplied(ctx, env.Client, ncs[0], ncs[1]) + res, err := nodeclaimutils.ListManaged(ctx, env.Client, cloudProvider, nodeclaimutils.ForNodePool(ncs[0].Labels[v1.NodePoolLabelKey])) + Expect(err).To(BeNil()) + Expect(len(res)).To(Equal(1)) + Expect(res[0].Name).To(Equal(ncs[0].Name)) + }) + It("should filter managed NodeClaims", func() { + managed := test.NodeClaim(v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "TestNodeClass", + Name: "default", + }, + }, + }) + unmanagedByGroup := test.NodeClaim(v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.unmanaged.sh", + Kind: "TestNodeClass", + Name: "default", + }, + }, + }) + unmanagedByKind := test.NodeClaim(v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + unmanaged := test.NodeClaim(v1.NodeClaim{ + Spec: v1.NodeClaimSpec{ + NodeClassRef: &v1.NodeClassReference{ + Group: "karpenter.test.sh", + Kind: "UnmanagedNodeClass", + Name: "default", + }, + }, + }) + ExpectApplied(ctx, env.Client, managed, unmanagedByGroup, unmanagedByKind, unmanaged) + res, err := nodeclaimutils.ListManaged(ctx, env.Client, cloudProvider) + Expect(err).To(BeNil()) + Expect(len(res)).To(Equal(1)) + Expect(res[0].Name).To(Equal(managed.Name)) + }) + }) }) diff --git a/pkg/utils/nodepool/nodepool.go b/pkg/utils/nodepool/nodepool.go index 19c16baa6c..298f133dbc 100644 --- a/pkg/utils/nodepool/nodepool.go +++ b/pkg/utils/nodepool/nodepool.go @@ -18,32 +18,102 @@ package nodepool import ( "context" + "sort" "github.com/awslabs/operatorpkg/object" + "github.com/awslabs/operatorpkg/status" "github.com/samber/lo" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" ) +func IsManaged(nodePool *v1.NodePool, cp cloudprovider.CloudProvider) bool { + return lo.ContainsBy(cp.GetSupportedNodeClasses(), func(nodeClass status.Object) bool { + return object.GVK(nodeClass).GroupKind() == nodePool.Spec.Template.Spec.NodeClassRef.GroupKind() + }) +} + +// IsManagedPredicateFuncs is used to filter controller-runtime NodeClaim watches to NodeClaims managed by the given cloudprovider. +func IsManagedPredicateFuncs(cp cloudprovider.CloudProvider) predicate.Funcs { + return predicate.NewPredicateFuncs(func(o client.Object) bool { + return IsManaged(o.(*v1.NodePool), cp) + }) +} + +func ForNodeClass(nc status.Object) client.ListOption { + return client.MatchingFields{ + "spec.template.spec.nodeClassRef.group": object.GVK(nc).Group, + "spec.template.spec.nodeClassRef.kind": object.GVK(nc).Kind, + "spec.template.spec.nodeClassRef.name": nc.GetName(), + } +} + +func ListManaged(ctx context.Context, c client.Client, cloudProvider cloudprovider.CloudProvider, opts ...client.ListOption) ([]*v1.NodePool, error) { + nodePoolList := &v1.NodePoolList{} + if err := c.List(ctx, nodePoolList, opts...); err != nil { + return nil, err + } + return lo.FilterMap(nodePoolList.Items, func(np v1.NodePool, _ int) (*v1.NodePool, bool) { + return &np, IsManaged(&np, cloudProvider) + }), nil +} + +func NodeClaimEventHandler() handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []reconcile.Request { + name, ok := o.GetLabels()[v1.NodePoolLabelKey] + if !ok { + return nil + } + return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: name}}} + }) + +} + +func NodeEventHandler() handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []reconcile.Request { + name, ok := o.GetLabels()[v1.NodePoolLabelKey] + if !ok { + return nil + } + return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: name}}} + }) + +} + // NodeClassEventHandler is a watcher on v1.NodePool that maps NodeClass to NodePools based // on the nodeClassRef and enqueues reconcile.Requests for the NodePool func NodeClassEventHandler(c client.Client) handler.EventHandler { return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) (requests []reconcile.Request) { - nodePoolList := &v1.NodePoolList{} - if err := c.List(ctx, nodePoolList, client.MatchingFields{ - "spec.template.spec.nodeClassRef.group": object.GVK(o).Group, - "spec.template.spec.nodeClassRef.kind": object.GVK(o).Kind, - "spec.template.spec.nodeClassRef.name": o.GetName(), - }); err != nil { - return requests + nps := &v1.NodePoolList{} + if err := c.List(ctx, nps, ForNodeClass(o.(status.Object))); err != nil { + return nil } - return lo.Map(nodePoolList.Items, func(n v1.NodePool, _ int) reconcile.Request { + return lo.Map(nps.Items, func(np v1.NodePool, _ int) reconcile.Request { return reconcile.Request{ - NamespacedName: client.ObjectKeyFromObject(&n), + NamespacedName: client.ObjectKeyFromObject(&np), } }) }) } + +// OrderByWeight orders the NodePools in the provided slice by their priority weight in-place. This priority evaluates +// the following things in precedence order: +// 1. NodePools that have a larger weight are ordered first +// 2. If two NodePools have the same weight, then the NodePool with the name later in the alphabet will come first +func OrderByWeight(nps []*v1.NodePool) { + sort.Slice(nps, func(a, b int) bool { + weightA := lo.FromPtr(nps[a].Spec.Weight) + weightB := lo.FromPtr(nps[b].Spec.Weight) + if weightA == weightB { + // Order NodePools by name for a consistent ordering when sorting equal weight + return nps[a].Name > nps[b].Name + } + return weightA > weightB + }) +} diff --git a/pkg/utils/nodepool/suite_test.go b/pkg/utils/nodepool/suite_test.go new file mode 100644 index 0000000000..ead9695db5 --- /dev/null +++ b/pkg/utils/nodepool/suite_test.go @@ -0,0 +1,97 @@ +/* +Copyright The Kubernetes Authors. + +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 nodepool_test + +import ( + "context" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/samber/lo" + "golang.org/x/exp/rand" + + "sigs.k8s.io/karpenter/pkg/apis" + v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/test" + . "sigs.k8s.io/karpenter/pkg/test/expectations" + "sigs.k8s.io/karpenter/pkg/test/v1alpha1" + nodepoolutils "sigs.k8s.io/karpenter/pkg/utils/nodepool" + . "sigs.k8s.io/karpenter/pkg/utils/testing" +) + +var ( + ctx context.Context + env *test.Environment +) + +func TestAPIs(t *testing.T) { + ctx = TestContextWithLogger(t) + RegisterFailHandler(Fail) + RunSpecs(t, "NodePoolUtils") +} + +var _ = BeforeSuite(func() { + env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...)) +}) + +var _ = AfterSuite(func() { + Expect(env.Stop()).To(Succeed(), "Failed to stop environment") +}) + +var _ = AfterEach(func() { + ExpectCleanedUp(ctx, env.Client) +}) + +var _ = Describe("NodePoolUtils", func() { + Context("OrderByWeight", func() { + It("should order the NodePools by weight", func() { + // Generate 10 NodePools that have random weights, some might have the same weights + nps := lo.Shuffle(lo.Times(10, func(_ int) *v1.NodePool { + return test.NodePool(v1.NodePool{ + Spec: v1.NodePoolSpec{ + Weight: lo.ToPtr[int32](int32(rand.Intn(100) + 1)), //nolint:gosec + }, + }) + })) + nodepoolutils.OrderByWeight(nps) + + lastWeight := 101 // This is above the allowed weight values + for _, np := range nps { + Expect(lo.FromPtr(np.Spec.Weight)).To(BeNumerically("<=", lastWeight)) + lastWeight = int(lo.FromPtr(np.Spec.Weight)) + } + }) + It("should order the NodePools by name when the weights are the same", func() { + // Generate 10 NodePools with the same weight + nps := lo.Shuffle(lo.Times(10, func(_ int) *v1.NodePool { + return test.NodePool(v1.NodePool{ + Spec: v1.NodePoolSpec{ + Weight: lo.ToPtr[int32](10), + }, + }) + })) + nodepoolutils.OrderByWeight(nps) + + lastName := "zzzzzzzzzzzzzzzzzzzzzzzz" // large string value + for _, np := range nps { + Expect(np.Name < lastName).To(BeTrue()) + lastName = np.Name + } + }) + }) +}) diff --git a/test/pkg/debug/nodeclaim.go b/test/pkg/debug/nodeclaim.go index 16b71efe54..fbeabdf281 100644 --- a/test/pkg/debug/nodeclaim.go +++ b/test/pkg/debug/nodeclaim.go @@ -37,9 +37,9 @@ type NodeClaimController struct { kubeClient client.Client } -func NewNodeClaimController(kubeClient client.Client) *NodeClaimController { +func NewNodeClaimController(kc client.Client) *NodeClaimController { return &NodeClaimController{ - kubeClient: kubeClient, + kubeClient: kc, } }