From a38b5188ab07cf8207430151fd74b05088dc23c0 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 15:43:57 -0700 Subject: [PATCH 1/9] feat: promote drift to stable --- kwok/charts/README.md | 5 ++-- kwok/charts/templates/deployment.yaml | 2 +- kwok/charts/values.yaml | 4 --- pkg/controllers/disruption/drift.go | 4 +-- pkg/controllers/disruption/drift_test.go | 19 ------------- .../disruption/orchestration/suite_test.go | 3 +- pkg/controllers/disruption/suite_test.go | 4 +-- .../node/termination/terminator/suite_test.go | 3 +- pkg/controllers/nodeclaim/disruption/drift.go | 15 ++-------- .../nodeclaim/disruption/drift_test.go | 4 +-- .../nodeclaim/disruption/suite_test.go | 4 +-- pkg/operator/options/options.go | 6 +--- pkg/operator/options/suite_test.go | 28 +++++++++---------- pkg/test/options.go | 2 -- 14 files changed, 30 insertions(+), 73 deletions(-) diff --git a/kwok/charts/README.md b/kwok/charts/README.md index d37c149104..054f88e20a 100644 --- a/kwok/charts/README.md +++ b/kwok/charts/README.md @@ -51,11 +51,10 @@ For full Karpenter documentation please checkout [https://karpenter.sh](https:// | serviceMonitor.additionalLabels | object | `{}` | Additional labels for the ServiceMonitor. | | serviceMonitor.enabled | bool | `false` | Specifies whether a ServiceMonitor should be created. | | serviceMonitor.endpointConfig | object | `{}` | Endpoint configuration for the ServiceMonitor. | -| settings | object | `{"batchIdleDuration":"1s","batchMaxDuration":"10s","featureGates":{"drift":true,"spotToSpotConsolidation":false}}` | Global Settings to configure Karpenter | +| settings | object | `{"batchIdleDuration":"1s","batchMaxDuration":"10s","featureGates":{"spotToSpotConsolidation":false}}` | Global Settings to configure Karpenter | | settings.batchIdleDuration | string | `"1s"` | The maximum amount of time with no new ending pods that if exceeded ends the current batching window. If pods arrive faster than this time, the batching window will be extended up to the maxDuration. If they arrive slower, the pods will be batched separately. | | settings.batchMaxDuration | string | `"10s"` | The maximum length of a batch window. The longer this is, the more pods we can consider for provisioning at one time which usually results in fewer but larger nodes. | -| settings.featureGates | object | `{"drift":true,"spotToSpotConsolidation":false}` | Feature Gate configuration values. Feature Gates will follow the same graduation process and requirements as feature gates in Kubernetes. More information here https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-alpha-or-beta-features | -| settings.featureGates.drift | bool | `true` | drift is in BETA and is enabled by default. Setting drift to false disables the drift disruption method to watch for drift between currently deployed nodes and the desired state of nodes set in nodepools and nodeclasses | +| settings.featureGates | object | `{"spotToSpotConsolidation":false}` | Feature Gate configuration values. Feature Gates will follow the same graduation process and requirements as feature gates in Kubernetes. More information here https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-alpha-or-beta-features | | settings.featureGates.spotToSpotConsolidation | bool | `false` | spotToSpotConsolidation is ALPHA and is disabled by default. Setting this to true will enable spot replacement consolidation for both single and multi-node consolidation. | | strategy | object | `{"rollingUpdate":{"maxUnavailable":1}}` | Strategy for updating the pod. | | terminationGracePeriodSeconds | string | `nil` | Override the default termination grace period for the pod. | diff --git a/kwok/charts/templates/deployment.yaml b/kwok/charts/templates/deployment.yaml index 80e4a70414..072f3b2ca4 100644 --- a/kwok/charts/templates/deployment.yaml +++ b/kwok/charts/templates/deployment.yaml @@ -95,7 +95,7 @@ spec: divisor: "0" resource: limits.memory - name: FEATURE_GATES - value: "Drift={{ .Values.settings.featureGates.drift }},SpotToSpotConsolidation={{ .Values.settings.featureGates.spotToSpotConsolidation }}" + value: "SpotToSpotConsolidation={{ .Values.settings.featureGates.spotToSpotConsolidation }}" {{- with .Values.settings.batchMaxDuration }} - name: BATCH_MAX_DURATION value: "{{ . }}" diff --git a/kwok/charts/values.yaml b/kwok/charts/values.yaml index a1489b7ab7..cb1c3a0fe5 100644 --- a/kwok/charts/values.yaml +++ b/kwok/charts/values.yaml @@ -136,10 +136,6 @@ settings: # -- Feature Gate configuration values. Feature Gates will follow the same graduation process and requirements as feature gates # in Kubernetes. More information here https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-alpha-or-beta-features featureGates: - # -- drift is in BETA and is enabled by default. - # Setting drift to false disables the drift disruption method to watch for drift between currently deployed nodes - # and the desired state of nodes set in nodepools and nodeclasses - drift: true # -- spotToSpotConsolidation is ALPHA and is disabled by default. # Setting this to true will enable spot replacement consolidation for both single and multi-node consolidation. spotToSpotConsolidation: false \ No newline at end of file diff --git a/pkg/controllers/disruption/drift.go b/pkg/controllers/disruption/drift.go index af3771f0fa..92602ffda5 100644 --- a/pkg/controllers/disruption/drift.go +++ b/pkg/controllers/disruption/drift.go @@ -30,7 +30,6 @@ import ( "sigs.k8s.io/karpenter/pkg/controllers/state" "sigs.k8s.io/karpenter/pkg/events" "sigs.k8s.io/karpenter/pkg/metrics" - "sigs.k8s.io/karpenter/pkg/operator/options" ) // Drift is a subreconciler that deletes drifted candidates. @@ -52,8 +51,7 @@ func NewDrift(kubeClient client.Client, cluster *state.Cluster, provisioner *pro // ShouldDisrupt is a predicate used to filter candidates func (d *Drift) ShouldDisrupt(ctx context.Context, c *Candidate) bool { - return options.FromContext(ctx).FeatureGates.Drift && - c.NodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).IsTrue() + return c.NodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).IsTrue() } // ComputeCommand generates a disruption command given candidates diff --git a/pkg/controllers/disruption/drift_test.go b/pkg/controllers/disruption/drift_test.go index 696e839016..51a6877900 100644 --- a/pkg/controllers/disruption/drift_test.go +++ b/pkg/controllers/disruption/drift_test.go @@ -37,7 +37,6 @@ import ( "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/controllers/disruption" - "sigs.k8s.io/karpenter/pkg/operator/options" "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" ) @@ -473,24 +472,6 @@ var _ = Describe("Drift", func() { }) Context("Drift", func() { - It("should ignore drifted nodes if the feature flag is disabled", func() { - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(false)}})) - ExpectApplied(ctx, env.Client, nodeClaim, node, nodePool) - - // inform cluster state about nodes and nodeclaims - ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, []*v1.Node{node}, []*v1beta1.NodeClaim{nodeClaim}) - - fakeClock.Step(10 * time.Minute) - - var wg sync.WaitGroup - ExpectTriggerVerifyAction(&wg) - ExpectReconcileSucceeded(ctx, disruptionController, types.NamespacedName{}) - wg.Wait() - - // Expect to not create or delete more nodeclaims - Expect(ExpectNodeClaims(ctx, env.Client)).To(HaveLen(1)) - ExpectExists(ctx, env.Client, nodeClaim) - }) It("should continue to the next drifted node if the first cannot reschedule all pods", func() { pod := test.Pod(test.PodOptions{ ResourceRequirements: v1.ResourceRequirements{ diff --git a/pkg/controllers/disruption/orchestration/suite_test.go b/pkg/controllers/disruption/orchestration/suite_test.go index d500bac90b..14e47c9933 100644 --- a/pkg/controllers/disruption/orchestration/suite_test.go +++ b/pkg/controllers/disruption/orchestration/suite_test.go @@ -23,7 +23,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/samber/lo" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -73,7 +72,7 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { env = test.NewEnvironment(scheme.Scheme, test.WithCRDs(apis.CRDs...)) - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(true)}})) + ctx = options.ToContext(ctx, test.Options()) fakeClock = clock.NewFakeClock(time.Now()) cloudProvider = fake.NewCloudProvider() cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) diff --git a/pkg/controllers/disruption/suite_test.go b/pkg/controllers/disruption/suite_test.go index 80fa3a1a2c..a67d209900 100644 --- a/pkg/controllers/disruption/suite_test.go +++ b/pkg/controllers/disruption/suite_test.go @@ -81,7 +81,7 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { env = test.NewEnvironment(scheme.Scheme, test.WithCRDs(coreapis.CRDs...)) - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(true)}})) + ctx = options.ToContext(ctx, test.Options()) cloudProvider = fake.NewCloudProvider() fakeClock = clock.NewFakeClock(time.Now()) cluster = state.NewCluster(fakeClock, env.Client, cloudProvider) @@ -113,7 +113,7 @@ var _ = BeforeEach(func() { cluster.MarkUnconsolidated() // Reset Feature Flags to test defaults - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(true)}})) + ctx = options.ToContext(ctx, test.Options()) onDemandInstances = lo.Filter(cloudProvider.InstanceTypes, func(i *cloudprovider.InstanceType, _ int) bool { for _, o := range i.Offerings.Available() { diff --git a/pkg/controllers/node/termination/terminator/suite_test.go b/pkg/controllers/node/termination/terminator/suite_test.go index bd756ac601..4ced2555e1 100644 --- a/pkg/controllers/node/termination/terminator/suite_test.go +++ b/pkg/controllers/node/termination/terminator/suite_test.go @@ -23,7 +23,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/samber/lo" v1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -56,7 +55,7 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { env = test.NewEnvironment(scheme.Scheme, test.WithCRDs(apis.CRDs...)) - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(true)}})) + ctx = options.ToContext(ctx, test.Options()) recorder = test.NewEventRecorder() queue = terminator.NewQueue(env.Client, recorder) }) diff --git a/pkg/controllers/nodeclaim/disruption/drift.go b/pkg/controllers/nodeclaim/disruption/drift.go index 6d86b1d5dc..7f79b11b8e 100644 --- a/pkg/controllers/nodeclaim/disruption/drift.go +++ b/pkg/controllers/nodeclaim/disruption/drift.go @@ -30,7 +30,6 @@ import ( "sigs.k8s.io/karpenter/pkg/apis/v1beta1" "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/metrics" - "sigs.k8s.io/karpenter/pkg/operator/options" "sigs.k8s.io/karpenter/pkg/scheduling" ) @@ -47,16 +46,8 @@ type Drift struct { func (d *Drift) Reconcile(ctx context.Context, nodePool *v1beta1.NodePool, nodeClaim *v1beta1.NodeClaim) (reconcile.Result, error) { hasDriftedCondition := nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted) != nil - // From here there are three scenarios to handle: - // 1. If drift is not enabled but the NodeClaim is drifted, remove the status condition - if !options.FromContext(ctx).FeatureGates.Drift { - _ = nodeClaim.StatusConditions().Clear(v1beta1.ConditionTypeDrifted) - if hasDriftedCondition { - logging.FromContext(ctx).Debugf("removing drift status condition, drift has been disabled") - } - return reconcile.Result{}, nil - } - // 2. If NodeClaim is not launched, remove the drift status condition + // From here there are two scenarios to handle: + // 1. If NodeClaim is not launched, remove the drift status condition if !nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeLaunched).IsTrue() { _ = nodeClaim.StatusConditions().Clear(v1beta1.ConditionTypeDrifted) if hasDriftedCondition { @@ -68,7 +59,7 @@ func (d *Drift) Reconcile(ctx context.Context, nodePool *v1beta1.NodePool, nodeC if err != nil { return reconcile.Result{}, cloudprovider.IgnoreNodeClaimNotFoundError(fmt.Errorf("getting drift, %w", err)) } - // 3. Otherwise, if the NodeClaim isn't drifted, but has the status condition, remove it. + // 2. Otherwise, if the NodeClaim isn't drifted, but has the status condition, remove it. if driftedReason == "" { _ = nodeClaim.StatusConditions().Clear(v1beta1.ConditionTypeDrifted) if hasDriftedCondition { diff --git a/pkg/controllers/nodeclaim/disruption/drift_test.go b/pkg/controllers/nodeclaim/disruption/drift_test.go index cde634ff47..69182919f0 100644 --- a/pkg/controllers/nodeclaim/disruption/drift_test.go +++ b/pkg/controllers/nodeclaim/disruption/drift_test.go @@ -156,7 +156,7 @@ var _ = Describe("Drift", func() { }) It("should not detect drift if the feature flag is disabled", func() { cp.Drifted = "drifted" - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(false)}})) + ctx = options.ToContext(ctx, test.Options()) ExpectApplied(ctx, env.Client, nodePool, nodeClaim) ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) @@ -165,7 +165,7 @@ var _ = Describe("Drift", func() { }) It("should remove the status condition from the nodeClaim if the feature flag is disabled", func() { cp.Drifted = "drifted" - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(false)}})) + ctx = options.ToContext(ctx, test.Options()) nodeClaim.StatusConditions().SetTrue(v1beta1.ConditionTypeDrifted) ExpectApplied(ctx, env.Client, nodePool, nodeClaim) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index 76b58f782a..6ea2fe51d1 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -74,7 +74,7 @@ var _ = AfterSuite(func() { }) var _ = BeforeEach(func() { - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(true)}})) + ctx = options.ToContext(ctx, test.Options()) fakeClock.SetTime(time.Now()) }) @@ -126,7 +126,7 @@ var _ = Describe("Disruption", func() { ExpectMakeNodeClaimsInitialized(ctx, env.Client, nodeClaim) // Drift, Expiration, and Emptiness are disabled through configuration - ctx = options.ToContext(ctx, test.Options(test.OptionsFields{FeatureGates: test.FeatureGates{Drift: lo.ToPtr(false)}})) + ctx = options.ToContext(ctx, test.Options()) ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) diff --git a/pkg/operator/options/options.go b/pkg/operator/options/options.go index 81168d66e5..0ce02f47e1 100644 --- a/pkg/operator/options/options.go +++ b/pkg/operator/options/options.go @@ -41,7 +41,6 @@ type optionsKey struct{} type FeatureGates struct { inputStr string - Drift bool SpotToSpotConsolidation bool } @@ -96,7 +95,7 @@ func (o *Options) AddFlags(fs *FlagSet) { fs.StringVar(&o.LogLevel, "log-level", env.WithDefaultString("LOG_LEVEL", "info"), "Log verbosity level. Can be one of 'debug', 'info', or 'error'") fs.DurationVar(&o.BatchMaxDuration, "batch-max-duration", env.WithDefaultDuration("BATCH_MAX_DURATION", 10*time.Second), "The maximum length of a batch window. The longer this is, the more pods we can consider for provisioning at one time which usually results in fewer but larger nodes.") fs.DurationVar(&o.BatchIdleDuration, "batch-idle-duration", env.WithDefaultDuration("BATCH_IDLE_DURATION", time.Second), "The maximum amount of time with no new pending pods that if exceeded ends the current batching window. If pods arrive faster than this time, the batching window will be extended up to the maxDuration. If they arrive slower, the pods will be batched separately.") - fs.StringVar(&o.FeatureGates.inputStr, "feature-gates", env.WithDefaultString("FEATURE_GATES", "Drift=true,SpotToSpotConsolidation=false"), "Optional features can be enabled / disabled using feature gates. Current options are: Drift,SpotToSpotConsolidation") + fs.StringVar(&o.FeatureGates.inputStr, "feature-gates", env.WithDefaultString("FEATURE_GATES", "SpotToSpotConsolidation=false"), "Optional features can be enabled / disabled using feature gates. Current options are: SpotToSpotConsolidation") } func (o *Options) Parse(fs *FlagSet, args ...string) error { @@ -131,9 +130,6 @@ func ParseFeatureGates(gateStr string) (FeatureGates, error) { if err := cliflag.NewMapStringBool(&gateMap).Set(gateStr); err != nil { return gates, err } - if val, ok := gateMap["Drift"]; ok { - gates.Drift = val - } if val, ok := gateMap["SpotToSpotConsolidation"]; ok { gates.SpotToSpotConsolidation = val } diff --git a/pkg/operator/options/suite_test.go b/pkg/operator/options/suite_test.go index 906e1677eb..a4e19839e9 100644 --- a/pkg/operator/options/suite_test.go +++ b/pkg/operator/options/suite_test.go @@ -78,15 +78,15 @@ var _ = Describe("Options", func() { Context("FeatureGates", func() { DescribeTable( "should successfully parse well formed feature gate strings", - func(str string, driftVal bool) { + func(str string, spotToSpotConsolidationVal bool) { gates, err := options.ParseFeatureGates(str) Expect(err).To(BeNil()) - Expect(gates.Drift).To(Equal(driftVal)) + Expect(gates.SpotToSpotConsolidation).To(Equal(spotToSpotConsolidationVal)) }, - Entry("basic true", "Drift=true", true), - Entry("basic false", "Drift=false", false), - Entry("with whitespace", "Drift\t= false", false), - Entry("multiple values", "Hello=true,Drift=false,World=true", false), + Entry("basic true", "SpotToSpotConsolidation=true", true), + Entry("basic false", "SpotToSpotConsolidation=false", false), + Entry("with whitespace", "SpotToSpotConsolidation\t= false", false), + Entry("multiple values", "Hello=true,SpotToSpotConsolidation=false,World=true", false), ) }) @@ -110,7 +110,7 @@ var _ = Describe("Options", func() { BatchMaxDuration: lo.ToPtr(10 * time.Second), BatchIdleDuration: lo.ToPtr(time.Second), FeatureGates: test.FeatureGates{ - Drift: lo.ToPtr(true), + SpotToSpotConsolidation: lo.ToPtr(true), }, })) }) @@ -132,7 +132,7 @@ var _ = Describe("Options", func() { "--log-level", "debug", "--batch-max-duration", "5s", "--batch-idle-duration", "5s", - "--feature-gates", "Drift=true", + "--feature-gates", "SpotToSpotConsolidation=true", ) Expect(err).To(BeNil()) expectOptionsMatch(opts, test.Options(test.OptionsFields{ @@ -151,7 +151,7 @@ var _ = Describe("Options", func() { BatchMaxDuration: lo.ToPtr(5 * time.Second), BatchIdleDuration: lo.ToPtr(5 * time.Second), FeatureGates: test.FeatureGates{ - Drift: lo.ToPtr(true), + SpotToSpotConsolidation: lo.ToPtr(true), }, })) }) @@ -171,7 +171,7 @@ var _ = Describe("Options", func() { os.Setenv("LOG_LEVEL", "debug") os.Setenv("BATCH_MAX_DURATION", "5s") os.Setenv("BATCH_IDLE_DURATION", "5s") - os.Setenv("FEATURE_GATES", "Drift=true") + os.Setenv("FEATURE_GATES", "SpotToSpotConsolidation=true") fs = &options.FlagSet{ FlagSet: flag.NewFlagSet("karpenter", flag.ContinueOnError), } @@ -194,7 +194,7 @@ var _ = Describe("Options", func() { BatchMaxDuration: lo.ToPtr(5 * time.Second), BatchIdleDuration: lo.ToPtr(5 * time.Second), FeatureGates: test.FeatureGates{ - Drift: lo.ToPtr(true), + SpotToSpotConsolidation: lo.ToPtr(true), }, })) }) @@ -212,7 +212,7 @@ var _ = Describe("Options", func() { os.Setenv("LOG_LEVEL", "debug") os.Setenv("BATCH_MAX_DURATION", "5s") os.Setenv("BATCH_IDLE_DURATION", "5s") - os.Setenv("FEATURE_GATES", "Drift=true") + os.Setenv("FEATURE_GATES", "SpotToSpotConsolidation=true") fs = &options.FlagSet{ FlagSet: flag.NewFlagSet("karpenter", flag.ContinueOnError), } @@ -239,7 +239,7 @@ var _ = Describe("Options", func() { BatchMaxDuration: lo.ToPtr(5 * time.Second), BatchIdleDuration: lo.ToPtr(5 * time.Second), FeatureGates: test.FeatureGates{ - Drift: lo.ToPtr(true), + SpotToSpotConsolidation: lo.ToPtr(true), }, })) }) @@ -298,5 +298,5 @@ func expectOptionsMatch(optsA, optsB *options.Options) { Expect(optsA.LogLevel).To(Equal(optsB.LogLevel)) Expect(optsA.BatchMaxDuration).To(Equal(optsB.BatchMaxDuration)) Expect(optsA.BatchIdleDuration).To(Equal(optsB.BatchIdleDuration)) - Expect(optsA.FeatureGates.Drift).To(Equal(optsB.FeatureGates.Drift)) + Expect(optsA.FeatureGates.SpotToSpotConsolidation).To(Equal(optsB.FeatureGates.SpotToSpotConsolidation)) } diff --git a/pkg/test/options.go b/pkg/test/options.go index 9ee76f4bb9..3e3de9c041 100644 --- a/pkg/test/options.go +++ b/pkg/test/options.go @@ -46,7 +46,6 @@ type OptionsFields struct { } type FeatureGates struct { - Drift *bool SpotToSpotConsolidation *bool } @@ -74,7 +73,6 @@ func Options(overrides ...OptionsFields) *options.Options { BatchMaxDuration: lo.FromPtrOr(opts.BatchMaxDuration, 10*time.Second), BatchIdleDuration: lo.FromPtrOr(opts.BatchIdleDuration, time.Second), FeatureGates: options.FeatureGates{ - Drift: lo.FromPtrOr(opts.FeatureGates.Drift, false), SpotToSpotConsolidation: lo.FromPtrOr(opts.FeatureGates.SpotToSpotConsolidation, false), }, } From 5672f7e5f8f25514b96df5e2533350f2f1f95bca Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 15:55:22 -0700 Subject: [PATCH 2/9] fix: imports --- pkg/controllers/disruption/drift_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controllers/disruption/drift_test.go b/pkg/controllers/disruption/drift_test.go index a874af7a2d..0c4edbc5fc 100644 --- a/pkg/controllers/disruption/drift_test.go +++ b/pkg/controllers/disruption/drift_test.go @@ -34,6 +34,7 @@ import ( "sigs.k8s.io/karpenter/pkg/cloudprovider" "sigs.k8s.io/karpenter/pkg/cloudprovider/fake" "sigs.k8s.io/karpenter/pkg/controllers/disruption" + "sigs.k8s.io/karpenter/pkg/scheduling" "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" ) From d9b7f3a41af39b54a9267dce92d22844b6958da5 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 17:16:29 -0700 Subject: [PATCH 3/9] fix: test --- pkg/operator/options/suite_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/operator/options/suite_test.go b/pkg/operator/options/suite_test.go index 346538a585..98fc2db6a3 100644 --- a/pkg/operator/options/suite_test.go +++ b/pkg/operator/options/suite_test.go @@ -111,7 +111,7 @@ var _ = Describe("Options", func() { BatchMaxDuration: lo.ToPtr(10 * time.Second), BatchIdleDuration: lo.ToPtr(time.Second), FeatureGates: test.FeatureGates{ - SpotToSpotConsolidation: lo.ToPtr(true), + SpotToSpotConsolidation: lo.ToPtr(false), }, })) }) From 78de62c06fdb2ea8f6229b678473ced93b00bded Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 17:40:33 -0700 Subject: [PATCH 4/9] fix: more testing --- pkg/controllers/nodeclaim/disruption/drift.go | 4 ++-- .../nodeclaim/disruption/drift_test.go | 21 ------------------- .../nodeclaim/disruption/suite_test.go | 4 ++-- 3 files changed, 4 insertions(+), 25 deletions(-) diff --git a/pkg/controllers/nodeclaim/disruption/drift.go b/pkg/controllers/nodeclaim/disruption/drift.go index 3ea9a0b890..767d2e6f36 100644 --- a/pkg/controllers/nodeclaim/disruption/drift.go +++ b/pkg/controllers/nodeclaim/disruption/drift.go @@ -46,7 +46,7 @@ type Drift struct { func (d *Drift) Reconcile(ctx context.Context, nodePool *v1beta1.NodePool, nodeClaim *v1beta1.NodeClaim) (reconcile.Result, error) { hasDriftedCondition := nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted) != nil - // From here there are two scenarios to handle: + // From here there are three scenarios to handle: // 1. If NodeClaim is not launched, remove the drift status condition if !nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeLaunched).IsTrue() { _ = nodeClaim.StatusConditions().Clear(v1beta1.ConditionTypeDrifted) @@ -67,7 +67,7 @@ func (d *Drift) Reconcile(ctx context.Context, nodePool *v1beta1.NodePool, nodeC } return reconcile.Result{RequeueAfter: 5 * time.Minute}, nil } - // 4. Finally, if the NodeClaim is drifted, but doesn't have status condition, add it. + // 3. Finally, if the NodeClaim is drifted, but doesn't have status condition, add it. nodeClaim.StatusConditions().SetTrueWithReason(v1beta1.ConditionTypeDrifted, string(driftedReason), string(driftedReason)) if !hasDriftedCondition { log.FromContext(ctx).V(1).WithValues("reason", string(driftedReason)).Info("marking drifted") diff --git a/pkg/controllers/nodeclaim/disruption/drift_test.go b/pkg/controllers/nodeclaim/disruption/drift_test.go index a3a4d02f61..4fe63a8111 100644 --- a/pkg/controllers/nodeclaim/disruption/drift_test.go +++ b/pkg/controllers/nodeclaim/disruption/drift_test.go @@ -27,7 +27,6 @@ import ( "sigs.k8s.io/karpenter/pkg/apis/v1beta1" "sigs.k8s.io/karpenter/pkg/controllers/nodeclaim/disruption" "sigs.k8s.io/karpenter/pkg/controllers/nodepool/hash" - "sigs.k8s.io/karpenter/pkg/operator/options" . "sigs.k8s.io/karpenter/pkg/test/expectations" "sigs.k8s.io/karpenter/pkg/test" @@ -153,26 +152,6 @@ var _ = Describe("Drift", func() { Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).IsTrue()).To(BeTrue()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).Reason).To(Equal(string(disruption.RequirementsDrifted))) }) - It("should not detect drift if the feature flag is disabled", func() { - cp.Drifted = "drifted" - ctx = options.ToContext(ctx, test.Options()) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) - - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)).To(BeNil()) - }) - It("should remove the status condition from the nodeClaim if the feature flag is disabled", func() { - cp.Drifted = "drifted" - ctx = options.ToContext(ctx, test.Options()) - nodeClaim.StatusConditions().SetTrue(v1beta1.ConditionTypeDrifted) - ExpectApplied(ctx, env.Client, nodePool, nodeClaim) - - ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) - - nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)).To(BeNil()) - }) It("should remove the status condition from the nodeClaim when the nodeClaim launch condition is unknown", func() { cp.Drifted = "drifted" nodeClaim.StatusConditions().SetTrue(v1beta1.ConditionTypeDrifted) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index 7648b2bd26..88df434097 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -126,12 +126,12 @@ var _ = Describe("Disruption", func() { ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node) ExpectMakeNodeClaimsInitialized(ctx, env.Client, nodeClaim) - // Drift, Expiration, and Emptiness are disabled through configuration + // Expiration, and Emptiness are disabled through configuration ctx = options.ToContext(ctx, test.Options()) ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)).To(BeNil()) + Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)).To(BeTrue()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeEmpty)).To(BeNil()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeExpired)).To(BeNil()) }) From c10386b4e3e4a1e5045ca506cb799c83816427cf Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 18:18:00 -0700 Subject: [PATCH 5/9] fix: add logging to test --- pkg/controllers/nodeclaim/disruption/suite_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index 88df434097..56fc790473 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -18,6 +18,7 @@ package disruption_test import ( "context" + "fmt" "testing" "time" @@ -131,6 +132,7 @@ var _ = Describe("Disruption", func() { ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + fmt.Println("Testing nodeclaim: ", nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)).To(BeTrue()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeEmpty)).To(BeNil()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeExpired)).To(BeNil()) From 9cdc43ec1df009d8829abf0c9ca799cd7a2122d6 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 18:47:40 -0700 Subject: [PATCH 6/9] fix: status object --- pkg/controllers/nodeclaim/disruption/suite_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index 56fc790473..74f8fefca9 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -133,7 +133,7 @@ var _ = Describe("Disruption", func() { nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) fmt.Println("Testing nodeclaim: ", nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)) - Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).Status).To(BeTrue()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeEmpty)).To(BeNil()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeExpired)).To(BeNil()) }) From bc472e2405657705e6537a4b4667f3e2b5279346 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 19:08:38 -0700 Subject: [PATCH 7/9] fix: status get --- pkg/controllers/nodeclaim/disruption/suite_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index 74f8fefca9..f9ce5c4ac0 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -18,7 +18,6 @@ package disruption_test import ( "context" - "fmt" "testing" "time" @@ -132,8 +131,7 @@ var _ = Describe("Disruption", func() { ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - fmt.Println("Testing nodeclaim: ", nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)) - Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).Status).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).GetStatus()).To(BeTrue()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeEmpty)).To(BeNil()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeExpired)).To(BeNil()) }) From 947679e65e9aeed37129d667f3c403b40856c1a4 Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Sun, 9 Jun 2024 21:59:54 -0700 Subject: [PATCH 8/9] fix: test --- pkg/controllers/nodeclaim/disruption/suite_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index f9ce5c4ac0..0dc971acac 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -131,7 +131,7 @@ var _ = Describe("Disruption", func() { ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) - Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).GetStatus()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).IsTrue()).To(BeTrue()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeEmpty)).To(BeNil()) Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeExpired)).To(BeNil()) }) From 92e8d838768bc4f7f74c766ff65a76ac4172f15d Mon Sep 17 00:00:00 2001 From: Reed Schalo Date: Mon, 10 Jun 2024 13:59:18 -0700 Subject: [PATCH 9/9] PR response --- pkg/controllers/nodeclaim/disruption/suite_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/controllers/nodeclaim/disruption/suite_test.go b/pkg/controllers/nodeclaim/disruption/suite_test.go index 0dc971acac..97ef49ff92 100644 --- a/pkg/controllers/nodeclaim/disruption/suite_test.go +++ b/pkg/controllers/nodeclaim/disruption/suite_test.go @@ -125,9 +125,6 @@ var _ = Describe("Disruption", func() { ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node) ExpectMakeNodeClaimsInitialized(ctx, env.Client, nodeClaim) - - // Expiration, and Emptiness are disabled through configuration - ctx = options.ToContext(ctx, test.Options()) ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim) nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)