diff --git a/.gitignore b/.gitignore index 798c8b42..933e619e 100644 --- a/.gitignore +++ b/.gitignore @@ -50,9 +50,11 @@ dashboards/redfish-telemetry/redfish-telemetry # percli intermediate output directories (dashboards/built/*.json are the canonical outputs) dashboards/built/*/ -# Local server lists for hack/verify-test-events contain real BMC credentials. +# Local server lists and configs for hack/verify-test-events may contain real BMC credentials. +hack/verify-test-events/servers.yaml hack/verify-test-events/servers.local.yaml hack/verify-test-events/*.local.yaml +hack/verify-test-events/config.yaml plugins hack/verify-test-events/servers.yaml hack/verify-test-events/config.yaml diff --git a/cmd/main.go b/cmd/main.go index 447edf7b..409fd413 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -12,8 +12,6 @@ import ( "time" "github.com/ironcore-dev/metal-maintenance-operator/internal/cli" - "github.com/ironcore-dev/metal-maintenance-operator/internal/configloader" - "github.com/ironcore-dev/metal-maintenance-operator/internal/controller/config" "github.com/ironcore-dev/metal-maintenance-operator/internal/discovery" "github.com/ironcore-dev/metal-maintenance-operator/internal/ignition" "github.com/ironcore-dev/metal-maintenance-operator/internal/server" @@ -59,6 +57,7 @@ import ( // +kubebuilder:scaffold:imports ) +const maxRepositoryPassesLimit = 5 const maxFailedAutoRetryCount = 10 var ( @@ -104,6 +103,8 @@ func main() { var resyncInterval time.Duration var biosSettingsTimeoutExpiry time.Duration var rebootTimeoutExpiry time.Duration + var scheduledJobPowerOnTimeout time.Duration + var maxRepositoryPasses int flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ "Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") @@ -155,29 +156,15 @@ func main() { "Timeout for BIOS settings application before the task is considered expired.") flag.DurationVar(&rebootTimeoutExpiry, "reboot-timeout-expiry", 10*time.Minute, "Timeout waiting for a server to complete a reboot/power-cycle before the task is considered expired.") + flag.IntVar(&maxRepositoryPasses, "max-repository-passes", maxRepositoryPassesLimit, + "Maximum number of check->apply->track->recheck passes a FirmwareUpdate may take before being marked Failed.") var minBMCResetInterval time.Duration flag.DurationVar(&minBMCResetInterval, "min-bmc-reset-interval", 24*time.Hour, "Minimum time that must have passed since a BMC's last completed reset to reissue another reset.") - - // FirmwareUpdate ConfigMap flags. When --firmwareupdate-config-name is - // set, a live-reloading configloader.Loader watches the named ConfigMap - // and the FirmwareUpdateReconciler prefers its values over its own - // built-in defaults (ResyncInterval, DefaultFailedAutoRetryCount, - // MaxRepositoryPasses, MinBMCResetInterval, ScheduledJobPowerOnTimeout, - // RepositoryCheckInterval) for any field the ConfigMap sets, falling back - // to those defaults otherwise. Empty (the default) disables the feature - // entirely, leaving the FirmwareUpdate controller on its built-in - // defaults. - var ( - firmwareUpdateConfigName string - firmwareUpdateConfigNamespace string - ) - flag.StringVar(&firmwareUpdateConfigName, "firmwareupdate-config-name", "", - "Name of the ConfigMap holding FirmwareUpdate controller overrides. Leave empty (the default) to disable "+ - "ConfigMap-based overrides.") - flag.StringVar(&firmwareUpdateConfigNamespace, "firmwareupdate-config-namespace", "", - "Namespace of the FirmwareUpdate ConfigMap. Defaults to POD_NAMESPACE. Ignored when "+ - "--firmwareupdate-config-name is empty.") + flag.DurationVar(&scheduledJobPowerOnTimeout, "scheduled-job-power-on-timeout", 30*time.Minute, + "How long a firmware upgrade job may remain in the \"Scheduled\" state (queued, awaiting a reboot/power "+ + "cycle to execute) with the server powered off and no observed progress, before the controller powers the "+ + "server back on to let the job proceed. A value <= 0 disables this check.") // Telemetry collector flags. The whole pipeline is gated by // --enable-telemetry — when off (the default), zero telemetry @@ -228,6 +215,13 @@ func main() { ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) + if maxRepositoryPasses < 1 || maxRepositoryPasses > maxRepositoryPassesLimit { + setupLog.Error( + nil, "Failed to start FirmwareUpdate controller: invalid --max-repository-passes value", + "value", maxRepositoryPasses, "min", 1, "max", maxRepositoryPassesLimit) + os.Exit(1) + } + if defaultFailedAutoRetryCountInt < 0 || defaultFailedAutoRetryCountInt > maxFailedAutoRetryCount { setupLog.Error(nil, "Failed to start FirmwareUpdate controller: invalid --default-failed-auto-retry-count value", "value", defaultFailedAutoRetryCountInt, "min", 0, "max", maxFailedAutoRetryCount) @@ -263,16 +257,6 @@ func main() { } } - if firmwareUpdateConfigName != "" && firmwareUpdateConfigNamespace == "" { - firmwareUpdateConfigNamespace = os.Getenv("POD_NAMESPACE") - if firmwareUpdateConfigNamespace == "" { - setupLog.Error(nil, - "FirmwareUpdate ConfigMap namespace resolution failed: neither "+ - "--firmwareupdate-config-namespace nor POD_NAMESPACE is set") - os.Exit(1) - } - } - // 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 @@ -351,7 +335,6 @@ func main() { splitAndTrim(configMapCacheNamespaces), namedConfigMapRefs( enableTelemetry, telemetryConfigNamespace, telemetryConfigName, - firmwareUpdateConfigNamespace, firmwareUpdateConfigName, )..., ), }, @@ -543,34 +526,19 @@ func main() { os.Exit(1) } - var firmwareUpdateConfigLoader *configloader.Loader[config.FirmwareUpdateConfig] - if firmwareUpdateConfigName != "" && firmwareUpdateConfigNamespace != "" { - firmwareUpdateConfigLoader = &configloader.Loader[config.FirmwareUpdateConfig]{ - Cache: mgr.GetCache(), - Client: mgr.GetClient(), - Namespace: firmwareUpdateConfigNamespace, - Name: firmwareUpdateConfigName, - Parse: config.ParseFirmwareUpdateConfig, - Log: ctrl.Log.WithName("firmwareupdate-config-loader"), - EventRecorder: mgr.GetEventRecorder("metal-maintenance-operator"), - } - if err := mgr.Add(firmwareUpdateConfigLoader); err != nil { - setupLog.Error(err, "Unable to register FirmwareUpdate ConfigMap loader") - os.Exit(1) - } - setupLog.Info("Enabled FirmwareUpdate ConfigMap overrides", - "configMap", firmwareUpdateConfigNamespace+"/"+firmwareUpdateConfigName) - } - if err = (&systemctrl.FirmwareUpdateReconciler{ - Client: mgr.GetClient(), - ManagerNamespace: managerNamespace, - DefaultProtocol: protocol, - SkipCertValidation: skipCertValidation, - Scheme: mgr.GetScheme(), - Conditions: accessor, - BMCOptions: bmcOpts, - ConfigLoader: firmwareUpdateConfigLoader, + Client: mgr.GetClient(), + ManagerNamespace: managerNamespace, + DefaultProtocol: protocol, + SkipCertValidation: skipCertValidation, + Scheme: mgr.GetScheme(), + ResyncInterval: resyncInterval, + Conditions: accessor, + BMCOptions: bmcOpts, + DefaultFailedAutoRetryCount: int32(defaultFailedAutoRetryCountInt), + MaxRepositoryPasses: int32(maxRepositoryPasses), + MinBMCResetInterval: minBMCResetInterval, + ScheduledJobPowerOnTimeout: scheduledJobPowerOnTimeout, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "Unable to create FirmwareUpdate controller") os.Exit(1) @@ -667,7 +635,7 @@ func main() { // +kubebuilder:scaffold:builder if enableTelemetry { - if err := telemetryruntime.AddTo(mgr, telemetryruntime.Options{ + telemetryOpts := telemetryruntime.Options{ ConfigName: telemetryConfigName, ConfigNamespace: telemetryConfigNamespace, ReceiverURL: telemetryReceiverURL, @@ -675,23 +643,39 @@ func main() { InsecureTLS: telemetryInsecureTLS, SubscriberID: telemetrySubscriberID, EnableCriticalEventHandler: telemetryEnableCriticalHandler, - }); err != nil { + } + if err := telemetryruntime.AddTo(mgr, &telemetryOpts); err != nil { setupLog.Error(err, "Failed to add telemetry pipeline") os.Exit(1) } setupLog.Info("Enabled telemetry pipeline", "configMap", telemetryConfigNamespace+"/"+telemetryConfigName, "receiverURL", telemetryReceiverURL) - } - // BMC service-discovery endpoint. - sdHandler := &discovery.Handler{ - Client: mgr.GetClient(), - Log: ctrl.Log.WithName("discovery"), - } - if err := mgr.AddMetricsServerExtraHandler(discovery.Path, sdHandler); err != nil { - setupLog.Error(err, "Failed to register BMC service discovery handler", "path", discovery.Path) - os.Exit(1) + // BMC service-discovery endpoint: omit BMCs with active push + // subscriptions so the exporter only scrapes what the push pipeline + // hasn't claimed. + sdHandler := &discovery.Handler{ + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("discovery"), + SubscribedBMCs: telemetryOpts.ActiveSubscriptions, + UnsubscribedBMCReasons: telemetryOpts.UnsubscribedBMCReasons, + } + if err := mgr.AddMetricsServerExtraHandler(discovery.Path, sdHandler); err != nil { + setupLog.Error(err, "Failed to register BMC service discovery handler", "path", discovery.Path) + os.Exit(1) + } + } else { + // Telemetry disabled: register the SD endpoint without subscription + // filtering — the exporter scrapes all BMCs. + sdHandler := &discovery.Handler{ + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("discovery"), + } + if err := mgr.AddMetricsServerExtraHandler(discovery.Path, sdHandler); err != nil { + setupLog.Error(err, "Failed to register BMC service discovery handler", "path", discovery.Path) + os.Exit(1) + } } if metricsAddr == "0" || metricsAddr == "" { setupLog.Info("BMC service discovery registered but metrics server is disabled", @@ -733,15 +717,11 @@ type namedConfigMapRef struct { // "no restriction needed" for that entry). func namedConfigMapRefs( telemetryEnabled bool, telemetryNS, telemetryName string, - firmwareUpdateConfigNS, firmwareUpdateConfigName string, ) []namedConfigMapRef { var refs []namedConfigMapRef if telemetryEnabled && telemetryNS != "" && telemetryName != "" { refs = append(refs, namedConfigMapRef{Namespace: telemetryNS, Name: telemetryName}) } - if firmwareUpdateConfigName != "" && firmwareUpdateConfigNS != "" { - refs = append(refs, namedConfigMapRef{Namespace: firmwareUpdateConfigNS, Name: firmwareUpdateConfigName}) - } return refs } diff --git a/dist/chart/templates/extras/telemetry-configmap.yaml b/dist/chart/templates/extras/telemetry-configmap.yaml index 840dae00..35ebcf4f 100644 --- a/dist/chart/templates/extras/telemetry-configmap.yaml +++ b/dist/chart/templates/extras/telemetry-configmap.yaml @@ -17,8 +17,14 @@ data: {{- with .Values.telemetry.config.testEventTimeout }} testEventTimeout: {{ . }} {{- end }} - {{- if .Values.telemetry.config.eventBasedHardware }} - eventBasedHardware: -{{ .Values.telemetry.config.eventBasedHardware | toYaml | indent 6 }} + {{- if .Values.telemetry.config.excludeHardware }} + excludeHardware: +{{ .Values.telemetry.config.excludeHardware | toYaml | indent 6 }} + {{- end }} + {{- if .Values.telemetry.config.testEventOverrides }} + testEventOverrides: + {{- range $key := (.Values.telemetry.config.testEventOverrides | keys | sortAlpha) }} + - {{ get $.Values.telemetry.config.testEventOverrides $key | toYaml | trimSuffix "\n" | nindent 10 | trimPrefix "\n " }} + {{- end }} {{- end }} {{- end }} diff --git a/dist/chart/values.yaml b/dist/chart/values.yaml index d68cfc36..1357f805 100644 --- a/dist/chart/values.yaml +++ b/dist/chart/values.yaml @@ -303,24 +303,41 @@ telemetry: ## Timeout for the health check round-trip. Bounds: 5s – 5m. ## testEventTimeout: "" - ## Vendor/model rows that opt into event-based subscription. + ## Vendor/model rows that opt OUT of event-based subscription (exclusions). ## See config/telemetry/configmap.yaml for full field documentation. ## - eventBasedHardware: [] - ## Example: - # eventBasedHardware: - # - vendor: "Dell Inc." - # models: ["PowerEdge R760", "PowerEdge R660"] - # testMessageId: "FAN0001" - # testSeverity: "Informational" - # - vendor: "HPE" - # models: ["*"] - # testMessageId: "iLOEvents.1.0.TestAlert" - # testSeverity: "OK" - # testOriginOfCondition: "/redfish/v1/Managers/1" - # - vendor: "Lenovo" - # models: ["*"] - # testMessageId: "EventRegistry.1.0.0.FQXSPCA0012I" + excludeHardware: [] + ## Named map of vendor/model rows that opt into event-based subscription. + ## Keys are arbitrary identifiers used only for Helm merge; they do not + ## appear in the rendered ConfigMap. Because Helm merges maps (unlike + ## lists, which are replaced), end-users can add entries in their own + ## values.yaml without repeating the defaults: + ## + ## testEventOverrides: + ## dell-r840: + ## vendor: "Dell Inc." + ## models: ["PowerEdge R840"] + ## testMessageId: "Storage.1.0.DriveFailed" + ## testSeverity: "Informational" + ## + ## See config/telemetry/configmap.yaml for full field documentation. + ## + testEventOverrides: + dell: + vendor: "Dell Inc." + models: ["*"] + testMessageId: "FAN.1.0.FAN0001" + testSeverity: "Informational" + hpe: + vendor: "HPE" + models: ["*"] + testMessageId: "iLOEvents.1.0.TestAlert" + testSeverity: "OK" + testOriginOfCondition: "/redfish/v1/Managers/1" + lenovo: + vendor: "Lenovo" + models: ["*"] + testMessageId: "EventRegistry.1.0.0.FQXSPCA0012I" ## Additional Secret resources to render in the release namespace. ## Useful for operator-pattern credentials (e.g. Gardener token-requestor secrets). diff --git a/docs/concepts/biossettings.md b/docs/concepts/biossettings.md index 52c197e0..dae967da 100644 --- a/docs/concepts/biossettings.md +++ b/docs/concepts/biossettings.md @@ -18,21 +18,18 @@ checks, maintenance, and post-apply verification. - `status.state` reflects the overall lifecycle: `Pending`, `InProgress`, `Applied`, or `Failed`. Each entry in `status.flowState[]` mirrors this per settings-flow step, so a stuck rollout can be pinpointed to the exact step. - `spec.retryPolicy.maxAttempts` bounds automatic retries after a transient failure. -- `spec.readinessGates`/`spec.completionConditionType` (optional) let this object participate in a manual - cross-resource sequencing chain — see [Readiness Gates](readiness-gates.md). ## Workflow 1. The controller validates `spec.settingsFlow` for duplicate step names and duplicate setting keys across steps. -2. It waits until the server's BIOS version matches `spec.version`, and until `spec.readinessGates` (if set) are - satisfied on the server (see [Readiness Gates](readiness-gates.md)), then requests (or reuses) - `ServerMaintenance` for the server per `spec.serverMaintenancePolicy`. +2. It waits until the server's BIOS version matches `spec.version`, then requests (or reuses) `ServerMaintenance` + for the server per `spec.serverMaintenancePolicy`. 3. Once the server is in maintenance, the controller applies each `settingsFlow` step in priority order, powering the server off/on when a reboot is required to make settings take effect. 4. After each step is applied, the controller verifies it against the live BIOS settings before advancing to the next step, recording progress in `status.flowState[]`. -5. Once all steps are verified, `status.state` becomes `Applied` (patching `spec.completionConditionType` `True` - on the server, if set); on unrecoverable failure, `Failed` (subject to `spec.retryPolicy`). +5. Once all steps are verified, `status.state` becomes `Applied`; on unrecoverable failure, `Failed` (subject to + `spec.retryPolicy`). 6. On deletion, the controller cleans up the `ServerMaintenance` it requested and removes its finalizer once no maintenance for this object is still in progress. diff --git a/docs/concepts/biossettingsset.md b/docs/concepts/biossettingsset.md index 65f34b72..e25e79fd 100644 --- a/docs/concepts/biossettingsset.md +++ b/docs/concepts/biossettingsset.md @@ -12,10 +12,6 @@ by creating and managing one child [`BIOSSettings`](biossettings.md) per selecte children whose server no longer matches the selector (once the child is no longer `InProgress`). - `status` aggregates rollout progress across all children: `fullyLabeledServers`, `availableBIOSSettings`, `pendingBIOSSettings`, `inProgressBIOSSettings`, `completedBIOSSettings`, and `failedBIOSSettings`. -- `spec.biosSettingsTemplate.readinessGates`/`completionConditionType` (optional) are propagated as-is to every - child, so the whole fleet can participate in a manual [Readiness Gates](readiness-gates.md) chain; since each - child targets a distinct `Server`, using the same `completionConditionType` across all children is safe (no - collision, as each is patched onto that child's own server). ## Workflow diff --git a/docs/concepts/biosversion.md b/docs/concepts/biosversion.md index 200407dc..03effcee 100644 --- a/docs/concepts/biosversion.md +++ b/docs/concepts/biosversion.md @@ -16,21 +16,17 @@ served from an external location. BMC-reported upgrade task (state, status, percent complete). - `status.state` reflects the overall lifecycle: `Pending`, `InProgress`, `Completed`, or `Failed`. - `spec.retryPolicy.maxAttempts` bounds automatic retries after a transient failure. -- `spec.readinessGates`/`spec.completionConditionType` (optional) let this object participate in a manual - cross-resource sequencing chain — see [Readiness Gates](readiness-gates.md). ## Workflow -1. If `spec.readinessGates` is set, the controller waits until those conditions are satisfied on the server before - proceeding (see [Readiness Gates](readiness-gates.md)). -2. The controller requests (or reuses) `ServerMaintenance` for the server per `spec.serverMaintenancePolicy`. -3. Once the server is in maintenance, it issues the firmware upgrade using `spec.image`, then powers the server +1. The controller requests (or reuses) `ServerMaintenance` for the server per `spec.serverMaintenancePolicy`. +2. Once the server is in maintenance, it issues the firmware upgrade using `spec.image`, then powers the server off/on as needed to complete the upgrade. -4. It polls the BMC-reported task in `status.upgradeTask` until it completes, fails, or times out +3. It polls the BMC-reported task in `status.upgradeTask` until it completes, fails, or times out (`spec.rebootTimeoutExpiry` bounds the reboot wait, configured at the operator level). -5. On success, `status.state` becomes `Completed` (patching `spec.completionConditionType` `True` on the server, if - set); on unrecoverable failure, `Failed` (subject to `spec.retryPolicy`). -6. On deletion, the controller cleans up the `ServerMaintenance` it requested and removes its finalizer once the +4. On success, `status.state` becomes `Completed`; on unrecoverable failure, `Failed` (subject to + `spec.retryPolicy`). +5. On deletion, the controller cleans up the `ServerMaintenance` it requested and removes its finalizer once the upgrade is no longer in progress. ## Example diff --git a/docs/concepts/biosversionset.md b/docs/concepts/biosversionset.md index 6ea1fa78..63fd0ab8 100644 --- a/docs/concepts/biosversionset.md +++ b/docs/concepts/biosversionset.md @@ -12,10 +12,6 @@ by creating and managing one child [`BIOSVersion`](biosversion.md) per selected children whose server no longer matches the selector (once the child is no longer `InProgress`). - `status` aggregates rollout progress across all children: `fullyLabeledServers`, `availableBIOSVersion`, `pendingBIOSVersion`, `inProgressBIOSVersion`, `completedBIOSVersion`, and `failedBIOSVersion`. -- `spec.biosVersionTemplate.readinessGates`/`completionConditionType` (optional) are propagated as-is to every - child, so the whole fleet can participate in a manual [Readiness Gates](readiness-gates.md) chain; since each - child targets a distinct `Server`, using the same `completionConditionType` across all children is safe (no - collision, as each is patched onto that child's own server). ## Workflow diff --git a/docs/concepts/bmcsettings.md b/docs/concepts/bmcsettings.md index 4e00bdbc..2a71b125 100644 --- a/docs/concepts/bmcsettings.md +++ b/docs/concepts/bmcsettings.md @@ -20,24 +20,19 @@ - `status.state` reflects the overall lifecycle: `Pending`, `InProgress`, `Applied`, or `Failed`. - `spec.retryPolicy.maxAttempts` bounds automatic retries after a transient failure; if unset, the operator-level default (`--default-failed-auto-retry-count`) is used. -- `spec.readinessGates`/`spec.completionConditionType` (optional) let this object participate in a manual - cross-resource sequencing chain — see [Readiness Gates](readiness-gates.md). ## Workflow 1. The controller resolves the referenced `BMC` and waits until its firmware version matches `spec.version`. -2. If `spec.readinessGates` is set, it waits until those conditions are satisfied on every `Server` managed by the - BMC before proceeding (see [Readiness Gates](readiness-gates.md)). -3. It fetches every `Server` managed by the BMC and requests `ServerMaintenance` for each one that is not already +2. It fetches every `Server` managed by the BMC and requests `ServerMaintenance` for each one that is not already in maintenance, according to `spec.serverMaintenancePolicy`. -4. Once all required servers are in maintenance, the controller resolves `spec.variables` and diffs the desired +3. Once all required servers are in maintenance, the controller resolves `spec.variables` and diffs the desired `spec.settings` against the BMC's current manager settings. -5. Any drifted settings are issued to the BMC. If the BMC requires a reset to apply them, the controller powers the +4. Any drifted settings are issued to the BMC. If the BMC requires a reset to apply them, the controller powers the BMC off/on as needed and waits for it to come back. -6. The controller verifies the settings converged and marks `status.state` as `Applied` (patching - `spec.completionConditionType` `True` on every related `Server`, if set); on unrecoverable failure, `Failed` - (subject to `spec.retryPolicy`). -7. On deletion, the controller cleans up the `ServerMaintenance` objects it created and removes its finalizer once +5. The controller verifies the settings converged and marks `status.state` as `Applied`; on unrecoverable failure, + `Failed` (subject to `spec.retryPolicy`). +6. On deletion, the controller cleans up the `ServerMaintenance` objects it created and removes its finalizer once no maintenance for this object is still in progress. ## Example diff --git a/docs/concepts/bmcsettingsset.md b/docs/concepts/bmcsettingsset.md index 5940b959..cd0da088 100644 --- a/docs/concepts/bmcsettingsset.md +++ b/docs/concepts/bmcsettingsset.md @@ -12,10 +12,6 @@ objects, by creating and managing one child [`BMCSettings`](bmcsettings.md) per children whose BMC no longer matches the selector (once the child is no longer `InProgress`). - `status` aggregates rollout progress across all children: `fullyLabeledBMCs`, `availableBMCSettings`, `pendingBMCSettings`, `inProgressBMCSettings`, `completedBMCSettings`, and `failedBMCSettings`. -- `spec.bmcSettingsTemplate.readinessGates`/`completionConditionType` (optional) are propagated as-is to every - child, so the whole fleet can participate in a manual [Readiness Gates](readiness-gates.md) chain; since each - child targets a distinct `BMC`, using the same `completionConditionType` across all children is safe (no - collision, as each is patched onto that child's own servers). ## Workflow diff --git a/docs/concepts/bmcversion.md b/docs/concepts/bmcversion.md index e4b81c71..d5be5469 100644 --- a/docs/concepts/bmcversion.md +++ b/docs/concepts/bmcversion.md @@ -17,20 +17,16 @@ from an external location. Because a BMC upgrade can affect every `Server` it ma BMC-reported upgrade task (URI, state, status, percent complete). - `status.state` reflects the overall lifecycle: `Pending`, `InProgress`, `Completed`, or `Failed`. - `spec.retryPolicy.maxAttempts` bounds automatic retries after a transient failure. -- `spec.readinessGates`/`spec.completionConditionType` (optional) let this object participate in a manual - cross-resource sequencing chain — see [Readiness Gates](readiness-gates.md). ## Workflow -1. If `spec.readinessGates` is set, the controller waits until those conditions are satisfied on every `Server` - managed by the BMC before proceeding (see [Readiness Gates](readiness-gates.md)). -2. The controller requests `ServerMaintenance` for every server managed by the referenced BMC, per +1. The controller requests `ServerMaintenance` for every server managed by the referenced BMC, per `spec.serverMaintenancePolicy`. -3. Once all required servers are in maintenance, it issues the firmware upgrade to the BMC using `spec.image`. -4. It polls the BMC-reported task in `status.upgradeTask` until it completes, fails, or times out. -5. On success, `status.state` becomes `Completed` (patching `spec.completionConditionType` `True` on every related - `Server`, if set); on unrecoverable failure, `Failed` (subject to `spec.retryPolicy`). -6. On deletion, the controller cleans up the `ServerMaintenance` objects it created and removes its finalizer once +2. Once all required servers are in maintenance, it issues the firmware upgrade to the BMC using `spec.image`. +3. It polls the BMC-reported task in `status.upgradeTask` until it completes, fails, or times out. +4. On success, `status.state` becomes `Completed`; on unrecoverable failure, `Failed` (subject to + `spec.retryPolicy`). +5. On deletion, the controller cleans up the `ServerMaintenance` objects it created and removes its finalizer once the upgrade is no longer in progress. ## Example diff --git a/docs/concepts/bmcversionset.md b/docs/concepts/bmcversionset.md index 2c888494..209df693 100644 --- a/docs/concepts/bmcversionset.md +++ b/docs/concepts/bmcversionset.md @@ -12,10 +12,6 @@ creating and managing one child [`BMCVersion`](bmcversion.md) per selected BMC. children whose BMC no longer matches the selector (once the child is no longer `InProgress`). - `status` aggregates rollout progress across all children: `fullyLabeledBMCs`, `availableBMCVersion`, `pendingBMCVersion`, `inProgressBMCVersion`, `completedBMCVersion`, and `failedBMCVersion`. -- `spec.bmcVersionTemplate.readinessGates`/`completionConditionType` (optional) are propagated as-is to every - child, so the whole fleet can participate in a manual [Readiness Gates](readiness-gates.md) chain; since each - child targets a distinct `BMC`, using the same `completionConditionType` across all children is safe (no - collision, as each is patched onto that child's own servers). ## Workflow diff --git a/docs/concepts/readiness-gates.md b/docs/concepts/readiness-gates.md deleted file mode 100644 index 5de226a3..00000000 --- a/docs/concepts/readiness-gates.md +++ /dev/null @@ -1,116 +0,0 @@ -# Readiness Gates - -Readiness gates let independent [`BMCSettings`](bmcsettings.md), [`BMCVersion`](bmcversion.md), -[`BIOSVersion`](biosversion.md), and [`BIOSSettings`](biossettings.md) objects be chained into a deterministic, -manually-defined sequence — e.g. "upgrade the BMC firmware, then apply BMC settings, then upgrade the BIOS, then -apply BIOS settings" — without any of them needing to know about each other directly. Semantics mirror Kubernetes -`Pod.Spec.ReadinessGates`: each object can declare conditions that must be present (with a required status) on its -related `Server`(s) before it starts applying, and/or publish its own completion as a condition on those same -`Server`(s) for downstream objects to gate on. - -This is an optional, opt-in mechanism — resources with no `readinessGates`/`completionConditionType` configured -behave exactly as described in their own concept pages, unaffected by any of this. - -## Key Points - -- Both fields come from the shared `ReadinessGating` struct, inlined into every resource's own template type - (`BMCSettingsTemplate`, `BMCVersionTemplate`, `BIOSVersionTemplate`, `BIOSSettingsTemplate`) — so the `*Set` - variants ([`BMCSettingsSet`](bmcsettingsset.md), [`BMCVersionSet`](bmcversionset.md), - [`BIOSVersionSet`](biosversionset.md), [`BIOSSettingsSet`](biossettingsset.md)) propagate them to every child - for free, just like any other template field. -- `spec.readinessGates[]` is a list of `{type, requiredStatus}` pairs. `requiredStatus` defaults to `True` if - omitted. A gate is satisfied only when a condition of the given `type` is present on the related `Server` with - exactly that status — a missing condition never satisfies a gate, regardless of `requiredStatus`. -- For `BMCSettings`/`BMCVersion` (which can affect every `Server` managed by one `BMC`), gates use AND semantics - across all related servers: every listed gate must be satisfied on every related `Server` before the object - proceeds. -- `spec.completionConditionType`, if set, is the condition `Type` this object patches (status `True`) onto its - related `Server`(s) once it reaches its own terminal success state (`Applied` for `BMCSettings`/`BIOSSettings`, - `Completed` for `BMCVersion`/`BIOSVersion`). Downstream objects reference this exact string in their own - `readinessGates[].type` to gate on it. It must be a valid qualified name (optionally `prefix/name`), max 316 - characters. -- There is no built-in ownership/uniqueness check on `completionConditionType` — any object can declare (and - overwrite) any condition `Type` on a `Server`, including one another resource already uses. Pick a distinct, - intentional name per logical signal in the chain (e.g. a shared prefix like `e2e-gate/step1-done`) to avoid two - unrelated resources colliding on the same `Server` condition. This is a non-issue across `*Set` children, since - each child acts on its own distinct `Server`/`BMC`. -- Each gated object also tracks its own last gate-check outcome via a `ReadinessGatesSatisfied` condition in its - **own** `status.conditions` (not the `Server`'s) — `False` with reasons while blocked, `True` once satisfied. - Objects with no `readinessGates` configured never gain this condition. - -## How Gating Works - -1. On every reconcile while `Pending`, the controller first ensures its own `completionConditionType` (if set) - exists on the related `Server`(s), initializing it to `False` if absent. This happens *before* checking its - own gates, so a sibling relying on this condition never races against it simply not existing yet. -2. The controller then evaluates `spec.readinessGates` against the related `Server`(s)' current conditions. If any - gate is unsatisfied, it records the reasons on its own `ReadinessGatesSatisfied` condition (`False`) and waits; - it is re-triggered automatically via a `Server` watch once the relevant condition changes, no polling required. -3. Once all gates are satisfied, `ReadinessGatesSatisfied` is marked `True` and the object proceeds with its normal - workflow (requesting `ServerMaintenance`, applying settings/firmware, etc. — see each resource's own page). -4. Once maintenance is granted and real apply work is about to start, the controller patches its - `completionConditionType` to `Unknown` — a transitional signal that this resource's completion state can no - longer be trusted by downstream/frozen gates until it either succeeds or fails. -5. On success, the controller patches `completionConditionType` to `True`. On an already-`Applied`/`Completed` - object, if a later reconcile detects the live hardware no longer matches spec ("drift"), the condition is reset - to `False` with `Reason: DriftDetected` while the object re-converges. -6. A condition with `Reason: DriftDetected` never satisfies *any* gate watching it — neither `requiredStatus: True` - nor `requiredStatus: False` — until it settles back to a stable `True`/`False`. This closes a race in the - "freeze gate" pattern below, where a transient drift-reset on a downstream condition could otherwise be - misread as "permanently unsatisfied" by an upstream object waiting for exactly that. - -## Example: A Multi-Step Chain - -Each step gates on the previous step's `completionConditionType`, all coordinating through the same `Server`: - -```yaml -apiVersion: baseboard.metal.ironcore.dev/v1alpha1 -kind: BMCVersion -metadata: - name: step1-bmc-version -spec: - bmcRef: - name: endpoint-sample - version: 1.46.455b66-rev1 - image: - URI: https://example.com/firmware/bmc-1.46.455b66-rev1.bin - completionConditionType: rollout/step1-bmc-version-done ---- -apiVersion: baseboard.metal.ironcore.dev/v1alpha1 -kind: BMCSettings -metadata: - name: step2-bmc-settings -spec: - bmcRef: - name: endpoint-sample - version: 1.46.455b66-rev1 - settingsFlow: - - name: boot-settings - priority: 10 - settings: - bootMode: "UEFI" - readinessGates: - - type: rollout/step1-bmc-version-done - requiredStatus: "True" - completionConditionType: rollout/step2-bmc-settings-done ---- -apiVersion: system.metal.ironcore.dev/v1alpha1 -kind: BIOSVersion -metadata: - name: step3-bios-version -spec: - serverRef: - name: endpoint-sample-system-0 - version: P79 v1.46 (01/15/2018) - image: - URI: https://example.com/firmware/bios-P79-v1.46.bin - readinessGates: - - type: rollout/step2-bmc-settings-done - requiredStatus: "True" - completionConditionType: rollout/step3-bios-version-done -``` - -`step2-bmc-settings` stays `Pending` (with `ReadinessGatesSatisfied: False`) until `step1-bmc-version` reaches -`Completed` and patches `rollout/step1-bmc-version-done: True` on the `Server`; `step3-bios-version` in turn waits -for `step2-bmc-settings` the same way. Ordering across the whole chain can be confirmed after the fact by -comparing each condition's `lastTransitionTime` on the `Server` object. diff --git a/hack/verify-test-events/main.go b/hack/verify-test-events/main.go index 3ad8127a..702b5ea3 100644 --- a/hack/verify-test-events/main.go +++ b/hack/verify-test-events/main.go @@ -73,6 +73,9 @@ func main() { serversPath := flag.String("servers", "", "path to the server list YAML") perServerTimeout := flag.Duration("timeout", 30*time.Second, "per-server connect+submit timeout") insecure := flag.Bool("insecure", true, "skip TLS verification") + probe := flag.Bool("probe", false, + "auto-discover a TestMessageId via the BMC's Redfish EventService registries "+ + "when no testEventOverrides row provides one") flag.Parse() if *configPath == "" || *serversPath == "" { @@ -95,7 +98,7 @@ func main() { for _, s := range list.Servers { username := firstNonEmpty(s.Username, list.Username) password := firstNonEmpty(s.Password, list.Password) - result := runOne(s, username, password, cfg, *perServerTimeout, *insecure) + result := runOne(s, username, password, cfg, *perServerTimeout, *insecure, *probe) fmt.Println(result.String()) switch result.outcome { case outcomeOK: @@ -141,31 +144,34 @@ func (r result) String() string { } // runOne connects to one BMC and calls SubmitTestEvent. It performs no -// other Redfish calls (no inventory lookup, no subscription management). +// subscription management. When probe is true and no testEventOverrides row +// provides a MessageId, it walks the BMC's Redfish event registries to +// auto-discover one. func runOne(s serverEntry, username, password string, - cfg *subscriptions.Config, timeout time.Duration, insecure bool, + cfg *subscriptions.Config, timeout time.Duration, insecure, probe bool, ) result { - hw := subscriptions.SubscribeToBMC(subscriptions.BMCRef{ + ref := subscriptions.BMCRef{ Name: s.Endpoint, Vendor: s.Vendor, Model: s.Model, FirmwareVersion: s.Firmware, - }, cfg) - if hw == nil { - return result{endpoint: s.Endpoint, outcome: outcomeSkip, - detail: fmt.Sprintf( - "no eventBasedHardware row matches vendor=%q model=%q firmware=%q", - s.Vendor, - s.Model, - s.Firmware, - )} } - if hw.TestMessageId == "" { + + if subscriptions.IsExcluded(ref, cfg) { return result{endpoint: s.Endpoint, outcome: outcomeSkip, detail: fmt.Sprintf( - "matched vendor=%q model=%q but row has no testMessageId configured", - s.Vendor, - s.Model, + "vendor=%q model=%q matched an excludeHardware rule", + s.Vendor, s.Model, )} } + override := subscriptions.TestOverrideFor(ref, cfg) + msgId := "" + severity := "" + origin := "" + if override != nil { + msgId = override.TestMessageId + severity = override.TestSeverity + origin = override.TestOriginOfCondition + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -185,10 +191,38 @@ func runOne(s serverEntry, username, password string, if err != nil { return result{endpoint: s.Endpoint, outcome: outcomeFail, detail: fmt.Sprintf("wrap client: %v", err)} } + + // Auto-discover a MessageId when probe is requested and none was configured. + if msgId == "" && probe { + caps, err := client.ProbeEventService(ctx, true) + if err != nil { + return result{endpoint: s.Endpoint, outcome: outcomeFail, + detail: fmt.Sprintf("probe EventService: %v", err)} + } + if !caps.ServiceEnabled || !caps.HasSubmitTestEvent { + return result{endpoint: s.Endpoint, outcome: outcomeSkip, + detail: "EventService not enabled or SubmitTestEvent not supported"} + } + if caps.DiscoveredTestMessageId == "" { + return result{endpoint: s.Endpoint, outcome: outcomeSkip, + detail: "EventService enabled but no MessageId could be discovered from registries"} + } + fmt.Printf(" [discovered] %s → %s\n", s.Endpoint, caps.DiscoveredTestMessageId) + msgId = caps.DiscoveredTestMessageId + } + + if msgId == "" { + return result{endpoint: s.Endpoint, outcome: outcomeSkip, + detail: fmt.Sprintf( + "no testEventOverrides row for vendor=%q model=%q and --probe not set", + s.Vendor, s.Model, + )} + } + params := subscriptions.TestEventParams{ - MessageId: hw.TestMessageId, - Severity: hw.TestSeverity, - OriginOfCondition: hw.TestOriginOfCondition, + MessageId: msgId, + Severity: severity, + OriginOfCondition: origin, } if err := client.SubmitTestEvent(ctx, params); err != nil { return result{endpoint: s.Endpoint, outcome: outcomeFail, @@ -196,7 +230,7 @@ func runOne(s serverEntry, username, password string, "SubmitTestEvent(vendor=%q model=%q messageId=%q): %v", s.Vendor, s.Model, - hw.TestMessageId, + msgId, err, )} } @@ -205,7 +239,7 @@ func runOne(s serverEntry, username, password string, "SubmitTestEvent accepted (vendor=%q model=%q messageId=%q)", s.Vendor, s.Model, - hw.TestMessageId, + msgId, )} } diff --git a/internal/controller/maintenance/suite_test.go b/internal/controller/maintenance/suite_test.go index 1c14ee6c..34194074 100644 --- a/internal/controller/maintenance/suite_test.go +++ b/internal/controller/maintenance/suite_test.go @@ -48,6 +48,10 @@ const ( sanitizationNamespace = "metal-maintenance-sanitization" sanitizationImage = "metal-maintenance-sanitization:latest" + // BMCMockServerIP and BMCMockServerPort back the mock Redfish server that + // simcontrollers.BMCReconciler/ServerReconciler talk to, mirroring the + // baseboard/system packages' test setup so Server objects backed by a real + // (mocked) BMC progress through Parked state the same way everywhere. BMCMockServerIP = "127.0.0.1" BMCMockServerPort = int32(8400) ) @@ -103,14 +107,25 @@ var _ = BeforeSuite(func() { ObjectMeta: metav1.ObjectMeta{Name: sanitizationNamespace}, })).To(Succeed()) - // Mock server binds a fixed port so it is started once for the whole suite. - mockServer := mock.NewMockServer(GinkgoLogr, ":8002") + // Hardware-manager mock server binds a fixed port; started once for the whole suite. + hwMockServer := mock.NewMockServer(GinkgoLogr, ":8002") mockCtx, cancel := context.WithCancel(context.Background()) DeferCleanup(cancel) go func() { defer GinkgoRecover() - Expect(mockServer.Start(mockCtx)).To(Succeed(), "failed to start mock server") + Expect(hwMockServer.Start(mockCtx)).To(Succeed(), "failed to start mock server") + }() + + // Redfish mock server also binds a fixed port; started once here so each test's + // BeforeEach never races on port re-bind after the previous test's shutdown. + mockServer = mockserver.NewMockServer(GinkgoLogr, fmt.Sprintf(":%d", BMCMockServerPort), mockserver.WithAuth()) + redfishCtx, redfishCancel := context.WithCancel(context.Background()) + DeferCleanup(redfishCancel) + + go func() { + defer GinkgoRecover() + Expect(mockServer.Start(redfishCtx)).To(Succeed(), "failed to start Redfish mock server") }() }) @@ -218,13 +233,8 @@ func SetupTest(opts ...Option) *corev1.Namespace { }, }).SetupWithManager(k8sManager)).To(Succeed()) - By("Starting the mock Redfish server") - ms := mockserver.NewMockServer(GinkgoLogr, fmt.Sprintf(":%d", BMCMockServerPort), mockserver.WithAuth()) - mockServer = ms + By("Attaching the suite-level mock Redfish server to this test's manager") Expect(k8sManager.Add(manager.RunnableFunc(func(ctx context.Context) error { - if err := ms.Start(ctx); err != nil { - return fmt.Errorf("failed to start mock Redfish server: %w", err) - } <-ctx.Done() return nil }))).To(Succeed()) diff --git a/internal/controller/system/firmware_dell.go b/internal/controller/system/firmware_dell.go index 9e15cc82..3ed73aba 100644 --- a/internal/controller/system/firmware_dell.go +++ b/internal/controller/system/firmware_dell.go @@ -118,41 +118,10 @@ func (dh *dellHandler) processRepositoryCheck(ctx context.Context, updater bmc.F return false, err } if checkIssued.Status != metav1.ConditionTrue { - - checkCompleted, err := utils.GetCondition(r.Conditions, fw.Status.Conditions, ConditionRepositoryCheckCompleted) - if err != nil { - return false, err - } - repositoryCheckInterval := r.repositoryCheckInterval() - if checkCompleted.Status == metav1.ConditionTrue && - checkCompleted.Reason == ReasonRepositoryCheckCompleted && - repositoryCheckInterval > 0 && - fw.Status.ObservedGeneration == fw.Generation { - age := time.Since(checkCompleted.LastTransitionTime.Time) - if age < repositoryCheckInterval { - log.V(1).Info("RepositoryCheck recently completed, deferring check", - "Server", server.Name, "Age", age, "RepositoryCheckInterval", repositoryCheckInterval) - return false, nil - } - log.V(1).Info("RepositoryCheck stale, issuing periodic drift-detection check", - "Server", server.Name, "Age", age, "RepositoryCheckInterval", repositoryCheckInterval) - } else if checkCompleted.Status == metav1.ConditionTrue && - checkCompleted.Reason == ReasonRepositoryCheckCompleted && - fw.Status.ObservedGeneration != fw.Generation { - log.V(1).Info("FirmwareUpdate spec changed since last RepositoryCheck, bypassing periodic check interval", - "Server", server.Name) - } else { - log.V(1).Info("RepositoryCheck not yet issued, issuing dry-run check", "Server", server.Name) - } + log.V(1).Info("RepositoryCheck not yet issued, issuing dry-run check", "Server", server.Name) return dh.issueRepositoryCheck(ctx, updater, fw, r, server, checkIssued) } - // A RepositoryCheck job is actually in flight: this is the only phase of - // the periodic-check cycle need to enforce ProgressDeadlineSeconds. - if exceeded, err := r.checkProgressDeadline(ctx, fw); err != nil || exceeded { - return false, err - } - checkCompleted, err := utils.GetCondition(r.Conditions, fw.Status.Conditions, ConditionRepositoryCheckCompleted) if err != nil { return false, err @@ -387,7 +356,7 @@ func (dh *dellHandler) pollRepositoryCheck(ctx context.Context, updater bmc.Firm // Packages are pending installation: bound how many times we allow a // check to (re-)discover pending packages before giving up. passCount := fw.Status.PassCount + 1 - if maxRepositoryPasses := r.maxRepositoryPasses(); maxRepositoryPasses > 0 && passCount > maxRepositoryPasses { + if maxRepositoryPasses := r.MaxRepositoryPasses; maxRepositoryPasses > 0 && passCount > maxRepositoryPasses { log.Info("Exceeded maximum repository update passes, marking as Failed", "PassCount", passCount, "MaxRepositoryPasses", maxRepositoryPasses) if err := r.Conditions.Update( condition, @@ -436,7 +405,7 @@ func (dh *dellHandler) handleStuckScheduledJob( scheduledJobIDs []string, ) (*metav1.Condition, error) { log := ctrl.LoggerFrom(ctx) - scheduledJobPowerOnTimeout := r.scheduledJobPowerOnTimeout() + scheduledJobPowerOnTimeout := r.ScheduledJobPowerOnTimeout if scheduledJobPowerOnTimeout <= 0 { return nil, nil } diff --git a/internal/controller/system/firmwareupdate_controller.go b/internal/controller/system/firmwareupdate_controller.go index d479025c..28199d11 100644 --- a/internal/controller/system/firmwareupdate_controller.go +++ b/internal/controller/system/firmwareupdate_controller.go @@ -15,9 +15,7 @@ import ( "github.com/ironcore-dev/controller-utils/conditionutils" maintenancev1alpha1 "github.com/ironcore-dev/metal-maintenance-operator/api/maintenance/v1alpha1" systemv1alpha1 "github.com/ironcore-dev/metal-maintenance-operator/api/system/v1alpha1" - "github.com/ironcore-dev/metal-maintenance-operator/internal/configloader" constants "github.com/ironcore-dev/metal-maintenance-operator/internal/constants" - "github.com/ironcore-dev/metal-maintenance-operator/internal/controller/config" utils "github.com/ironcore-dev/metal-maintenance-operator/internal/utils" metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" "github.com/ironcore-dev/metal-operator/bmc" @@ -66,7 +64,26 @@ type FirmwareUpdateReconciler struct { SkipCertValidation bool Scheme *runtime.Scheme BMCOptions bmc.Options - Conditions *conditionutils.Accessor + ResyncInterval time.Duration + // PollingInterval is the requeue delay used while a firmware update is + // actively in progress (pending job polls, component tracking, etc.). + // Defaults to 2 minutes when unset or zero. + PollingInterval time.Duration + Conditions *conditionutils.Accessor + DefaultFailedAutoRetryCount int32 + // MaxRepositoryPasses bounds how many times a dry-run repository check may + // find further packages pending (and thus re-enter InProgress) before the + // FirmwareUpdate is marked Failed instead. + MaxRepositoryPasses int32 + // MinBMCResetInterval is the minimum time that must have passed since a BMC's last + // completed reset. Set to 0 to always reset. + // Has no effect for servers using an inline Spec.BMC (no managed BMC object to check). + MinBMCResetInterval time.Duration + // ScheduledJobPowerOnTimeout bounds how long a job may remain in the "Scheduled" state (queued, awaiting a reboot/power + // cycle to execute) with the server powered off and no observed progress, + // before the controller powers the server back on to let the job proceed. + // A value <= 0 disables this check. + ScheduledJobPowerOnTimeout time.Duration // ServerPowerOffTimeout bounds how long to wait for a graceful power-off to complete, before // retrying a Failed resource, before escalating to a forced power-off. A value <= 0 disables // escalation. @@ -76,84 +93,15 @@ type FirmwareUpdateReconciler struct { // before giving up on the retry. Giving up stops further automatic retries but does not // release maintenance. A value <= 0 disables the give-up check. ServerResetGiveUpTimeout time.Duration - // ConfigLoader, when set, supplies live-reloaded overrides (from a - // ConfigMap) for ResyncInterval, DefaultFailedAutoRetryCount, - // MaxRepositoryPasses, MinBMCResetInterval, ScheduledJobPowerOnTimeout, and - // RepositoryCheckInterval. Any field left unset in the loaded config (or - // the loader itself being nil, or not having loaded a config yet) falls - // back to the config package's built-in default constants (config.Default*). - ConfigLoader *configloader.Loader[config.FirmwareUpdateConfig] } -// resyncInterval returns the live ConfigMap-overridden ResyncInterval, or -// config.DefaultResyncInterval when no override is set. -func (r *FirmwareUpdateReconciler) resyncInterval() time.Duration { - if r.ConfigLoader != nil { - if cfg := r.ConfigLoader.Config(); cfg != nil && cfg.ResyncInterval != nil { - return *cfg.ResyncInterval - } - } - return config.DefaultResyncInterval -} - -// defaultFailedAutoRetryCount returns the live ConfigMap-overridden -// DefaultFailedAutoRetryCount, or config.DefaultDefaultFailedAutoRetryCount -// when no override is set. -func (r *FirmwareUpdateReconciler) defaultFailedAutoRetryCount() int32 { - if r.ConfigLoader != nil { - if cfg := r.ConfigLoader.Config(); cfg != nil && cfg.DefaultFailedAutoRetryCount != nil { - return *cfg.DefaultFailedAutoRetryCount - } - } - return config.DefaultDefaultFailedAutoRetryCount -} - -// maxRepositoryPasses returns the live ConfigMap-overridden -// MaxRepositoryPasses, or config.DefaultMaxRepositoryPasses when no override -// is set. -func (r *FirmwareUpdateReconciler) maxRepositoryPasses() int32 { - if r.ConfigLoader != nil { - if cfg := r.ConfigLoader.Config(); cfg != nil && cfg.MaxRepositoryPasses != nil { - return *cfg.MaxRepositoryPasses - } - } - return config.DefaultMaxRepositoryPasses -} - -// minBMCResetInterval returns the live ConfigMap-overridden -// MinBMCResetInterval, or config.DefaultMinBMCResetInterval when no override -// is set. -func (r *FirmwareUpdateReconciler) minBMCResetInterval() time.Duration { - if r.ConfigLoader != nil { - if cfg := r.ConfigLoader.Config(); cfg != nil && cfg.MinBMCResetInterval != nil { - return *cfg.MinBMCResetInterval - } - } - return config.DefaultMinBMCResetInterval -} +const defaultFirmwarePollingInterval = 2 * time.Minute -// scheduledJobPowerOnTimeout returns the live ConfigMap-overridden -// ScheduledJobPowerOnTimeout, or config.DefaultScheduledJobPowerOnTimeout when -// no override is set. -func (r *FirmwareUpdateReconciler) scheduledJobPowerOnTimeout() time.Duration { - if r.ConfigLoader != nil { - if cfg := r.ConfigLoader.Config(); cfg != nil && cfg.ScheduledJobPowerOnTimeout != nil { - return *cfg.ScheduledJobPowerOnTimeout - } - } - return config.DefaultScheduledJobPowerOnTimeout -} - -// repositoryCheckInterval returns the live ConfigMap-overridden -// RepositoryCheckInterval, or config.DefaultRepositoryCheckInterval when no -// override is set. -func (r *FirmwareUpdateReconciler) repositoryCheckInterval() time.Duration { - if r.ConfigLoader != nil { - if cfg := r.ConfigLoader.Config(); cfg != nil && cfg.RepositoryCheckInterval != nil { - return *cfg.RepositoryCheckInterval - } +func (r *FirmwareUpdateReconciler) pollingInterval() time.Duration { + if r.PollingInterval > 0 { + return r.PollingInterval } - return config.DefaultRepositoryCheckInterval + return defaultFirmwarePollingInterval } // +kubebuilder:rbac:groups=system.metal.ironcore.dev,resources=firmwareupdates,verbs=get;list;watch;create;update;patch;delete @@ -277,7 +225,7 @@ func (r *FirmwareUpdateReconciler) reconcile(ctx context.Context, fw *systemv1al return ctrl.Result{}, err } if requeue { - return ctrl.Result{RequeueAfter: r.resyncInterval()}, nil + return ctrl.Result{RequeueAfter: r.pollingInterval()}, nil } log.V(1).Info("Reconciled FirmwareUpdate") @@ -332,16 +280,10 @@ func (r *FirmwareUpdateReconciler) transitionState(ctx context.Context, fw *syst case systemv1alpha1.FirmwareUpdateStateCompleted: return h.handleCompleted(ctx, fw, bmcClient, r, server) case systemv1alpha1.FirmwareUpdateStateInProgress: - // handleInProgress runs first so that any recovery action it takes - // (e.g. the Dell handler's stuck-scheduled-job PowerOn) has a chance - // to bump fw.Status.LastProgressTime via patchProgress before checking deadline - requeue, handleErr := h.handleInProgress(ctx, fw, bmcClient, r, server) - - if exceeded, deadlineErr := r.checkProgressDeadline(ctx, fw); deadlineErr != nil || exceeded { - return false, errors.Join(handleErr, deadlineErr) + if exceeded, err := r.checkProgressDeadline(ctx, fw); err != nil || exceeded { + return false, err } - - return requeue, handleErr + return h.handleInProgress(ctx, fw, bmcClient, r, server) case systemv1alpha1.FirmwareUpdateStateFailed: return r.processFailedState(ctx, bmcClient, fw, server) } @@ -443,7 +385,7 @@ func (r *FirmwareUpdateReconciler) handleBMCReset( // due to an earlier, unrelated reset falling within the cooldown. SkipRecentlyResetCheck: conditionType == constants.ConditionRetryResetIssued, RecentlyReset: func(ctx context.Context) (bool, error) { - return utils.BMCWasRecentlyReset(ctx, r.Client, server, r.minBMCResetInterval()) + return utils.BMCWasRecentlyReset(ctx, r.Client, server, r.MinBMCResetInterval) }, IssueReset: func(ctx context.Context) error { return utils.ResetBMCOfServer(ctx, r.Client, server, bmcClient) @@ -463,10 +405,7 @@ func (r *FirmwareUpdateReconciler) checkProgressDeadline(ctx context.Context, fw return false, nil } deadline := time.Duration(*fw.Spec.ProgressDeadlineSeconds) * time.Second - // take the max of the two deadlines, so that the power-on recovery always gets a chance to run first. - if scheduledJobPowerOnTimeout := r.scheduledJobPowerOnTimeout(); scheduledJobPowerOnTimeout > 0 && scheduledJobPowerOnTimeout > deadline { - deadline = scheduledJobPowerOnTimeout - } + if time.Since(fw.Status.LastProgressTime.Time) <= deadline { return false, nil } @@ -478,7 +417,7 @@ func (r *FirmwareUpdateReconciler) checkProgressDeadline(ctx context.Context, fw condition, conditionutils.UpdateStatus(corev1.ConditionTrue), conditionutils.UpdateReason(constants.ReasonProgressDeadlineExceeded), - conditionutils.UpdateMessage(fmt.Sprintf("No progress observed within %ds", int(deadline.Seconds()))), + conditionutils.UpdateMessage(fmt.Sprintf("No progress observed within %ds", *fw.Spec.ProgressDeadlineSeconds)), ); err != nil { return false, fmt.Errorf("failed to update ProgressDeadlineExceeded condition: %w", err) } @@ -549,9 +488,9 @@ func (r *FirmwareUpdateReconciler) processFailedState(ctx context.Context, bmcCl if fw.Spec.RetryPolicy != nil && fw.Spec.RetryPolicy.MaxAttempts != nil { // if RetryPolicy is given (even if MaxAttempts is 0), do not use the default value. maxAttempts = *fw.Spec.RetryPolicy.MaxAttempts - } else if defaultFailedAutoRetryCount := r.defaultFailedAutoRetryCount(); defaultFailedAutoRetryCount > 0 { + } else if r.DefaultFailedAutoRetryCount > 0 { // set the retry to this, if the optional RetryPolicy is not given and default retry count is set on the reconciler. - maxAttempts = defaultFailedAutoRetryCount + maxAttempts = r.DefaultFailedAutoRetryCount } if maxAttempts > 0 { if fw.Status.ObservedGeneration != fw.Generation { @@ -718,15 +657,12 @@ func (r *FirmwareUpdateReconciler) updateStatus( return nil } -// patchProgress patches the top-level State, optionally merges condition (and -// any extraConditions) into the conditions slice (preserving all other -// conditions), and applies mutate to update vendor-specific job-tracking -// fields (e.g. DellStatus.CheckJob/UpdateJob/ComponentJobs/BaselineJobIDs) - -// all in a single status patch. -// -// LastProgressTime is only bumped to now when something observable actually -// changed (a condition transition, a State change, or any vendor-specific -// job-tracking field mutate touched - see jobTrackingFields). +// patchProgress patches the top-level State, optionally merges condition into +// the conditions slice (preserving all other conditions), and applies mutate +// to update vendor-specific job-tracking fields (e.g. DellStatus.CheckJob/ +// UpdateJob/ComponentJobs/BaselineJobIDs) - all in a single status patch. +// LastProgressTime advances only when actual progress occurs: a state +// transition, a condition update, or a change in job-tracking fields. func (r *FirmwareUpdateReconciler) patchProgress( ctx context.Context, fw *systemv1alpha1.FirmwareUpdate, @@ -737,13 +673,11 @@ func (r *FirmwareUpdateReconciler) patchProgress( ) error { fwBase := fw.DeepCopy() prevState := fw.Status.State - prevJobTracking := jobTrackingFields(&fw.Status) + prevDellStatus := fw.Status.DellStatus.DeepCopy() fw.Status.State = state fw.Status.ObservedGeneration = fw.Generation - progressed := condition != nil || state != prevState - if condition != nil { if err := r.Conditions.UpdateSlice( &fw.Status.Conditions, @@ -775,10 +709,9 @@ func (r *FirmwareUpdateReconciler) patchProgress( mutate(&fw.Status) } - if !progressed { - progressed = !reflect.DeepEqual(prevJobTracking, jobTrackingFields(&fw.Status)) - } - + progressed := condition != nil || + state != prevState || + !reflect.DeepEqual(prevDellStatus, fw.Status.DellStatus) if progressed || fw.Status.LastProgressTime == nil { now := metav1.Now() fw.Status.LastProgressTime = &now @@ -791,22 +724,6 @@ func (r *FirmwareUpdateReconciler) patchProgress( return nil } -// jobTrackingFields returns a copy of status with the fields patchProgress -// already accounts for separately (State, ObservedGeneration, Conditions, -// LastProgressTime) cleared, leaving only vendor-specific job-tracking fields -// (DellStatus today) for comparison. Diffing the whole status - rather than -// hardcoding a single vendor's status field - keeps progress detection -// correct as other vendor handlers (e.g. HPE, Lenovo) gain their own status -// fields alongside DellStatus. -func jobTrackingFields(status *systemv1alpha1.FirmwareUpdateStatus) systemv1alpha1.FirmwareUpdateStatus { - cp := *status.DeepCopy() - cp.State = "" - cp.ObservedGeneration = 0 - cp.Conditions = nil - cp.LastProgressTime = nil - return cp -} - func (r *FirmwareUpdateReconciler) patchServerMaintenanceRef(ctx context.Context, fw *systemv1alpha1.FirmwareUpdate, serverMaintenance *maintenancev1alpha1.ServerMaintenance) error { fwBase := fw.DeepCopy() diff --git a/internal/controller/system/firmwareupdate_controller_test.go b/internal/controller/system/firmwareupdate_controller_test.go index 202419fd..07d2fe12 100644 --- a/internal/controller/system/firmwareupdate_controller_test.go +++ b/internal/controller/system/firmwareupdate_controller_test.go @@ -11,9 +11,7 @@ import ( "github.com/ironcore-dev/controller-utils/conditionutils" maintenancev1alpha1 "github.com/ironcore-dev/metal-maintenance-operator/api/maintenance/v1alpha1" systemv1alpha1 "github.com/ironcore-dev/metal-maintenance-operator/api/system/v1alpha1" - "github.com/ironcore-dev/metal-maintenance-operator/internal/configloader" constants "github.com/ironcore-dev/metal-maintenance-operator/internal/constants" - fwconfig "github.com/ironcore-dev/metal-maintenance-operator/internal/controller/config" metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" "github.com/ironcore-dev/metal-operator/bmc" mockserver "github.com/ironcore-dev/metal-operator/bmc/mock/server" @@ -261,22 +259,18 @@ var _ = Describe("handleStuckScheduledJob", func() { const powerOnTimeout = time.Minute var ( - dh *dellHandler - r *FirmwareUpdateReconciler - server *metalv1alpha1.Server - fw *systemv1alpha1.FirmwareUpdate - fakeBMCImpl *fakeStuckJobBMC - firmwareUpdateConfigLoader *configloader.Loader[fwconfig.FirmwareUpdateConfig] + dh *dellHandler + r *FirmwareUpdateReconciler + server *metalv1alpha1.Server + fw *systemv1alpha1.FirmwareUpdate + fakeBMCImpl *fakeStuckJobBMC ) BeforeEach(func() { dh = &dellHandler{} - firmwareUpdateConfigLoader = &configloader.Loader[fwconfig.FirmwareUpdateConfig]{} - timeout := powerOnTimeout - firmwareUpdateConfigLoader.Set(&fwconfig.FirmwareUpdateConfig{ScheduledJobPowerOnTimeout: &timeout}) r = &FirmwareUpdateReconciler{ - Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), - ConfigLoader: firmwareUpdateConfigLoader, + Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), + ScheduledJobPowerOnTimeout: powerOnTimeout, } server = &metalv1alpha1.Server{ Spec: metalv1alpha1.ServerSpec{SystemURI: "/redfish/v1/Systems/1"}, @@ -298,8 +292,7 @@ var _ = Describe("handleStuckScheduledJob", func() { } It("does nothing when ScheduledJobPowerOnTimeout is disabled", func() { - disabled := time.Duration(0) - firmwareUpdateConfigLoader.Set(&fwconfig.FirmwareUpdateConfig{ScheduledJobPowerOnTimeout: &disabled}) + r.ScheduledJobPowerOnTimeout = 0 stuckSince := metav1.NewTime(time.Now().Add(-2 * powerOnTimeout)) fw.Status.LastProgressTime = &stuckSince @@ -406,15 +399,9 @@ var _ = Describe("checkProgressDeadline", func() { ) BeforeEach(func(ctx SpecContext) { - disabledScheduledJobPowerOnTimeout := time.Duration(0) - firmwareUpdateConfigLoader := &configloader.Loader[fwconfig.FirmwareUpdateConfig]{} - firmwareUpdateConfigLoader.Set(&fwconfig.FirmwareUpdateConfig{ - ScheduledJobPowerOnTimeout: &disabledScheduledJobPowerOnTimeout, - }) r = &FirmwareUpdateReconciler{ - Client: k8sClient, - Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), - ConfigLoader: firmwareUpdateConfigLoader, + Client: k8sClient, + Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), } ns = &v1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "test-"}} diff --git a/internal/controller/system/suite_test.go b/internal/controller/system/suite_test.go index cede216b..8b79eb8b 100644 --- a/internal/controller/system/suite_test.go +++ b/internal/controller/system/suite_test.go @@ -16,9 +16,7 @@ import ( "github.com/ironcore-dev/controller-utils/modutils" maintenancev1alpha1 "github.com/ironcore-dev/metal-maintenance-operator/api/maintenance/v1alpha1" systemv1alpha1 "github.com/ironcore-dev/metal-maintenance-operator/api/system/v1alpha1" - "github.com/ironcore-dev/metal-maintenance-operator/internal/configloader" constants "github.com/ironcore-dev/metal-maintenance-operator/internal/constants" - fwconfig "github.com/ironcore-dev/metal-maintenance-operator/internal/controller/config" maintenancectrl "github.com/ironcore-dev/metal-maintenance-operator/internal/controller/maintenance" "github.com/ironcore-dev/metal-maintenance-operator/internal/testutil/simcontrollers" metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" @@ -214,30 +212,21 @@ func SetupTest(redfishMockServers []netip.AddrPort, mockServerOpts ...mockserver ResyncInterval: 10 * time.Millisecond, }).SetupWithManager(k8sManager)).To(Succeed()) - firmwareUpdateResyncInterval := 10 * time.Millisecond - firmwareUpdateScheduledJobPowerOnTimeout := 1000 * time.Millisecond - firmwareUpdateDefaultFailedAutoRetryCount := int32(0) - firmwareUpdateMinBMCResetInterval := time.Duration(0) - firmwareUpdateConfigLoader := &configloader.Loader[fwconfig.FirmwareUpdateConfig]{} - firmwareUpdateConfigLoader.Set(&fwconfig.FirmwareUpdateConfig{ - ResyncInterval: &firmwareUpdateResyncInterval, - ScheduledJobPowerOnTimeout: &firmwareUpdateScheduledJobPowerOnTimeout, - DefaultFailedAutoRetryCount: &firmwareUpdateDefaultFailedAutoRetryCount, - MinBMCResetInterval: &firmwareUpdateMinBMCResetInterval, - }) Expect((&FirmwareUpdateReconciler{ Client: k8sManager.GetClient(), ManagerNamespace: ns.Name, DefaultProtocol: metalv1alpha1.HTTPProtocolScheme, SkipCertValidation: true, Scheme: k8sManager.GetScheme(), + ResyncInterval: 10 * time.Millisecond, + PollingInterval: 10 * time.Millisecond, Conditions: accessor, BMCOptions: bmc.Options{ PowerPollingInterval: 50 * time.Millisecond, PowerPollingTimeout: 200 * time.Millisecond, BasicAuth: true, }, - ConfigLoader: firmwareUpdateConfigLoader, + MaxRepositoryPasses: 5, }).SetupWithManager(k8sManager)).To(Succeed()) // simcontrollers.BMCReconciler/ServerReconciler mimic metal-operator's real diff --git a/internal/controller/vendorconsole/console_controller.go b/internal/controller/vendorconsole/console_controller.go index 1f3207c7..01ba573e 100644 --- a/internal/controller/vendorconsole/console_controller.go +++ b/internal/controller/vendorconsole/console_controller.go @@ -89,11 +89,6 @@ func (r *ConsoleReconciler) reconcileExists(ctx context.Context, console *vendor } logger.Info("Successfully created console client", "manufacturer", console.Spec.Manufacturer) - if !consoleClient.HasConsole() { - logger.Info("Skipping import scheduling: vendor has no central management console", "manufacturer", console.Spec.Manufacturer) - return ctrl.Result{}, nil - } - if err := r.updateSecretToken(ctx, secret, consoleClient); err != nil { logger.Error(err, "unable to update console credential secret with token") return ctrl.Result{}, err diff --git a/internal/discovery/handler.go b/internal/discovery/handler.go index 917a14fe..b887fec4 100644 --- a/internal/discovery/handler.go +++ b/internal/discovery/handler.go @@ -36,9 +36,21 @@ type Target struct { // Handler serves the SD endpoint. Reads the live BMC list from the // controller-runtime cache on each request — no in-memory cache of // rendered output. +// +// SubscribedBMCs is optional. When set, it is called on each request to +// obtain the set of BMC names that currently have active Redfish event-push +// subscriptions. Those BMCs are omitted from the response so the external +// redfish-exporter does not scrape them — the operator's push pipeline +// covers them already. type Handler struct { - Client client.Client - Log logr.Logger + Client client.Client + Log logr.Logger + SubscribedBMCs func() map[string]struct{} + + // UnsubscribedBMCReasons is optional. When set, it is called on each request + // to obtain a map of bmcName → reason for BMCs not push-subscribed. The reason + // is added as __meta_bmc_subscription_reason label on each SD target. + UnsubscribedBMCReasons func() map[string]string } func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { @@ -55,7 +67,17 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { return } - targets := Render(list.Items) + var subscribed map[string]struct{} + if h.SubscribedBMCs != nil { + subscribed = h.SubscribedBMCs() + } + + var reasons map[string]string + if h.UnsubscribedBMCReasons != nil { + reasons = h.UnsubscribedBMCReasons() + } + + targets := Render(list.Items, subscribed, reasons) w.Header().Set("Content-Type", "application/json") w.Header().Set("Cache-Control", "no-store") if err := json.NewEncoder(w).Encode(targets); err != nil { @@ -64,9 +86,13 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { } // Render projects the BMC list onto Prometheus SD targets. +// subscribed is the optional set of BMC names to exclude (those with active +// push subscriptions that the external exporter should not scrape). +// reasons is the optional map of bmcName → subscription reason for BMCs +// that are not push-subscribed; the reason is added as a label on each target. // Extracted from ServeHTTP so the contract is unit-testable without an // HTTP server. -func Render(bmcs []metalv1alpha1.BMC) []Target { +func Render(bmcs []metalv1alpha1.BMC, subscribed map[string]struct{}, reasons map[string]string) []Target { out := make([]Target, 0, len(bmcs)) for i := range bmcs { b := &bmcs[i] @@ -76,9 +102,12 @@ func Render(bmcs []metalv1alpha1.BMC) []Target { if !b.Status.IP.IsValid() { continue } + if _, ok := subscribed[b.Name]; ok { + continue + } out = append(out, Target{ Targets: []string{hostPort(b)}, - Labels: labelsFor(b), + Labels: labelsFor(b, reasons[b.Name]), }) } return out @@ -93,7 +122,7 @@ func hostPort(b *metalv1alpha1.BMC) string { } // labelsFor builds the Prometheus meta-labels for one BMC. -func labelsFor(b *metalv1alpha1.BMC) map[string]string { +func labelsFor(b *metalv1alpha1.BMC, subscriptionReason string) map[string]string { labels := map[string]string{ "__meta_bmc_name": b.Name, "__meta_bmc_namespace": b.Namespace, @@ -119,6 +148,9 @@ func labelsFor(b *metalv1alpha1.BMC) map[string]string { labels["__meta_bmc_secret_name"] = name labels["__meta_bmc_secret_namespace"] = b.Namespace } + if subscriptionReason != "" { + labels["__meta_bmc_subscription_reason"] = subscriptionReason + } return labels } diff --git a/internal/discovery/handler_test.go b/internal/discovery/handler_test.go index 89de87f4..e36ee851 100644 --- a/internal/discovery/handler_test.go +++ b/internal/discovery/handler_test.go @@ -63,7 +63,7 @@ func readyBMC(name string, opts ...func(*metalv1alpha1.BMC)) *metalv1alpha1.BMC func TestRender_HappyPath(t *testing.T) { b := readyBMC("bmc-1") - got := discovery.Render([]metalv1alpha1.BMC{*b}) + got := discovery.Render([]metalv1alpha1.BMC{*b}, nil, nil) if len(got) != 1 { t.Fatalf("targets: got %d, want 1: %+v", len(got), got) @@ -92,7 +92,7 @@ func TestRender_DropsBMCsWithoutIP(t *testing.T) { noIP := readyBMC("no-ip", func(b *metalv1alpha1.BMC) { b.Status.IP = metalv1alpha1.IP{} }) - got := discovery.Render([]metalv1alpha1.BMC{*noIP}) + got := discovery.Render([]metalv1alpha1.BMC{*noIP}, nil, nil) if len(got) != 0 { t.Errorf("BMC without IP should be omitted; got %+v", got) } @@ -104,7 +104,7 @@ func TestRender_DropsDeletionPendingBMCs(t *testing.T) { b.DeletionTimestamp = &now b.Finalizers = []string{"a-finalizer-keeping-it-alive"} // required by fake client }) - got := discovery.Render([]metalv1alpha1.BMC{*deleting}) + got := discovery.Render([]metalv1alpha1.BMC{*deleting}, nil, nil) if len(got) != 0 { t.Errorf("deletion-pending BMC should be omitted; got %+v", got) } @@ -114,7 +114,7 @@ func TestRender_OmitsPortWhenZero(t *testing.T) { b := readyBMC("noport", func(b *metalv1alpha1.BMC) { b.Spec.Protocol.Port = 0 }) - got := discovery.Render([]metalv1alpha1.BMC{*b}) + got := discovery.Render([]metalv1alpha1.BMC{*b}, nil, nil) if len(got) != 1 || got[0].Targets[0] != "10.0.0.1" { t.Errorf("zero-port should yield bare host; got %v", got[0].Targets) } @@ -124,7 +124,7 @@ func TestRender_DefaultsSchemeToHTTPS(t *testing.T) { b := readyBMC("noscheme", func(b *metalv1alpha1.BMC) { b.Spec.Protocol.Scheme = "" }) - got := discovery.Render([]metalv1alpha1.BMC{*b}) + got := discovery.Render([]metalv1alpha1.BMC{*b}, nil, nil) if got[0].Labels["__meta_bmc_protocol"] != "https" { t.Errorf("default protocol: got %q, want https", got[0].Labels["__meta_bmc_protocol"]) } @@ -141,7 +141,7 @@ func TestRender_OmitsEmptyOptionalLabels(t *testing.T) { IP: metalv1alpha1.MustParseIP("10.0.0.2"), }, } - got := discovery.Render([]metalv1alpha1.BMC{*b}) + got := discovery.Render([]metalv1alpha1.BMC{*b}, nil, nil) if len(got) != 1 { t.Fatalf("expected 1 target: %+v", got) } @@ -162,6 +162,51 @@ func TestRender_OmitsEmptyOptionalLabels(t *testing.T) { // -- HTTP handler tests -- +func TestRender_OmitsSubscribedBMCs(t *testing.T) { + b1 := readyBMC("bmc-1") + b2 := readyBMC("bmc-2") + subscribed := map[string]struct{}{"bmc-1": {}} + got := discovery.Render([]metalv1alpha1.BMC{*b1, *b2}, subscribed, nil) + if len(got) != 1 { + t.Fatalf("expected 1 target (bmc-2 only); got %d: %+v", len(got), got) + } + if got[0].Labels["__meta_bmc_name"] != "bmc-2" { + t.Errorf("expected bmc-2; got %q", got[0].Labels["__meta_bmc_name"]) + } +} + +func TestRender_IncludesAllWhenNoSubscribed(t *testing.T) { + b1 := readyBMC("bmc-1") + b2 := readyBMC("bmc-2") + got := discovery.Render([]metalv1alpha1.BMC{*b1, *b2}, nil, nil) + if len(got) != 2 { + t.Errorf("expected 2 targets; got %d", len(got)) + } +} + +func TestServeHTTP_OmitsSubscribedWhenFuncSet(t *testing.T) { + b1 := readyBMC("bmc-1") + b2 := readyBMC("bmc-2") + c := newClient(t, b1, b2) + h := &discovery.Handler{ + Client: c, + SubscribedBMCs: func() map[string]struct{} { + return map[string]struct{}{"bmc-1": {}} + }, + } + req := httptest.NewRequest(http.MethodGet, discovery.Path, nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + var got []discovery.Target + if err := json.NewDecoder(rec.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got) != 1 || got[0].Labels["__meta_bmc_name"] != "bmc-2" { + t.Errorf("expected only bmc-2; got %+v", got) + } +} + func TestServeHTTP_GETReturnsSDJSON(t *testing.T) { c := newClient(t, readyBMC("bmc-1"), readyBMC("bmc-2")) h := &discovery.Handler{Client: c} @@ -237,6 +282,68 @@ func TestServeHTTP_EmptyInventoryReturnsEmptyArray(t *testing.T) { } } +func TestRender_SubscriptionReasonLabel(t *testing.T) { + b := readyBMC("bmc-1") + reasons := map[string]string{"bmc-1": "service_disabled"} + got := discovery.Render([]metalv1alpha1.BMC{*b}, nil, reasons) + if len(got) != 1 { + t.Fatalf("expected 1 target; got %d", len(got)) + } + if v := got[0].Labels["__meta_bmc_subscription_reason"]; v != "service_disabled" { + t.Errorf("__meta_bmc_subscription_reason: got %q, want %q", v, "service_disabled") + } +} + +func TestRender_NoSubscriptionReasonWhenReasonEmpty(t *testing.T) { + b := readyBMC("bmc-1") + // Pass a reasons map that has no entry for bmc-1. + reasons := map[string]string{"other-bmc": "excluded"} + got := discovery.Render([]metalv1alpha1.BMC{*b}, nil, reasons) + if len(got) != 1 { + t.Fatalf("expected 1 target; got %d", len(got)) + } + if _, ok := got[0].Labels["__meta_bmc_subscription_reason"]; ok { + t.Errorf("__meta_bmc_subscription_reason should be absent when reason is empty") + } +} + +func TestServeHTTP_ReasonLabelFromUnsubscribedFunc(t *testing.T) { + b1 := readyBMC("bmc-1") + b2 := readyBMC("bmc-2") + c := newClient(t, b1, b2) + h := &discovery.Handler{ + Client: c, + UnsubscribedBMCReasons: func() map[string]string { + return map[string]string{ + "bmc-1": "service_disabled", + "bmc-2": "excluded", + } + }, + } + req := httptest.NewRequest(http.MethodGet, discovery.Path, nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + var got []discovery.Target + if err := json.NewDecoder(rec.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got) != 2 { + t.Fatalf("expected 2 targets; got %d: %+v", len(got), got) + } + reasons := map[string]string{} + for _, tgt := range got { + name := tgt.Labels["__meta_bmc_name"] + reasons[name] = tgt.Labels["__meta_bmc_subscription_reason"] + } + if reasons["bmc-1"] != "service_disabled" { + t.Errorf("bmc-1 reason: got %q, want service_disabled", reasons["bmc-1"]) + } + if reasons["bmc-2"] != "excluded" { + t.Errorf("bmc-2 reason: got %q, want excluded", reasons["bmc-2"]) + } +} + // errListClient is a minimal client.Client that returns the given error // from List. Other methods panic — we don't exercise them. type errListClient struct { diff --git a/internal/hwmgr/fujitsu.go b/internal/hwmgr/fujitsu.go deleted file mode 100644 index 946358b8..00000000 --- a/internal/hwmgr/fujitsu.go +++ /dev/null @@ -1,100 +0,0 @@ -// SPDX-FileCopyrightText: 2025 SAP SE or an SAP affiliate company and IronCore contributors -// SPDX-License-Identifier: Apache-2.0 - -package hwmgr - -import ( - "github.com/go-logr/logr" - metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" -) - -// FujitsuClient is a placeholder implementation of ClientInterface for Fujitsu -// servers. Fujitsu does not have a central vendor management console (unlike -// Dell OME, HPE OneView, or Lenovo XClarity), so all operations are no-ops. -type FujitsuClient struct { - log logr.Logger -} - -// NewFujitsuClient creates a no-op client for Fujitsu servers. -func NewFujitsuClient(_ ClientOptions) (*FujitsuClient, error) { - return &FujitsuClient{ - log: logr.Discard(), - }, nil -} - -// NoConsole signals that Fujitsu has no central management console, -// so the controller will skip import scheduling for Fujitsu servers. -func (c *FujitsuClient) NoConsole() {} - -// ListServers returns an empty list. Fujitsu has no central vendor management -// console to query for managed servers. -func (c *FujitsuClient) ListServers() ([]Device, error) { - c.log.Info("ListServers is a no-op for Fujitsu: no central vendor management console") - return []Device{}, nil -} - -// ImportServer is a no-op. Fujitsu has no central vendor management console to -// import servers into. -func (c *FujitsuClient) ImportServer( - hostname string, - _ metalv1alpha1.IP, - _ string, - _ string, -) error { - c.log.Info("ImportServer is a no-op for Fujitsu: no central vendor management console", "hostname", hostname) - return nil -} - -// RemoveServer is a no-op. Fujitsu has no central vendor management console to -// remove servers from. -func (c *FujitsuClient) RemoveServer( - hostname string, - _ metalv1alpha1.IP, -) error { - c.log.Info("RemoveServer is a no-op for Fujitsu: no central vendor management console", "hostname", hostname) - return nil -} - -// GetAuthToken is a no-op. Fujitsu has no central vendor management console -// requiring authentication. -func (c *FujitsuClient) GetAuthToken() (string, error) { - return "", nil -} - -// ImportServerAsync is a no-op wrapper. -func (c *FujitsuClient) ImportServerAsync( - hostname string, - ip metalv1alpha1.IP, - _ string, - _ string, -) (string, error) { - return "", c.ImportServer(hostname, ip, "", "") -} - -// RemoveServerAsync is a no-op wrapper. -func (c *FujitsuClient) RemoveServerAsync( - hostname string, - ip metalv1alpha1.IP, -) (string, error) { - return "", c.RemoveServer(hostname, ip) -} - -// GetJobStatus returns a completed status. Fujitsu operations are synchronous -// no-ops so no real job tracking is needed. -func (c *FujitsuClient) GetJobStatus(jobID string) (*JobInfo, error) { - return &JobInfo{ - JobID: jobID, - Status: jobStatusCompleted, - Progress: 100, - }, nil -} - -// IsJobComplete always returns true for Fujitsu no-op operations. -func (c *FujitsuClient) IsJobComplete(_ *JobInfo) bool { - return true -} - -// IsJobSuccessful always returns true for Fujitsu no-op operations. -func (c *FujitsuClient) IsJobSuccessful(_ *JobInfo) bool { - return true -} diff --git a/internal/hwmgr/fujitsu_test.go b/internal/hwmgr/fujitsu_test.go deleted file mode 100644 index 4ac93c75..00000000 --- a/internal/hwmgr/fujitsu_test.go +++ /dev/null @@ -1,110 +0,0 @@ -// SPDX-FileCopyrightText: 2025 SAP SE or an SAP affiliate company and IronCore contributors -// SPDX-License-Identifier: Apache-2.0 - -package hwmgr - -import ( - "net/netip" - "testing" - - metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" -) - -func TestNewFujitsuClient(t *testing.T) { - c, err := NewFujitsuClient(ClientOptions{ - Endpoint: "https://irmc.example.com", - Username: "admin", - Password: "password", - }) - if err != nil { - t.Fatalf("NewFujitsuClient() returned error: %v", err) - } - - if c == nil { - t.Fatal("expected non-nil client") - } -} - -func TestFujitsuListServers(t *testing.T) { - c, _ := NewFujitsuClient(ClientOptions{}) - - devices, err := c.ListServers() - if err != nil { - t.Fatalf("ListServers() returned error: %v", err) - } - - if len(devices) != 0 { - t.Errorf("expected empty device list, got %d devices", len(devices)) - } -} - -func TestFujitsuImportServer(t *testing.T) { - c, _ := NewFujitsuClient(ClientOptions{}) - - ip := metalv1alpha1.IP{Addr: netip.MustParseAddr("192.168.1.100")} - - err := c.ImportServer("FujitsuServer", ip, "admin", "password") - if err != nil { - t.Fatalf("ImportServer() returned error: %v", err) - } -} - -func TestFujitsuRemoveServer(t *testing.T) { - c, _ := NewFujitsuClient(ClientOptions{}) - - ip := metalv1alpha1.IP{Addr: netip.MustParseAddr("192.168.1.100")} - - err := c.RemoveServer("FujitsuServer", ip) - if err != nil { - t.Fatalf("RemoveServer() returned error: %v", err) - } -} - -func TestFujitsuRemoveServerAsync(t *testing.T) { - c, _ := NewFujitsuClient(ClientOptions{}) - - ip := metalv1alpha1.IP{Addr: netip.MustParseAddr("192.168.1.100")} - - jobID, err := c.RemoveServerAsync("FujitsuServer", ip) - if err != nil { - t.Fatalf("RemoveServerAsync() returned error: %v", err) - } - - if jobID != "" { - t.Errorf("expected empty job ID, got %q", jobID) - } -} - -func TestFujitsuGetJobStatus(t *testing.T) { - c, _ := NewFujitsuClient(ClientOptions{}) - - jobInfo, err := c.GetJobStatus("job-123") - if err != nil { - t.Fatalf("GetJobStatus() returned error: %v", err) - } - - if jobInfo == nil { - t.Fatal("expected JobInfo, got nil") - } - - if jobInfo.JobID != "job-123" { - t.Errorf("expected job ID job-123, got %q", jobInfo.JobID) - } - - if jobInfo.Status != jobStatusCompleted { - t.Errorf("expected status %q, got %q", jobStatusCompleted, jobInfo.Status) - } - - if jobInfo.Progress != 100 { - t.Errorf("expected progress 100, got %d", jobInfo.Progress) - } -} - -func TestFujitsuHasNoConsole(t *testing.T) { - c, _ := NewFujitsuClient(ClientOptions{}) - - wrapped := &Client{ClientInterface: c} - if wrapped.HasConsole() { - t.Error("expected HasConsole to return false for Fujitsu") - } -} diff --git a/internal/hwmgr/httpclient.go b/internal/hwmgr/httpclient.go index 23a5d3f7..588ccd9f 100644 --- a/internal/hwmgr/httpclient.go +++ b/internal/hwmgr/httpclient.go @@ -78,12 +78,7 @@ func (c *client) DoRequest(req *http.Request, okCodes []int) ([]byte, error) { return nil, errors.New("the URL is mandatory") } if c.basicAuth { - // Accept: application/json is required for Fujitsu iRMC S6 — - // without it the server returns HTTP 406 (Fsas.1.0.AcceptNotSupported). - req.Header = http.Header{ - "Content-Type": []string{"application/json"}, - "Accept": []string{"application/json"}, - } + req.Header = http.Header{"Content-Type": []string{"application/json"}} req.SetBasicAuth(c.username, c.password) } else { if c.token == "" { diff --git a/internal/hwmgr/types.go b/internal/hwmgr/types.go index 95df1a64..460f9890 100644 --- a/internal/hwmgr/types.go +++ b/internal/hwmgr/types.go @@ -20,7 +20,6 @@ type Device struct { Name string `json:"Name"` Hostname string `json:"Hostname"` Model string `json:"Model"` - Serial string `json:"SerialNumber"` HealthStatus int `json:"HealthStatus"` // 4000 = OK, 4002 = Warning, etc. } @@ -55,24 +54,11 @@ type ClientInterface interface { IsJobSuccessful(jobInfo *JobInfo) bool } -// noConsoleBackend is implemented by vendors that have no central management -// console (e.g. Fujitsu). Used by HasConsole to skip import scheduling. -type noConsoleBackend interface { - NoConsole() -} - type Client struct { Manufacturer bmc.Manufacturer ClientInterface } -// HasConsole returns false for vendors that have no central management console, -// meaning import/remove operations should not be scheduled. -func (c *Client) HasConsole() bool { - _, noConsole := c.ClientInterface.(noConsoleBackend) - return !noConsole -} - func New(manufacturer bmc.Manufacturer, options ClientOptions) (client *Client, err error) { client = &Client{} client.Manufacturer = manufacturer @@ -86,9 +72,6 @@ func New(manufacturer bmc.Manufacturer, options ClientOptions) (client *Client, case bmc.ManufacturerHPE: client.ClientInterface, err = NewHPEClient(options) return - case bmc.ManufacturerFujitsu: - client.ClientInterface, err = NewFujitsuClient(options) - return } return } diff --git a/internal/telemetry/runtime/config_loader.go b/internal/telemetry/runtime/config_loader.go new file mode 100644 index 00000000..b68e0a9a --- /dev/null +++ b/internal/telemetry/runtime/config_loader.go @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package runtime + +import ( + "context" + "errors" + "fmt" + "sync/atomic" + + "github.com/go-logr/logr" + "github.com/ironcore-dev/metal-maintenance-operator/internal/telemetry/subscriptions" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + toolscache "k8s.io/client-go/tools/cache" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ConfigKey is the well-known ConfigMap data key the loader reads. +const ConfigKey = "config.yaml" + +// ConfigLoader watches the telemetry ConfigMap and atomic-swaps a +// *subscriptions.Config pointer on every change. The most-recent +// successfully-parsed config is always available via Config(); validation +// failures keep the last-good config in place and log a warning. +type ConfigLoader struct { + Cache cache.Cache // controller-runtime cache (manager.GetCache()) + Client client.Client // cache-backed reader, for the initial Get and reload reads + Namespace string + Name string + Log logr.Logger + + current atomic.Pointer[subscriptions.Config] +} + +// Config returns the most recent successfully-parsed config. +func (l *ConfigLoader) Config() *subscriptions.Config { + return l.current.Load() +} + +// Start installs the watch and blocks until ctx is cancelled. Implements +// manager.Runnable. +func (l *ConfigLoader) Start(ctx context.Context) error { + if l.Cache == nil { + return errors.New("ConfigLoader: Cache is required") + } + if l.Client == nil { + return errors.New("ConfigLoader: Client is required") + } + if l.Namespace == "" || l.Name == "" { + return errors.New("ConfigLoader: Namespace and Name are required") + } + + // Wait for the cache to be populated before the initial read; without + // this the cache-backed client returns "not ready" and the load is + // silently deferred until the first watch event. + if !l.Cache.WaitForCacheSync(ctx) { + if err := ctx.Err(); err != nil { + return err + } + return fmt.Errorf("ConfigLoader: cache sync timed out or context cancelled") + } + + if err := l.reload(ctx); err != nil { + l.Log.Info("Initial ConfigMap load deferred to watch", "reason", err.Error()) + } + + // The cache is scoped to l.Namespace/l.Name via cache.ByObject in cmd/main.go. + inf, err := l.Cache.GetInformer(ctx, &corev1.ConfigMap{}) + if err != nil { + return fmt.Errorf("ConfigLoader: get ConfigMap informer: %w", err) + } + reg, err := inf.AddEventHandler(l.handler(ctx)) + if err != nil { + return fmt.Errorf("ConfigLoader: add event handler: %w", err) + } + defer func() { _ = inf.RemoveEventHandler(reg) }() + + <-ctx.Done() + l.Log.Info("Config loader stopped") + return nil +} + +func (l *ConfigLoader) handler(ctx context.Context) toolscache.ResourceEventHandler { + onChange := func(obj any) { + if _, ok := obj.(*corev1.ConfigMap); !ok { + return + } + if err := l.reload(ctx); err != nil { + l.Log.Error(err, "ConfigMap reload failed; keeping last-good config") + } + } + return toolscache.ResourceEventHandlerFuncs{ + AddFunc: onChange, + UpdateFunc: func(_, newObj any) { onChange(newObj) }, + // Deletes leave the last-good config in place. + } +} + +// reload fetches the ConfigMap, parses + validates the config.yaml key. +func (l *ConfigLoader) reload(ctx context.Context) error { + cm := &corev1.ConfigMap{} + if err := l.Client.Get(ctx, types.NamespacedName{Name: l.Name, Namespace: l.Namespace}, cm); err != nil { + if apierrors.IsNotFound(err) { + return fmt.Errorf("ConfigMap %s/%s not found", l.Namespace, l.Name) + } + return fmt.Errorf("get ConfigMap %s/%s: %w", l.Namespace, l.Name, err) + } + raw, ok := cm.Data[ConfigKey] + if !ok { + return fmt.Errorf("ConfigMap %s/%s is missing key %q", l.Namespace, l.Name, ConfigKey) + } + cfg, errList := subscriptions.Parse([]byte(raw)) + if len(errList) > 0 { + return fmt.Errorf("invalid telemetry config: %w", errors.New(errList.ToAggregate().Error())) + } + previous := l.current.Swap(cfg) + if previous == nil { + l.Log.Info("Initial telemetry config loaded", + "subscriptionReconcileInterval", cfg.SubscriptionReconcileInterval, + "perBMCTimeout", cfg.PerBMCTimeout, + "excludeRows", len(cfg.ExcludeHardware), + "testOverrideRows", len(cfg.TestEventOverrides)) + } else { + l.Log.V(1).Info("Telemetry config reloaded") + } + return nil +} diff --git a/internal/telemetry/runtime/runtime.go b/internal/telemetry/runtime/runtime.go index 2b29f516..8eefd0df 100644 --- a/internal/telemetry/runtime/runtime.go +++ b/internal/telemetry/runtime/runtime.go @@ -18,9 +18,12 @@ import ( "context" "fmt" "net/url" + "slices" + "strings" "time" "github.com/stmcginnis/gofish" + redfishschemas "github.com/stmcginnis/gofish/schemas" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/manager" ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -59,6 +62,18 @@ type Options struct { // Warning-severity Redfish alerts. Can be enabled independently of // EnableCriticalEventHandler. EnableWarningEventHandler bool + + // ActiveSubscriptions is populated by AddTo with a function that + // returns the current set of BMC names with active push subscriptions. + // Callers can pass this to discovery.Handler.SubscribedBMCs so the + // Prometheus SD endpoint omits subscribed BMCs. + // This field is an output; callers must not set it before calling AddTo. + ActiveSubscriptions func() map[string]struct{} + + // UnsubscribedBMCReasons is populated by AddTo with a function that + // returns a map of bmcName → reason for BMCs not push-subscribed. + // This field is an output; callers must not set it before calling AddTo. + UnsubscribedBMCReasons func() map[string]string } // +kubebuilder:rbac:groups=metal.ironcore.dev,resources=bmcs,verbs=get;list;patch;watch @@ -69,8 +84,13 @@ type Options struct { // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // AddTo registers the event-push pipeline on the given manager. -func AddTo(mgr manager.Manager, opts Options) error { - if err := validateOptions(&opts); err != nil { +// opts must be non-nil. On return, opts.ActiveSubscriptions and +// opts.UnsubscribedBMCReasons are populated with live callbacks. +func AddTo(mgr manager.Manager, opts *Options) error { + if opts == nil { + return fmt.Errorf("options are required") + } + if err := validateOptions(opts); err != nil { return err } if err := validateReceiverURL(opts.ReceiverURL); err != nil { @@ -95,6 +115,10 @@ func AddTo(mgr manager.Manager, opts Options) error { if err != nil { return fmt.Errorf("init test-event sink: %w", err) } + statusSink, err := promsink.NewSubscriptionStatusSink(ctrlmetrics.Registry) + if err != nil { + return fmt.Errorf("init subscription status sink: %w", err) + } var criticalHandler *criticalevent.ConditionHandler @@ -186,6 +210,7 @@ func AddTo(mgr manager.Manager, opts Options) error { MetricReportSink: metricSink, AlertHandler: criticalHandler, TestRecorder: testSink, + StatusRecorder: statusSink, ReceiverURL: opts.ReceiverURL, SubscriberID: opts.SubscriberID, // ReconcileInterval and PerBMCTimeout intentionally unset: @@ -198,6 +223,11 @@ func AddTo(mgr manager.Manager, opts Options) error { return fmt.Errorf("setup subscription reconciler: %w", err) } + // Expose the live set of subscribed BMC names so callers can wire it into + // the discovery endpoint to omit push-subscribed BMCs from scrape targets. + opts.ActiveSubscriptions = subReconciler.ActiveSubscriptions + opts.UnsubscribedBMCReasons = subReconciler.UnsubscribedBMCReasons + // Wire the receiver's test notifier to the reconciler so round-trip // confirmations update the health-check metric. receiver.SetTestNotifier(subReconciler) @@ -205,12 +235,6 @@ func AddTo(mgr manager.Manager, opts Options) error { return nil } -// ConfigKey is the ConfigMap data key holding the telemetry config.yaml -// payload. Kept as a package-level const (rather than requiring callers to -// depend on internal/configloader.DefaultKey directly) for backward -// compatibility with existing external references (e.g. hack/verify-test-events). -const ConfigKey = configloader.DefaultKey - // parseSubscriptionsConfig adapts subscriptions.Parse (which returns a // field.ErrorList) to the plain `func([]byte) (*T, error)` signature // internal/configloader.Loader[T] requires. @@ -357,6 +381,84 @@ func (c *extendedClient) SubmitTestEvent(_ context.Context, params subscriptions return err } +func (c *extendedClient) ProbeEventService(_ context.Context, needsDiscovery bool) (*subscriptions.EventServiceCapabilities, error) { + svc := c.api.GetService() + if svc == nil { + return nil, fmt.Errorf("probe event service: no service root") + } + es, err := svc.EventService() + if err != nil { + return nil, fmt.Errorf("probe event service: %w", err) + } + caps := &subscriptions.EventServiceCapabilities{ + ServiceEnabled: es.ServiceEnabled, + HasSubmitTestEvent: es.SubmitTestEventTarget != "", + } + if !caps.ServiceEnabled || !caps.HasSubmitTestEvent || !needsDiscovery { + return caps, nil + } + caps.DiscoveredTestMessageId = discoverTestMessageId(svc, es.RegistryPrefixes) + return caps, nil +} + +// discoverTestMessageId walks the BMC's MessageRegistryFile collection and +// returns the first viable MessageId in the form +// "{RegistryPrefix}.{RegistryVersion}.{Key}" for a registry whose prefix +// appears in the advertised prefixes list. Returns an empty string when no +// usable entry is found. +func discoverTestMessageId(svc *gofish.Service, prefixes []string) string { + if len(prefixes) == 0 { + return "" + } + // Build a set for O(1) prefix lookups. + want := make(map[string]struct{}, len(prefixes)) + for _, p := range prefixes { + want[p] = struct{}{} + } + + files, err := svc.Registries() + if err != nil || len(files) == 0 { + return "" + } + + gofishClient := svc.GetClient() + for _, f := range files { + // f.Registry is e.g. "iLO.1.0" or "Base.1.15"; the prefix is the + // part before the first dot. + prefix := registryPrefix(f.Registry) + if _, ok := want[prefix]; !ok { + continue + } + for _, loc := range f.Location { + if loc.URI == "" { + continue + } + mr, err := redfishschemas.GetMessageRegistry(gofishClient, loc.URI) + if err != nil || mr == nil || len(mr.Messages) == 0 { + continue + } + keys := make([]string, 0, len(mr.Messages)) + for key := range mr.Messages { + keys = append(keys, key) + } + slices.Sort(keys) + ver := mr.RegistryVersion + if parts := strings.SplitN(ver, ".", 3); len(parts) >= 2 { + ver = parts[0] + "." + parts[1] + } + return mr.RegistryPrefix + "." + ver + "." + keys[0] + } + } + return "" +} + +// registryPrefix returns the registry name prefix (the part before the first +// dot) from a MessageRegistryFile.Registry value like "iLO.1.0" or "Base.1.15". +func registryPrefix(registry string) string { + prefix, _, _ := strings.Cut(registry, ".") + return prefix +} + func (f *subscriptionClientFactory) NewClient(ctx context.Context, r *subscriptions.Resolved) (subscriptions.Client, error) { opts := metalbmc.Options{ Endpoint: subscriptions.BMCEndpoint(r.BMC), diff --git a/internal/telemetry/runtime/runtime_test.go b/internal/telemetry/runtime/runtime_test.go index b3e8f4f1..f279c42b 100644 --- a/internal/telemetry/runtime/runtime_test.go +++ b/internal/telemetry/runtime/runtime_test.go @@ -22,7 +22,7 @@ const ( // either mention or don't mention specific option names. func TestAddTo_RequiresConfigName(t *testing.T) { - err := telemetryruntime.AddTo(nil, telemetryruntime.Options{ + err := telemetryruntime.AddTo(nil, &telemetryruntime.Options{ ConfigNamespace: "ns", ReceiverURL: testReceiverURL, EventsAddr: testEventsAddr, @@ -36,7 +36,7 @@ func TestAddTo_RequiresConfigName(t *testing.T) { } func TestAddTo_RequiresConfigNamespace(t *testing.T) { - err := telemetryruntime.AddTo(nil, telemetryruntime.Options{ + err := telemetryruntime.AddTo(nil, &telemetryruntime.Options{ ConfigName: "cm", ReceiverURL: testReceiverURL, EventsAddr: testEventsAddr, @@ -50,7 +50,7 @@ func TestAddTo_RequiresConfigNamespace(t *testing.T) { } func TestAddTo_RequiresReceiverURL(t *testing.T) { - err := telemetryruntime.AddTo(nil, telemetryruntime.Options{ + err := telemetryruntime.AddTo(nil, &telemetryruntime.Options{ ConfigName: "cm", ConfigNamespace: "ns", EventsAddr: testEventsAddr, @@ -64,7 +64,7 @@ func TestAddTo_RequiresReceiverURL(t *testing.T) { } func TestAddTo_RequiresEventsAddr(t *testing.T) { - err := telemetryruntime.AddTo(nil, telemetryruntime.Options{ + err := telemetryruntime.AddTo(nil, &telemetryruntime.Options{ ConfigName: "cm", ConfigNamespace: "ns", ReceiverURL: testReceiverURL, @@ -123,7 +123,7 @@ func TestAddTo_OptionsCarryThrough(t *testing.T) { // mutated; from here we can only check that a missing SubscriberID // doesn't itself surface as a validation error. func TestAddTo_MissingSubscriberIDIsNotAValidationError(t *testing.T) { - err := telemetryruntime.AddTo(nil, telemetryruntime.Options{ + err := telemetryruntime.AddTo(nil, &telemetryruntime.Options{ ConfigName: "cm", ConfigNamespace: "ns", ReceiverURL: testReceiverURL, diff --git a/internal/telemetry/sink/prometheus/subscriptionsink.go b/internal/telemetry/sink/prometheus/subscriptionsink.go new file mode 100644 index 00000000..16758f36 --- /dev/null +++ b/internal/telemetry/sink/prometheus/subscriptionsink.go @@ -0,0 +1,94 @@ +// SPDX-FileCopyrightText: SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package prometheus + +import ( + "sync" + + "github.com/prometheus/client_golang/prometheus" +) + +const subscriptionStatusName = "bmc_event_subscription_status" + +// SubscriptionStatusSink records the push-subscription status of each BMC as +// the redfish_bmc_event_subscription_status gauge. +// +// Label set: {hostname, vendor, model, reason}. +// Value is 1 when the BMC has an active push subscription, 0 otherwise. +// The reason label explains why push is not active (excluded, service_disabled, +// no_submit_test_event, probe_failed) or confirms it is active (subscribed). +type SubscriptionStatusSink struct { + gauge *prometheus.GaugeVec + + mu sync.Mutex + tracked map[string]prometheus.Labels // bmcName → current label set +} + +// SubscriptionStatusRecorder is the interface the subscription reconciler uses +// to record per-BMC subscription status. Kept separate from the Prometheus +// implementation so the reconciler package has no prometheus dependency. +type SubscriptionStatusRecorder interface { + RecordSubscriptionStatus(bmcName, vendor, model, reason string) + ForgetSubscriptionStatus(bmcName string) +} + +var _ SubscriptionStatusRecorder = (*SubscriptionStatusSink)(nil) + +func NewSubscriptionStatusSink(reg prometheus.Registerer) (*SubscriptionStatusSink, error) { + s := &SubscriptionStatusSink{ + tracked: make(map[string]prometheus.Labels), + } + s.gauge = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Namespace: metricNamespace, + Name: subscriptionStatusName, + Help: "Redfish event subscription status per BMC: 1 = active push subscription, " + + "0 = pull scraping mode. The reason label explains why push is not active.", + }, []string{labelHostname, "vendor", "model", "reason"}) + if err := reg.Register(s.gauge); err != nil { + return nil, err + } + return s, nil +} + +func (s *SubscriptionStatusSink) RecordSubscriptionStatus(bmcName, vendor, model, reason string) { + var v float64 + if reason == "subscribed" { + v = 1 + } + labels := prometheus.Labels{ + labelHostname: bmcName, + "vendor": vendor, + "model": model, + "reason": reason, + } + s.mu.Lock() + defer s.mu.Unlock() + if prev, ok := s.tracked[bmcName]; ok && !labelsEqual(prev, labels) { + s.gauge.Delete(prev) + } + s.tracked[bmcName] = labels + s.gauge.With(labels).Set(v) +} + +// ForgetSubscriptionStatus removes the series for the given BMC. Idempotent. +func (s *SubscriptionStatusSink) ForgetSubscriptionStatus(bmcName string) { + s.mu.Lock() + defer s.mu.Unlock() + if prev, ok := s.tracked[bmcName]; ok { + s.gauge.Delete(prev) + delete(s.tracked, bmcName) + } +} + +func labelsEqual(a, b prometheus.Labels) bool { + if len(a) != len(b) { + return false + } + for k, v := range a { + if b[k] != v { + return false + } + } + return true +} diff --git a/internal/telemetry/sink/prometheus/subscriptionsink_test.go b/internal/telemetry/sink/prometheus/subscriptionsink_test.go new file mode 100644 index 00000000..6aa4267b --- /dev/null +++ b/internal/telemetry/sink/prometheus/subscriptionsink_test.go @@ -0,0 +1,167 @@ +// SPDX-FileCopyrightText: SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package prometheus_test + +import ( + "sync" + "testing" + + psink "github.com/ironcore-dev/metal-maintenance-operator/internal/telemetry/sink/prometheus" + "github.com/prometheus/client_golang/prometheus" +) + +const subscriptionStatusMetricName = "redfish_bmc_event_subscription_status" + +// gatherSubscriptionValue returns the gauge value and whether the series was +// found for the given bmcName and reason combination. +func gatherSubscriptionValue(t *testing.T, reg prometheus.Gatherer, bmcName, reason string) (float64, bool) { + t.Helper() + families, err := reg.Gather() + if err != nil { + t.Fatalf("Gather: %v", err) + } + for _, f := range families { + if f.GetName() != subscriptionStatusMetricName { + continue + } + for _, s := range f.GetMetric() { + if labelVal(s, "hostname") == bmcName && labelVal(s, "reason") == reason { + return s.GetGauge().GetValue(), true + } + } + } + return 0, false +} + +// gatherSubscriptionCount returns the total number of series for the +// subscription status metric. +func gatherSubscriptionCount(t *testing.T, reg prometheus.Gatherer) int { + t.Helper() + families, err := reg.Gather() + if err != nil { + t.Fatalf("Gather: %v", err) + } + for _, f := range families { + if f.GetName() == subscriptionStatusMetricName { + return len(f.GetMetric()) + } + } + return 0 +} + +func TestSubscriptionStatusSink_SubscribedValueOne(t *testing.T) { + reg := prometheus.NewRegistry() + s, err := psink.NewSubscriptionStatusSink(reg) + if err != nil { + t.Fatalf("NewSubscriptionStatusSink: %v", err) + } + s.RecordSubscriptionStatus(bmc1, "Dell Inc.", "R650", "subscribed") + v, ok := gatherSubscriptionValue(t, reg, bmc1, "subscribed") + if !ok { + t.Fatal("series not found") + } + if v != 1 { + t.Errorf("value: got %v, want 1 (subscribed)", v) + } +} + +func TestSubscriptionStatusSink_UnsubscribedValueZero(t *testing.T) { + reg := prometheus.NewRegistry() + s, _ := psink.NewSubscriptionStatusSink(reg) + s.RecordSubscriptionStatus(bmc1, "Dell Inc.", "R650", "service_disabled") + v, ok := gatherSubscriptionValue(t, reg, bmc1, "service_disabled") + if !ok { + t.Fatal("series not found") + } + if v != 0 { + t.Errorf("value: got %v, want 0 (not subscribed)", v) + } +} + +func TestSubscriptionStatusSink_LabelChangeCleansOldSeries(t *testing.T) { + reg := prometheus.NewRegistry() + s, _ := psink.NewSubscriptionStatusSink(reg) + + // First record with reason=service_disabled. + s.RecordSubscriptionStatus(bmc1, "Dell Inc.", "R650", "service_disabled") + if n := gatherSubscriptionCount(t, reg); n != 1 { + t.Fatalf("series count after first record: got %d, want 1", n) + } + + // Now change reason to subscribed — old series must be deleted. + s.RecordSubscriptionStatus(bmc1, "Dell Inc.", "R650", "subscribed") + if n := gatherSubscriptionCount(t, reg); n != 1 { + t.Fatalf("series count after label change: got %d, want 1 (old deleted, new created)", n) + } + // Old series gone. + if _, ok := gatherSubscriptionValue(t, reg, bmc1, "service_disabled"); ok { + t.Error("old series with reason=service_disabled should be deleted") + } + // New series present with value 1. + v, ok := gatherSubscriptionValue(t, reg, bmc1, "subscribed") + if !ok { + t.Error("new series with reason=subscribed not found") + } + if v != 1 { + t.Errorf("new series value: got %v, want 1", v) + } +} + +func TestSubscriptionStatusSink_ForgetRemovesSeries(t *testing.T) { + reg := prometheus.NewRegistry() + s, _ := psink.NewSubscriptionStatusSink(reg) + s.RecordSubscriptionStatus(bmc1, "Dell Inc.", "R650", "subscribed") + s.RecordSubscriptionStatus("bmc-2", "HPE", "iLO 6", "service_disabled") + + s.ForgetSubscriptionStatus(bmc1) + + if _, ok := gatherSubscriptionValue(t, reg, bmc1, "subscribed"); ok { + t.Error("bmc-1 series should be deleted after Forget") + } + if _, ok := gatherSubscriptionValue(t, reg, "bmc-2", "service_disabled"); !ok { + t.Error("bmc-2 series should remain after bmc-1 Forget") + } +} + +func TestSubscriptionStatusSink_ForgetUnknownIsNoop(t *testing.T) { + reg := prometheus.NewRegistry() + s, _ := psink.NewSubscriptionStatusSink(reg) + s.RecordSubscriptionStatus(bmc1, "Dell Inc.", "R650", "subscribed") + s.ForgetSubscriptionStatus("never-seen") + if _, ok := gatherSubscriptionValue(t, reg, bmc1, "subscribed"); !ok { + t.Error("bmc-1 series should be untouched after Forget of unknown BMC") + } +} + +func TestSubscriptionStatusSink_DuplicateRegistrationFails(t *testing.T) { + reg := prometheus.NewRegistry() + if _, err := psink.NewSubscriptionStatusSink(reg); err != nil { + t.Fatalf("first registration: %v", err) + } + if _, err := psink.NewSubscriptionStatusSink(reg); err == nil { + t.Error("second registration should fail") + } +} + +func TestSubscriptionStatusSink_ConcurrentIsRaceFree(t *testing.T) { + reg := prometheus.NewRegistry() + s, _ := psink.NewSubscriptionStatusSink(reg) + + var wg sync.WaitGroup + reasons := []string{"subscribed", "service_disabled", "excluded", "probe_failed", "no_submit_test_event"} + for i := range 4 { + wg.Go(func() { + bmcName := "bmc-" + string(rune('a'+i)) + for j := range 50 { + s.RecordSubscriptionStatus(bmcName, "vendor", "model", reasons[j%len(reasons)]) + } + }) + } + wg.Go(func() { + for range 25 { + s.ForgetSubscriptionStatus("bmc-c") + } + }) + wg.Wait() +} diff --git a/internal/telemetry/subscriptions/config_types.go b/internal/telemetry/subscriptions/config_types.go index 402fcf14..172a289a 100644 --- a/internal/telemetry/subscriptions/config_types.go +++ b/internal/telemetry/subscriptions/config_types.go @@ -12,13 +12,11 @@ const APIVersion = "telemetry.metal.ironcore.dev/v1alpha1" // Config is the schema for the operator's event-push telemetry // configuration. Breaking changes must bump APIVersion. // -// The ConfigMap exists to tell the operator which BMCs to subscribe for -// event-format pushes (eventBasedHardware) and how the subscription -// manager paces its reconciles (subscriptionReconcileInterval, -// perBMCTimeout). BMCs not matched by an eventBasedHardware row are -// left alone — sensor metric scraping for those is the responsibility -// of an external redfish-exporter; nothing in this schema concerns -// metric collection. +// Every BMC whose EventService is reachable and enabled is subscribed for +// event-format pushes by default. Use ExcludeHardware to suppress specific +// vendor/model ranges (e.g. known-broken firmware). TestEventOverrides +// provides per-vendor test-event parameters; when absent the operator +// auto-discovers a suitable MessageId from the BMC's event registry. type Config struct { APIVersion string `yaml:"apiVersion"` @@ -30,16 +28,13 @@ type Config struct { SubscriptionReconcileInterval time.Duration `yaml:"subscriptionReconcileInterval"` // PerBMCTimeout caps each Redfish operation the subscription manager - // performs (subscription create/delete, list). Defaults to 30s when - // zero. Operators with slow or flaky BMCs may raise this so a - // single slow BMC doesn't fail-fast through the reconcile loop. + // performs (subscription create/delete, list, probe). Defaults to 30s + // when zero. PerBMCTimeout time.Duration `yaml:"perBMCTimeout"` // TestEventInterval controls how often the operator fires a // SubmitTestEvent on each subscribed BMC to verify the full // push pipeline. Zero (the default) disables the health check. - // When enabled, the test fires at most once per interval per BMC, - // piggybacking on the normal subscription reconcile loop. TestEventInterval time.Duration `yaml:"testEventInterval,omitempty"` // TestEventTimeout is how long the operator waits for the test @@ -47,46 +42,52 @@ type Config struct { // Defaults to 30s when zero (and TestEventInterval > 0). TestEventTimeout time.Duration `yaml:"testEventTimeout,omitempty"` - EventBasedHardware []HardwareMatch `yaml:"eventBasedHardware,omitempty"` + // ExcludeHardware is an optional opt-out list. BMCs matching any row + // are never subscribed regardless of EventService capability. Rows that + // are absent (or an empty list) mean all BMCs are eligible. + ExcludeHardware []ExcludeRule `yaml:"excludeHardware,omitempty"` + + // TestEventOverrides provides per-vendor test-event parameters. When a + // BMC matches an override row the configured MessageId/Severity/ + // OriginOfCondition are used directly, skipping auto-discovery. When no + // row matches, the operator walks the BMC's Redfish event registries to + // find a suitable MessageId at runtime. + TestEventOverrides []TestOverride `yaml:"testEventOverrides,omitempty"` } -// HardwareMatch opts a vendor/model combination into event-based delivery. -// Default-deny: anything not matched here is left alone. -type HardwareMatch struct { +// ExcludeRule opts a vendor/model combination OUT of event-based delivery. +// BMCs matching this rule are never subscribed; the external redfish-exporter +// scrapes them via the discovery endpoint instead. +type ExcludeRule struct { // Vendor must be one of metal-operator's canonical bmc.Manufacturer - // values: "Dell Inc.", "HPE", "Lenovo", or "Supermicro". These are - // the exact strings metal-operator writes to BMC.Status.Manufacturer, - // so the runtime match is plain equality — short forms like "Dell" - // or "dell" are rejected by Validate (they'd silently never match). + // values: "Dell Inc.", "HPE", "Lenovo", or "Supermicro". Vendor string `yaml:"vendor"` - // Models is the list of model strings to match against the hardware model; - // ["*"] matches all models. The runtime resolves the model from - // BMC.Status.Model when set, falling back to Server.Status.Model - // (e.g. Dell iDRAC does not populate BMC.Status.Model, so the Server - // model string must be used here instead). + // Models is the list of model strings to exclude; ["*"] excludes all + // models for this vendor. Models []string `yaml:"models"` - // MinFirmware is the minimum firmware version required for event-based - // delivery. Must be a semver string if set. BMCs below this version - // are not subscribed. + // MinFirmware is optional. When set, only firmware versions below this + // semver are excluded (i.e. "exclude this vendor/model on old firmware + // where the EventService is broken"). MinFirmware string `yaml:"minFirmware,omitempty"` +} + +// TestOverride carries the per-vendor SubmitTestEvent parameters used by the +// pipeline health check. When a BMC matches a TestOverride row the configured +// fields are used directly. When no row matches, the operator auto-discovers +// a suitable MessageId from the BMC's Redfish event registry. +type TestOverride struct { + // Vendor must be one of metal-operator's canonical bmc.Manufacturer values. + Vendor string `yaml:"vendor"` + // Models is the list of model strings to match; ["*"] matches all models. + Models []string `yaml:"models"` - // TestMessageId is the Redfish MessageId sent in SubmitTestEvent. Required - // when testEventInterval is set. Must match the registry format accepted by - // this vendor's firmware (e.g. "iLO.2.2.PowerSystemBoot" for HPE, - // "SYS.1.0.SYS1000" for newer Dell iDRAC). + // TestMessageId is the Redfish MessageId sent in SubmitTestEvent. + // When empty, the operator auto-discovers a MessageId at runtime. TestMessageId string `yaml:"testMessageId,omitempty"` // TestSeverity is the Severity value sent in SubmitTestEvent. Passed - // through as-is; empty omits the field from the request entirely (some - // iDRAC/Lenovo firmware rejects unknown action parameters). HPE iLO - // requires this field to be set — leaving it empty will cause - // SubmitTestEvent to be rejected. Older Redfish implementations may - // only accept "OK", "Warning", or "Critical"; newer ones also accept - // "Informational". + // through as-is; empty omits the field from the request entirely. TestSeverity string `yaml:"testSeverity,omitempty"` // TestOriginOfCondition is the OriginOfCondition URI sent in - // SubmitTestEvent, as a plain string (HPE iLO and Lenovo XCC reject - // the Redfish {"@odata.id": ...} link-object form with a type error). - // When empty the field is omitted entirely — required for Dell and - // Lenovo compatibility. HPE iLO requires this field to be set. + // SubmitTestEvent, as a plain string. When empty the field is omitted. TestOriginOfCondition string `yaml:"testOriginOfCondition,omitempty"` } diff --git a/internal/telemetry/subscriptions/config_validate.go b/internal/telemetry/subscriptions/config_validate.go index 15cf3548..c71b3735 100644 --- a/internal/telemetry/subscriptions/config_validate.go +++ b/internal/telemetry/subscriptions/config_validate.go @@ -37,7 +37,7 @@ var knownVendors = map[bmc.Manufacturer]struct{}{ } // supportedVendorList returns the canonical vendor strings in a stable -// order for use in error messages. Built once, not allocated per error. +// order for use in error messages. var supportedVendorList = []string{ string(bmc.ManufacturerDell), string(bmc.ManufacturerHPE), @@ -107,48 +107,57 @@ func Validate(cfg *Config) field.ErrorList { fmt.Sprintf("must be between %s and %s when set", minTestEventTimeout, maxTestEventTimeout))) } - // metrics — no allow-list validated here. The collector publishes every - // numeric value from every MetricReport on the BMC; metric selection - // happens at Prometheus scrape time via metric_relabel_configs. - - // eventBasedHardware — vendor must be a canonical metal-operator - // bmc.Manufacturer value, models non-empty (and individually - // non-blank), valid semver for MinFirmware. Multiple rows per vendor - // are allowed to support per-model overrides (e.g. different - // testMessageId for old vs new firmware). - for i, hw := range cfg.EventBasedHardware { - hwPath := root.Child("eventBasedHardware").Index(i) - vendor := strings.TrimSpace(hw.Vendor) + // excludeHardware — vendor must be a canonical metal-operator bmc.Manufacturer value, + // models non-empty. MinFirmware, if set, must be valid semver. + for i, ex := range cfg.ExcludeHardware { + exPath := root.Child("excludeHardware").Index(i) + vendor := strings.TrimSpace(ex.Vendor) switch { case vendor == "": - errs = append(errs, field.Required(hwPath.Child("vendor"), "")) - case hw.Vendor != vendor || !isKnownVendor(vendor): - // Reject non-canonical values up front. hw.Vendor != vendor - // catches whitespace-padded forms (e.g. " Dell Inc. ") that - // would pass validation but never match the exact-compare - // runtime path in vendorMatches. - errs = append(errs, field.NotSupported(hwPath.Child("vendor"), hw.Vendor, supportedVendorList)) + errs = append(errs, field.Required(exPath.Child("vendor"), "")) + case ex.Vendor != vendor || !isKnownVendor(vendor): + errs = append(errs, field.NotSupported(exPath.Child("vendor"), ex.Vendor, supportedVendorList)) } - if len(hw.Models) == 0 { - errs = append(errs, field.Required(hwPath.Child("models"), "at least one model or \"*\" required")) + if len(ex.Models) == 0 { + errs = append(errs, field.Required(exPath.Child("models"), "at least one model or \"*\" required")) } else { - for j, model := range hw.Models { + for j, model := range ex.Models { if strings.TrimSpace(model) == "" { - errs = append(errs, field.Invalid(hwPath.Child("models").Index(j), model, + errs = append(errs, field.Invalid(exPath.Child("models").Index(j), model, "model entries must not be blank")) } } } - if hw.MinFirmware != "" { - if _, err := semver.ParseTolerant(hw.MinFirmware); err != nil { - errs = append(errs, field.Invalid(hwPath.Child("minFirmware"), hw.MinFirmware, + if ex.MinFirmware != "" { + if _, err := semver.ParseTolerant(ex.MinFirmware); err != nil { + errs = append(errs, field.Invalid(exPath.Child("minFirmware"), ex.MinFirmware, fmt.Sprintf("must be a valid semver: %v", err))) } } - if cfg.TestEventInterval > 0 && hw.TestMessageId == "" { - errs = append(errs, field.Required(hwPath.Child("testMessageId"), - "required when testEventInterval is set")) + } + + // testEventOverrides — vendor must be canonical, models non-empty. + // testMessageId is optional (auto-discovered at runtime when absent). + for i, ov := range cfg.TestEventOverrides { + ovPath := root.Child("testEventOverrides").Index(i) + vendor := strings.TrimSpace(ov.Vendor) + switch { + case vendor == "": + errs = append(errs, field.Required(ovPath.Child("vendor"), "")) + case ov.Vendor != vendor || !isKnownVendor(vendor): + errs = append(errs, field.NotSupported(ovPath.Child("vendor"), ov.Vendor, supportedVendorList)) + } + if len(ov.Models) == 0 { + errs = append(errs, field.Required(ovPath.Child("models"), "at least one model or \"*\" required")) + } else { + for j, model := range ov.Models { + if strings.TrimSpace(model) == "" { + errs = append(errs, field.Invalid(ovPath.Child("models").Index(j), model, + "model entries must not be blank")) + } + } } } + return errs } diff --git a/internal/telemetry/subscriptions/config_validate_test.go b/internal/telemetry/subscriptions/config_validate_test.go index b5446c43..8205a314 100644 --- a/internal/telemetry/subscriptions/config_validate_test.go +++ b/internal/telemetry/subscriptions/config_validate_test.go @@ -76,40 +76,69 @@ func TestValidate_PerBMCTimeoutRange(t *testing.T) { } } -func TestValidate_EventBasedHardware_MissingVendor(t *testing.T) { +// TestValidate_OldEventBasedHardwareRejected pins the hard-break requirement: +// a ConfigMap that still uses the old "eventBasedHardware" key must be +// rejected immediately via strict YAML parsing (unknown field). +func TestValidate_OldEventBasedHardwareRejected(t *testing.T) { + raw := []byte(` +apiVersion: telemetry.metal.ironcore.dev/v1alpha1 +subscriptionReconcileInterval: 30s +eventBasedHardware: + - vendor: Dell Inc. + models: ["*"] + testMessageId: FAN.1.0.FAN0001 +`) + _, errs := Parse(raw) + if len(errs) == 0 { + t.Fatal("expected strict-YAML error for unknown field 'eventBasedHardware', got none") + } +} + +// -- excludeHardware -- + +func TestValidate_ExcludeHardware_MissingVendor(t *testing.T) { cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{{Models: []string{"*"}}} + cfg.ExcludeHardware = []ExcludeRule{{Models: []string{"*"}}} errs := Validate(cfg) - assertFieldError(t, errs, "eventBasedHardware[0].vendor") + assertFieldError(t, errs, "excludeHardware[0].vendor") } -// TestValidate_EventBasedHardware_NonCanonicalVendor pins the contract: -// short forms ("Dell", "dell", "Hewlett Packard Enterprise") are -// rejected because metal-operator only ever writes the canonical -// bmc.Manufacturer constants ("Dell Inc.", "HPE", "Lenovo", -// "Supermicro") to BMC.Status.Manufacturer. Accepting other variants -// here would silently leave those rows un-matchable at runtime. -func TestValidate_EventBasedHardware_NonCanonicalVendor(t *testing.T) { +func TestValidate_ExcludeHardware_NonCanonicalVendor(t *testing.T) { for _, vendor := range []string{"Dell", "dell", "DELL", "Hewlett Packard Enterprise", "lenovo", " Dell Inc. ", "HPE "} { t.Run(vendor, func(t *testing.T) { cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendor, Models: []string{"*"}}} + cfg.ExcludeHardware = []ExcludeRule{{Vendor: vendor, Models: []string{"*"}}} errs := Validate(cfg) - assertFieldError(t, errs, "eventBasedHardware[0].vendor") + assertFieldError(t, errs, "excludeHardware[0].vendor") }) } } -func TestValidate_EventBasedHardware_EmptyModels(t *testing.T) { +func TestValidate_ExcludeHardware_EmptyModels(t *testing.T) { cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{}}} + cfg.ExcludeHardware = []ExcludeRule{{Vendor: vendorDellInc, Models: []string{}}} errs := Validate(cfg) - assertFieldError(t, errs, "eventBasedHardware[0].models") + assertFieldError(t, errs, "excludeHardware[0].models") } -func TestValidate_EventBasedHardware_MultipleRowsSameVendor(t *testing.T) { +func TestValidate_ExcludeHardware_BadSemver(t *testing.T) { cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{ + cfg.ExcludeHardware = []ExcludeRule{{Vendor: vendorDellInc, Models: []string{"*"}, MinFirmware: "not-semver"}} + errs := Validate(cfg) + assertFieldError(t, errs, "excludeHardware[0].minFirmware") +} + +func TestValidate_ExcludeHardware_ValidSemver(t *testing.T) { + cfg := minimalValid() + cfg.ExcludeHardware = []ExcludeRule{{Vendor: vendorDellInc, Models: []string{"*"}, MinFirmware: firmware5_10_0}} + if errs := Validate(cfg); len(errs) != 0 { + t.Fatalf("unexpected errors with valid semver: %v", errs) + } +} + +func TestValidate_ExcludeHardware_MultipleRowsSameVendor(t *testing.T) { + cfg := minimalValid() + cfg.ExcludeHardware = []ExcludeRule{ {Vendor: vendorDellInc, Models: []string{"PowerEdge R750"}}, {Vendor: vendorDellInc, Models: []string{"PowerEdge R640"}}, } @@ -118,37 +147,62 @@ func TestValidate_EventBasedHardware_MultipleRowsSameVendor(t *testing.T) { } } -func TestValidate_EventBasedHardware_BadSemver(t *testing.T) { +// -- testEventOverrides -- + +func TestValidate_TestEventOverrides_MissingVendor(t *testing.T) { cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{"*"}, MinFirmware: "not-semver"}} + cfg.TestEventOverrides = []TestOverride{{Models: []string{"*"}}} errs := Validate(cfg) - assertFieldError(t, errs, "eventBasedHardware[0].minFirmware") + assertFieldError(t, errs, "testEventOverrides[0].vendor") } -func TestValidate_EventBasedHardware_ValidSemver(t *testing.T) { - cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{"*"}, MinFirmware: firmware5_10_0}} - if errs := Validate(cfg); len(errs) != 0 { - t.Fatalf("unexpected errors with valid semver: %v", errs) +func TestValidate_TestEventOverrides_NonCanonicalVendor(t *testing.T) { + for _, vendor := range []string{"Dell", "dell", "DELL", " Dell Inc. ", "HPE "} { + t.Run(vendor, func(t *testing.T) { + cfg := minimalValid() + cfg.TestEventOverrides = []TestOverride{{Vendor: vendor, Models: []string{"*"}}} + errs := Validate(cfg) + assertFieldError(t, errs, "testEventOverrides[0].vendor") + }) } } -func TestValidate_EventBasedHardware_TestMessageIdRequiredWhenIntervalSet(t *testing.T) { +func TestValidate_TestEventOverrides_EmptyModels(t *testing.T) { cfg := minimalValid() - cfg.TestEventInterval = time.Minute - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{"*"}}} + cfg.TestEventOverrides = []TestOverride{{Vendor: vendorDellInc, Models: []string{}}} errs := Validate(cfg) - assertFieldError(t, errs, "testMessageId") + assertFieldError(t, errs, "testEventOverrides[0].models") } -func TestValidate_EventBasedHardware_TestMessageIdNotRequiredWithoutInterval(t *testing.T) { +func TestValidate_TestEventOverrides_TestMessageIdOptional(t *testing.T) { + // TestMessageId is now optional — the operator auto-discovers it. cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{"*"}}} + cfg.TestEventInterval = time.Hour + cfg.TestEventOverrides = []TestOverride{{Vendor: vendorDellInc, Models: []string{"*"}}} if errs := Validate(cfg); len(errs) != 0 { - t.Fatalf("unexpected errors without testEventInterval: %v", errs) + t.Errorf("testMessageId should be optional in testEventOverrides, got: %v", errs) } } +func TestValidate_TestEventOverrides_AllFieldsValid(t *testing.T) { + cfg := minimalValid() + cfg.TestEventInterval = time.Hour + cfg.TestEventOverrides = []TestOverride{ + { + Vendor: vendorDellInc, + Models: []string{"*"}, + TestMessageId: "FAN.1.0.FAN0001", + TestSeverity: "Informational", + TestOriginOfCondition: "/redfish/v1/Managers/1", + }, + } + if errs := Validate(cfg); len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } +} + +// -- testEventInterval / testEventTimeout -- + func TestValidate_TestEventInterval_Range(t *testing.T) { cases := []struct { name string @@ -176,38 +230,6 @@ func TestValidate_TestEventInterval_Range(t *testing.T) { } } -func TestValidate_TestMessageId_RequiredWhenIntervalSet(t *testing.T) { - cfg := minimalValid() - cfg.TestEventInterval = time.Hour - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{"*"}}} - errs := Validate(cfg) - assertFieldError(t, errs, "testMessageId") -} - -func TestValidate_TestMessageId_NotRequiredWhenIntervalZero(t *testing.T) { - cfg := minimalValid() - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{"*"}}} - if errs := Validate(cfg); len(errs) != 0 { - t.Errorf("testMessageId should not be required when interval is zero, got: %v", errs) - } -} - -func TestValidate_TestMessageId_AnyNonEmptyAccepted(t *testing.T) { - // Format validation is deliberately omitted: vendors use different - // conventions (e.g. iDRAC accepts "SYS1000", HPE uses full registry - // paths). The BMC will reject a bad value at runtime. - for _, id := range []string{"SYS1000", "SYS.1.0.SYS1000", "iDRAC.2.9.RAC0182", "Base.1.12.GeneralError", "anything"} { - t.Run(id, func(t *testing.T) { - cfg := minimalValid() - cfg.TestEventInterval = time.Hour - cfg.EventBasedHardware = []HardwareMatch{{Vendor: vendorDellInc, Models: []string{"*"}, TestMessageId: id}} - if errs := Validate(cfg); len(errs) != 0 { - t.Errorf("non-empty testMessageId %q should be accepted, got: %v", id, errs) - } - }) - } -} - func TestValidate_TestEventTimeout_Range(t *testing.T) { cases := []struct { name string @@ -235,6 +257,8 @@ func TestValidate_TestEventTimeout_Range(t *testing.T) { } } +// -- Parse strict YAML -- + func TestParse_UnknownField(t *testing.T) { raw := []byte(` apiVersion: telemetry.metal.ironcore.dev/v1alpha1 @@ -252,10 +276,14 @@ func TestParse_RoundTrip(t *testing.T) { apiVersion: telemetry.metal.ironcore.dev/v1alpha1 subscriptionReconcileInterval: 1m perBMCTimeout: 25s -eventBasedHardware: +excludeHardware: - vendor: Dell Inc. models: ["*"] minFirmware: 5.10.0 +testEventOverrides: + - vendor: HPE + models: ["*"] + testMessageId: iLO.1.0.TestAlert `) cfg, errs := Parse(raw) if len(errs) != 0 { @@ -267,8 +295,11 @@ eventBasedHardware: if cfg.PerBMCTimeout != 25*time.Second { t.Errorf("perBMCTimeout: got %v, want 25s", cfg.PerBMCTimeout) } - if len(cfg.EventBasedHardware) != 1 { - t.Errorf("eventBasedHardware: got %d, want 1", len(cfg.EventBasedHardware)) + if len(cfg.ExcludeHardware) != 1 { + t.Errorf("excludeHardware: got %d, want 1", len(cfg.ExcludeHardware)) + } + if len(cfg.TestEventOverrides) != 1 { + t.Errorf("testEventOverrides: got %d, want 1", len(cfg.TestEventOverrides)) } } diff --git a/internal/telemetry/subscriptions/helpers_test.go b/internal/telemetry/subscriptions/helpers_test.go index 2e75364e..b3a93a07 100644 --- a/internal/telemetry/subscriptions/helpers_test.go +++ b/internal/telemetry/subscriptions/helpers_test.go @@ -64,6 +64,12 @@ type fakeClient struct { subs []subscriptions.Subscription listErr error logouts atomic.Int32 + + // ProbeEventService controls + probeErr error + probeEnabled bool + probeHasSubmit bool + probeDiscovered string } type createCall struct { @@ -115,6 +121,19 @@ func (c *fakeClient) SubmitTestEvent(_ context.Context, params subscriptions.Tes return nil } +func (c *fakeClient) ProbeEventService(_ context.Context, _ bool) (*subscriptions.EventServiceCapabilities, error) { + c.mu.Lock() + defer c.mu.Unlock() + if c.probeErr != nil { + return nil, c.probeErr + } + return &subscriptions.EventServiceCapabilities{ + ServiceEnabled: c.probeEnabled, + HasSubmitTestEvent: c.probeHasSubmit, + DiscoveredTestMessageId: c.probeDiscovered, + }, nil +} + func (c *fakeClient) snapshotSubmits() []string { c.mu.Lock() defer c.mu.Unlock() @@ -342,15 +361,88 @@ func newRecWithTestRecorder(t *testing.T, c client.Client, cfg *subscriptions.Co return r } -// dellVendorMatch is the canonical EventBased policy used by most -// retargeted tests: any Dell BMC is event-eligible. +// dellVendorMatch is the canonical TestEventOverrides policy used by most +// retargeted tests: any Dell BMC uses a known testMessageId. func dellVendorMatch() *subscriptions.Config { - return &subscriptions.Config{EventBasedHardware: []subscriptions.HardwareMatch{ - {Vendor: vendorDellInc, Models: []string{"*"}, TestMessageId: testMessageId}, - }} + return &subscriptions.Config{ + TestEventOverrides: []subscriptions.TestOverride{ + {Vendor: vendorDellInc, Models: []string{"*"}, TestMessageId: testMessageId}, + }, + } +} + +// capableFakeClient returns a fakeClient whose ProbeEventService signals that +// the EventService is enabled and SubmitTestEvent is supported. Use this in +// tests that assert subscription creation; a bare &fakeClient{} has +// probeEnabled=false and will therefore not trigger subscription creation. +func capableFakeClient() *fakeClient { + return &fakeClient{ + probeEnabled: true, + probeHasSubmit: true, + } +} + +// fakeStatusRecorder records RecordSubscriptionStatus and ForgetSubscriptionStatus calls. +type fakeStatusRecorder struct { + mu sync.Mutex + statuses []subscriptionStatus + forgotten []string +} + +type subscriptionStatus struct { + bmcName string + vendor string + model string + reason string +} + +func (r *fakeStatusRecorder) RecordSubscriptionStatus(bmcName, vendor, model, reason string) { + r.mu.Lock() + defer r.mu.Unlock() + r.statuses = append(r.statuses, subscriptionStatus{bmcName: bmcName, vendor: vendor, model: model, reason: reason}) +} + +func (r *fakeStatusRecorder) ForgetSubscriptionStatus(bmcName string) { + r.mu.Lock() + defer r.mu.Unlock() + r.forgotten = append(r.forgotten, bmcName) +} + +func (r *fakeStatusRecorder) snapshotStatuses() []subscriptionStatus { + r.mu.Lock() + defer r.mu.Unlock() + out := make([]subscriptionStatus, len(r.statuses)) + copy(out, r.statuses) + return out +} + +func (r *fakeStatusRecorder) snapshotForgottenStatuses() []string { + r.mu.Lock() + defer r.mu.Unlock() + out := make([]string, len(r.forgotten)) + copy(out, r.forgotten) + return out +} + +// newRecWithStatusRecorder builds a reconciler with a fakeStatusRecorder. +func newRecWithStatusRecorder(t *testing.T, c client.Client, cfg *subscriptions.Config, res *fakeResolver, fac *fakeFactory, rec *fakeStatusRecorder) *subscriptions.BMCReconciler { + t.Helper() + r := &subscriptions.BMCReconciler{ + Client: c, + Config: func() *subscriptions.Config { return cfg }, + Resolver: res, + Factory: fac, + ReceiverURL: testReceiverURL, + ReconcileInterval: time.Hour, + PerBMCTimeout: time.Second, + StatusRecorder: rec, + } + if err := subscriptions.InitForTest(r); err != nil { + t.Fatalf("InitForTest: %v", err) + } + return r } -// newRec builds a fully-initialised BMCReconciler for unit tests. // ReconcileInterval is set to time.Hour by default so a successful // reconcile's RequeueAfter doesn't drive surprise re-runs in tests. func newRec(t *testing.T, c client.Client, cfg *subscriptions.Config, res *fakeResolver, fac *fakeFactory) *subscriptions.BMCReconciler { diff --git a/internal/telemetry/subscriptions/reconciler.go b/internal/telemetry/subscriptions/reconciler.go index 5278961f..42b90ce3 100644 --- a/internal/telemetry/subscriptions/reconciler.go +++ b/internal/telemetry/subscriptions/reconciler.go @@ -34,6 +34,24 @@ type TestResultRecorder interface { const bmcSubscriptionFinalizer = "telemetry.metal.ironcore.dev/subscriptions" +// Reason constants used by StatusRecorder to explain the per-BMC subscription +// status. These are stable strings — Prometheus label values must not change. +const ( + ReasonSubscribed = "subscribed" + ReasonExcluded = "excluded" + ReasonServiceDisabled = "service_disabled" + ReasonNoSubmitTestEvent = "no_submit_test_event" + ReasonProbeFailed = "probe_failed" +) + +// SubscriptionStatusRecorder records the subscription status of each BMC +// so the Prometheus sink can expose it as a gauge series. The production +// implementation updates a GaugeVec; nil disables recording. +type SubscriptionStatusRecorder interface { + RecordSubscriptionStatus(bmcName, vendor, model, reason string) + ForgetSubscriptionStatus(bmcName string) +} + // +kubebuilder:rbac:groups=metal.ironcore.dev,resources=bmcs/finalizers,verbs=update // BMCReconciler keeps each metal-operator BMC's Redfish event @@ -96,6 +114,11 @@ type BMCReconciler struct { // each SubmitTestEvent round-trip. Nil disables recording. TestRecorder TestResultRecorder + // StatusRecorder is optional. When set, the reconciler records per-BMC + // subscription status via RecordSubscriptionStatus after each reconcileOne. + // Nil disables recording. + StatusRecorder SubscriptionStatusRecorder + Log logr.Logger // srvURL is the parsed ReceiverURL, computed during SetupWithManager @@ -107,6 +130,18 @@ type BMCReconciler struct { testPending sync.Map // lastTestTime tracks when we last fired a test per BMC: bmcName → time.Time. lastTestTime sync.Map + // discoveredMsgId caches the auto-discovered test MessageId per BMC so the + // registry walk only happens once (bmcName → string). + discoveredMsgId sync.Map + // subscribedBMCs is the live set of BMC names that have active push + // subscriptions managed by this reconciler (bmcName → struct{}{}). Used by + // the discovery endpoint to omit subscribed BMCs from the Prometheus SD + // response so the external exporter does not scrape them. + subscribedBMCs sync.Map + + // unsubscribedBMCs tracks BMC names NOT subscribed and the reason why + // (bmcName → reason string). Paired with subscribedBMCs. + unsubscribedBMCs sync.Map } // SetupWithManager registers the reconciler against the controller-runtime manager. @@ -255,8 +290,10 @@ func (r *BMCReconciler) perBMCTimeout() time.Duration { func (r *BMCReconciler) reconcileOne(ctx context.Context, bmc *metalv1alpha1.BMC) { ref := r.bmcRefWithServerFallback(ctx, bmc) cfg := r.Config() - hw := SubscribeToBMC(ref, cfg) - wantSubscribed := hw != nil + + // Clear any previously stored subscription state before fallible work. + // It is re-set only after we confirm subscriptions are in place. + r.subscribedBMCs.Delete(ref.Name) ctx, cancel := context.WithTimeout(ctx, r.perBMCTimeout()) defer cancel() @@ -279,6 +316,41 @@ func (r *BMCReconciler) reconcileOne(ctx context.Context, bmc *metalv1alpha1.BMC } defer c.Logout() + // Probe the BMC's EventService to decide whether to subscribe. We only + // trigger a registry discovery walk when testEventInterval is set and no + // static override provides a MessageId for this BMC. + override := TestOverrideFor(ref, cfg) + if id := bmc.Annotations[discoveredMsgIdAnnotation]; id != "" { + r.discoveredMsgId.LoadOrStore(ref.Name, id) + } + _, haveCached := r.discoveredMsgId.Load(ref.Name) + needsDiscovery := cfg != nil && cfg.TestEventInterval > 0 && + (override == nil || override.TestMessageId == "") && !haveCached + caps, err := c.ProbeEventService(ctx, needsDiscovery) + if err != nil { + r.Log.V(1).Info("EventService probe failed", + "bmc", ref.Name, "err", err.Error()) + reason := ReasonProbeFailed + if IsExcluded(ref, cfg) { + reason = ReasonExcluded + } + r.unsubscribedBMCs.Store(ref.Name, reason) + r.subscribedBMCs.Delete(ref.Name) + if r.StatusRecorder != nil { + r.StatusRecorder.RecordSubscriptionStatus(ref.Name, ref.Vendor, ref.Model, reason) + } + return + } + + // Cache and persist a discovered MessageId on first successful find. + if caps.DiscoveredTestMessageId != "" { + if _, already := r.discoveredMsgId.LoadOrStore(ref.Name, caps.DiscoveredTestMessageId); !already { + r.persistDiscoveredMsgId(ctx, bmc, caps.DiscoveredTestMessageId) + } + } + + wantSubscribed := !IsExcluded(ref, cfg) && caps.ServiceEnabled && caps.HasSubmitTestEvent + existing, err := c.ListEventSubscriptions(ctx) if err != nil { r.Log.V(1).Info("Cannot list subscriptions", @@ -305,9 +377,27 @@ func (r *BMCReconciler) reconcileOne(ctx context.Context, bmc *metalv1alpha1.BMC "bmc", ref.Name, "err", err.Error()) return } - r.ensureSubscriptions(ctx, c, ref.Name, current) - r.maybeRunTestEvent(ctx, c, ref.Name, cfg, hw) + if r.ensureSubscriptions(ctx, c, ref.Name, current) { + r.subscribedBMCs.Store(ref.Name, struct{}{}) + r.unsubscribedBMCs.Delete(ref.Name) + if r.StatusRecorder != nil { + r.StatusRecorder.RecordSubscriptionStatus(ref.Name, ref.Vendor, ref.Model, ReasonSubscribed) + } + } else { + r.subscribedBMCs.Delete(ref.Name) + } + + // Resolve the MessageId to use for the test event: static override + // takes precedence, then in-memory cache, then just-discovered value. + msgId, severity, origin := r.resolveTestParams(override, caps.DiscoveredTestMessageId, ref.Name) + r.maybeRunTestEvent(ctx, c, ref.Name, cfg, msgId, severity, origin) } else { + r.subscribedBMCs.Delete(ref.Name) + reason := notSubscribedReason(ref, cfg, *caps) + r.unsubscribedBMCs.Store(ref.Name, reason) + if r.StatusRecorder != nil { + r.StatusRecorder.RecordSubscriptionStatus(ref.Name, ref.Vendor, ref.Model, reason) + } // BMC isn't in the event-based set (or was and is no longer): // tear off any subscriptions we previously created here, then // release the finalizer if we still hold one — otherwise a @@ -325,6 +415,19 @@ func (r *BMCReconciler) reconcileOne(ctx context.Context, bmc *metalv1alpha1.BMC } } +// notSubscribedReason returns the reason string explaining why a BMC is NOT +// being push-subscribed. It matches the ReasonExcluded / ReasonServiceDisabled +// / ReasonNoSubmitTestEvent constants in order of priority. +func notSubscribedReason(ref BMCRef, cfg *Config, caps EventServiceCapabilities) string { + if IsExcluded(ref, cfg) { + return ReasonExcluded + } + if !caps.ServiceEnabled { + return ReasonServiceDisabled + } + return ReasonNoSubmitTestEvent +} + func (r *BMCReconciler) tearDownOne(ctx context.Context, bmcName string) { ctx, cancel := context.WithTimeout(ctx, r.perBMCTimeout()) defer cancel() @@ -357,7 +460,9 @@ func (r *BMCReconciler) tearDownOne(ctx context.Context, bmcName string) { // ensureSubscriptions converges `current` to exactly one MetricReport // subscription and one Event subscription, both pointing at our receiver. -func (r *BMCReconciler) ensureSubscriptions(ctx context.Context, c Client, bmcName string, current []Subscription) { +// Returns true if both subscriptions are confirmed present (existing or +// newly created), false if either creation failed. +func (r *BMCReconciler) ensureSubscriptions(ctx context.Context, c Client, bmcName string, current []Subscription) bool { metrics, alerts := groupByFormat(current) if keep, extras := pickAndExtras(metrics); len(extras) > 0 { @@ -371,6 +476,8 @@ func (r *BMCReconciler) ensureSubscriptions(ctx context.Context, c Client, bmcNa _ = r.deleteSubscriptions(ctx, c, bmcName, extras) } + ok := true + if len(metrics) == 0 { dest := r.destinationFor("metricsreport", bmcName) uri, err := c.CreateEventSubscription( @@ -379,6 +486,7 @@ func (r *BMCReconciler) ensureSubscriptions(ctx context.Context, c Client, bmcNa schemas.RetryForeverDeliveryRetryPolicy, ) if err != nil { + ok = false r.Log.V(1).Info("Failed to create metrics subscription", "bmc", bmcName, "destination", dest, "err", err.Error()) } else { @@ -395,6 +503,7 @@ func (r *BMCReconciler) ensureSubscriptions(ctx context.Context, c Client, bmcNa schemas.RetryForeverDeliveryRetryPolicy, ) if err != nil { + ok = false r.Log.V(1).Info("Failed to create alerts subscription", "bmc", bmcName, "destination", dest, "err", err.Error()) } else { @@ -402,6 +511,8 @@ func (r *BMCReconciler) ensureSubscriptions(ctx context.Context, c Client, bmcNa "bmc", bmcName, "uri", uri) } } + + return ok } func groupByFormat(subs []Subscription) (metrics, alerts []Subscription) { @@ -580,15 +691,77 @@ const ( testResultFailure = "failure" ) +// discoveredMsgIdAnnotation is the BMC annotation key used to persist a +// runtime-discovered test event MessageId across operator restarts. +const discoveredMsgIdAnnotation = "telemetry.metal.ironcore.dev/discovered-test-message-id" + +// persistDiscoveredMsgId writes the annotation onto the BMC object. +// Failures are logged but not fatal — the in-memory cache covers this session. +func (r *BMCReconciler) persistDiscoveredMsgId(ctx context.Context, bmc *metalv1alpha1.BMC, msgId string) { + patch := client.MergeFrom(bmc.DeepCopy()) + if bmc.Annotations == nil { + bmc.Annotations = make(map[string]string) + } + bmc.Annotations[discoveredMsgIdAnnotation] = msgId + if err := r.Client.Patch(ctx, bmc, patch); err != nil { + r.Log.V(1).Info("Could not persist discovered test MessageId annotation", + "bmc", bmc.Name, "err", err.Error()) + } +} + +// resolveTestParams returns the MessageId, Severity, and OriginOfCondition to +// use for a SubmitTestEvent call. Priority order: +// 1. Static TestOverride row from config (has explicit values). +// 2. In-memory discovered MessageId cache (populated by ProbeEventService). +// 3. BMC annotation written on a previous run (survives operator restart). +func (r *BMCReconciler) resolveTestParams(override *TestOverride, discoveredFromProbe, bmcName string) (msgId, severity, origin string) { + if override != nil && override.TestMessageId != "" { + return override.TestMessageId, override.TestSeverity, override.TestOriginOfCondition + } + // In-memory cache populated this reconcile (or a prior one). + if raw, ok := r.discoveredMsgId.Load(bmcName); ok { + return raw.(string), "", "" + } + // Fall back to annotation on the BMC object (survives restarts). + // (discoveredFromProbe is already stored in the cache via LoadOrStore in + // reconcileOne; if that succeeded, the Load above would have returned it. + // This path handles the unusual case where the probe returned empty but + // a previous run wrote the annotation.) + return discoveredFromProbe, "", "" +} + +// ActiveSubscriptions returns a snapshot of the current set of BMC names that +// have active push subscriptions managed by this reconciler. Callers receive +// a copy; mutations do not affect the internal state. +func (r *BMCReconciler) ActiveSubscriptions() map[string]struct{} { + result := make(map[string]struct{}) + r.subscribedBMCs.Range(func(k, _ any) bool { + result[k.(string)] = struct{}{} + return true + }) + return result +} + +// UnsubscribedBMCReasons returns a snapshot map of bmcName → reason for all +// BMCs that are currently NOT subscribed (i.e. using pull-based scraping). +func (r *BMCReconciler) UnsubscribedBMCReasons() map[string]string { + result := make(map[string]string) + r.unsubscribedBMCs.Range(func(k, v any) bool { + result[k.(string)] = v.(string) + return true + }) + return result +} + // maybeRunTestEvent fires a SubmitTestEvent health check if the configured // interval has elapsed since the last test for this BMC. It also enforces // the deadline for any pending test that has not yet been confirmed, recording // a failure if it expired. -func (r *BMCReconciler) maybeRunTestEvent(ctx context.Context, c Client, bmcName string, cfg *Config, hw *HardwareMatch) { +func (r *BMCReconciler) maybeRunTestEvent(ctx context.Context, c Client, bmcName string, cfg *Config, msgId, severity, origin string) { if cfg == nil || cfg.TestEventInterval <= 0 { return } - if hw == nil || hw.TestMessageId == "" { + if msgId == "" { return } @@ -620,9 +793,9 @@ func (r *BMCReconciler) maybeRunTestEvent(ctx context.Context, c Client, bmcName } params := TestEventParams{ - MessageId: hw.TestMessageId, - Severity: hw.TestSeverity, - OriginOfCondition: hw.TestOriginOfCondition, + MessageId: msgId, + Severity: severity, + OriginOfCondition: origin, } if err := c.SubmitTestEvent(ctx, params); err != nil { r.Log.V(1).Info("Failed to submit test event", "bmc", bmcName, "err", err.Error()) @@ -659,9 +832,15 @@ func (r *BMCReconciler) NotifyTestEvent(bmcName, messageId string) { func (r *BMCReconciler) ForgetTestState(bmcName string) { r.testPending.Delete(bmcName) r.lastTestTime.Delete(bmcName) + r.discoveredMsgId.Delete(bmcName) + r.subscribedBMCs.Delete(bmcName) + r.unsubscribedBMCs.Delete(bmcName) if r.TestRecorder != nil { r.TestRecorder.Forget(bmcName) } + if r.StatusRecorder != nil { + r.StatusRecorder.ForgetSubscriptionStatus(bmcName) + } } // Compile-time check: BMCReconciler satisfies the controller-runtime diff --git a/internal/telemetry/subscriptions/reconciler_test.go b/internal/telemetry/subscriptions/reconciler_test.go index f7f52372..47ec3894 100644 --- a/internal/telemetry/subscriptions/reconciler_test.go +++ b/internal/telemetry/subscriptions/reconciler_test.go @@ -109,7 +109,7 @@ func TestReconcile_EventBased_ServerVendorFallback(t *testing.T) { c := newClientWithServerIndex(t, bmc, srv) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) if _, err := r.Reconcile(context.Background(), req()); err != nil { @@ -126,7 +126,7 @@ func TestReconcile_EventBased_NoExisting_CreatesBoth(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) if _, err := r.Reconcile(context.Background(), req()); err != nil { @@ -165,7 +165,7 @@ func TestReconcile_EventBased_BothPresent_NoChange(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() fc.setSubscriptions([]subscriptions.Subscription{ {URI: "/redfish/v1/EventService/Subscriptions/1", Destination: metricReportDest, EventFormat: eventFormatMetricReport}, {URI: "/redfish/v1/EventService/Subscriptions/2", Destination: alertsDest, EventFormat: eventFormatEvent}, @@ -189,7 +189,7 @@ func TestReconcile_EventBased_OneMissing_CreatesOne(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() fc.setSubscriptions([]subscriptions.Subscription{ {URI: "/redfish/v1/EventService/Subscriptions/1", Destination: metricReportDest, EventFormat: eventFormatMetricReport}, }) @@ -213,7 +213,7 @@ func TestReconcile_EventBased_DuplicateMetrics_DeletesExtras(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() fc.setSubscriptions([]subscriptions.Subscription{ // Two MetricReport subscriptions, both ours — must dedupe. {URI: "/redfish/v1/EventService/Subscriptions/9", Destination: metricReportDest, EventFormat: eventFormatMetricReport}, @@ -243,7 +243,7 @@ func TestReconcile_StaleHost_DeletedRegardlessOfMode(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() fc.setSubscriptions([]subscriptions.Subscription{ // Old receiver host. Same path pattern, same BMC name. Must be deleted. {URI: "/redfish/v1/EventService/Subscriptions/old-m", Destination: "http://OLD-recv:9092/serverevents/metricsreport/bmc-1", EventFormat: eventFormatMetricReport}, @@ -288,7 +288,7 @@ func TestReconcile_FormatMismatchClassifiedAsStale(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() fc.setSubscriptions([]subscriptions.Subscription{ // Path says metricsreport, but EventFormat is Event — corrupt. {URI: "/redfish/v1/EventService/Subscriptions/bad-m", Destination: metricReportDest, EventFormat: eventFormatEvent}, @@ -323,8 +323,8 @@ func TestReconcile_FormatMismatchClassifiedAsStale(t *testing.T) { // -- Reconcile: unmatched BMC, delete our subscriptions -- func TestReconcile_UnmatchedBMC_DeletesCurrent(t *testing.T) { - // Vendor "Lenovo" is not in the Dell-only eventBasedHardware policy, - // so any of our existing subscriptions on that BMC must be deleted. + // Vendor "Lenovo" has a disabled EventService probe (probeEnabled=false), + // so wantSubscribed=false and our existing subscriptions must be deleted. c := newClientWith(t, bmcObject(testBMCName, "Lenovo", "")) res := &fakeResolver{} res.set(makeResolved(), nil) @@ -443,7 +443,7 @@ func TestReconcile_EventBased_AddsFinalizer(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) if _, err := r.Reconcile(context.Background(), req()); err != nil { @@ -460,8 +460,8 @@ func TestReconcile_EventBased_AddsFinalizer(t *testing.T) { } // TestReconcile_UnmatchedBMC_RemovesStaleFinalizer covers the transition -// case: a BMC that was previously event-based (carries our finalizer) -// no longer matches an eventBasedHardware row. The reconciler must +// case: a BMC that was previously subscribed (carries our finalizer) but +// whose EventService probe now returns disabled. The reconciler must // delete its subscriptions AND strip the finalizer so future deletes // aren't blocked by an operator that no longer manages the BMC. func TestReconcile_UnmatchedBMC_RemovesStaleFinalizer(t *testing.T) { @@ -530,7 +530,7 @@ func TestReconcile_ListSubscriptionsError_SkipsBMC(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() fc.listErr = errors.New("list 500") r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) @@ -574,7 +574,7 @@ func TestReconcile_Success_ReturnsRequeueAfter(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) result, err := r.Reconcile(context.Background(), req()) @@ -588,10 +588,10 @@ func TestReconcile_Success_ReturnsRequeueAfter(t *testing.T) { // TestReconcile_NilConfig_DefersInsteadOfTearingDown pins the startup- // race guard. Before this guard, a Reconcile that fired before -// ConfigLoader's first successful load would call SubscribeToBMC(ref, -// nil) → false → deleteSubscriptions, tearing live subscriptions off -// every event-capable BMC in the fleet on every operator restart. The -// reconciler must defer instead. +// ConfigLoader's first successful load would call IsExcluded(ref, nil) / +// ProbeEventService → wantSubscribed=false → deleteSubscriptions, tearing +// live subscriptions off every event-capable BMC in the fleet on every +// operator restart. The reconciler must defer instead. func TestReconcile_NilConfig_DefersInsteadOfTearingDown(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} @@ -633,7 +633,7 @@ func TestSubscriberID_DestinationIncludesSegment(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) r.SubscriberID = "redfish-exporter" // SubscriberID is read from the parsed URL path only inside @@ -661,7 +661,7 @@ func TestSubscriberID_IgnoresForeignSubscriptions(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() fc.setSubscriptions([]subscriptions.Subscription{ // Owned by a different subscriber on the SAME receiver host. { @@ -697,7 +697,7 @@ func TestSubscriberID_EmptyProducesBareShape(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, "")) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) // SubscriberID intentionally unset. @@ -726,7 +726,7 @@ func TestReconcile_TestEvent_DisabledWhenIntervalZero(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() rec := &fakeTestRecorder{} r := newRecWithTestRecorder(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}, rec) @@ -742,7 +742,7 @@ func TestReconcile_TestEvent_FiresWhenDue(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() rec := &fakeTestRecorder{} cfg := cfgWithTestInterval(time.Millisecond) // interval already elapsed r := newRecWithTestRecorder(t, c, cfg, res, &fakeFactory{client: fc}, rec) @@ -759,7 +759,7 @@ func TestReconcile_TestEvent_DoesNotFireTwiceWithinInterval(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() rec := &fakeTestRecorder{} cfg := cfgWithTestInterval(time.Hour) // large interval r := newRecWithTestRecorder(t, c, cfg, res, &fakeFactory{client: fc}, rec) @@ -787,7 +787,8 @@ func TestReconcile_TestEvent_SubmitErrorLeavesNoPending(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{submitErr: errors.New("bmc unreachable")} + fc := capableFakeClient() + fc.submitErr = errors.New("bmc unreachable") rec := &fakeTestRecorder{} cfg := cfgWithTestInterval(time.Millisecond) r := newRecWithTestRecorder(t, c, cfg, res, &fakeFactory{client: fc}, rec) @@ -806,7 +807,7 @@ func TestNotifyTestEvent_MatchingID_RecordsSuccess(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() rec := &fakeTestRecorder{} cfg := cfgWithTestInterval(time.Millisecond) r := newRecWithTestRecorder(t, c, cfg, res, &fakeFactory{client: fc}, rec) @@ -835,7 +836,7 @@ func TestNotifyTestEvent_AnyMessageId_RecordsSuccess(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() rec := &fakeTestRecorder{} cfg := cfgWithTestInterval(time.Millisecond) r := newRecWithTestRecorder(t, c, cfg, res, &fakeFactory{client: fc}, rec) @@ -855,7 +856,7 @@ func TestNotifyTestEvent_AfterDeadline_NoOp(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() rec := &fakeTestRecorder{} cfg := cfgWithTestInterval(time.Millisecond) cfg.TestEventTimeout = 5 * time.Millisecond // very short deadline @@ -887,7 +888,7 @@ func TestForgetTestState_ClearsAndCallsRecorderForget(t *testing.T) { c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) res := &fakeResolver{} res.set(makeResolved(), nil) - fc := &fakeClient{} + fc := capableFakeClient() rec := &fakeTestRecorder{} cfg := cfgWithTestInterval(time.Millisecond) r := newRecWithTestRecorder(t, c, cfg, res, &fakeFactory{client: fc}, rec) @@ -912,3 +913,157 @@ func TestForgetTestState_ClearsAndCallsRecorderForget(t *testing.T) { t.Errorf("after ForgetTestState, NotifyTestEvent should be a no-op, got %+v", results) } } + +// -- StatusRecorder / reason constants -- + +func TestStatusRecorder_ReasonSubscribed(t *testing.T) { + c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) + res := &fakeResolver{} + res.set(makeResolved(), nil) + fc := capableFakeClient() + rec := &fakeStatusRecorder{} + r := newRecWithStatusRecorder(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}, rec) + + if _, err := r.Reconcile(context.Background(), req()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + statuses := rec.snapshotStatuses() + if len(statuses) == 0 { + t.Fatal("expected at least one status recording") + } + last := statuses[len(statuses)-1] + if last.reason != subscriptions.ReasonSubscribed { + t.Errorf("reason: got %q, want %q", last.reason, subscriptions.ReasonSubscribed) + } + if last.bmcName != testBMCName { + t.Errorf("bmcName: got %q, want %q", last.bmcName, testBMCName) + } +} + +func TestStatusRecorder_ReasonExcluded(t *testing.T) { + // Use a Lenovo BMC — dellVendorMatch() only covers Dell, so Lenovo is excluded. + c := newClientWith(t, bmcObject(testBMCName, "Lenovo", "")) + res := &fakeResolver{} + res.set(makeResolved(), nil) + fc := &fakeClient{} + rec := &fakeStatusRecorder{} + // Config explicitly excludes Lenovo. + cfg := &subscriptions.Config{ + ExcludeHardware: []subscriptions.ExcludeRule{{Vendor: "Lenovo", Models: []string{"*"}}}, + } + r := newRecWithStatusRecorder(t, c, cfg, res, &fakeFactory{client: fc}, rec) + + if _, err := r.Reconcile(context.Background(), req()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + statuses := rec.snapshotStatuses() + if len(statuses) == 0 { + t.Fatal("expected at least one status recording") + } + last := statuses[len(statuses)-1] + if last.reason != subscriptions.ReasonExcluded { + t.Errorf("reason: got %q, want %q", last.reason, subscriptions.ReasonExcluded) + } +} + +func TestStatusRecorder_ReasonServiceDisabled(t *testing.T) { + c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) + res := &fakeResolver{} + res.set(makeResolved(), nil) + // ServiceEnabled=false → ReasonServiceDisabled. + fc := &fakeClient{probeEnabled: false, probeHasSubmit: false} + rec := &fakeStatusRecorder{} + r := newRecWithStatusRecorder(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}, rec) + + if _, err := r.Reconcile(context.Background(), req()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + statuses := rec.snapshotStatuses() + if len(statuses) == 0 { + t.Fatal("expected at least one status recording") + } + last := statuses[len(statuses)-1] + if last.reason != subscriptions.ReasonServiceDisabled { + t.Errorf("reason: got %q, want %q", last.reason, subscriptions.ReasonServiceDisabled) + } +} + +func TestStatusRecorder_ReasonNoSubmitTestEvent(t *testing.T) { + c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) + res := &fakeResolver{} + res.set(makeResolved(), nil) + // ServiceEnabled=true but HasSubmitTestEvent=false → ReasonNoSubmitTestEvent. + fc := &fakeClient{probeEnabled: true, probeHasSubmit: false} + rec := &fakeStatusRecorder{} + r := newRecWithStatusRecorder(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}, rec) + + if _, err := r.Reconcile(context.Background(), req()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + statuses := rec.snapshotStatuses() + if len(statuses) == 0 { + t.Fatal("expected at least one status recording") + } + last := statuses[len(statuses)-1] + if last.reason != subscriptions.ReasonNoSubmitTestEvent { + t.Errorf("reason: got %q, want %q", last.reason, subscriptions.ReasonNoSubmitTestEvent) + } +} + +func TestStatusRecorder_ReasonProbeFailed(t *testing.T) { + c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) + res := &fakeResolver{} + res.set(makeResolved(), nil) + fc := capableFakeClient() + fc.probeErr = errors.New("connection refused") + rec := &fakeStatusRecorder{} + r := newRecWithStatusRecorder(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}, rec) + + if _, err := r.Reconcile(context.Background(), req()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + statuses := rec.snapshotStatuses() + if len(statuses) == 0 { + t.Fatal("expected at least one status recording for probe failure") + } + last := statuses[len(statuses)-1] + if last.reason != subscriptions.ReasonProbeFailed { + t.Errorf("reason: got %q, want %q", last.reason, subscriptions.ReasonProbeFailed) + } +} + +func TestStatusRecorder_ForgetCalledOnForgetTestState(t *testing.T) { + c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) + res := &fakeResolver{} + res.set(makeResolved(), nil) + fc := capableFakeClient() + rec := &fakeStatusRecorder{} + r := newRecWithStatusRecorder(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}, rec) + + if _, err := r.Reconcile(context.Background(), req()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + r.ForgetTestState(testBMCName) + + if forgotten := rec.snapshotForgottenStatuses(); len(forgotten) == 0 || forgotten[0] != testBMCName { + t.Errorf("ForgetTestState should call StatusRecorder.ForgetSubscriptionStatus(%q), got %v", testBMCName, forgotten) + } +} + +func TestUnsubscribedBMCReasons_PopulatedAfterReconcile(t *testing.T) { + c := newClientWith(t, bmcObject(testBMCName, vendorDellInc, modelR650)) + res := &fakeResolver{} + res.set(makeResolved(), nil) + // ServiceEnabled=false so BMC is unsubscribed. + fc := &fakeClient{probeEnabled: false, probeHasSubmit: false} + r := newRec(t, c, dellVendorMatch(), res, &fakeFactory{client: fc}) + + if _, err := r.Reconcile(context.Background(), req()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + reasons := r.UnsubscribedBMCReasons() + if reasons[testBMCName] == "" { + t.Errorf("expected a reason for %q, got empty map %v", testBMCName, reasons) + } +} diff --git a/internal/telemetry/subscriptions/subscribe_matcher.go b/internal/telemetry/subscriptions/subscribe_matcher.go index db9263c9..2031bb2e 100644 --- a/internal/telemetry/subscriptions/subscribe_matcher.go +++ b/internal/telemetry/subscriptions/subscribe_matcher.go @@ -9,38 +9,49 @@ import ( "github.com/blang/semver/v4" ) -// SubscribeToBMC decides whether the reconciler should keep Redfish -// event subscriptions on this BMC. It returns the matching HardwareMatch -// row (which carries vendor-specific settings like TestMessageId), or nil -// if no row matches and subscriptions should not be created. -func SubscribeToBMC(ref BMCRef, cfg *Config) *HardwareMatch { - if cfg == nil { - return nil - } - // An unknown vendor (informer hasn't populated Status yet, or BMC - // never reported one) does not qualify — we have no signal that - // this BMC supports subscriptions. - if ref.Vendor == "" { - return nil +// IsExcluded reports whether the BMC should be kept out of event-based +// delivery based on the excludeHardware opt-out list. Returns false when the +// vendor is not yet known (informer hasn't populated Status) so that the +// EventService probe — not a vendor mismatch — makes the final call. +func IsExcluded(ref BMCRef, cfg *Config) bool { + if cfg == nil || ref.Vendor == "" { + return false } - for i := range cfg.EventBasedHardware { - hw := &cfg.EventBasedHardware[i] - if !vendorMatches(hw.Vendor, ref.Vendor) { + for _, ex := range cfg.ExcludeHardware { + if !vendorMatches(ex.Vendor, ref.Vendor) { continue } - if !modelsMatch(hw.Models, ref.Model) { + if !modelsMatch(ex.Models, ref.Model) { continue } - if !firmwareSatisfies(hw.MinFirmware, ref.FirmwareVersion) { + if !excludeFirmwareSatisfies(ex.MinFirmware, ref.FirmwareVersion) { continue } - return hw + return true + } + return false +} + +// TestOverrideFor returns the first TestOverride row that matches the BMC, +// or nil when no row matches (caller should auto-discover a MessageId). +func TestOverrideFor(ref BMCRef, cfg *Config) *TestOverride { + if cfg == nil { + return nil + } + for i := range cfg.TestEventOverrides { + ov := &cfg.TestEventOverrides[i] + if !vendorMatches(ov.Vendor, ref.Vendor) { + continue + } + if !modelsMatch(ov.Models, ref.Model) { + continue + } + return ov } return nil } -// vendorMatches compares the configured vendor against the BMC's -// reported manufacturer. +// vendorMatches compares the configured vendor against the BMC's reported manufacturer. func vendorMatches(configured, reported string) bool { if configured == "" || reported == "" { return false @@ -49,12 +60,9 @@ func vendorMatches(configured, reported string) bool { } // modelsMatch returns true if model is in the configured list, or if -// the list contains "*". Comparison is case-insensitive — vendor -// product naming has the same casing chaos as vendor strings. +// the list contains "*". Comparison is case-insensitive. func modelsMatch(models []string, reported string) bool { if len(models) == 0 { - // Defensive: schema validation rejects empty lists, but under- - // specified rows should never match. return false } reported = strings.TrimSpace(reported) @@ -69,24 +77,25 @@ func modelsMatch(models []string, reported string) bool { return false } -// firmwareSatisfies returns true when minFirmware is empty, or when -// reported parses as a semver >= minFirmware. -func firmwareSatisfies(minFirmware, reported string) bool { +// excludeFirmwareSatisfies returns true when minFirmware is empty (no floor), +// or when the reported firmware is BELOW the floor — meaning the exclude rule +// applies to old firmware only ("exclude this model if firmware < X"). +// An empty reported version with a non-empty floor is treated as "unknown, +// apply the exclusion conservatively". +func excludeFirmwareSatisfies(minFirmware, reported string) bool { if minFirmware == "" { return true } - if reported == "" { - return false - } min, err := semver.ParseTolerant(minFirmware) if err != nil { - // Config validation rejects invalid semver, but treat it as - // "never matches" if one reaches us anyway. - return false + return true + } + if reported == "" { + return true } got, err := semver.ParseTolerant(reported) if err != nil { - return false + return true } - return got.GTE(min) + return got.LT(min) } diff --git a/internal/telemetry/subscriptions/subscribe_matcher_test.go b/internal/telemetry/subscriptions/subscribe_matcher_test.go index cda8db56..460fd775 100644 --- a/internal/telemetry/subscriptions/subscribe_matcher_test.go +++ b/internal/telemetry/subscriptions/subscribe_matcher_test.go @@ -9,178 +9,173 @@ import ( "github.com/ironcore-dev/metal-maintenance-operator/internal/telemetry/subscriptions" ) -// cfgWith returns a Config preloaded with the given hardware rows. -func cfgWith(rows ...subscriptions.HardwareMatch) *subscriptions.Config { - return &subscriptions.Config{EventBasedHardware: rows} +// excludeCfg returns a Config preloaded with the given exclude rows. +func excludeCfg(rows ...subscriptions.ExcludeRule) *subscriptions.Config { + return &subscriptions.Config{ExcludeHardware: rows} } -func TestSubscribeToBMC_NilConfig_IsFalse(t *testing.T) { - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Name: testBMCName, Vendor: vendorDellInc}, nil) != nil { - t.Error("nil cfg matched: want nil") +// overrideCfg returns a Config preloaded with the given override rows. +func overrideCfg(rows ...subscriptions.TestOverride) *subscriptions.Config { + return &subscriptions.Config{TestEventOverrides: rows} +} + +// -- IsExcluded -- + +func TestIsExcluded_NilConfig_NotExcluded(t *testing.T) { + if subscriptions.IsExcluded(subscriptions.BMCRef{Name: testBMCName, Vendor: vendorDellInc}, nil) { + t.Error("nil cfg: want not excluded") } } -func TestSubscribeToBMC_NoRows_IsFalse(t *testing.T) { - // No eventBasedHardware rows → nothing subscribes. Default-deny. +func TestIsExcluded_NoRows_NotExcluded(t *testing.T) { cfg := &subscriptions.Config{} - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) != nil { - t.Error("empty cfg matched: want nil") + if subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) { + t.Error("empty cfg: want not excluded") + } +} + +func TestIsExcluded_EmptyVendor_NotExcluded(t *testing.T) { + // A BMC whose Status.Manufacturer hasn't been populated yet must NOT be + // excluded — we haven't identified it yet, so the probe decides. + cfg := excludeCfg(subscriptions.ExcludeRule{Vendor: vendorDellInc, Models: []string{"*"}}) + if subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: ""}, cfg) { + t.Error("empty vendor: want not excluded (let probe decide)") } } // -- vendor matching -- -// TestSubscribeToBMC_VendorMatch_CanonicalExact pins that both sides of -// the comparison are canonical metal-operator bmc.Manufacturer values -// (matched on the ConfigMap side by Validate, and produced by -// metal-operator on the BMC.Status side). Match is plain string equality -// — short forms or case variations do NOT match, because they'd also -// never appear in BMC.Status.Manufacturer. -func TestSubscribeToBMC_VendorMatch_CanonicalExact(t *testing.T) { - cfg := cfgWith(subscriptions.HardwareMatch{ +func TestIsExcluded_VendorMatch_Canonical(t *testing.T) { + cfg := excludeCfg(subscriptions.ExcludeRule{ Vendor: vendorDellInc, Models: []string{"*"}, }) t.Run("match/Dell Inc.", func(t *testing.T) { - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) == nil { - t.Error("canonical vendor: want non-nil") + if !subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) { + t.Error("canonical vendor: want excluded") } }) - // Non-canonical reported values must NOT match — metal-operator - // will never write these to BMC.Status.Manufacturer, so accepting - // them here would diverge from what Validate enforces. + // Non-canonical reported values must NOT match. for _, v := range []string{"Dell", "DELL", "dell", " Dell Inc. "} { t.Run("no-match/"+v, func(t *testing.T) { - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: v, Model: modelR650}, cfg) != nil { - t.Errorf("vendor %q matched: want nil (non-canonical reported value)", v) + if subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: v, Model: modelR650}, cfg) { + t.Errorf("vendor %q excluded: want not excluded (non-canonical)", v) } }) } } -func TestSubscribeToBMC_VendorMismatch_IsFalse(t *testing.T) { - cfg := cfgWith(subscriptions.HardwareMatch{ - Vendor: vendorDellInc, - Models: []string{"*"}, - }) - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: vendorHPE, Model: "ProLiant"}, cfg) != nil { - t.Error("vendor mismatch matched: want nil") - } -} - -func TestSubscribeToBMC_EmptyVendor_NeverMatches(t *testing.T) { - // A BMC whose Status.Manufacturer hasn't been populated yet must NOT - // silently match — that would push events at a BMC we can't identify. - cfg := cfgWith(subscriptions.HardwareMatch{ +func TestIsExcluded_VendorMismatch_NotExcluded(t *testing.T) { + cfg := excludeCfg(subscriptions.ExcludeRule{ Vendor: vendorDellInc, Models: []string{"*"}, }) - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: ""}, cfg) != nil { - t.Error("empty vendor matched: want nil") + if subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: vendorHPE, Model: "ProLiant"}, cfg) { + t.Error("vendor mismatch: want not excluded") } } // -- model matching -- -func TestSubscribeToBMC_WildcardModel_MatchesAny(t *testing.T) { - cfg := cfgWith(subscriptions.HardwareMatch{ +func TestIsExcluded_WildcardModel_ExcludesAny(t *testing.T) { + cfg := excludeCfg(subscriptions.ExcludeRule{ Vendor: vendorDellInc, Models: []string{"*"}, }) for _, m := range []string{modelR650, modelPowerEdgeR750, "anything"} { t.Run(m, func(t *testing.T) { - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: vendorDellInc, Model: m}, cfg) == nil { - t.Errorf("model %q: want non-nil (wildcard)", m) + if !subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: vendorDellInc, Model: m}, cfg) { + t.Errorf("model %q: want excluded (wildcard)", m) } }) } } -func TestSubscribeToBMC_SpecificModel_MatchesExact(t *testing.T) { - cfg := cfgWith(subscriptions.HardwareMatch{ +func TestIsExcluded_SpecificModel_MatchesExact(t *testing.T) { + cfg := excludeCfg(subscriptions.ExcludeRule{ Vendor: vendorDellInc, Models: []string{"PowerEdge R650", modelPowerEdgeR750}, }) cases := []struct { - model string - wantNil bool + model string + wantExcluded bool }{ - {"PowerEdge R650", false}, - {modelPowerEdgeR750, false}, - {"powerEDGE r650", false}, // case-insensitive - {" PowerEdge R650 ", false}, // whitespace tolerant - {"PowerEdge R840", true}, // not in list - {modelR650, true}, // partial match → no + {"PowerEdge R650", true}, + {modelPowerEdgeR750, true}, + {"powerEDGE r650", true}, // case-insensitive + {" PowerEdge R650 ", true}, // whitespace tolerant + {"PowerEdge R840", false}, // not in list + {modelR650, false}, // partial match → no } for _, tc := range cases { t.Run(tc.model, func(t *testing.T) { - got := subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: vendorDellInc, Model: tc.model}, cfg) - if tc.wantNil && got != nil { - t.Errorf("model %q: got non-nil, want nil", tc.model) + got := subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: vendorDellInc, Model: tc.model}, cfg) + if tc.wantExcluded && !got { + t.Errorf("model %q: want excluded", tc.model) } - if !tc.wantNil && got == nil { - t.Errorf("model %q: got nil, want non-nil", tc.model) + if !tc.wantExcluded && got { + t.Errorf("model %q: want not excluded", tc.model) } }) } } -func TestSubscribeToBMC_EmptyModelsList_DoesNotMatch(t *testing.T) { - // Defensive: schema validation rejects empty Models, but the function - // must not panic or wildcard-match if it somehow sees one. - cfg := cfgWith(subscriptions.HardwareMatch{Vendor: vendorDellInc, Models: []string{}}) - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) != nil { - t.Error("empty models matched: want nil") +func TestIsExcluded_EmptyModelsList_NotExcluded(t *testing.T) { + // Defensive: validation rejects empty Models, but the function must not panic. + cfg := excludeCfg(subscriptions.ExcludeRule{Vendor: vendorDellInc, Models: []string{}}) + if subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) { + t.Error("empty models: want not excluded") } } -// -- firmware gating -- +// -- firmware gating (MinFirmware on ExcludeRule means "exclude when firmware is BELOW this") -- -func TestSubscribeToBMC_MinFirmwareUnset_AlwaysSatisfied(t *testing.T) { - cfg := cfgWith(subscriptions.HardwareMatch{ +func TestIsExcluded_MinFirmwareUnset_AlwaysExcluded(t *testing.T) { + cfg := excludeCfg(subscriptions.ExcludeRule{ Vendor: vendorDellInc, Models: []string{"*"}, - // MinFirmware omitted + // MinFirmware omitted: exclude for all firmware versions }) - // Even with unparseable firmware, no constraint means match. - got := subscriptions.SubscribeToBMC(subscriptions.BMCRef{ + got := subscriptions.IsExcluded(subscriptions.BMCRef{ Vendor: vendorDellInc, Model: modelR650, FirmwareVersion: "garbage", }, cfg) - if got == nil { - t.Error("want non-nil (no firmware constraint)") + if !got { + t.Error("want excluded (no firmware constraint means always excluded)") } } -func TestSubscribeToBMC_FirmwareGate(t *testing.T) { - cfg := cfgWith(subscriptions.HardwareMatch{ +func TestIsExcluded_FirmwareGate(t *testing.T) { + // MinFirmware="5.10.0" means "exclude firmware BELOW 5.10.0". + // Firmware at or above the floor is NOT excluded. + cfg := excludeCfg(subscriptions.ExcludeRule{ Vendor: vendorDellInc, Models: []string{"*"}, MinFirmware: firmware5_10_0, }) cases := []struct { - firmware string - wantNil bool - why string + firmware string + wantExcluded bool + why string }{ - {firmware5_10_0, false, "exact match"}, - {"5.10.5", false, "patch above"}, - {"6.0.0", false, "major above"}, - {"5.9.99", true, "patch below"}, - {"4.99.99", true, "major below"}, - {"", true, "missing firmware"}, - {"not-semver", true, "unparseable"}, - {"5.10", false, "two-component semver (tolerant parse)"}, + {firmware5_10_0, false, "exact match — at floor, not excluded"}, + {"5.10.5", false, "patch above — not excluded"}, + {"6.0.0", false, "major above — not excluded"}, + {"5.9.99", true, "patch below — excluded"}, + {"4.99.99", true, "major below — excluded"}, + {"", true, "missing firmware — excluded (safe default)"}, + {"not-semver", true, "unparseable — excluded (safe default)"}, + {"5.10", false, "two-component semver (tolerant parse) — not excluded"}, } for _, tc := range cases { t.Run(tc.why, func(t *testing.T) { - got := subscriptions.SubscribeToBMC(subscriptions.BMCRef{ + got := subscriptions.IsExcluded(subscriptions.BMCRef{ Vendor: vendorDellInc, Model: modelR650, FirmwareVersion: tc.firmware, }, cfg) - if tc.wantNil && got != nil { - t.Errorf("firmware %q: got non-nil, want nil (%s)", tc.firmware, tc.why) + if tc.wantExcluded && !got { + t.Errorf("firmware %q: want excluded (%s)", tc.firmware, tc.why) } - if !tc.wantNil && got == nil { - t.Errorf("firmware %q: got nil, want non-nil (%s)", tc.firmware, tc.why) + if !tc.wantExcluded && got { + t.Errorf("firmware %q: want not excluded (%s)", tc.firmware, tc.why) } }) } @@ -188,28 +183,125 @@ func TestSubscribeToBMC_FirmwareGate(t *testing.T) { // -- first-match-wins -- -func TestSubscribeToBMC_FirstMatchingRowWins(t *testing.T) { - // Two rows for the same vendor with different firmware constraints. - // The first row matches less strictly; it must win even when the - // second row would also match. - cfg := cfgWith( - subscriptions.HardwareMatch{Vendor: vendorDellInc, Models: []string{"*"}}, - subscriptions.HardwareMatch{Vendor: vendorDellInc, Models: []string{modelR650}, MinFirmware: "5.0.0"}, +func TestIsExcluded_FirstMatchingRowWins(t *testing.T) { + cfg := excludeCfg( + subscriptions.ExcludeRule{Vendor: vendorDellInc, Models: []string{"*"}}, + subscriptions.ExcludeRule{Vendor: vendorDellInc, Models: []string{modelR650}, MinFirmware: "5.0.0"}, ) - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{ + if !subscriptions.IsExcluded(subscriptions.BMCRef{ Vendor: vendorDellInc, Model: modelR650, FirmwareVersion: "6.0.0", - }, cfg) == nil { - t.Error("want non-nil (first matching row should win)") + }, cfg) { + t.Error("want excluded (first matching row should win)") } } -func TestSubscribeToBMC_LaterRow_StillMatches(t *testing.T) { - // Earlier row doesn't match (different vendor); later row does. - cfg := cfgWith( - subscriptions.HardwareMatch{Vendor: vendorDellInc, Models: []string{"*"}}, - subscriptions.HardwareMatch{Vendor: vendorHPE, Models: []string{"*"}}, +func TestIsExcluded_LaterRow_StillExcludes(t *testing.T) { + cfg := excludeCfg( + subscriptions.ExcludeRule{Vendor: vendorDellInc, Models: []string{"*"}}, + subscriptions.ExcludeRule{Vendor: vendorHPE, Models: []string{"*"}}, ) - if subscriptions.SubscribeToBMC(subscriptions.BMCRef{Vendor: vendorHPE, Model: "ProLiant"}, cfg) == nil { - t.Error("want non-nil (later row matches)") + if !subscriptions.IsExcluded(subscriptions.BMCRef{Vendor: vendorHPE, Model: "ProLiant"}, cfg) { + t.Error("want excluded (later row matches)") + } +} + +// -- TestOverrideFor -- + +func TestTestOverrideFor_NilConfig_Nil(t *testing.T) { + got := subscriptions.TestOverrideFor(subscriptions.BMCRef{Vendor: vendorDellInc}, nil) + if got != nil { + t.Errorf("nil cfg: want nil, got %+v", got) + } +} + +func TestTestOverrideFor_NoRows_Nil(t *testing.T) { + got := subscriptions.TestOverrideFor(subscriptions.BMCRef{Vendor: vendorDellInc}, &subscriptions.Config{}) + if got != nil { + t.Errorf("empty cfg: want nil, got %+v", got) + } +} + +func TestTestOverrideFor_VendorMatch_ReturnsOverride(t *testing.T) { + cfg := overrideCfg(subscriptions.TestOverride{ + Vendor: vendorDellInc, + Models: []string{"*"}, + TestMessageId: testMessageId, + }) + got := subscriptions.TestOverrideFor(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) + if got == nil { + t.Fatal("vendor match: want non-nil override") + } + if got.TestMessageId != testMessageId { + t.Errorf("TestMessageId: got %q, want %q", got.TestMessageId, testMessageId) + } +} + +func TestTestOverrideFor_VendorMismatch_Nil(t *testing.T) { + cfg := overrideCfg(subscriptions.TestOverride{Vendor: vendorDellInc, Models: []string{"*"}}) + got := subscriptions.TestOverrideFor(subscriptions.BMCRef{Vendor: vendorHPE, Model: "ProLiant"}, cfg) + if got != nil { + t.Errorf("vendor mismatch: want nil, got %+v", got) + } +} + +func TestTestOverrideFor_WildcardModel_MatchesAny(t *testing.T) { + cfg := overrideCfg(subscriptions.TestOverride{Vendor: vendorDellInc, Models: []string{"*"}, TestMessageId: testMessageId}) + for _, m := range []string{modelR650, modelPowerEdgeR750, "anything"} { + t.Run(m, func(t *testing.T) { + got := subscriptions.TestOverrideFor(subscriptions.BMCRef{Vendor: vendorDellInc, Model: m}, cfg) + if got == nil { + t.Errorf("model %q: want non-nil (wildcard)", m) + } + }) + } +} + +func TestTestOverrideFor_SpecificModel_MatchesExact(t *testing.T) { + cfg := overrideCfg(subscriptions.TestOverride{ + Vendor: vendorDellInc, + Models: []string{"PowerEdge R650", modelPowerEdgeR750}, + }) + cases := []struct { + model string + wantNil bool + }{ + {"PowerEdge R650", false}, + {modelPowerEdgeR750, false}, + {"powerEDGE r650", false}, // case-insensitive + {" PowerEdge R650 ", false}, // whitespace tolerant + {"PowerEdge R840", true}, // not in list + {modelR650, true}, // partial match → no + } + for _, tc := range cases { + t.Run(tc.model, func(t *testing.T) { + got := subscriptions.TestOverrideFor(subscriptions.BMCRef{Vendor: vendorDellInc, Model: tc.model}, cfg) + if tc.wantNil && got != nil { + t.Errorf("model %q: got non-nil, want nil", tc.model) + } + if !tc.wantNil && got == nil { + t.Errorf("model %q: got nil, want non-nil", tc.model) + } + }) + } +} + +func TestTestOverrideFor_FirstMatchWins(t *testing.T) { + first := subscriptions.TestOverride{ + Vendor: vendorDellInc, + Models: []string{"*"}, + TestMessageId: "first.1.0.First", + } + second := subscriptions.TestOverride{ + Vendor: vendorDellInc, + Models: []string{modelR650}, + TestMessageId: "second.1.0.Second", + } + cfg := overrideCfg(first, second) + got := subscriptions.TestOverrideFor(subscriptions.BMCRef{Vendor: vendorDellInc, Model: modelR650}, cfg) + if got == nil { + t.Fatal("want non-nil override") + } + if got.TestMessageId != "first.1.0.First" { + t.Errorf("first-match-wins: got %q, want first row", got.TestMessageId) } } diff --git a/internal/telemetry/subscriptions/types.go b/internal/telemetry/subscriptions/types.go index 705b2f8f..1b74b88f 100644 --- a/internal/telemetry/subscriptions/types.go +++ b/internal/telemetry/subscriptions/types.go @@ -59,6 +59,19 @@ type Subscription struct { EventFormat string // "MetricReport" or "Event". } +// EventServiceCapabilities holds the result of a live EventService probe. +type EventServiceCapabilities struct { + // ServiceEnabled is true when the BMC's EventService is present and enabled. + ServiceEnabled bool + // HasSubmitTestEvent is true when the EventService advertises a + // SubmitTestEvent action target. + HasSubmitTestEvent bool + // DiscoveredTestMessageId is the first viable MessageId found by walking + // the BMC's RegistryPrefixes. Empty when the walk was skipped (an override + // was provided) or when no registry entry could be found. + DiscoveredTestMessageId string +} + // Client is the reconciler's contract with a BMC over Redfish. It's the // bmc.BMC surface for Create/Delete/Logout, augmented with the list // operation that upstream bmc.BMC does not expose today. The production @@ -79,6 +92,11 @@ type Client interface { // caller can match it when the event arrives at the receiver to // confirm round-trip. SubmitTestEvent(ctx context.Context, params TestEventParams) error + // ProbeEventService interrogates the BMC's EventService to determine + // whether it supports event-push subscriptions. When needsDiscovery is + // true and the service is capable, it also walks the BMC's Redfish + // message registries to find a suitable SubmitTestEvent MessageId. + ProbeEventService(ctx context.Context, needsDiscovery bool) (*EventServiceCapabilities, error) Logout() } diff --git a/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook.go b/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook.go index 0251279d..4f57fe01 100644 --- a/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook.go +++ b/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook.go @@ -118,11 +118,8 @@ func checkForDuplicateBMCSettingsRefToBMC(settingsList *baseboardv1alpha1.BMCSet if bs.Spec.BMCRef == nil { continue } - if bs.Spec.BMCRef.Name != settings.Spec.BMCRef.Name { - continue - } - if len(settings.Spec.ReadinessGates) == 0 && len(bs.Spec.ReadinessGates) == 0 { - err := fmt.Errorf("BMC (%s) referred in %s is duplicate of BMC (%s) referred in %s without readinessGates", + if bs.Spec.BMCRef.Name == settings.Spec.BMCRef.Name { + err := fmt.Errorf("BMC (%s) referred in %s is duplicate of BMC (%s) referred in %s", settings.Spec.BMCRef.Name, settings.Name, bs.Spec.BMCRef.Name, bs.Name) return nil, apierrors.NewInvalid( schema.GroupKind{Group: settings.GroupVersionKind().Group, Kind: settings.Kind}, diff --git a/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook_test.go b/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook_test.go index c8a42718..ad7386f4 100644 --- a/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook_test.go +++ b/internal/webhook/baseboard/v1alpha1/bmcsettings_webhook_test.go @@ -57,7 +57,7 @@ var _ = Describe("BMCSettings Webhook", func() { Context("When creating or updating BMCSettings under Validating Webhook", func() { - It("should deny creation if an existing sibling referring to the same BMC has no ReadinessGates", func(ctx SpecContext) { + It("should deny creation if a BMC referred is already referred by another", func(ctx SpecContext) { By("Creating another BMCSettings with reference to existing referred BMC") BMCSettingsV2 := &baseboardv1alpha1.BMCSettings{ ObjectMeta: metav1.ObjectMeta{ @@ -75,29 +75,6 @@ var _ = Describe("BMCSettings Webhook", func() { Expect(validator.ValidateCreate(ctx, BMCSettingsV2)).Error().To(HaveOccurred()) }) - It("should allow creation if every existing sibling referring to the same BMC already has ReadinessGates", func(ctx SpecContext) { - By("Giving BMCSettingsV1 non-empty ReadinessGates") - Eventually(Update(BMCSettingsV1, func() { - BMCSettingsV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating another BMCSettings with reference to existing referred BMC") - BMCSettingsV2 := &baseboardv1alpha1.BMCSettings{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-bmc-", - }, - Spec: baseboardv1alpha1.BMCSettingsSpec{ - BMCRef: &v1.LocalObjectReference{Name: "foo"}, - BMCSettingsTemplate: baseboardv1alpha1.BMCSettingsTemplate{ - SettingsTemplate: api.SettingsTemplate{ - Version: "1.45.455b66-rev4", - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }}, - } - Expect(validator.ValidateCreate(ctx, BMCSettingsV2)).Error().ToNot(HaveOccurred()) - }) - It("should create if a referenced BMC is NOT duplicate", func() { By("Creating another BMCSetting for different BMCRef") BMCSettingsV2 := &baseboardv1alpha1.BMCSettings{ @@ -116,7 +93,7 @@ var _ = Describe("BMCSettings Webhook", func() { Expect(k8sClient.Create(ctx, BMCSettingsV2)).To(Succeed()) }) - It("should deny update if an existing sibling referring to the same BMC has no ReadinessGates", func() { + It("should deny update if a BMC referred is already referred by another", func() { By("Creating another BMCSetting with different BMCRef") BMCSettingsV2 := &baseboardv1alpha1.BMCSettings{ ObjectMeta: metav1.ObjectMeta{ @@ -139,34 +116,6 @@ var _ = Describe("BMCSettings Webhook", func() { Expect(validator.ValidateUpdate(ctx, BMCSettingsV2, BMCSettingsV2Updated)).Error().To(HaveOccurred()) }) - It("should allow update if every existing sibling referring to the same BMC already has ReadinessGates", func() { - By("Giving BMCSettingsV1 non-empty ReadinessGates") - Eventually(Update(BMCSettingsV1, func() { - BMCSettingsV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating another BMCSetting with different BMCRef") - BMCSettingsV2 := &baseboardv1alpha1.BMCSettings{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: baseboardv1alpha1.BMCSettingsSpec{ - BMCRef: &v1.LocalObjectReference{Name: "bar"}, - BMCSettingsTemplate: baseboardv1alpha1.BMCSettingsTemplate{ - SettingsTemplate: api.SettingsTemplate{ - Version: "P70 v1.45 (12/06/2017)", - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }}, - } - Expect(k8sClient.Create(ctx, BMCSettingsV2)).To(Succeed()) - - By("Updating an BMCSettingsV2 to refer to existing BMC") - BMCSettingsV2Updated := BMCSettingsV2.DeepCopy() - BMCSettingsV2Updated.Spec.BMCRef = BMCSettingsV1.Spec.BMCRef - Expect(validator.ValidateUpdate(ctx, BMCSettingsV2, BMCSettingsV2Updated)).Error().ToNot(HaveOccurred()) - }) - It("should update if a BMC referred is not referred by another", func() { By("Creating another BMCSetting with different BMCref") BMCSettingsV2 := &baseboardv1alpha1.BMCSettings{ diff --git a/internal/webhook/baseboard/v1alpha1/bmcversion_webhook.go b/internal/webhook/baseboard/v1alpha1/bmcversion_webhook.go index 0d862631..cdaf5521 100644 --- a/internal/webhook/baseboard/v1alpha1/bmcversion_webhook.go +++ b/internal/webhook/baseboard/v1alpha1/bmcversion_webhook.go @@ -105,11 +105,8 @@ func checkForDuplicateBMCVersionsRefToBMC(versionList *baseboardv1alpha1.BMCVers if v.Spec.BMCRef == nil { continue } - if v.Spec.BMCRef.Name != version.Spec.BMCRef.Name { - continue - } - if len(version.Spec.ReadinessGates) == 0 && len(v.Spec.ReadinessGates) == 0 { - err := fmt.Errorf("BMC (%s) referred in %s is duplicate of BMC (%s) referred in %s without readinessGates", + if v.Spec.BMCRef.Name == version.Spec.BMCRef.Name { + err := fmt.Errorf("BMC (%s) referred in %s is duplicate of BMC (%s) referred in %s", version.Spec.BMCRef.Name, version.Name, v.Spec.BMCRef.Name, v.Name) return nil, apierrors.NewInvalid( schema.GroupKind{Group: version.GroupVersionKind().Group, Kind: version.Kind}, diff --git a/internal/webhook/baseboard/v1alpha1/bmcversion_webhook_test.go b/internal/webhook/baseboard/v1alpha1/bmcversion_webhook_test.go index e686aa66..326e4aa6 100644 --- a/internal/webhook/baseboard/v1alpha1/bmcversion_webhook_test.go +++ b/internal/webhook/baseboard/v1alpha1/bmcversion_webhook_test.go @@ -58,7 +58,7 @@ var _ = Describe("BMCVersion Webhook", func() { }) Context("When creating or updating BMCVersion under Validating Webhook", func() { - It("should deny creation if an existing sibling referring to the same BMC has no ReadinessGates", func(ctx SpecContext) { + It("should deny creation if a BMC referred is already referred by another", func(ctx SpecContext) { By("Creating another BMCVersion with reference to existing referred BMC") BMCVersionV2 := &baseboardv1alpha1.BMCVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -78,31 +78,6 @@ var _ = Describe("BMCVersion Webhook", func() { Expect(validator.ValidateCreate(ctx, BMCVersionV2)).Error().To(HaveOccurred()) }) - It("should allow creation if every existing sibling referring to the same BMC already has ReadinessGates", func(ctx SpecContext) { - By("Giving BMCVersionV1 non-empty ReadinessGates") - Eventually(Update(BMCVersionV1, func() { - BMCVersionV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating another BMCVersion with reference to existing referred BMC") - BMCVersionV2 := &baseboardv1alpha1.BMCVersion{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-bmc-ver", - }, - Spec: baseboardv1alpha1.BMCVersionSpec{ - BMCVersionTemplate: baseboardv1alpha1.BMCVersionTemplate{ - VersionTemplate: api.VersionTemplate{ - Version: "P71 v1.45 (12/06/2017)", - Image: api.ImageSpec{URI: "P71 v1.45 (12/06/2017)"}, - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }, - BMCRef: &v1.LocalObjectReference{Name: "foo"}, - }, - } - Expect(validator.ValidateCreate(ctx, BMCVersionV2)).Error().ToNot(HaveOccurred()) - }) - It("should create if a referenced BMC is NOT duplicate", func() { By("Creating another BMCVersion for different BMCRef") BMCVersionV2 := &baseboardv1alpha1.BMCVersion{ @@ -123,7 +98,7 @@ var _ = Describe("BMCVersion Webhook", func() { Expect(k8sClient.Create(ctx, BMCVersionV2)).To(Succeed()) }) - It("should deny update if an existing sibling referring to the same BMC has no ReadinessGates", func() { + It("should deny update if a BMC referred is already referred by another", func() { By("Creating another BMCVersion with different BMCRef") BMCVersionV2 := &baseboardv1alpha1.BMCVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -148,36 +123,6 @@ var _ = Describe("BMCVersion Webhook", func() { Expect(validator.ValidateUpdate(ctx, BMCVersionV1, BMCVersionV2Updated)).Error().To(HaveOccurred()) }) - It("should allow update if every existing sibling referring to the same BMC already has ReadinessGates", func() { - By("Giving BMCVersionV1 non-empty ReadinessGates") - Eventually(Update(BMCVersionV1, func() { - BMCVersionV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating another BMCVersion with different BMCRef") - BMCVersionV2 := &baseboardv1alpha1.BMCVersion{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-bmc-ver", - }, - Spec: baseboardv1alpha1.BMCVersionSpec{ - BMCVersionTemplate: baseboardv1alpha1.BMCVersionTemplate{ - VersionTemplate: api.VersionTemplate{ - Version: "P71 v1.45 (12/06/2017)", - Image: api.ImageSpec{URI: "P71 v1.45 (12/06/2017)"}, - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }, - BMCRef: &v1.LocalObjectReference{Name: "bar"}, - }, - } - Expect(k8sClient.Create(ctx, BMCVersionV2)).To(Succeed()) - - By("Updating an BMCVersionV2 to refer to existing BMC") - BMCVersionV2Updated := BMCVersionV2.DeepCopy() - BMCVersionV2Updated.Spec.BMCRef = BMCVersionV1.Spec.BMCRef - Expect(validator.ValidateUpdate(ctx, BMCVersionV1, BMCVersionV2Updated)).Error().ToNot(HaveOccurred()) - }) - It("should update if a BMC referred is not referred by another", func() { By("Creating another BMCVersion with different BMCref") BMCVersionV2 := &baseboardv1alpha1.BMCVersion{ diff --git a/internal/webhook/system/v1alpha1/biossettings_webhook.go b/internal/webhook/system/v1alpha1/biossettings_webhook.go index 5d3f7d6f..4702ed5f 100644 --- a/internal/webhook/system/v1alpha1/biossettings_webhook.go +++ b/internal/webhook/system/v1alpha1/biossettings_webhook.go @@ -106,11 +106,8 @@ func checkForDuplicateBIOSSettingsRefToServer(settingsList *systemv1alpha1.BIOSS if bs.Spec.ServerRef == nil { continue } - if settings.Spec.ServerRef.Name != bs.Spec.ServerRef.Name { - continue - } - if len(settings.Spec.ReadinessGates) == 0 && len(bs.Spec.ReadinessGates) == 0 { - err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s without readinessGates", + if settings.Spec.ServerRef.Name == bs.Spec.ServerRef.Name { + err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s", settings.Spec.ServerRef.Name, settings.Name, bs.Spec.ServerRef.Name, bs.Name) return nil, apierrors.NewInvalid( schema.GroupKind{Group: settings.GroupVersionKind().Group, Kind: settings.Kind}, diff --git a/internal/webhook/system/v1alpha1/biossettings_webhook_test.go b/internal/webhook/system/v1alpha1/biossettings_webhook_test.go index 2fa3c53a..b9f9408e 100644 --- a/internal/webhook/system/v1alpha1/biossettings_webhook_test.go +++ b/internal/webhook/system/v1alpha1/biossettings_webhook_test.go @@ -60,7 +60,7 @@ var _ = Describe("BIOSSettings Webhook", func() { Expect(client.IgnoreNotFound(k8sClient.DeleteAllOf(ctx, &maintenancev1alpha1.ServerMaintenance{}))).To(Succeed()) }) - It("should deny creation if an existing sibling targeting the same Server has no ReadinessGates", func(ctx SpecContext) { + It("should deny creation if a Server already has a BIOSSettings", func(ctx SpecContext) { By("Creating another BIOSSettings targeting the same Server") biosSettingsV2 := &systemv1alpha1.BIOSSettings{ ObjectMeta: metav1.ObjectMeta{ @@ -84,35 +84,6 @@ var _ = Describe("BIOSSettings Webhook", func() { Expect(validator.ValidateCreate(ctx, biosSettingsV2)).Error().To(HaveOccurred()) }) - It("should allow creation if every existing sibling targeting the same Server already has ReadinessGates", func(ctx SpecContext) { - By("Giving biosSettingsV1 non-empty ReadinessGates") - Eventually(Update(biosSettingsV1, func() { - biosSettingsV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating another BIOSSettings targeting the same Server") - biosSettingsV2 := &systemv1alpha1.BIOSSettings{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: systemv1alpha1.BIOSSettingsSpec{ - ServerRef: &v1.LocalObjectReference{Name: "foo"}, - BIOSSettingsTemplate: systemv1alpha1.BIOSSettingsTemplate{ - SettingsTemplate: api.SettingsTemplate{ - Version: defaultMockUpServerBiosVersion, - SettingsFlow: []api.SettingsFlowItem{{ - Settings: map[string]string{}, - Priority: 1, - Name: "one", - }}, - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }, - }, - } - Expect(validator.ValidateCreate(ctx, biosSettingsV2)).Error().ToNot(HaveOccurred()) - }) - It("should allow creating a BIOSSettings for a Server without one", func() { By("Creating a BIOSSettings targeting a new Server") biosSettingsV2 := &systemv1alpha1.BIOSSettings{ @@ -137,7 +108,7 @@ var _ = Describe("BIOSSettings Webhook", func() { Expect(k8sClient.Create(ctx, biosSettingsV2)).To(Succeed()) }) - It("should deny update if an existing sibling targeting the same Server has no ReadinessGates", func() { + It("should deny update if spec.serverRef is duplicate", func() { By("Creating a BIOSSettings with different ServerRef") biosSettingsV2 := &systemv1alpha1.BIOSSettings{ ObjectMeta: metav1.ObjectMeta{ @@ -166,40 +137,6 @@ var _ = Describe("BIOSSettings Webhook", func() { Expect(validator.ValidateUpdate(ctx, biosSettingsV2, biosSettingsV2Updated)).Error().To(HaveOccurred()) }) - It("should allow update if every existing sibling targeting the same Server already has ReadinessGates", func() { - By("Giving biosSettingsV1 non-empty ReadinessGates") - Eventually(Update(biosSettingsV1, func() { - biosSettingsV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating a BIOSSettings with different ServerRef") - biosSettingsV2 := &systemv1alpha1.BIOSSettings{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: systemv1alpha1.BIOSSettingsSpec{ - ServerRef: &v1.LocalObjectReference{Name: "bar"}, - BIOSSettingsTemplate: systemv1alpha1.BIOSSettingsTemplate{ - SettingsTemplate: api.SettingsTemplate{ - Version: anotherMockUpServerBiosVersion, - SettingsFlow: []api.SettingsFlowItem{{ - Settings: map[string]string{}, - Priority: 1, - Name: "one", - }}, - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }, - }, - } - Expect(k8sClient.Create(ctx, biosSettingsV2)).To(Succeed()) - - By("Updating an biosSettingsV2 with a conflicting ServerRef") - biosSettingsV2Updated := biosSettingsV2.DeepCopy() - biosSettingsV2Updated.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"} - Expect(validator.ValidateUpdate(ctx, biosSettingsV2, biosSettingsV2Updated)).Error().ToNot(HaveOccurred()) - }) - It("should allow update if a different field is duplicate", func() { By("Creating a BIOSSetting with different ServerRef") biosSettingsV2 := &systemv1alpha1.BIOSSettings{ diff --git a/internal/webhook/system/v1alpha1/biosversion_webhook.go b/internal/webhook/system/v1alpha1/biosversion_webhook.go index 52916e27..e044a64a 100644 --- a/internal/webhook/system/v1alpha1/biosversion_webhook.go +++ b/internal/webhook/system/v1alpha1/biosversion_webhook.go @@ -106,11 +106,8 @@ func checkForDuplicateBIOSVersionRefToServer(versions *systemv1alpha1.BIOSVersio if bv.Spec.ServerRef == nil { continue } - if version.Spec.ServerRef.Name != bv.Spec.ServerRef.Name { - continue - } - if len(version.Spec.ReadinessGates) == 0 && len(bv.Spec.ReadinessGates) == 0 { - err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s without readinessGates", + if version.Spec.ServerRef.Name == bv.Spec.ServerRef.Name { + err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s", version.Spec.ServerRef.Name, version.Name, bv.Spec.ServerRef.Name, bv.Name) return nil, apierrors.NewInvalid( schema.GroupKind{Group: version.GroupVersionKind().Group, Kind: version.Kind}, diff --git a/internal/webhook/system/v1alpha1/biosversion_webhook_test.go b/internal/webhook/system/v1alpha1/biosversion_webhook_test.go index c77c26ba..1571cf17 100644 --- a/internal/webhook/system/v1alpha1/biosversion_webhook_test.go +++ b/internal/webhook/system/v1alpha1/biosversion_webhook_test.go @@ -54,7 +54,7 @@ var _ = Describe("BIOSVersion Webhook", func() { Expect(client.IgnoreNotFound(k8sClient.DeleteAllOf(ctx, &maintenancev1alpha1.ServerMaintenance{}, client.InNamespace(metav1.NamespaceDefault)))).To(Succeed()) }) - It("should deny creation if an existing sibling referring to the same server has no ReadinessGates", func(ctx SpecContext) { + It("should deny creation if spec.serverRef is duplicate", func(ctx SpecContext) { By("Creating another BIOSVersion with existing ServerRef") biosVersionV2 := &systemv1alpha1.BIOSVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -74,31 +74,6 @@ var _ = Describe("BIOSVersion Webhook", func() { Expect(validator.ValidateCreate(ctx, biosVersionV2)).Error().To(HaveOccurred()) }) - It("should allow creation if every existing sibling referring to the same server already has ReadinessGates", func(ctx SpecContext) { - By("Giving biosVersionV1 non-empty ReadinessGates") - Eventually(Update(biosVersionV1, func() { - biosVersionV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating another BIOSVersion with existing ServerRef") - biosVersionV2 := &systemv1alpha1.BIOSVersion{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: systemv1alpha1.BIOSVersionSpec{ - BIOSVersionTemplate: systemv1alpha1.BIOSVersionTemplate{ - VersionTemplate: api.VersionTemplate{ - Version: "P71 v1.45 (12/06/2017)", - Image: api.ImageSpec{URI: "two"}, - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }, - ServerRef: &v1.LocalObjectReference{Name: "foo"}, - }, - } - Expect(validator.ValidateCreate(ctx, biosVersionV2)).Error().ToNot(HaveOccurred()) - }) - It("should create if a spec.serverRef field is not a duplicate", func() { By("Creating another BIOSVersion with different ServerRef") biosVersionV2 := &systemv1alpha1.BIOSVersion{ @@ -119,7 +94,7 @@ var _ = Describe("BIOSVersion Webhook", func() { Expect(validator.ValidateCreate(ctx, biosVersionV2)).Error().ToNot(HaveOccurred()) }) - It("should deny update if an existing sibling referring to the same server has no ReadinessGates", func() { + It("should deny update if spec.serverRef is duplicate", func() { By("Creating a BIOSVersion with different ServerRef") biosVersionV2 := &systemv1alpha1.BIOSVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -144,36 +119,6 @@ var _ = Describe("BIOSVersion Webhook", func() { Expect(validator.ValidateUpdate(ctx, biosVersionV2, biosVersionV2Updated)).Error().To(HaveOccurred()) }) - It("should allow update if every existing sibling referring to the same server already has ReadinessGates", func() { - By("Giving biosVersionV1 non-empty ReadinessGates") - Eventually(Update(biosVersionV1, func() { - biosVersionV1.Spec.ReadinessGates = []metalv1alpha1.ConditionRequirement{{Type: "SomeGate", RequiredStatus: metav1.ConditionTrue}} - })).Should(Succeed()) - - By("Creating a BIOSVersion with different ServerRef") - biosVersionV2 := &systemv1alpha1.BIOSVersion{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: systemv1alpha1.BIOSVersionSpec{ - BIOSVersionTemplate: systemv1alpha1.BIOSVersionTemplate{ - VersionTemplate: api.VersionTemplate{ - Version: "P71 v1.45 (12/06/2017)", - Image: api.ImageSpec{URI: "asd"}, - ServerMaintenancePolicy: maintenancev1alpha1.ServerMaintenancePolicyEnforced, - }, - }, - ServerRef: &v1.LocalObjectReference{Name: "bar"}, - }, - } - Expect(k8sClient.Create(ctx, biosVersionV2)).To(Succeed()) - - By("Updating a BIOSVersion V2 to conflicting spec.serverRef") - biosVersionV2Updated := biosVersionV2.DeepCopy() - biosVersionV2Updated.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"} - Expect(validator.ValidateUpdate(ctx, biosVersionV2, biosVersionV2Updated)).Error().ToNot(HaveOccurred()) - }) - It("should allow update if a different field is duplicate", func() { By("Creating a BIOSVersion with different ServerRef") biosVersionV2 := &systemv1alpha1.BIOSVersion{