From d0055a20cea69f4a3ed7e7894b03ef1022247852 Mon Sep 17 00:00:00 2001 From: Jonathan Innis Date: Tue, 24 Jan 2023 12:58:43 -0800 Subject: [PATCH] Remove runtime resolution of configMap settings --- .../karpenter-core/templates/deployment.yaml | 7 +- pkg/apis/apis.go | 5 +- pkg/apis/config/registration.go | 41 ------ pkg/apis/settings/injectable.go | 27 ++++ pkg/apis/{config => }/settings/settings.go | 35 +++-- pkg/apis/{config => }/settings/suite_test.go | 24 ++-- pkg/controllers/controllers.go | 2 - pkg/controllers/deprovisioning/drift.go | 2 +- pkg/controllers/deprovisioning/suite_test.go | 2 +- pkg/controllers/inflightchecks/suite_test.go | 2 +- pkg/controllers/metrics/state/suite_test.go | 2 +- pkg/controllers/node/controller.go | 17 ++- pkg/controllers/node/drift.go | 5 - pkg/controllers/node/suite_test.go | 20 +-- pkg/controllers/provisioning/batcher.go | 2 +- .../scheduling/scheduling_benchmark_test.go | 2 +- .../provisioning/scheduling/suite_test.go | 2 +- pkg/controllers/provisioning/suite_test.go | 2 +- pkg/controllers/state/node.go | 2 +- pkg/controllers/state/suite_test.go | 2 +- pkg/operator/controller/injectsettings.go | 52 -------- .../controller/injectsettings_test.go | 126 ------------------ pkg/operator/controller/suite_test.go | 91 ++++++++++++- pkg/operator/controller/typed_test.go | 117 ---------------- .../fake/settings.go | 39 +++--- pkg/operator/injection/settings.go | 64 +++++++++ .../suite_test.go | 32 ++--- pkg/operator/operator.go | 16 +-- pkg/operator/options/options.go | 7 - pkg/operator/settingsstore/settingsstore.go | 98 -------------- pkg/test/expectations/expectations.go | 6 - pkg/test/settings.go | 20 +-- 32 files changed, 274 insertions(+), 597 deletions(-) delete mode 100644 pkg/apis/config/registration.go create mode 100644 pkg/apis/settings/injectable.go rename pkg/apis/{config => }/settings/settings.go (75%) rename pkg/apis/{config => }/settings/suite_test.go (79%) delete mode 100644 pkg/operator/controller/injectsettings.go delete mode 100644 pkg/operator/controller/injectsettings_test.go delete mode 100644 pkg/operator/controller/typed_test.go rename pkg/operator/{settingsstore => injection}/fake/settings.go (54%) create mode 100644 pkg/operator/injection/settings.go rename pkg/operator/{settingsstore => injection}/suite_test.go (79%) delete mode 100644 pkg/operator/settingsstore/settingsstore.go diff --git a/charts/karpenter-core/templates/deployment.yaml b/charts/karpenter-core/templates/deployment.yaml index 898f0ac24f..6a8d593dfe 100644 --- a/charts/karpenter-core/templates/deployment.yaml +++ b/charts/karpenter-core/templates/deployment.yaml @@ -5,10 +5,11 @@ metadata: namespace: {{ .Release.Namespace }} labels: {{- include "karpenter.labels" . | nindent 4 }} - {{- with .Values.additionalAnnotations }} annotations: - {{- toYaml . | nindent 4 }} - {{- end }} + {{- with .Values.additionalAnnotations }} + {{- toYaml . | nindent 6 }} + {{- end }} + checksum/settings: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }} spec: replicas: {{ .Values.replicas }} revisionHistoryLimit: {{ .Values.revisionHistoryLimit }} diff --git a/pkg/apis/apis.go b/pkg/apis/apis.go index 378835958b..d60fb37ed1 100644 --- a/pkg/apis/apis.go +++ b/pkg/apis/apis.go @@ -24,10 +24,9 @@ import ( "github.com/samber/lo" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/utils/functional" - "github.com/aws/karpenter-core/pkg/utils/sets" ) var ( @@ -41,7 +40,7 @@ var ( Resources = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{ v1alpha5.SchemeGroupVersion.WithKind("Provisioner"): &v1alpha5.Provisioner{}, } - Settings = sets.New(settings.Registration) + Settings = []settings.Injectable{&settings.Settings{}} ) //go:generate controller-gen crd object:headerFile="../../hack/boilerplate.go.txt" paths="./..." output:crd:artifacts:config=crds diff --git a/pkg/apis/config/registration.go b/pkg/apis/config/registration.go deleted file mode 100644 index b022e487ea..0000000000 --- a/pkg/apis/config/registration.go +++ /dev/null @@ -1,41 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package config - -import ( - "fmt" - - "knative.dev/pkg/configmap" -) - -// Constructor should take the form func(*v1.ConfigMap) (T, error) -type Constructor interface{} - -// Registration defines a ConfigMap registration to be watched by the settingsstore.Watcher -// and to be injected into the Reconcile() contexts of controllers -type Registration struct { - ConfigMapName string - Constructor interface{} -} - -func (r Registration) Validate() error { - if r.ConfigMapName == "" { - return fmt.Errorf("configMap cannot be empty in SettingsStore registration") - } - if err := configmap.ValidateConstructor(r.Constructor); err != nil { - return fmt.Errorf("constructor validation failed in SettingsStore registration, %w", err) - } - return nil -} diff --git a/pkg/apis/settings/injectable.go b/pkg/apis/settings/injectable.go new file mode 100644 index 0000000000..36457a136b --- /dev/null +++ b/pkg/apis/settings/injectable.go @@ -0,0 +1,27 @@ +/* +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 settings + +import ( + "context" + + v1 "k8s.io/api/core/v1" +) + +// Injectable defines a ConfigMap registration to be loaded into context on startup +type Injectable interface { + ConfigMap() string + Inject(context.Context, *v1.ConfigMap) (context.Context, error) +} diff --git a/pkg/apis/config/settings/settings.go b/pkg/apis/settings/settings.go similarity index 75% rename from pkg/apis/config/settings/settings.go rename to pkg/apis/settings/settings.go index 3cce499bac..45aab3af37 100644 --- a/pkg/apis/config/settings/settings.go +++ b/pkg/apis/settings/settings.go @@ -24,18 +24,13 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/configmap" - - "github.com/aws/karpenter-core/pkg/apis/config" ) -var ContextKey = Registration +type settingsKeyType struct{} -var Registration = &config.Registration{ - ConfigMapName: "karpenter-global-settings", - Constructor: NewSettingsFromConfigMap, -} +var ContextKey = settingsKeyType{} -var defaultSettings = Settings{ +var defaultSettings = &Settings{ BatchMaxDuration: metav1.Duration{Duration: time.Second * 10}, BatchIdleDuration: metav1.Duration{Duration: time.Second * 1}, DriftEnabled: false, @@ -48,8 +43,12 @@ type Settings struct { DriftEnabled bool } -// NewSettingsFromConfigMap creates a Settings from the supplied ConfigMap -func NewSettingsFromConfigMap(cm *v1.ConfigMap) (Settings, error) { +func (*Settings) ConfigMap() string { + return "karpenter-global-settings" +} + +// Inject creates a Settings from the supplied ConfigMap +func (*Settings) Inject(ctx context.Context, cm *v1.ConfigMap) (context.Context, error) { s := defaultSettings if err := configmap.Parse(cm.Data, @@ -57,14 +56,12 @@ func NewSettingsFromConfigMap(cm *v1.ConfigMap) (Settings, error) { AsMetaDuration("batchIdleDuration", &s.BatchIdleDuration), configmap.AsBool("featureGates.driftEnabled", &s.DriftEnabled), ); err != nil { - // Failing to parse means that there is some error in the Settings, so we should crash - panic(fmt.Sprintf("parsing settings, %v", err)) + return ctx, fmt.Errorf("parsing settings, %w", err) } if err := s.Validate(); err != nil { - // Failing to validate means that there is some error in the Settings, so we should crash - panic(fmt.Sprintf("validating settings, %v", err)) + return ctx, fmt.Errorf("validating settings, %w", err) } - return s, nil + return ToContext(ctx, s), nil } // Validate leverages struct tags with go-playground/validator so you can define a struct with custom @@ -73,7 +70,7 @@ func NewSettingsFromConfigMap(cm *v1.ConfigMap) (Settings, error) { // type ExampleStruct struct { // Example metav1.Duration `json:"example" validate:"required,min=10m"` // } -func (s Settings) Validate() (err error) { +func (s *Settings) Validate() (err error) { validate := validator.New() if s.BatchMaxDuration.Duration <= 0 { err = multierr.Append(err, fmt.Errorf("batchMaxDuration cannot be negative")) @@ -98,15 +95,15 @@ func AsMetaDuration(key string, target *metav1.Duration) configmap.ParseFunc { } } -func ToContext(ctx context.Context, s Settings) context.Context { +func ToContext(ctx context.Context, s *Settings) context.Context { return context.WithValue(ctx, ContextKey, s) } -func FromContext(ctx context.Context) Settings { +func FromContext(ctx context.Context) *Settings { data := ctx.Value(ContextKey) if data == nil { // This is developer error if this happens, so we should panic panic("settings doesn't exist in context") } - return data.(Settings) + return data.(*Settings) } diff --git a/pkg/apis/config/settings/suite_test.go b/pkg/apis/settings/suite_test.go similarity index 79% rename from pkg/apis/config/settings/suite_test.go rename to pkg/apis/settings/suite_test.go index 3267e3e39d..e52083f6fa 100644 --- a/pkg/apis/config/settings/suite_test.go +++ b/pkg/apis/settings/suite_test.go @@ -24,9 +24,7 @@ import ( v1 "k8s.io/api/core/v1" . "knative.dev/pkg/logging/testing" - . "github.com/aws/karpenter-core/pkg/test/expectations" - - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" ) var ctx context.Context @@ -42,7 +40,9 @@ var _ = Describe("Validation", func() { cm := &v1.ConfigMap{ Data: map[string]string{}, } - s, _ := settings.NewSettingsFromConfigMap(cm) + ctx, err := (&settings.Settings{}).Inject(ctx, cm) + Expect(err).ToNot(HaveOccurred()) + s := settings.FromContext(ctx) Expect(s.BatchMaxDuration.Duration).To(Equal(time.Second * 10)) Expect(s.BatchIdleDuration.Duration).To(Equal(time.Second)) Expect(s.DriftEnabled).To(BeFalse()) @@ -55,36 +55,38 @@ var _ = Describe("Validation", func() { "featureGates.driftEnabled": "true", }, } - s, _ := settings.NewSettingsFromConfigMap(cm) + ctx, err := (&settings.Settings{}).Inject(ctx, cm) + Expect(err).ToNot(HaveOccurred()) + s := settings.FromContext(ctx) Expect(s.BatchMaxDuration.Duration).To(Equal(time.Second * 30)) Expect(s.BatchIdleDuration.Duration).To(Equal(time.Second * 5)) Expect(s.DriftEnabled).To(BeTrue()) }) It("should fail validation with panic when batchMaxDuration is negative", func() { - defer ExpectPanic() cm := &v1.ConfigMap{ Data: map[string]string{ "batchMaxDuration": "-10s", }, } - _, _ = settings.NewSettingsFromConfigMap(cm) + _, err := (&settings.Settings{}).Inject(ctx, cm) + Expect(err).To(HaveOccurred()) }) It("should fail validation with panic when batchIdleDuration is negative", func() { - defer ExpectPanic() cm := &v1.ConfigMap{ Data: map[string]string{ "batchIdleDuration": "-1s", }, } - _, _ = settings.NewSettingsFromConfigMap(cm) + _, err := (&settings.Settings{}).Inject(ctx, cm) + Expect(err).To(HaveOccurred()) }) It("should fail validation with panic when driftEnabled is not a valid boolean value", func() { - defer ExpectPanic() cm := &v1.ConfigMap{ Data: map[string]string{ "featureGates.driftEnabled": "foobar", }, } - _, _ = settings.NewSettingsFromConfigMap(cm) + _, err := (&settings.Settings{}).Inject(ctx, cm) + Expect(err).To(HaveOccurred()) }) }) diff --git a/pkg/controllers/controllers.go b/pkg/controllers/controllers.go index d6b13d6775..20caa75eaa 100644 --- a/pkg/controllers/controllers.go +++ b/pkg/controllers/controllers.go @@ -36,7 +36,6 @@ import ( "github.com/aws/karpenter-core/pkg/events" "github.com/aws/karpenter-core/pkg/metrics" "github.com/aws/karpenter-core/pkg/operator/controller" - "github.com/aws/karpenter-core/pkg/operator/settingsstore" ) func init() { @@ -50,7 +49,6 @@ func NewControllers( kubernetesInterface kubernetes.Interface, cluster *state.Cluster, eventRecorder events.Recorder, - settingsStore settingsstore.Store, cloudProvider cloudprovider.CloudProvider, ) []controller.Controller { provisioner := provisioning.NewProvisioner(ctx, kubeClient, kubernetesInterface.CoreV1(), eventRecorder, cloudProvider, cluster) diff --git a/pkg/controllers/deprovisioning/drift.go b/pkg/controllers/deprovisioning/drift.go index 5b8818ff06..14bb38c0d5 100644 --- a/pkg/controllers/deprovisioning/drift.go +++ b/pkg/controllers/deprovisioning/drift.go @@ -23,7 +23,7 @@ import ( "knative.dev/pkg/logging" "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/controllers/provisioning" "github.com/aws/karpenter-core/pkg/controllers/state" diff --git a/pkg/controllers/deprovisioning/suite_test.go b/pkg/controllers/deprovisioning/suite_test.go index ff0e8da390..6e09460e63 100644 --- a/pkg/controllers/deprovisioning/suite_test.go +++ b/pkg/controllers/deprovisioning/suite_test.go @@ -38,7 +38,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/cloudprovider" "github.com/aws/karpenter-core/pkg/cloudprovider/fake" diff --git a/pkg/controllers/inflightchecks/suite_test.go b/pkg/controllers/inflightchecks/suite_test.go index bc8d229242..724751b177 100644 --- a/pkg/controllers/inflightchecks/suite_test.go +++ b/pkg/controllers/inflightchecks/suite_test.go @@ -31,7 +31,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/cloudprovider/fake" "github.com/aws/karpenter-core/pkg/controllers/inflightchecks" diff --git a/pkg/controllers/metrics/state/suite_test.go b/pkg/controllers/metrics/state/suite_test.go index 7b81d85125..ff6750028e 100644 --- a/pkg/controllers/metrics/state/suite_test.go +++ b/pkg/controllers/metrics/state/suite_test.go @@ -28,7 +28,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" metricsstate "github.com/aws/karpenter-core/pkg/controllers/metrics/state" "github.com/aws/karpenter-core/pkg/controllers/state/informer" diff --git a/pkg/controllers/node/controller.go b/pkg/controllers/node/controller.go index 777189b6c9..3a9e9ae57b 100644 --- a/pkg/controllers/node/controller.go +++ b/pkg/controllers/node/controller.go @@ -31,6 +31,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" corecontroller "github.com/aws/karpenter-core/pkg/operator/controller" @@ -39,6 +40,10 @@ import ( "github.com/aws/karpenter-core/pkg/utils/result" ) +type nodeReconciler interface { + Reconcile(context.Context, *v1alpha5.Provisioner, *v1.Node) (reconcile.Result, error) +} + var _ corecontroller.TypedController[*v1.Node] = (*Controller)(nil) // Controller manages a set of properties on karpenter provisioned nodes, such as @@ -85,14 +90,16 @@ func (c *Controller) Reconcile(ctx context.Context, node *v1.Node) (reconcile.Re // Execute Reconcilers var results []reconcile.Result var errs error - for _, reconciler := range []interface { - Reconcile(context.Context, *v1alpha5.Provisioner, *v1.Node) (reconcile.Result, error) - }{ + + reconcilers := []nodeReconciler{ c.initialization, c.emptiness, c.finalizer, - c.drift, - } { + } + if settings.FromContext(ctx).DriftEnabled { + reconcilers = append(reconcilers, c.drift) + } + for _, reconciler := range reconcilers { res, err := reconciler.Reconcile(ctx, provisioner, node) errs = multierr.Append(errs, err) results = append(results, res) diff --git a/pkg/controllers/node/drift.go b/pkg/controllers/node/drift.go index 8d0ee3dd8d..ce19347635 100644 --- a/pkg/controllers/node/drift.go +++ b/pkg/controllers/node/drift.go @@ -25,7 +25,6 @@ import ( "github.com/samber/lo" - "github.com/aws/karpenter-core/pkg/apis/config/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/cloudprovider" "github.com/aws/karpenter-core/pkg/utils/machine" @@ -37,10 +36,6 @@ type Drift struct { } func (d *Drift) Reconcile(ctx context.Context, provisioner *v1alpha5.Provisioner, node *v1.Node) (reconcile.Result, error) { - if !settings.FromContext(ctx).DriftEnabled { - return reconcile.Result{RequeueAfter: 5 * time.Minute}, nil - } - if _, ok := node.Annotations[v1alpha5.VoluntaryDisruptionAnnotationKey]; ok { return reconcile.Result{}, nil } diff --git a/pkg/controllers/node/suite_test.go b/pkg/controllers/node/suite_test.go index dce1ce52a4..b13b10463a 100644 --- a/pkg/controllers/node/suite_test.go +++ b/pkg/controllers/node/suite_test.go @@ -33,7 +33,7 @@ import ( . "knative.dev/pkg/logging/testing" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/cloudprovider/fake" "github.com/aws/karpenter-core/pkg/operator/controller" "github.com/aws/karpenter-core/pkg/operator/scheme" @@ -50,7 +50,6 @@ var ctx context.Context var nodeController controller.Controller var env *test.Environment var fakeClock *clock.FakeClock -var settingsStore test.SettingsStore var cp *fake.CloudProvider func TestAPIs(t *testing.T) { @@ -79,9 +78,7 @@ var _ = Describe("Controller", func() { ObjectMeta: metav1.ObjectMeta{Name: test.RandomName()}, Spec: v1alpha5.ProvisionerSpec{}, } - settingsStore = test.SettingsStore{ - settings.ContextKey: test.Settings(), - } + ctx = settings.ToContext(ctx, test.Settings(test.SettingsOptions{DriftEnabled: true})) }) AfterEach(func() { @@ -92,10 +89,7 @@ var _ = Describe("Controller", func() { Context("Drift", func() { It("should not detect drift if the feature flag is disabled", func() { cp.Drifted = true - settingsStore = test.SettingsStore{ - settings.ContextKey: test.Settings(test.SettingsOptions{DriftEnabled: false}), - } - ctx = settingsStore.InjectSettings(ctx) + ctx = settings.ToContext(ctx, test.Settings(test.SettingsOptions{DriftEnabled: false})) node := test.Node(test.NodeOptions{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -111,10 +105,6 @@ var _ = Describe("Controller", func() { }) It("should not detect drift if the provisioner does not exist", func() { cp.Drifted = true - settingsStore = test.SettingsStore{ - settings.ContextKey: test.Settings(test.SettingsOptions{DriftEnabled: true}), - } - ctx = settingsStore.InjectSettings(ctx) node := test.Node(test.NodeOptions{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -130,10 +120,6 @@ var _ = Describe("Controller", func() { }) It("should annotate the node when it has drifted in the cloud provider", func() { cp.Drifted = true - settingsStore = test.SettingsStore{ - settings.ContextKey: test.Settings(test.SettingsOptions{DriftEnabled: true}), - } - ctx = settingsStore.InjectSettings(ctx) node := test.Node(test.NodeOptions{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ diff --git a/pkg/controllers/provisioning/batcher.go b/pkg/controllers/provisioning/batcher.go index 48552772f9..9e32c4916e 100644 --- a/pkg/controllers/provisioning/batcher.go +++ b/pkg/controllers/provisioning/batcher.go @@ -18,7 +18,7 @@ import ( "context" "time" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" ) // Batcher separates a stream of Trigger() calls into windowed slices. The diff --git a/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go b/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go index 6e97ad73ea..8bb5b2eabb 100644 --- a/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go +++ b/pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go @@ -31,7 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/clock" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/cloudprovider" "github.com/aws/karpenter-core/pkg/cloudprovider/fake" "github.com/aws/karpenter-core/pkg/controllers/provisioning/scheduling" diff --git a/pkg/controllers/provisioning/scheduling/suite_test.go b/pkg/controllers/provisioning/scheduling/suite_test.go index 59df25f07b..6535e60833 100644 --- a/pkg/controllers/provisioning/scheduling/suite_test.go +++ b/pkg/controllers/provisioning/scheduling/suite_test.go @@ -32,7 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/cloudprovider" "github.com/aws/karpenter-core/pkg/cloudprovider/fake" diff --git a/pkg/controllers/provisioning/suite_test.go b/pkg/controllers/provisioning/suite_test.go index 35d52739f0..29860ad8bd 100644 --- a/pkg/controllers/provisioning/suite_test.go +++ b/pkg/controllers/provisioning/suite_test.go @@ -32,7 +32,7 @@ import ( clock "k8s.io/utils/clock/testing" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/cloudprovider" "github.com/aws/karpenter-core/pkg/cloudprovider/fake" diff --git a/pkg/controllers/state/node.go b/pkg/controllers/state/node.go index ed998225bd..4ba0196ec1 100644 --- a/pkg/controllers/state/node.go +++ b/pkg/controllers/state/node.go @@ -24,7 +24,7 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/scheduling" podutils "github.com/aws/karpenter-core/pkg/utils/pod" diff --git a/pkg/controllers/state/suite_test.go b/pkg/controllers/state/suite_test.go index bcc23df9fd..264660cec6 100644 --- a/pkg/controllers/state/suite_test.go +++ b/pkg/controllers/state/suite_test.go @@ -25,7 +25,7 @@ import ( "knative.dev/pkg/ptr" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" "github.com/aws/karpenter-core/pkg/apis/v1alpha5" "github.com/aws/karpenter-core/pkg/controllers/state/informer" "github.com/aws/karpenter-core/pkg/operator/controller" diff --git a/pkg/operator/controller/injectsettings.go b/pkg/operator/controller/injectsettings.go deleted file mode 100644 index 6cc09989d3..0000000000 --- a/pkg/operator/controller/injectsettings.go +++ /dev/null @@ -1,52 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "context" - - "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "github.com/aws/karpenter-core/pkg/operator/settingsstore" -) - -type injectSettingsDecorator struct { - controller Controller - settingsStore settingsstore.Store -} - -// InjectSettings wraps a Controller to inject the global settings config as an in-memory object into -// the Reconcile context. This allows pulling settings out of the context by deserialization with functions -// like settings.FromContext(ctx) -func InjectSettings(controller Controller, ss settingsstore.Store) Controller { - return &injectSettingsDecorator{ - controller: controller, - settingsStore: ss, - } -} - -func (sd *injectSettingsDecorator) Name() string { - return sd.controller.Name() -} - -func (sd *injectSettingsDecorator) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { - ctx = sd.settingsStore.InjectSettings(ctx) - return sd.controller.Reconcile(ctx, req) -} - -func (sd *injectSettingsDecorator) Builder(ctx context.Context, mgr manager.Manager) Builder { - return sd.controller.Builder(ctx, mgr) -} diff --git a/pkg/operator/controller/injectsettings_test.go b/pkg/operator/controller/injectsettings_test.go deleted file mode 100644 index b64fb8e366..0000000000 --- a/pkg/operator/controller/injectsettings_test.go +++ /dev/null @@ -1,126 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller_test - -import ( - "context" - "time" - - v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "knative.dev/pkg/system" - "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "github.com/aws/karpenter-core/pkg/apis/config/settings" - "github.com/aws/karpenter-core/pkg/operator/controller" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - - . "github.com/aws/karpenter-core/pkg/test/expectations" -) - -var _ = Describe("Inject Settings", func() { - BeforeEach(func() { - defaultConfigMap = &v1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "karpenter-global-settings", - Namespace: system.Namespace(), - }, - } - ExpectApplied(ctx, env.Client, defaultConfigMap) - }) - AfterEach(func() { - ExpectCleanedUp(ctx, env.Client) - }) - - It("should inject default settings into Reconcile loop", func() { - ExpectApplied(ctx, env.Client, defaultConfigMap.DeepCopy()) - expected := settings.Settings{ - BatchMaxDuration: metav1.Duration{Duration: time.Second * 10}, - BatchIdleDuration: metav1.Duration{Duration: time.Second * 1}, - } - - fakeController := &FakeController{ - ReconcileAssertions: []ReconcileAssertion{ - ExpectOperatorSettingsInjected(expected), - }, - } - c := controller.InjectSettings(fakeController, ss) - Eventually(func(g Gomega) { - innerCtx := GomegaWithContext(ctx, g) - _, err := c.Reconcile(innerCtx, reconcile.Request{}) - g.Expect(err).To(Succeed()) - }).Should(Succeed()) - }) - It("should inject custom settings into Reconcile loop", func() { - expected := settings.Settings{ - BatchMaxDuration: metav1.Duration{Duration: time.Second * 30}, - BatchIdleDuration: metav1.Duration{Duration: time.Second * 5}, - } - cm := defaultConfigMap.DeepCopy() - cm.Data = map[string]string{ - "batchMaxDuration": expected.BatchMaxDuration.Duration.String(), - "batchIdleDuration": expected.BatchIdleDuration.Duration.String(), - } - ExpectApplied(ctx, env.Client, cm) - - fakeController := &FakeController{ - ReconcileAssertions: []ReconcileAssertion{ - ExpectOperatorSettingsInjected(expected), - }, - } - c := controller.InjectSettings(fakeController, ss) - Eventually(func(g Gomega) { - innerCtx := GomegaWithContext(ctx, g) - _, err := c.Reconcile(innerCtx, reconcile.Request{}) - g.Expect(err).To(Succeed()) - }).Should(Succeed()) - }) -}) - -func ExpectSettingsMatch(g Gomega, a settings.Settings, b settings.Settings) { - g.Expect(a.BatchMaxDuration.Duration == b.BatchMaxDuration.Duration && - a.BatchIdleDuration.Duration == b.BatchIdleDuration.Duration).To(BeTrue()) -} - -func ExpectOperatorSettingsInjected(expected settings.Settings) ReconcileAssertion { - return func(ctx context.Context, _ reconcile.Request) { - settings := settings.FromContext(ctx) - ExpectSettingsMatch(GomegaFromContext(ctx), expected, settings) - } -} - -type ReconcileAssertion func(context.Context, reconcile.Request) - -type FakeController struct { - ReconcileAssertions []ReconcileAssertion -} - -func (c *FakeController) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { - for _, elem := range c.ReconcileAssertions { - elem(ctx, req) - } - return reconcile.Result{}, nil -} - -func (c *FakeController) Name() string { - return "" -} - -func (c *FakeController) Builder(_ context.Context, _ manager.Manager) controller.Builder { - return nil -} diff --git a/pkg/operator/controller/suite_test.go b/pkg/operator/controller/suite_test.go index 894f4465c4..3ad1423f9d 100644 --- a/pkg/operator/controller/suite_test.go +++ b/pkg/operator/controller/suite_test.go @@ -22,11 +22,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/configmap/informer" "knative.dev/pkg/system" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/aws/karpenter-core/pkg/apis" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/v1alpha5" + "github.com/aws/karpenter-core/pkg/operator/controller" "github.com/aws/karpenter-core/pkg/operator/scheme" - "github.com/aws/karpenter-core/pkg/operator/settingsstore" "github.com/aws/karpenter-core/pkg/test" . "github.com/onsi/ginkgo/v2" @@ -39,7 +42,6 @@ import ( var ctx context.Context var env *test.Environment var cmw *informer.InformedWatcher -var ss settingsstore.Store var defaultConfigMap *v1.ConfigMap func TestAPIs(t *testing.T) { @@ -58,10 +60,91 @@ var _ = BeforeSuite(func() { }, } ExpectApplied(ctx, env.Client, defaultConfigMap) - ss = settingsstore.NewWatcherOrDie(ctx, env.KubernetesInterface, cmw, settings.Registration) Expect(cmw.Start(env.Done)) }) var _ = AfterSuite(func() { Expect(env.Stop()).To(Succeed()) }) + +var _ = Describe("Typed", func() { + AfterEach(func() { + ExpectCleanedUp(ctx, env.Client) + }) + + It("should pass in expected node into reconcile", func() { + node := test.Node(test.NodeOptions{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1alpha5.ProvisionerNameLabelKey: "default", + }, + }, + }) + ExpectApplied(ctx, env.Client, node) + fakeController := &FakeTypedController[*v1.Node]{ + ReconcileAssertions: []TypedReconcileAssertion[*v1.Node]{ + func(ctx context.Context, n *v1.Node) { + Expect(n.Name).To(Equal(node.Name)) + Expect(n.Labels).To(HaveKeyWithValue(v1alpha5.ProvisionerNameLabelKey, "default")) + }, + }, + } + typedController := controller.Typed[*v1.Node](env.Client, fakeController) + ExpectReconcileSucceeded(ctx, typedController, client.ObjectKeyFromObject(node)) + }) + It("should call finalizer func when finalizing", func() { + node := test.Node(test.NodeOptions{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1alpha5.ProvisionerNameLabelKey: "default", + }, + Finalizers: []string{ + v1alpha5.TestingGroup + "/finalizer", + }, + }, + }) + ExpectApplied(ctx, env.Client, node) + Expect(env.Client.Delete(ctx, node)).To(Succeed()) + + called := false + fakeController := &FakeTypedController[*v1.Node]{ + FinalizeAssertions: []TypedReconcileAssertion[*v1.Node]{ + func(ctx context.Context, n *v1.Node) { + called = true + }, + }, + } + typedController := controller.Typed[*v1.Node](env.Client, fakeController) + ExpectReconcileSucceeded(ctx, typedController, client.ObjectKeyFromObject(node)) + Expect(called).To(BeTrue()) + }) +}) + +type TypedReconcileAssertion[T client.Object] func(context.Context, T) + +type FakeTypedController[T client.Object] struct { + ReconcileAssertions []TypedReconcileAssertion[T] + FinalizeAssertions []TypedReconcileAssertion[T] +} + +func (c *FakeTypedController[T]) Name() string { + return "" +} + +func (c *FakeTypedController[T]) Reconcile(ctx context.Context, obj T) (reconcile.Result, error) { + for _, elem := range c.ReconcileAssertions { + elem(ctx, obj) + } + return reconcile.Result{}, nil +} + +func (c *FakeTypedController[T]) Finalize(ctx context.Context, obj T) (reconcile.Result, error) { + for _, elem := range c.FinalizeAssertions { + elem(ctx, obj) + } + return reconcile.Result{}, nil +} + +func (c *FakeTypedController[T]) Builder(_ context.Context, _ manager.Manager) controller.Builder { + return nil +} diff --git a/pkg/operator/controller/typed_test.go b/pkg/operator/controller/typed_test.go deleted file mode 100644 index 79bf261312..0000000000 --- a/pkg/operator/controller/typed_test.go +++ /dev/null @@ -1,117 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller_test - -import ( - "context" - - v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/aws/karpenter-core/pkg/apis/v1alpha5" - "github.com/aws/karpenter-core/pkg/operator/controller" - "github.com/aws/karpenter-core/pkg/test" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - - . "github.com/aws/karpenter-core/pkg/test/expectations" -) - -var _ = Describe("Typed", func() { - AfterEach(func() { - ExpectCleanedUp(ctx, env.Client) - }) - - It("should pass in expected node into reconcile", func() { - node := test.Node(test.NodeOptions{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1alpha5.ProvisionerNameLabelKey: "default", - }, - }, - }) - ExpectApplied(ctx, env.Client, node) - fakeController := &FakeTypedController[*v1.Node]{ - ReconcileAssertions: []TypedReconcileAssertion[*v1.Node]{ - func(ctx context.Context, n *v1.Node) { - Expect(n.Name).To(Equal(node.Name)) - Expect(n.Labels).To(HaveKeyWithValue(v1alpha5.ProvisionerNameLabelKey, "default")) - }, - }, - } - typedController := controller.Typed[*v1.Node](env.Client, fakeController) - ExpectReconcileSucceeded(ctx, typedController, client.ObjectKeyFromObject(node)) - }) - It("should call finalizer func when finalizing", func() { - node := test.Node(test.NodeOptions{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - v1alpha5.ProvisionerNameLabelKey: "default", - }, - Finalizers: []string{ - v1alpha5.TestingGroup + "/finalizer", - }, - }, - }) - ExpectApplied(ctx, env.Client, node) - Expect(env.Client.Delete(ctx, node)).To(Succeed()) - - called := false - fakeController := &FakeTypedController[*v1.Node]{ - FinalizeAssertions: []TypedReconcileAssertion[*v1.Node]{ - func(ctx context.Context, n *v1.Node) { - called = true - }, - }, - } - typedController := controller.Typed[*v1.Node](env.Client, fakeController) - ExpectReconcileSucceeded(ctx, typedController, client.ObjectKeyFromObject(node)) - Expect(called).To(BeTrue()) - }) -}) - -type TypedReconcileAssertion[T client.Object] func(context.Context, T) - -type FakeTypedController[T client.Object] struct { - ReconcileAssertions []TypedReconcileAssertion[T] - FinalizeAssertions []TypedReconcileAssertion[T] -} - -func (c *FakeTypedController[T]) Name() string { - return "" -} - -func (c *FakeTypedController[T]) Reconcile(ctx context.Context, obj T) (reconcile.Result, error) { - for _, elem := range c.ReconcileAssertions { - elem(ctx, obj) - } - return reconcile.Result{}, nil -} - -func (c *FakeTypedController[T]) Finalize(ctx context.Context, obj T) (reconcile.Result, error) { - for _, elem := range c.FinalizeAssertions { - elem(ctx, obj) - } - return reconcile.Result{}, nil -} - -func (c *FakeTypedController[T]) Builder(_ context.Context, _ manager.Manager) controller.Builder { - return nil -} diff --git a/pkg/operator/settingsstore/fake/settings.go b/pkg/operator/injection/fake/settings.go similarity index 54% rename from pkg/operator/settingsstore/fake/settings.go rename to pkg/operator/injection/fake/settings.go index ea33a3bf9b..b8a3ae1c60 100644 --- a/pkg/operator/settingsstore/fake/settings.go +++ b/pkg/operator/injection/fake/settings.go @@ -16,22 +16,17 @@ package fake import ( "context" - "encoding/json" "fmt" - "github.com/samber/lo" v1 "k8s.io/api/core/v1" "knative.dev/pkg/configmap" - - "github.com/aws/karpenter-core/pkg/apis/config" ) -var SettingsRegistration = &config.Registration{ - ConfigMapName: "karpenter-global-settings", - Constructor: NewFakeSettingsFromConfigMap, -} +type settingsKeyType struct{} -var defaultSettings = Settings{ +var ContextKey = settingsKeyType{} + +var defaultSettings = &Settings{ TestArg: "default", } @@ -39,32 +34,30 @@ type Settings struct { TestArg string `json:"testArg"` } -func (s Settings) Data() (map[string]string, error) { - d := map[string]string{} - - if err := json.Unmarshal(lo.Must(json.Marshal(defaultSettings)), &d); err != nil { - return d, err - } - return d, nil +func (*Settings) ConfigMap() string { + return "karpenter-global-settings" } -func NewFakeSettingsFromConfigMap(cm *v1.ConfigMap) (Settings, error) { +func (*Settings) Inject(ctx context.Context, cm *v1.ConfigMap) (context.Context, error) { s := defaultSettings if err := configmap.Parse(cm.Data, configmap.AsString("testArg", &s.TestArg), ); err != nil { - // Failing to parse means that there is some error in the Settings, so we should crash - panic(fmt.Sprintf("parsing config data, %v", err)) + return ctx, fmt.Errorf("parsing config data, %w", err) } - return s, nil + return ToContext(ctx, s), nil +} + +func ToContext(ctx context.Context, s *Settings) context.Context { + return context.WithValue(ctx, ContextKey, s) } -func SettingsFromContext(ctx context.Context) Settings { - data := ctx.Value(SettingsRegistration) +func FromContext(ctx context.Context) *Settings { + data := ctx.Value(ContextKey) if data == nil { // This is developer error if this happens, so we should panic panic("settings doesn't exist in context") } - return data.(Settings) + return data.(*Settings) } diff --git a/pkg/operator/injection/settings.go b/pkg/operator/injection/settings.go new file mode 100644 index 0000000000..c3451a8c54 --- /dev/null +++ b/pkg/operator/injection/settings.go @@ -0,0 +1,64 @@ +/* +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 injection + +import ( + "context" + "fmt" + "time" + + "github.com/samber/lo" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/cache" + "knative.dev/pkg/logging" + "knative.dev/pkg/system" + + "github.com/aws/karpenter-core/pkg/apis/settings" +) + +// WithSettingsOrDie injects the settings into the context for all configMaps passed through the registrations +func WithSettingsOrDie(ctx context.Context, kubernetesInterface kubernetes.Interface, settings ...settings.Injectable) context.Context { + cancelCtx, cancel := context.WithCancel(ctx) + defer cancel() + + logging.FromContext(ctx).Debugf("waiting for configmaps") + factory := informers.NewSharedInformerFactoryWithOptions(kubernetesInterface, time.Second*30, informers.WithNamespace(system.Namespace())) + informer := factory.Core().V1().ConfigMaps().Informer() + factory.Start(cancelCtx.Done()) + + for _, setting := range settings { + cm := lo.Must(waitForConfigMap(ctx, setting.ConfigMap(), informer)) + ctx = lo.Must(setting.Inject(ctx, cm)) + } + return ctx +} + +// waitForConfigMap waits until all registered configMaps in the settingsStore are created +func waitForConfigMap(ctx context.Context, name string, informer cache.SharedIndexInformer) (*v1.ConfigMap, error) { + for { + configMap, exists, err := informer.GetStore().GetByKey(types.NamespacedName{Namespace: system.Namespace(), Name: name}.String()) + if configMap != nil && exists && err == nil { + return configMap.(*v1.ConfigMap), nil + } + select { + case <-ctx.Done(): + return nil, fmt.Errorf("context canceled") + case <-time.After(time.Millisecond * 500): + } + } +} diff --git a/pkg/operator/settingsstore/suite_test.go b/pkg/operator/injection/suite_test.go similarity index 79% rename from pkg/operator/settingsstore/suite_test.go rename to pkg/operator/injection/suite_test.go index ff583338fd..47d4e93750 100644 --- a/pkg/operator/settingsstore/suite_test.go +++ b/pkg/operator/injection/suite_test.go @@ -12,7 +12,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package settingsstore_test +package injection_test import ( "context" @@ -21,13 +21,12 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "knative.dev/pkg/configmap/informer" "knative.dev/pkg/system" - "github.com/aws/karpenter-core/pkg/apis/config/settings" + "github.com/aws/karpenter-core/pkg/apis/settings" + "github.com/aws/karpenter-core/pkg/operator/injection" + "github.com/aws/karpenter-core/pkg/operator/injection/fake" "github.com/aws/karpenter-core/pkg/operator/scheme" - "github.com/aws/karpenter-core/pkg/operator/settingsstore" - "github.com/aws/karpenter-core/pkg/operator/settingsstore/fake" "github.com/aws/karpenter-core/pkg/test" . "github.com/onsi/ginkgo/v2" @@ -39,19 +38,16 @@ import ( var ctx context.Context var env *test.Environment -var cmw *informer.InformedWatcher -var ss settingsstore.Store var defaultConfigMap *v1.ConfigMap func TestAPIs(t *testing.T) { ctx = TestContextWithLogger(t) RegisterFailHandler(Fail) - RunSpecs(t, "SettingsStore") + RunSpecs(t, "Injection") } var _ = BeforeSuite(func() { env = test.NewEnvironment(scheme.Scheme) - cmw = informer.NewInformedWatcher(env.KubernetesInterface, system.Namespace()) defaultConfigMap = &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "karpenter-global-settings", @@ -59,8 +55,6 @@ var _ = BeforeSuite(func() { }, } ExpectApplied(ctx, env.Client, defaultConfigMap) - ss = settingsstore.NewWatcherOrDie(ctx, env.KubernetesInterface, cmw, settings.Registration, fake.SettingsRegistration) - Expect(cmw.Start(env.Done)).To(Succeed()) }) var _ = AfterSuite(func() { @@ -81,11 +75,11 @@ var _ = AfterEach(func() { ExpectDeleted(ctx, env.Client, defaultConfigMap.DeepCopy()) }) -var _ = Describe("SettingsStore", func() { +var _ = Describe("Settings", func() { Context("Operator Settings", func() { It("should have default values", func() { Eventually(func(g Gomega) { - testCtx := ss.InjectSettings(ctx) + testCtx := injection.WithSettingsOrDie(ctx, env.KubernetesInterface, &settings.Settings{}) s := settings.FromContext(testCtx) g.Expect(s.BatchIdleDuration.Duration).To(Equal(1 * time.Second)) g.Expect(s.BatchMaxDuration.Duration).To(Equal(10 * time.Second)) @@ -93,7 +87,7 @@ var _ = Describe("SettingsStore", func() { }) It("should update if values are changed", func() { Eventually(func(g Gomega) { - testCtx := ss.InjectSettings(ctx) + testCtx := injection.WithSettingsOrDie(ctx, env.KubernetesInterface, &settings.Settings{}) s := settings.FromContext(testCtx) g.Expect(s.BatchIdleDuration.Duration).To(Equal(1 * time.Second)) g.Expect(s.BatchMaxDuration.Duration).To(Equal(10 * time.Second)) @@ -106,7 +100,7 @@ var _ = Describe("SettingsStore", func() { ExpectApplied(ctx, env.Client, cm) Eventually(func(g Gomega) { - testCtx := ss.InjectSettings(ctx) + testCtx := injection.WithSettingsOrDie(ctx, env.KubernetesInterface, &settings.Settings{}) s := settings.FromContext(testCtx) g.Expect(s.BatchIdleDuration.Duration).To(Equal(2 * time.Second)) g.Expect(s.BatchMaxDuration.Duration).To(Equal(15 * time.Second)) @@ -116,8 +110,8 @@ var _ = Describe("SettingsStore", func() { Context("Multiple Settings", func() { It("should get operator settings and features from same configMap", func() { Eventually(func(g Gomega) { - testCtx := ss.InjectSettings(ctx) - s := fake.SettingsFromContext(testCtx) + testCtx := injection.WithSettingsOrDie(ctx, env.KubernetesInterface, &settings.Settings{}, &fake.Settings{}) + s := fake.FromContext(testCtx) g.Expect(s.TestArg).To(Equal("default")) }).Should(Succeed()) }) @@ -131,9 +125,9 @@ var _ = Describe("SettingsStore", func() { ExpectApplied(ctx, env.Client, cm) Eventually(func(g Gomega) { - testCtx := ss.InjectSettings(ctx) + testCtx := injection.WithSettingsOrDie(ctx, env.KubernetesInterface, &settings.Settings{}, &fake.Settings{}) s := settings.FromContext(testCtx) - fs := fake.SettingsFromContext(testCtx) + fs := fake.FromContext(testCtx) g.Expect(s.BatchIdleDuration.Duration).To(Equal(2 * time.Second)) g.Expect(s.BatchMaxDuration.Duration).To(Equal(15 * time.Second)) g.Expect(fs.TestArg).To(Equal("my-value")) diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 1396cadcfc..596669524c 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -46,7 +46,6 @@ import ( "github.com/aws/karpenter-core/pkg/operator/injection" "github.com/aws/karpenter-core/pkg/operator/options" "github.com/aws/karpenter-core/pkg/operator/scheme" - "github.com/aws/karpenter-core/pkg/operator/settingsstore" ) const ( @@ -59,7 +58,6 @@ type Operator struct { RESTConfig *rest.Config KubernetesInterface kubernetes.Interface - SettingsStore settingsstore.Store EventRecorder events.Recorder Clock clock.Clock @@ -95,17 +93,14 @@ func NewOperator() (context.Context, *Operator) { // Client kubernetesInterface := kubernetes.NewForConfigOrDie(config) configMapWatcher := informer.NewInformedWatcher(kubernetesInterface, system.Namespace()) + lo.Must0(configMapWatcher.Start(ctx.Done())) // Logging logger := NewLogger(ctx, component, config, configMapWatcher) ctx = logging.WithLogger(ctx, logger) - // Create the settingsStore for settings injection - settingsStore := settingsstore.NewWatcherOrDie(ctx, kubernetesInterface, configMapWatcher, apis.Settings.List()...) - - // Inject settings after starting the ConfigMapWatcher - lo.Must0(configMapWatcher.Start(ctx.Done())) - ctx = settingsStore.InjectSettings(ctx) + // Inject settings from the ConfigMap(s) into the context + ctx = injection.WithSettingsOrDie(ctx, kubernetesInterface, apis.Settings...) // Manager manager, err := controllerruntime.NewManager(config, controllerruntime.Options{ @@ -119,6 +114,7 @@ func NewOperator() (context.Context, *Operator) { BaseContext: func() context.Context { ctx := context.Background() ctx = logging.WithLogger(ctx, logger) + ctx = injection.WithSettingsOrDie(ctx, kubernetesInterface, apis.Settings...) ctx = injection.WithConfig(ctx, config) ctx = injection.WithOptions(ctx, *opts) return ctx @@ -136,7 +132,6 @@ func NewOperator() (context.Context, *Operator) { Manager: manager, RESTConfig: config, KubernetesInterface: kubernetesInterface, - SettingsStore: settingsStore, EventRecorder: events.NewRecorder(manager.GetEventRecorderFor(appName)), Clock: clock.RealClock{}, } @@ -144,9 +139,6 @@ func NewOperator() (context.Context, *Operator) { func (o *Operator) WithControllers(ctx context.Context, controllers ...corecontroller.Controller) *Operator { for _, c := range controllers { - // Wrap the controllers with any decorators - c = corecontroller.InjectSettings(c, o.SettingsStore) - lo.Must0(c.Builder(ctx, o.Manager).Complete(c), "failed to register controller") } lo.Must0(o.AddHealthzCheck("healthz", healthz.Ping), "failed to setup liveness probe") diff --git a/pkg/operator/options/options.go b/pkg/operator/options/options.go index 0521f98fd4..3983f5c9bd 100644 --- a/pkg/operator/options/options.go +++ b/pkg/operator/options/options.go @@ -23,13 +23,6 @@ import ( "github.com/aws/karpenter-core/pkg/utils/env" ) -type AWSNodeNameConvention string - -const ( - IPName AWSNodeNameConvention = "ip-name" - ResourceName AWSNodeNameConvention = "resource-name" -) - // Options for running this binary type Options struct { *flag.FlagSet diff --git a/pkg/operator/settingsstore/settingsstore.go b/pkg/operator/settingsstore/settingsstore.go deleted file mode 100644 index dbd7babb6e..0000000000 --- a/pkg/operator/settingsstore/settingsstore.go +++ /dev/null @@ -1,98 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package settingsstore - -import ( - "context" - "fmt" - "time" - - "github.com/samber/lo" - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes" - "knative.dev/pkg/configmap" - "knative.dev/pkg/configmap/informer" - "knative.dev/pkg/logging" - - "github.com/aws/karpenter-core/pkg/apis/config" -) - -type Store interface { - InjectSettings(context.Context) context.Context -} - -type store struct { - registrations []*config.Registration - // We store multiple untyped stores, so we can watch the same ConfigMap but convert it in different ways - // For instance, we have karpenter-global-settings that converts into a cloudprovider-specific config and a global config - stores map[*config.Registration]*configmap.UntypedStore -} - -// NewWatcherOrDie creates the settings store watchers to watch for configMap updates to any settings store in registrations -// Before returning, it waits for all ConfigMaps passed through registration to be created -func NewWatcherOrDie(ctx context.Context, kubernetesInterface kubernetes.Interface, cmw *informer.InformedWatcher, registrations ...*config.Registration) Store { - ss := &store{ - registrations: registrations, - stores: map[*config.Registration]*configmap.UntypedStore{}, - } - for _, registration := range registrations { - if err := registration.Validate(); err != nil { - panic(fmt.Sprintf("Validating settings registration, %v", err)) - } - ss.stores[registration] = configmap.NewUntypedStore( - registration.ConfigMapName, - logging.FromContext(ctx), - configmap.Constructors{ - registration.ConfigMapName: registration.Constructor, - }, - ) - ss.stores[registration].WatchConfigs(cmw) - } - // Waits for all the ConfigMaps to be created before we continue onto the - ss.waitForConfigMapsOrDie(ctx, kubernetesInterface, cmw) - return ss -} - -// waitForConfigMapsOrDie waits until all registered configMaps in the settingsStore are created -func (s *store) waitForConfigMapsOrDie(ctx context.Context, kubernetesInterface kubernetes.Interface, configMapWatcher *informer.InformedWatcher) { - ctx, cancel := context.WithCancel(ctx) - defer cancel() - - factory := informers.NewSharedInformerFactoryWithOptions(kubernetesInterface, time.Second*30, informers.WithNamespace(configMapWatcher.Namespace)) - informer := factory.Core().V1().ConfigMaps().Informer() - factory.Start(ctx.Done()) - expected := sets.NewString(lo.Map(s.registrations, func(r *config.Registration, _ int) string { return r.ConfigMapName })...) - logging.FromContext(ctx).With("configmaps", expected.List()).Debugf("waiting for configmaps") - for { - if sets.NewString( - lo.Map(informer.GetStore().List(), func(obj interface{}, _ int) string { return obj.(*v1.ConfigMap).Name })..., - ).IsSuperset(expected) { - break - } - select { - case <-ctx.Done(): - return - case <-time.After(time.Millisecond * 500): - } - } -} - -func (s *store) InjectSettings(ctx context.Context) context.Context { - return lo.Reduce(s.registrations, func(c context.Context, registration *config.Registration, _ int) context.Context { - return context.WithValue(c, registration, s.stores[registration].UntypedLoad(registration.ConfigMapName)) - }, ctx) -} diff --git a/pkg/test/expectations/expectations.go b/pkg/test/expectations/expectations.go index 17fe82fcb8..4794771782 100644 --- a/pkg/test/expectations/expectations.go +++ b/pkg/test/expectations/expectations.go @@ -366,12 +366,6 @@ func ExpectResources(expected, real v1.ResourceList) { } } -// ExpectPanic is a function that should be deferred at the beginning of a test like "defer ExpectPanic()" -// It asserts that the test should panic -func ExpectPanic() { - ExpectWithOffset(1, recover()).ToNot(BeNil()) -} - type gomegaKeyType struct{} var gomegaKey = gomegaKeyType{} diff --git a/pkg/test/settings.go b/pkg/test/settings.go index b14c719f4c..dc04cd0da2 100644 --- a/pkg/test/settings.go +++ b/pkg/test/settings.go @@ -15,41 +15,27 @@ limitations under the License. package test import ( - "context" "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/imdario/mergo" - "github.com/aws/karpenter-core/pkg/apis/config/settings" - "github.com/aws/karpenter-core/pkg/operator/settingsstore" + "github.com/aws/karpenter-core/pkg/apis/settings" ) -var _ settingsstore.Store = SettingsStore{} - -// SettingsStore is a map from ContextKey to settings/config data -type SettingsStore map[interface{}]interface{} - -func (ss SettingsStore) InjectSettings(ctx context.Context) context.Context { - for k, v := range ss { - ctx = context.WithValue(ctx, k, v) - } - return ctx -} - type SettingsOptions struct { DriftEnabled bool } -func Settings(overrides ...SettingsOptions) settings.Settings { +func Settings(overrides ...SettingsOptions) *settings.Settings { options := SettingsOptions{} for _, opts := range overrides { if err := mergo.Merge(&options, opts, mergo.WithOverride); err != nil { panic(fmt.Sprintf("Failed to merge pod options: %s", err)) } } - return settings.Settings{ + return &settings.Settings{ BatchMaxDuration: metav1.Duration{}, BatchIdleDuration: metav1.Duration{}, DriftEnabled: options.DriftEnabled,