From 4b7215c5a34232fa5fc93b309b18a3bbad59a64d Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Fri, 30 Oct 2026 18:44:08 +0100 Subject: [PATCH 1/5] Read operator settings from env into typed options structs Kubernetes uses the environment as its configuration interface. Keep types, defaults, and domain validation in the owning modules so new fields need no command registry or central field-default edits. Decode the squashed aggregate once with generic map, list, and text hooks. Plain strings and explicit empty values keep their meaning. Use ordinary integer maps instead of a custom trait parser. Native environment maps use JSON. Nonempty TRAIT=N lists are unsupported and must migrate to JSON objects with double-quoted keys. Empty environment text clears a map. Validate effective domain limits before Kubernetes setup. Keep optional CLI registration and descriptions in the later commit. Signed-off-by: Fabian Wiesel --- cmd/config.go | 89 +++++++++++++++++ cmd/main.go | 96 ++++--------------- go.mod | 6 ++ go.sum | 12 +++ internal/config/options.go | 55 +++++++++++ internal/controller/eviction/controller.go | 7 +- .../controller/eviction/controller_test.go | 25 +++-- internal/controller/eviction/options.go | 73 ++++++++++++++ internal/controller/hypervisor_controller.go | 15 +-- .../controller/hypervisor_controller_test.go | 5 +- .../controller/node_certificate_controller.go | 6 +- internal/controller/options.go | 46 +++++++++ internal/global/global.go | 41 -------- internal/global/global_test.go | 38 ++++---- 14 files changed, 350 insertions(+), 164 deletions(-) create mode 100644 cmd/config.go create mode 100644 internal/config/options.go create mode 100644 internal/controller/eviction/options.go create mode 100644 internal/controller/options.go diff --git a/cmd/config.go b/cmd/config.go new file mode 100644 index 00000000..e278a991 --- /dev/null +++ b/cmd/config.go @@ -0,0 +1,89 @@ +/* +SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.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 main + +import ( + "encoding/json" + "reflect" + "strings" + + "github.com/go-viper/mapstructure/v2" + "github.com/knadh/koanf/providers/env" + "github.com/knadh/koanf/v2" + + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/config" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/eviction" +) + +type options struct { + config.Shared `koanf:",squash"` + Certificate controller.CertificateOptions `koanf:",squash"` + Eviction eviction.Options `koanf:",squash"` +} + +// loadOptions reads the operator settings from the environment, starting from +// the built-in defaults for settings whose env var is not set. Each module +// owns its options struct and defaults. A new tagged field needs no loader +// changes or flag registration. +func loadOptions() (options, error) { + cfg := options{ + Shared: config.DefaultOptions(), + Certificate: controller.DefaultCertificateOptions(), + Eviction: eviction.DefaultOptions(), + } + k := koanf.New(".") + + // Environment variables use the same names as before, lowercased as keys. + if err := k.Load(env.ProviderWithValue("", ".", func(name, value string) (string, any) { + return strings.ToLower(name), value + }), nil); err != nil { + return cfg, err + } + + // Decode lists and JSON maps by target type. Keep text unmarshaling for + // other types. Plain strings, including label selectors, stay unchanged. + decoderConfig := &mapstructure.DecoderConfig{ + DecodeHook: mapstructure.ComposeDecodeHookFunc( + mapstructure.StringToSliceHookFunc(","), + stringToMapJSONHook, + mapstructure.TextUnmarshallerHookFunc(), + ), + WeaklyTypedInput: true, + } + + err := k.UnmarshalWithConf("", &cfg, koanf.UnmarshalConf{ + DecoderConfig: decoderConfig, + }) + return cfg, err +} + +// stringToMapJSONHook decodes JSON into a fresh map of the target type. +// An empty environment value clears the map. Errors do not change the target. +func stringToMapJSONHook(from, to reflect.Type, data any) (any, error) { + if from.Kind() != reflect.String || to.Kind() != reflect.Map { + return data, nil + } + result := reflect.New(to) + if text := data.(string); text != "" { + if err := json.Unmarshal([]byte(text), result.Interface()); err != nil { + return nil, err + } + } + return result.Elem().Interface(), nil +} diff --git a/cmd/main.go b/cmd/main.go index 41982a56..f906d487 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -20,13 +20,10 @@ package main import ( "crypto/sha256" "crypto/tls" - "errors" "flag" "fmt" "os" gruntime "runtime" - "strconv" - "strings" "github.com/sapcc/go-api-declarations/bininfo" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) @@ -55,7 +52,6 @@ import ( "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/eviction" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/ready" - "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/logger" // +kubebuilder:scaffold:imports @@ -85,8 +81,6 @@ func main() { var secureMetrics bool var enableHTTP2 bool var tlsOpts []func(*tls.Config) - var certificateNamespace string - var certificateIssuerName string flag.BoolVar(&version, "version", false, "Print the version and exit.") @@ -100,23 +94,12 @@ func main() { "If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.") flag.BoolVar(&enableHTTP2, "enable-http2", false, "If set, HTTP/2 will be enabled for the metrics and webhook servers") - flag.StringVar(&global.LabelSelector, "label-selector", "", "Label selector to filter watched resources (namely nodes).") - var agentNamespacesFlag string - flag.StringVar(&agentNamespacesFlag, "agent-namespaces", "", - "Comma-separated list of namespaces to search for agent pods (nova-compute, neutron) during offboarding.") - - flag.IntVar(&global.EvictionConcurrency, "eviction-concurrency", 1, - "Default maximum number of VM migrations to run concurrently while draining a hypervisor. Defaults to 1 (serial).") - - var evictionTraitConcurrencyFlag string - flag.StringVar(&evictionTraitConcurrencyFlag, "eviction-trait-concurrency", "", - "Comma-separated TRAIT=N overrides for per-host migration concurrency, e.g. "+ - "CUSTOM_HANA_EXCLUSIVE_HOST=1,CUSTOM_FOO=2. The lowest matching trait limit wins.") - - flag.StringVar(&certificateNamespace, "certificate-namespace", "monsoon3", "The namespace for the certificates. ") - flag.StringVar(&certificateIssuerName, "certificate-issuer-name", "nova-hypervisor-agents-ca-issuer", - "Name of the certificate issuer.") + cfg, err := loadOptions() + if err != nil { + fmt.Fprintf(os.Stderr, "invalid configuration: %v\n", err) + os.Exit(1) + } opts := ctrlzap.Options{ Development: true, @@ -137,62 +120,23 @@ func main() { os.Exit(0) } - if agentNamespacesFlag != "" { - // Deduplicate. Without this, repeated entries (e.g. from templated - // values) would cause redundant pod-list calls and inflate the - // AgentPodsEvicted condition message with duplicate pod names. - seen := map[string]struct{}{} - for ns := range strings.SplitSeq(agentNamespacesFlag, ",") { - ns = strings.TrimSpace(ns) - if ns == "" { - continue - } - if _, ok := seen[ns]; ok { - continue - } - seen[ns] = struct{}{} - global.AgentNamespaces = append(global.AgentNamespaces, ns) - } - } - - if len(global.AgentNamespaces) == 0 { - setupLog.Error(errors.New("--agent-namespaces is required"), "invalid configuration") - os.Exit(1) - } + ctrl.SetLogger(ctrlzap.New(ctrlzap.UseFlagOptions(&opts))) - if global.EvictionConcurrency < 1 { - setupLog.Error(errors.New("--eviction-concurrency must be >= 1"), "invalid configuration") + if err := cfg.Validate(); err != nil { + setupLog.Error(err, "invalid configuration") os.Exit(1) } - for entry := range strings.SplitSeq(evictionTraitConcurrencyFlag, ",") { - entry = strings.TrimSpace(entry) - if entry == "" { - continue - } - trait, value, ok := strings.Cut(entry, "=") - trait = strings.TrimSpace(trait) - n, err := strconv.Atoi(strings.TrimSpace(value)) - if !ok || trait == "" || err != nil || n < 1 { - setupLog.Error(errors.New("invalid --eviction-trait-concurrency entry: "+entry), - "invalid configuration", "expected", "TRAIT=N with N >= 1") - os.Exit(1) - } - global.EvictionTraitConcurrency[trait] = n - } - - if certificateIssuerName == "" { - setupLog.Error(errors.New("certificate-issuer-name cannot be empty"), "invalid certificate issuer name") + if err := cfg.Eviction.Validate(); err != nil { + setupLog.Error(err, "invalid configuration") os.Exit(1) } - if certificateNamespace == "" { - setupLog.Error(errors.New("certificate-namespace cannot be empty"), "invalid certificate namespace") + if err := cfg.Certificate.Validate(); err != nil { + setupLog.Error(err, "invalid configuration") os.Exit(1) } - ctrl.SetLogger(ctrlzap.New(ctrlzap.UseFlagOptions(&opts))) - // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will // prevent from being vulnerable to the HTTP/2 Stream Cancellation and @@ -260,9 +204,9 @@ func main() { &policyv1.PodDisruptionBudget{}: maintenanceCacheConfig, }, } - if global.LabelSelector != "" { - setupLog.Info("setting up cache with label selector", "selector", global.LabelSelector) - selector, err := labels.Parse(global.LabelSelector) + if cfg.LabelSelector != "" { + setupLog.Info("setting up cache with label selector", "selector", cfg.LabelSelector) + selector, err := labels.Parse(cfg.LabelSelector) if err != nil { setupLog.Error(err, "unable to parse label selector") os.Exit(1) @@ -273,7 +217,7 @@ func main() { h := sha256.New() h.Write([]byte(leaderElectionID)) // Seed it with something "unique" to the project - h.Write([]byte(global.LabelSelector)) + h.Write([]byte(cfg.LabelSelector)) leaderElectionID = fmt.Sprintf("%x.cloud.sap", h.Sum(nil)) } @@ -315,7 +259,7 @@ func main() { if err = (&controller.HypervisorController{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), - }).SetupWithManager(mgr); err != nil { + }).SetupWithManager(mgr, cfg.Shared); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Hypervisor") os.Exit(1) } @@ -331,7 +275,7 @@ func main() { if err = (&eviction.EvictionReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), - }).SetupWithManager(mgr); err != nil { + }).SetupWithManager(mgr, cfg.Eviction); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Eviction") os.Exit(1) } @@ -355,7 +299,7 @@ func main() { if err = (&controller.NodeCertificateController{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), - }).SetupWithManager(mgr, certificateNamespace, certificateIssuerName); err != nil { + }).SetupWithManager(mgr, cfg.Certificate); err != nil { setupLog.Error(err, "unable to create controller", "certificate", "NodeCertificate") os.Exit(1) } @@ -363,7 +307,7 @@ func main() { if err = (&controller.GardenerNodeLifecycleController{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), - }).SetupWithManager(mgr, certificateNamespace); err != nil { + }).SetupWithManager(mgr, cfg.Certificate.Namespace); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Maintenance") os.Exit(1) } diff --git a/go.mod b/go.mod index ef3922c4..dec88b51 100644 --- a/go.mod +++ b/go.mod @@ -8,8 +8,11 @@ go 1.27 require ( github.com/cert-manager/cert-manager v1.21.2 github.com/go-logr/logr v1.4.4 + github.com/go-viper/mapstructure/v2 v2.4.0 github.com/gophercloud/gophercloud/v2 v2.15.0 github.com/gophercloud/utils/v2 v2.0.0-20260922174841-b4759ea0529b + github.com/knadh/koanf/providers/env v1.1.0 + github.com/knadh/koanf/v2 v2.4.1 github.com/onsi/ginkgo/v2 v2.33.1 github.com/onsi/gomega v1.44.0 github.com/sapcc/go-api-declarations v1.25.1 @@ -64,6 +67,9 @@ require ( github.com/hashicorp/go-uuid v1.0.3 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/json-iterator/go v1.1.12 // indirect + github.com/knadh/koanf/maps v0.1.2 // indirect + github.com/mitchellh/copystructure v1.2.0 // indirect + github.com/mitchellh/reflectwalk v1.0.2 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect diff --git a/go.sum b/go.sum index b5f8236b..aa940088 100644 --- a/go.sum +++ b/go.sum @@ -80,6 +80,8 @@ github.com/go-openapi/testify/v2 v2.6.0 h1:5PKH2HE7YJ/LuRPQGvSxBRlFXNQhSetBLlGAg github.com/go-openapi/testify/v2 v2.6.0/go.mod h1:SgsVHtfooshd0tublTtJ50FPKhujf47YRqauXXOUxfw= github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI= github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8= +github.com/go-viper/mapstructure/v2 v2.4.0 h1:EBsztssimR/CONLSZZ04E8qAkxNYq4Qp9LvH92wZUgs= +github.com/go-viper/mapstructure/v2 v2.4.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM= github.com/goccy/go-yaml v1.18.0 h1:8W7wMFS12Pcas7KU+VVkaiCng+kG8QiFeFwzFb+rwuw= github.com/goccy/go-yaml v1.18.0/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= github.com/gofrs/uuid/v5 v5.5.1 h1:z1Ce19/JwNidXpy3tOQc3241lnJLKdKyq/xlNvlD4Ng= @@ -115,6 +117,12 @@ github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnr github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/klauspost/compress v1.20.1 h1:T7kKElXUMXrUJ2E9QhQhxFtcK5rPyLdsGZvdbLMPdiQ= github.com/klauspost/compress v1.20.1/go.mod h1:LUdAzn7YLVvxLpc7y3V1m40wESHTgc1422pwwBSKYuI= +github.com/knadh/koanf/maps v0.1.2 h1:RBfmAW5CnZT+PJ1CVc1QSJKf4Xu9kxfQgYVQSu8hpbo= +github.com/knadh/koanf/maps v0.1.2/go.mod h1:npD/QZY3V6ghQDdcQzl1W4ICNVTkohC8E73eI2xW4yI= +github.com/knadh/koanf/providers/env v1.1.0 h1:U2VXPY0f+CsNDkvdsG8GcsnK4ah85WwWyJgef9oQMSc= +github.com/knadh/koanf/providers/env v1.1.0/go.mod h1:QhHHHZ87h9JxJAn2czdEl6pdkNnDh/JS1Vtsyt65hTY= +github.com/knadh/koanf/v2 v2.4.1 h1:MK2JDkLy4UGzEUkW1CCCKQftd9+M+NX8hAZ2axbf/jo= +github.com/knadh/koanf/v2 v2.4.1/go.mod h1:gRb40VRAbd4iJMYYD5IxZ6hfuopFcXBpc9bbQpZwo28= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= @@ -125,6 +133,10 @@ github.com/maruel/natural v1.1.1 h1:Hja7XhhmvEFhcByqDoHz9QZbkWey+COd9xWfCfn1ioo= github.com/maruel/natural v1.1.1/go.mod h1:v+Rfd79xlw1AgVBjbO0BEQmptqb5HvL/k9GRHB7ZKEg= github.com/mfridman/tparse v0.18.0 h1:wh6dzOKaIwkUGyKgOntDW4liXSo37qg5AXbIhkMV3vE= github.com/mfridman/tparse v0.18.0/go.mod h1:gEvqZTuCgEhPbYk/2lS3Kcxg1GmTxxU7kTC8DvP0i/A= +github.com/mitchellh/copystructure v1.2.0 h1:vpKXTN4ewci03Vljg/q9QvCGUDttBOGBIa15WveJJGw= +github.com/mitchellh/copystructure v1.2.0/go.mod h1:qLl+cE2AmVv+CoeAwDPye/v+N2HKCj9FbZEVFJRxO9s= +github.com/mitchellh/reflectwalk v1.0.2 h1:G2LzWKi524PWgd3mLHV8Y5k7s6XUvT0Gef6zxSIeXaQ= +github.com/mitchellh/reflectwalk v1.0.2/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= diff --git a/internal/config/options.go b/internal/config/options.go new file mode 100644 index 00000000..05e96798 --- /dev/null +++ b/internal/config/options.go @@ -0,0 +1,55 @@ +/* +SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.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 ( + "errors" + "slices" + "strings" +) + +// Shared contains configuration options used by more than one controller. +type Shared struct { + LabelSelector string `koanf:"label_selector"` + AgentNamespaces []string `koanf:"agent_namespaces"` +} + +// DefaultOptions returns the shared controller defaults. +func DefaultOptions() Shared { + return Shared{} +} + +// Validate normalizes the agent namespace list and requires at least one +// namespace to remain. +func (s *Shared) Validate() error { + namespaces := make([]string, 0, len(s.AgentNamespaces)) + for _, namespace := range s.AgentNamespaces { + namespace = strings.TrimSpace(namespace) + if namespace == "" || slices.Contains(namespaces, namespace) { + continue + } + namespaces = append(namespaces, namespace) + } + s.AgentNamespaces = namespaces + + if len(namespaces) == 0 { + return errors.New("--agent-namespaces (AGENT_NAMESPACES) must contain at least one namespace") + } + + return nil +} diff --git a/internal/controller/eviction/controller.go b/internal/controller/eviction/controller.go index 8e80c15f..9d47042a 100644 --- a/internal/controller/eviction/controller.go +++ b/internal/controller/eviction/controller.go @@ -47,6 +47,7 @@ type EvictionReconciler struct { client.Client Scheme *runtime.Scheme computeClient *gophercloud.ServiceClient + Options Options } const ( @@ -173,7 +174,7 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1. // That should leave us with "Running" and the hypervisor should be deactivated if len(eviction.Status.OutstandingInstances) > 0 { - limit := global.ResolveConcurrency(hypervisor.Status.Traits) + limit := r.Options.ResolveConcurrency(hypervisor.Status.Traits) return r.evictNext(ctx, eviction, limit) } @@ -533,7 +534,9 @@ func (r *EvictionReconciler) registerWithManager(mgr ctrl.Manager) error { } // SetupWithManager sets up the controller with the Manager. -func (r *EvictionReconciler) SetupWithManager(mgr ctrl.Manager) error { +func (r *EvictionReconciler) SetupWithManager(mgr ctrl.Manager, options Options) error { + r.Options = options + ctx := context.Background() var err error diff --git a/internal/controller/eviction/controller_test.go b/internal/controller/eviction/controller_test.go index 4be3a0ef..ef905faa 100644 --- a/internal/controller/eviction/controller_test.go +++ b/internal/controller/eviction/controller_test.go @@ -33,7 +33,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" ) var _ = Describe("Instance slice helpers", func() { @@ -663,9 +662,9 @@ var _ = Describe("Eviction Controller", func() { } It("starts at most `limit` migrations per pass and never exceeds it in flight", func(ctx SpecContext) { - orig := global.EvictionConcurrency - global.EvictionConcurrency = 3 - DeferCleanup(func() { global.EvictionConcurrency = orig }) + orig := evictionReconciler.Options.Concurrency + evictionReconciler.Options.Concurrency = 3 + DeferCleanup(func() { evictionReconciler.Options.Concurrency = orig }) seedVMs(ctx, 5) resource := &kvmv1.Eviction{} @@ -704,9 +703,9 @@ var _ = Describe("Eviction Controller", func() { // with task_state "migrating" for a while before Status becomes // MIGRATING. If those aren't counted as in-flight, the loop keeps // triggering and exceeds the concurrency limit. - orig := global.EvictionConcurrency - global.EvictionConcurrency = 2 - DeferCleanup(func() { global.EvictionConcurrency = orig }) + orig := evictionReconciler.Options.Concurrency + evictionReconciler.Options.Concurrency = 2 + DeferCleanup(func() { evictionReconciler.Options.Concurrency = orig }) const tmpl = `{ "server": { @@ -798,13 +797,13 @@ var _ = Describe("Eviction Controller", func() { }) It("forces serial migration for hosts carrying the exclusive trait", func(ctx SpecContext) { - orig := global.EvictionConcurrency - origMap := global.EvictionTraitConcurrency - global.EvictionConcurrency = 4 - global.EvictionTraitConcurrency = map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1} + orig := evictionReconciler.Options.Concurrency + origMap := evictionReconciler.Options.TraitConcurrency + evictionReconciler.Options.Concurrency = 4 + evictionReconciler.Options.TraitConcurrency = TraitConcurrency{"CUSTOM_HANA_EXCLUSIVE_HOST": 1} DeferCleanup(func() { - global.EvictionConcurrency = orig - global.EvictionTraitConcurrency = origMap + evictionReconciler.Options.Concurrency = orig + evictionReconciler.Options.TraitConcurrency = origMap }) By("Marking the hypervisor with the exclusive trait") diff --git a/internal/controller/eviction/options.go b/internal/controller/eviction/options.go new file mode 100644 index 00000000..3d06b3a3 --- /dev/null +++ b/internal/controller/eviction/options.go @@ -0,0 +1,73 @@ +/* +SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.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 eviction + +import ( + "errors" + "fmt" + "strings" +) + +// Options configures the eviction controller. +type Options struct { + Concurrency int `koanf:"eviction_concurrency"` + TraitConcurrency map[string]int `koanf:"eviction_trait_concurrency"` +} + +// TraitConcurrency preserves the existing name for an ordinary integer map. +type TraitConcurrency = map[string]int + +// DefaultOptions returns the eviction controller defaults. +func DefaultOptions() Options { + return Options{Concurrency: 1} +} + +// ResolveConcurrency returns the maximum number of concurrent migrations +// for a host given its traits: the default, lowered by the lowest matching +// trait limit, and never below 1. +func (o Options) ResolveConcurrency(traits []string) int { + limit := o.Concurrency + for _, trait := range traits { + if traitLimit, ok := o.TraitConcurrency[trait]; ok && traitLimit < limit { + limit = traitLimit + } + } + return max(limit, 1) +} + +// Validate checks that the concurrency and every trait limit are >= 1 and +// that trait names are non-empty and carry no surrounding whitespace. +func (o Options) Validate() error { + if o.Concurrency < 1 { + return fmt.Errorf("--eviction-concurrency (EVICTION_CONCURRENCY) must be >= 1, got %d", o.Concurrency) + } + + for trait, limit := range o.TraitConcurrency { + if limit < 1 { + return fmt.Errorf("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait %q must have an integer limit >= 1, got %d", trait, limit) + } + if trait != strings.TrimSpace(trait) { + return fmt.Errorf("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait name %q must not contain surrounding whitespace", trait) + } + if trait == "" { + return errors.New("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait name must not be empty") + } + } + + return nil +} diff --git a/internal/controller/hypervisor_controller.go b/internal/controller/hypervisor_controller.go index 76685d90..3c79d8f2 100644 --- a/internal/controller/hypervisor_controller.go +++ b/internal/controller/hypervisor_controller.go @@ -41,7 +41,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/config" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) @@ -68,7 +68,8 @@ var transferLabels = append([]string{}, defaultTransferLabels...) type HypervisorController struct { k8sclient.Client - Scheme *runtime.Scheme + Scheme *runtime.Scheme + Options config.Shared } // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch @@ -213,7 +214,9 @@ func syncLabelsAndAnnotations(nodeLabels labels.Set, hypervisor *kvmv1.Hyperviso transportAggregatesAndTraits(&node.ObjectMeta, hypervisor) } -func (hv *HypervisorController) SetupWithManager(mgr ctrl.Manager) error { +func (hv *HypervisorController) SetupWithManager(mgr ctrl.Manager, options config.Shared) error { + hv.Options = options + novaVirtLabeledPredicate, err := predicate.LabelSelectorPredicate(metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { @@ -227,8 +230,8 @@ func (hv *HypervisorController) SetupWithManager(mgr ctrl.Manager) error { } // append the custom label selector from global config - if global.LabelSelector != "" { - requirements, err := labels.ParseToRequirements(global.LabelSelector) + if hv.Options.LabelSelector != "" { + requirements, err := labels.ParseToRequirements(hv.Options.LabelSelector) if err != nil { return fmt.Errorf("failed to parse global label selector: %w", err) } @@ -320,7 +323,7 @@ func (hv *HypervisorController) computeAgentPodsEvictedCondition(ctx context.Con } var agentPods []string - for _, ns := range global.AgentNamespaces { + for _, ns := range hv.Options.AgentNamespaces { var continueToken string for { pods := &corev1.PodList{} diff --git a/internal/controller/hypervisor_controller_test.go b/internal/controller/hypervisor_controller_test.go index c0f45899..46508b93 100644 --- a/internal/controller/hypervisor_controller_test.go +++ b/internal/controller/hypervisor_controller_test.go @@ -33,7 +33,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" ) // envtest does not actually GC pods when their namespace is deleted, so @@ -455,8 +454,8 @@ var _ = Describe("Hypervisor Controller", func() { }) // Restrict pod listing to the test namespace. - global.AgentNamespaces = []string{agentNamespace} - DeferCleanup(func() { global.AgentNamespaces = nil }) + hypervisorController.Options.AgentNamespaces = []string{agentNamespace} + DeferCleanup(func() { hypervisorController.Options.AgentNamespaces = nil }) // The condition is only computed during termination. _, err := hypervisorController.Reconcile(ctx, ctrl.Request{ diff --git a/internal/controller/node_certificate_controller.go b/internal/controller/node_certificate_controller.go index 8287c482..d1d68d69 100644 --- a/internal/controller/node_certificate_controller.go +++ b/internal/controller/node_certificate_controller.go @@ -156,9 +156,9 @@ func (r *NodeCertificateController) Reconcile(ctx context.Context, req ctrl.Requ } // SetupWithManager sets up the controller with the Manager. -func (r *NodeCertificateController) SetupWithManager(mgr ctrl.Manager, namespace, issuerName string) error { - r.namespace = namespace - r.issuerName = issuerName +func (r *NodeCertificateController) SetupWithManager(mgr ctrl.Manager, options CertificateOptions) error { + r.namespace = options.Namespace + r.issuerName = options.IssuerName novaVirtLabeledPredicate, err := predicate.LabelSelectorPredicate(metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ diff --git a/internal/controller/options.go b/internal/controller/options.go new file mode 100644 index 00000000..5bb54295 --- /dev/null +++ b/internal/controller/options.go @@ -0,0 +1,46 @@ +/* +SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.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 "errors" + +// CertificateOptions configures the node certificate controller. +type CertificateOptions struct { + Namespace string `koanf:"certificate_namespace"` + IssuerName string `koanf:"certificate_issuer_name"` +} + +// DefaultCertificateOptions returns the certificate controller defaults. +func DefaultCertificateOptions() CertificateOptions { + return CertificateOptions{ + Namespace: "monsoon3", + IssuerName: "nova-hypervisor-agents-ca-issuer", + } +} + +// Validate checks that the certificate controller has the required settings. +func (o CertificateOptions) Validate() error { + if o.Namespace == "" { + return errors.New("--certificate-namespace (CERTIFICATE_NAMESPACE) cannot be empty") + } + if o.IssuerName == "" { + return errors.New("--certificate-issuer-name (CERTIFICATE_ISSUER_NAME) cannot be empty") + } + + return nil +} diff --git a/internal/global/global.go b/internal/global/global.go index 9e40d29e..ec63c20b 100644 --- a/internal/global/global.go +++ b/internal/global/global.go @@ -30,44 +30,3 @@ const ( // poll cycle. ShortRetryTime = 1 * time.Second ) - -var ( - // LabelSelector is a custom label that is used to select resources managed by the operator. - LabelSelector = "" - - // AgentNamespaces is the list of namespaces in which agent pods (nova-compute, - // neutron) are scheduled. The pod list during offboarding is restricted to - // these namespaces. Must be non-empty; set via --agent-namespaces. - AgentNamespaces []string - - // EvictionConcurrency is the default maximum number of VM migrations that may - // run concurrently while draining a single hypervisor. Defaults to 1 (serial), - // which preserves the historical one-at-a-time behavior. Set via - // --eviction-concurrency. - EvictionConcurrency = 1 - - // EvictionTraitConcurrency maps a Placement trait name to a maximum number of - // concurrent migrations for hosts carrying that trait. It overrides - // EvictionConcurrency for matching hosts. Typical use is forcing exclusive host - // classes (e.g. CUSTOM_HANA_EXCLUSIVE_HOST) to migrate serially. Set via - // --eviction-trait-concurrency. - EvictionTraitConcurrency = map[string]int{} -) - -// ResolveConcurrency returns the maximum number of concurrent migrations for a -// host given its traits. It starts from EvictionConcurrency and, for every trait -// present in EvictionTraitConcurrency, keeps the lowest configured limit. The -// result is always clamped to at least 1 so a misconfiguration can never stall a -// drain entirely. -func ResolveConcurrency(traits []string) int { - limit := EvictionConcurrency - for _, t := range traits { - if traitLimit, ok := EvictionTraitConcurrency[t]; ok && traitLimit < limit { - limit = traitLimit - } - } - if limit < 1 { - limit = 1 - } - return limit -} diff --git a/internal/global/global_test.go b/internal/global/global_test.go index 167cdfc0..692567f0 100644 --- a/internal/global/global_test.go +++ b/internal/global/global_test.go @@ -15,65 +15,61 @@ See the License for the specific language governing permissions and limitations under the License. */ -package global +package global_test -import "testing" +import ( + "testing" -func TestResolveConcurrency(t *testing.T) { - // Save and restore the package globals so tests don't leak into each other. - origDefault := EvictionConcurrency - origMap := EvictionTraitConcurrency - t.Cleanup(func() { - EvictionConcurrency = origDefault - EvictionTraitConcurrency = origMap - }) + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/eviction" +) +func TestResolveConcurrency(t *testing.T) { tests := []struct { name string defaultVal int - traitMap map[string]int + traitMap eviction.TraitConcurrency traits []string want int }{ { name: "no traits falls back to default", defaultVal: 3, - traitMap: map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, + traitMap: eviction.TraitConcurrency{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, traits: nil, want: 3, }, { name: "non-matching trait falls back to default", defaultVal: 3, - traitMap: map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, + traitMap: eviction.TraitConcurrency{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, traits: []string{"CUSTOM_SOMETHING_ELSE"}, want: 3, }, { name: "matching trait overrides default", defaultVal: 5, - traitMap: map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, + traitMap: eviction.TraitConcurrency{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, traits: []string{"CUSTOM_HANA_EXCLUSIVE_HOST"}, want: 1, }, { name: "lowest matching trait wins", defaultVal: 10, - traitMap: map[string]int{"CUSTOM_A": 4, "CUSTOM_B": 2, "CUSTOM_C": 7}, + traitMap: eviction.TraitConcurrency{"CUSTOM_A": 4, "CUSTOM_B": 2, "CUSTOM_C": 7}, traits: []string{"CUSTOM_A", "CUSTOM_B", "CUSTOM_C"}, want: 2, }, { name: "trait limit above default is ignored (default is the cap)", defaultVal: 2, - traitMap: map[string]int{"CUSTOM_BIG": 8}, + traitMap: eviction.TraitConcurrency{"CUSTOM_BIG": 8}, traits: []string{"CUSTOM_BIG"}, want: 2, }, { name: "result is clamped to at least 1", defaultVal: 0, - traitMap: map[string]int{}, + traitMap: eviction.TraitConcurrency{}, traits: nil, want: 1, }, @@ -81,9 +77,11 @@ func TestResolveConcurrency(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - EvictionConcurrency = tt.defaultVal - EvictionTraitConcurrency = tt.traitMap - if got := ResolveConcurrency(tt.traits); got != tt.want { + options := eviction.Options{ + Concurrency: tt.defaultVal, + TraitConcurrency: tt.traitMap, + } + if got := options.ResolveConcurrency(tt.traits); got != tt.want { t.Errorf("ResolveConcurrency(%v) = %d, want %d", tt.traits, got, tt.want) } }) From 2f3ac6b95d12ac2aed300cd9bc578ac1517a07f7 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Fri, 30 Oct 2026 18:45:37 +0100 Subject: [PATCH 2/5] Read manager settings from the environment in the chart The manager reads its settings from the environment, so remove the --flag=$(ENV_VAR) container arguments. Document JSON trait maps so chart users can migrate old TRAIT=N values to the native input form. Signed-off-by: Fabian Wiesel --- charts/openstack-hypervisor-operator/values.yaml | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/charts/openstack-hypervisor-operator/values.yaml b/charts/openstack-hypervisor-operator/values.yaml index 22f61295..bc0a6615 100644 --- a/charts/openstack-hypervisor-operator/values.yaml +++ b/charts/openstack-hypervisor-operator/values.yaml @@ -4,12 +4,6 @@ controllerManager: - --metrics-bind-address=:8443 - --leader-elect - --health-probe-bind-address=:8081 - - --certificate-namespace=$(CERTIFICATE_NAMESPACE) - - --certificate-issuer-name=$(CERTIFICATE_ISSUER_NAME) - - --label-selector=$(LABEL_SELECTOR) - - --agent-namespaces=$(AGENT_NAMESPACES) - - --eviction-concurrency=$(EVICTION_CONCURRENCY) - - --eviction-trait-concurrency=$(EVICTION_TRAIT_CONCURRENCY) containerSecurityContext: allowPrivilegeEscalation: false capabilities: @@ -23,8 +17,10 @@ controllerManager: # Default maximum number of VM migrations to run concurrently while draining a hypervisor. # Keep this quoted because it is rendered as an environment variable. evictionConcurrency: "1" - # Optional comma-separated TRAIT=N overrides for per-host migration concurrency. - # The lowest matching trait limit wins, e.g. "CUSTOM_HANA_EXCLUSIVE_HOST=1,CUSTOM_FOO=2". + # Optional JSON object with integer per-host migration limits >= 1. + # Example: '{"CUSTOM_HANA_EXCLUSIVE_HOST":1,"CUSTOM_FOO":2}'. Empty text clears the map. + # Trait names must be nonempty without surrounding whitespace. The lowest matching limit wins. + # CLI flags repeat --eviction-trait-concurrency=TRAIT:N to merge keys. Empty CLI input is invalid. evictionTraitConcurrency: "" osAuthUrl: "" osProjectDomainName: "" From baba03ae8af905ed81b12632593d63640dfb55e9 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Fri, 30 Oct 2026 18:48:28 +0100 Subject: [PATCH 3/5] Add CLI flags overriding the env settings Operators need optional command-line inspection and overrides. Generate flags once from the aggregate so new supported fields in existing modules need no registration changes in cmd. Let sflags use native maps and lists rather than custom trait methods. Repeated TRAIT:N flags merge map keys. The last value for a repeated key wins, and unchanged environment keys remain. Old TRAIT=N CLI lists are unsupported and must migrate to repeated colon entries. Empty CLI map input is invalid, not a whole-map clear operation. Use empty environment text when the map must start empty. Keep descriptions here. Owning modules retain types and defaults. Signed-off-by: Fabian Wiesel --- cmd/main.go | 10 ++++++++++ go.mod | 1 + go.sum | 2 ++ internal/config/options.go | 4 ++-- internal/controller/eviction/options.go | 4 ++-- internal/controller/options.go | 4 ++-- 6 files changed, 19 insertions(+), 6 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index f906d487..49be90b5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -25,7 +25,9 @@ import ( "os" gruntime "runtime" + "github.com/octago/sflags/gen/gflag" "github.com/sapcc/go-api-declarations/bininfo" + // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" @@ -101,6 +103,14 @@ func main() { os.Exit(1) } + // Register CLI flags from the env-filled structs so that explicit + // command-line arguments override environment values; the current + // (env-derived) field values become the flag defaults. + if err := gflag.ParseTo(&cfg, flag.CommandLine); err != nil { + fmt.Fprintf(os.Stderr, "invalid configuration: %v\n", err) + os.Exit(1) + } + opts := ctrlzap.Options{ Development: true, TimeEncoder: zapcore.ISO8601TimeEncoder, diff --git a/go.mod b/go.mod index dec88b51..5550e0de 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/gophercloud/utils/v2 v2.0.0-20260922174841-b4759ea0529b github.com/knadh/koanf/providers/env v1.1.0 github.com/knadh/koanf/v2 v2.4.1 + github.com/octago/sflags v0.3.1 github.com/onsi/ginkgo/v2 v2.33.1 github.com/onsi/gomega v1.44.0 github.com/sapcc/go-api-declarations v1.25.1 diff --git a/go.sum b/go.sum index aa940088..04a1f9e7 100644 --- a/go.sum +++ b/go.sum @@ -145,6 +145,8 @@ github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFd github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= +github.com/octago/sflags v0.3.1 h1:LW65z20iAQKteEyjsnnc+/lyoCUnIoRuAocggr6RB6A= +github.com/octago/sflags v0.3.1/go.mod h1:hVUkbnYwMU9kZiZJyOAIVN56YiVMMPxgJ46kRZ19jh0= github.com/onsi/ginkgo/v2 v2.33.1 h1:62XfWJbEIjPSCSDYrsCPP/SHcmH7O0M8ne8zfqQ2iz4= github.com/onsi/ginkgo/v2 v2.33.1/go.mod h1:+aXOY+vzZ5mu2iI2HpTZUPmM//oQfsNFX6gU9kNcA44= github.com/onsi/gomega v1.44.0 h1:eAiGl3Pw5jz5GQdDff0BcxYpAX1JxW8xD7mFUuwNfZQ= diff --git a/internal/config/options.go b/internal/config/options.go index 05e96798..5fe77937 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -25,8 +25,8 @@ import ( // Shared contains configuration options used by more than one controller. type Shared struct { - LabelSelector string `koanf:"label_selector"` - AgentNamespaces []string `koanf:"agent_namespaces"` + LabelSelector string `koanf:"label_selector" desc:"Label selector to filter watched resources (namely nodes)."` + AgentNamespaces []string `koanf:"agent_namespaces" desc:"Comma-separated list of namespaces to search for agent pods (nova-compute, neutron) during offboarding."` } // DefaultOptions returns the shared controller defaults. diff --git a/internal/controller/eviction/options.go b/internal/controller/eviction/options.go index 3d06b3a3..fbe7061c 100644 --- a/internal/controller/eviction/options.go +++ b/internal/controller/eviction/options.go @@ -25,8 +25,8 @@ import ( // Options configures the eviction controller. type Options struct { - Concurrency int `koanf:"eviction_concurrency"` - TraitConcurrency map[string]int `koanf:"eviction_trait_concurrency"` + Concurrency int `koanf:"eviction_concurrency" desc:"Default maximum number of VM migrations to run concurrently while draining a hypervisor. Defaults to 1 (serial)."` + TraitConcurrency map[string]int `koanf:"eviction_trait_concurrency" desc:"Per-host migration limits. Env: JSON object, e.g. {\"CUSTOM_HANA_EXCLUSIVE_HOST\":1,\"CUSTOM_FOO\":2}. Empty env clears. CLI: repeat TRAIT:N flags to merge keys, e.g. CUSTOM_FOO:2. Empty CLI is invalid. Names must be nonempty without surrounding whitespace. Limits must be >= 1. The lowest matching limit wins."` } // TraitConcurrency preserves the existing name for an ordinary integer map. diff --git a/internal/controller/options.go b/internal/controller/options.go index 5bb54295..6de85cc7 100644 --- a/internal/controller/options.go +++ b/internal/controller/options.go @@ -21,8 +21,8 @@ import "errors" // CertificateOptions configures the node certificate controller. type CertificateOptions struct { - Namespace string `koanf:"certificate_namespace"` - IssuerName string `koanf:"certificate_issuer_name"` + Namespace string `koanf:"certificate_namespace" desc:"The namespace for the certificates."` + IssuerName string `koanf:"certificate_issuer_name" desc:"Name of the certificate issuer."` } // DefaultCertificateOptions returns the certificate controller defaults. From 700085ba2750d46aeb779f0dc65c0012d7678347 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Sun, 1 Nov 2026 18:13:49 +0100 Subject: [PATCH 4/5] Colocate options with their owning controllers settings now live next to their owning controllers, so adding a setting touches one file per controller plus the chart Signed-off-by: Fabian Wiesel --- cmd/config.go | 9 +-- internal/config/options.go | 55 -------------- internal/controller/eviction/controller.go | 49 +++++++++++++ internal/controller/eviction/options.go | 73 ------------------- internal/controller/hypervisor_controller.go | 41 +++++++++-- .../controller/node_certificate_controller.go | 27 +++++++ internal/controller/options.go | 46 ------------ 7 files changed, 116 insertions(+), 184 deletions(-) delete mode 100644 internal/config/options.go delete mode 100644 internal/controller/eviction/options.go delete mode 100644 internal/controller/options.go diff --git a/cmd/config.go b/cmd/config.go index e278a991..dddd0b8f 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -26,15 +26,14 @@ import ( "github.com/knadh/koanf/providers/env" "github.com/knadh/koanf/v2" - "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/config" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/eviction" ) type options struct { - config.Shared `koanf:",squash"` - Certificate controller.CertificateOptions `koanf:",squash"` - Eviction eviction.Options `koanf:",squash"` + controller.Shared `koanf:",squash"` + Certificate controller.CertificateOptions `koanf:",squash"` + Eviction eviction.Options `koanf:",squash"` } // loadOptions reads the operator settings from the environment, starting from @@ -43,7 +42,7 @@ type options struct { // changes or flag registration. func loadOptions() (options, error) { cfg := options{ - Shared: config.DefaultOptions(), + Shared: controller.DefaultOptions(), Certificate: controller.DefaultCertificateOptions(), Eviction: eviction.DefaultOptions(), } diff --git a/internal/config/options.go b/internal/config/options.go deleted file mode 100644 index 5fe77937..00000000 --- a/internal/config/options.go +++ /dev/null @@ -1,55 +0,0 @@ -/* -SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors -SPDX-License-Identifier: Apache-2.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 ( - "errors" - "slices" - "strings" -) - -// Shared contains configuration options used by more than one controller. -type Shared struct { - LabelSelector string `koanf:"label_selector" desc:"Label selector to filter watched resources (namely nodes)."` - AgentNamespaces []string `koanf:"agent_namespaces" desc:"Comma-separated list of namespaces to search for agent pods (nova-compute, neutron) during offboarding."` -} - -// DefaultOptions returns the shared controller defaults. -func DefaultOptions() Shared { - return Shared{} -} - -// Validate normalizes the agent namespace list and requires at least one -// namespace to remain. -func (s *Shared) Validate() error { - namespaces := make([]string, 0, len(s.AgentNamespaces)) - for _, namespace := range s.AgentNamespaces { - namespace = strings.TrimSpace(namespace) - if namespace == "" || slices.Contains(namespaces, namespace) { - continue - } - namespaces = append(namespaces, namespace) - } - s.AgentNamespaces = namespaces - - if len(namespaces) == 0 { - return errors.New("--agent-namespaces (AGENT_NAMESPACES) must contain at least one namespace") - } - - return nil -} diff --git a/internal/controller/eviction/controller.go b/internal/controller/eviction/controller.go index 9d47042a..cb2cb0a9 100644 --- a/internal/controller/eviction/controller.go +++ b/internal/controller/eviction/controller.go @@ -42,6 +42,55 @@ import ( "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) +// Options configures the eviction controller. +type Options struct { + Concurrency int `koanf:"eviction_concurrency" desc:"Default maximum number of VM migrations to run concurrently while draining a hypervisor. Defaults to 1 (serial)."` + TraitConcurrency map[string]int `koanf:"eviction_trait_concurrency" desc:"Per-host migration limits. Env: JSON object, e.g. {\"CUSTOM_HANA_EXCLUSIVE_HOST\":1,\"CUSTOM_FOO\":2}. Empty env clears. CLI: repeat TRAIT:N flags to merge keys, e.g. CUSTOM_FOO:2. Empty CLI is invalid. Names must be nonempty without surrounding whitespace. Limits must be >= 1. The lowest matching limit wins."` +} + +// TraitConcurrency preserves the existing name for an ordinary integer map. +type TraitConcurrency = map[string]int + +// DefaultOptions returns the eviction controller defaults. +func DefaultOptions() Options { + return Options{Concurrency: 1} +} + +// ResolveConcurrency returns the maximum number of concurrent migrations +// for a host given its traits: the default, lowered by the lowest matching +// trait limit, and never below 1. +func (o Options) ResolveConcurrency(traits []string) int { + limit := o.Concurrency + for _, trait := range traits { + if traitLimit, ok := o.TraitConcurrency[trait]; ok && traitLimit < limit { + limit = traitLimit + } + } + return max(limit, 1) +} + +// Validate checks that the concurrency and every trait limit are >= 1 and +// that trait names are non-empty and carry no surrounding whitespace. +func (o Options) Validate() error { + if o.Concurrency < 1 { + return fmt.Errorf("--eviction-concurrency (EVICTION_CONCURRENCY) must be >= 1, got %d", o.Concurrency) + } + + for trait, limit := range o.TraitConcurrency { + if limit < 1 { + return fmt.Errorf("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait %q must have an integer limit >= 1, got %d", trait, limit) + } + if trait != strings.TrimSpace(trait) { + return fmt.Errorf("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait name %q must not contain surrounding whitespace", trait) + } + if trait == "" { + return errors.New("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait name must not be empty") + } + } + + return nil +} + // EvictionReconciler reconciles a Eviction object type EvictionReconciler struct { client.Client diff --git a/internal/controller/eviction/options.go b/internal/controller/eviction/options.go deleted file mode 100644 index fbe7061c..00000000 --- a/internal/controller/eviction/options.go +++ /dev/null @@ -1,73 +0,0 @@ -/* -SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors -SPDX-License-Identifier: Apache-2.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 eviction - -import ( - "errors" - "fmt" - "strings" -) - -// Options configures the eviction controller. -type Options struct { - Concurrency int `koanf:"eviction_concurrency" desc:"Default maximum number of VM migrations to run concurrently while draining a hypervisor. Defaults to 1 (serial)."` - TraitConcurrency map[string]int `koanf:"eviction_trait_concurrency" desc:"Per-host migration limits. Env: JSON object, e.g. {\"CUSTOM_HANA_EXCLUSIVE_HOST\":1,\"CUSTOM_FOO\":2}. Empty env clears. CLI: repeat TRAIT:N flags to merge keys, e.g. CUSTOM_FOO:2. Empty CLI is invalid. Names must be nonempty without surrounding whitespace. Limits must be >= 1. The lowest matching limit wins."` -} - -// TraitConcurrency preserves the existing name for an ordinary integer map. -type TraitConcurrency = map[string]int - -// DefaultOptions returns the eviction controller defaults. -func DefaultOptions() Options { - return Options{Concurrency: 1} -} - -// ResolveConcurrency returns the maximum number of concurrent migrations -// for a host given its traits: the default, lowered by the lowest matching -// trait limit, and never below 1. -func (o Options) ResolveConcurrency(traits []string) int { - limit := o.Concurrency - for _, trait := range traits { - if traitLimit, ok := o.TraitConcurrency[trait]; ok && traitLimit < limit { - limit = traitLimit - } - } - return max(limit, 1) -} - -// Validate checks that the concurrency and every trait limit are >= 1 and -// that trait names are non-empty and carry no surrounding whitespace. -func (o Options) Validate() error { - if o.Concurrency < 1 { - return fmt.Errorf("--eviction-concurrency (EVICTION_CONCURRENCY) must be >= 1, got %d", o.Concurrency) - } - - for trait, limit := range o.TraitConcurrency { - if limit < 1 { - return fmt.Errorf("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait %q must have an integer limit >= 1, got %d", trait, limit) - } - if trait != strings.TrimSpace(trait) { - return fmt.Errorf("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait name %q must not contain surrounding whitespace", trait) - } - if trait == "" { - return errors.New("--eviction-trait-concurrency (EVICTION_TRAIT_CONCURRENCY): trait name must not be empty") - } - } - - return nil -} diff --git a/internal/controller/hypervisor_controller.go b/internal/controller/hypervisor_controller.go index 3c79d8f2..a4516398 100644 --- a/internal/controller/hypervisor_controller.go +++ b/internal/controller/hypervisor_controller.go @@ -19,6 +19,7 @@ package controller import ( "context" + "errors" "fmt" "slices" "sort" @@ -28,7 +29,7 @@ import ( "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/apimachinery/pkg/api/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -41,10 +42,40 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/config" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) +// Shared contains configuration options used by more than one controller. +type Shared struct { + LabelSelector string `koanf:"label_selector" desc:"Label selector to filter watched resources (namely nodes)."` + AgentNamespaces []string `koanf:"agent_namespaces" desc:"Comma-separated list of namespaces to search for agent pods (nova-compute, neutron) during offboarding."` +} + +// DefaultOptions returns the shared controller defaults. +func DefaultOptions() Shared { + return Shared{} +} + +// Validate normalizes the agent namespace list and requires at least one +// namespace to remain. +func (s *Shared) Validate() error { + namespaces := make([]string, 0, len(s.AgentNamespaces)) + for _, namespace := range s.AgentNamespaces { + namespace = strings.TrimSpace(namespace) + if namespace == "" || slices.Contains(namespaces, namespace) { + continue + } + namespaces = append(namespaces, namespace) + } + s.AgentNamespaces = namespaces + + if len(namespaces) == 0 { + return errors.New("--agent-namespaces (AGENT_NAMESPACES) must contain at least one namespace") + } + + return nil +} + const ( labelLifecycleMode = "cobaltcore.cloud.sap/node-hypervisor-lifecycle" annotationAggregates = "nova.openstack.cloud.sap/aggregates" @@ -69,7 +100,7 @@ var transferLabels = append([]string{}, defaultTransferLabels...) type HypervisorController struct { k8sclient.Client Scheme *runtime.Scheme - Options config.Shared + Options Shared } // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch @@ -99,7 +130,7 @@ func (hv *HypervisorController) Reconcile(ctx context.Context, req ctrl.Request) // Check if hypervisor already exists if err := hv.Get(ctx, k8sclient.ObjectKeyFromObject(hypervisor), hypervisor); err != nil { - if !errors.IsNotFound(err) { + if !k8serrors.IsNotFound(err) { return ctrl.Result{}, fmt.Errorf("failed to get hypervisor: %w", err) } // continue with creation @@ -214,7 +245,7 @@ func syncLabelsAndAnnotations(nodeLabels labels.Set, hypervisor *kvmv1.Hyperviso transportAggregatesAndTraits(&node.ObjectMeta, hypervisor) } -func (hv *HypervisorController) SetupWithManager(mgr ctrl.Manager, options config.Shared) error { +func (hv *HypervisorController) SetupWithManager(mgr ctrl.Manager, options Shared) error { hv.Options = options novaVirtLabeledPredicate, err := predicate.LabelSelectorPredicate(metav1.LabelSelector{ diff --git a/internal/controller/node_certificate_controller.go b/internal/controller/node_certificate_controller.go index d1d68d69..c417d07a 100644 --- a/internal/controller/node_certificate_controller.go +++ b/internal/controller/node_certificate_controller.go @@ -19,6 +19,7 @@ package controller import ( "context" + "errors" "fmt" "slices" "time" @@ -36,6 +37,32 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" ) +// CertificateOptions configures the node certificate controller. +type CertificateOptions struct { + Namespace string `koanf:"certificate_namespace" desc:"The namespace for the certificates."` + IssuerName string `koanf:"certificate_issuer_name" desc:"Name of the certificate issuer."` +} + +// DefaultCertificateOptions returns the certificate controller defaults. +func DefaultCertificateOptions() CertificateOptions { + return CertificateOptions{ + Namespace: "monsoon3", + IssuerName: "nova-hypervisor-agents-ca-issuer", + } +} + +// Validate checks that the certificate controller has the required settings. +func (o CertificateOptions) Validate() error { + if o.Namespace == "" { + return errors.New("--certificate-namespace (CERTIFICATE_NAMESPACE) cannot be empty") + } + if o.IssuerName == "" { + return errors.New("--certificate-issuer-name (CERTIFICATE_ISSUER_NAME) cannot be empty") + } + + return nil +} + const ( NodeCertificateControllerName = "certificate" ) diff --git a/internal/controller/options.go b/internal/controller/options.go deleted file mode 100644 index 6de85cc7..00000000 --- a/internal/controller/options.go +++ /dev/null @@ -1,46 +0,0 @@ -/* -SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors -SPDX-License-Identifier: Apache-2.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 "errors" - -// CertificateOptions configures the node certificate controller. -type CertificateOptions struct { - Namespace string `koanf:"certificate_namespace" desc:"The namespace for the certificates."` - IssuerName string `koanf:"certificate_issuer_name" desc:"Name of the certificate issuer."` -} - -// DefaultCertificateOptions returns the certificate controller defaults. -func DefaultCertificateOptions() CertificateOptions { - return CertificateOptions{ - Namespace: "monsoon3", - IssuerName: "nova-hypervisor-agents-ca-issuer", - } -} - -// Validate checks that the certificate controller has the required settings. -func (o CertificateOptions) Validate() error { - if o.Namespace == "" { - return errors.New("--certificate-namespace (CERTIFICATE_NAMESPACE) cannot be empty") - } - if o.IssuerName == "" { - return errors.New("--certificate-issuer-name (CERTIFICATE_ISSUER_NAME) cannot be empty") - } - - return nil -} From 6286311f6d423d9e883ce3856fe060573cfa4c10 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Sun, 1 Nov 2026 18:15:55 +0100 Subject: [PATCH 5/5] Use the native koanf key-only env provider the value callback added no transformation, so the native key-only form is sufficient Signed-off-by: Fabian Wiesel --- cmd/config.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/config.go b/cmd/config.go index dddd0b8f..cafedcc9 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -49,9 +49,7 @@ func loadOptions() (options, error) { k := koanf.New(".") // Environment variables use the same names as before, lowercased as keys. - if err := k.Load(env.ProviderWithValue("", ".", func(name, value string) (string, any) { - return strings.ToLower(name), value - }), nil); err != nil { + if err := k.Load(env.Provider("", ".", strings.ToLower), nil); err != nil { return cfg, err }