From 9a2d2dd5ba70dcc4dbd66122412d1e3bb107bf17 Mon Sep 17 00:00:00 2001 From: Andreas Fritzler Date: Tue, 28 Jul 2026 10:43:46 +0200 Subject: [PATCH 1/2] Drop BMC Pending state in favor of ConditionReset Signed-off-by: Andreas Fritzler --- .../api/v1alpha1/bmcstatus.go | 3 +- .../applyconfiguration/internal/internal.go | 1 - api/v1alpha1/bmc_types.go | 10 +- cmd/main.go | 26 +- config/crd/bases/metal.ironcore.dev_bmcs.yaml | 6 +- .../crd/bmcs.metal.ironcore.dev.yaml | 6 +- dist/install.yaml | 6 +- docs/api-reference/api.md | 4 +- internal/controller/bmc_controller.go | 241 ++++++++---------- internal/controller/bmc_controller_test.go | 2 +- internal/controller/conditions.go | 8 +- internal/controller/suite_test.go | 22 +- 12 files changed, 151 insertions(+), 184 deletions(-) diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.go index 724ab33d1..a0dafefc9 100644 --- a/api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.go +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.go @@ -30,8 +30,7 @@ type BMCStatusApplyConfiguration struct { SerialNumber *string `json:"serialNumber,omitempty"` // FirmwareVersion is the version of the firmware currently running on the BMC. FirmwareVersion *string `json:"firmwareVersion,omitempty"` - // State represents the current state of the BMC. - // kubebuilder:validation:Enum=Enabled;Error;Pending + // State represents the current state of the BMC as reported by Redfish. State *apiv1alpha1.BMCState `json:"state,omitempty"` // PowerState represents the current power state of the BMC. PowerState *apiv1alpha1.BMCPowerState `json:"powerState,omitempty"` diff --git a/api/v1alpha1/applyconfiguration/internal/internal.go b/api/v1alpha1/applyconfiguration/internal/internal.go index 26302b2d4..40dbab1df 100644 --- a/api/v1alpha1/applyconfiguration/internal/internal.go +++ b/api/v1alpha1/applyconfiguration/internal/internal.go @@ -592,7 +592,6 @@ var schemaYAML = typed.YAMLObject(`types: - name: state type: namedType: com.github.ironcore-dev.metal-operator.api.v1alpha1.BMCState - default: Pending - name: com.github.ironcore-dev.metal-operator.api.v1alpha1.BMCUser map: fields: diff --git a/api/v1alpha1/bmc_types.go b/api/v1alpha1/bmc_types.go index 2668a951f..8c65bd339 100644 --- a/api/v1alpha1/bmc_types.go +++ b/api/v1alpha1/bmc_types.go @@ -197,9 +197,7 @@ type BMCStatus struct { // +optional FirmwareVersion string `json:"firmwareVersion,omitempty"` - // State represents the current state of the BMC. - // kubebuilder:validation:Enum=Enabled;Error;Pending - // +kubebuilder:default=Pending + // State represents the current state of the BMC as reported by Redfish. // +optional State BMCState `json:"state,omitempty"` @@ -232,12 +230,6 @@ type BMCState string const ( // BMCStateEnabled indicates that the BMC is enabled and functioning correctly. BMCStateEnabled BMCState = "Enabled" - - // BMCStateError indicates that there is an error with the BMC. - BMCStateError BMCState = "Error" - - // BMCStatePending indicates that there is an error connecting with the BMC. - BMCStatePending BMCState = "Pending" ) // +kubebuilder:object:root=true diff --git a/cmd/main.go b/cmd/main.go index 876dbaf9e..1810e0f58 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -130,7 +130,7 @@ func main() { // nolint: gocyclo flag.DurationVar(&bmcFailureResetDelay, "bmc-failure-reset-delay", 0, "Reset the BMC after this duration of consecutive failures. 0 to disable.") flag.DurationVar(&bmcResetResyncInterval, "bmc-reset-resync-interval", 2*time.Minute, - "Defines the interval at which the bmc is polled when bmc reset is in-progress.") + "Defines the interval at which the BMC is requeued after a connection failure or while a reset is in progress.") flag.DurationVar(&bmcResetWaitingInterval, "bmc-reset-waiting-interval", 2*time.Minute, "Defines the duration which the bmc waits before reconciling again when bmc has been reset.") flag.DurationVar(&maintenanceResyncInterval, "maintenance-resync-interval", 2*time.Minute, @@ -425,18 +425,18 @@ func main() { // nolint: gocyclo os.Exit(1) } if err = (&controller.BMCReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - DefaultProtocol: effectiveProtocol, - SkipCertValidation: effectiveSkipCert, - BMCFailureResetDelay: bmcFailureResetDelay, - BMCResetWaitTime: bmcResetWaitingInterval, - BMCClientRetryInterval: bmcResetResyncInterval, - ManagerNamespace: managerNamespace, - EventURL: eventURL, - DNSRecordTemplate: dnsRecordTemplate, - Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), - BMCOptions: bmc.Options{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + DefaultProtocol: effectiveProtocol, + SkipCertValidation: effectiveSkipCert, + FailureResetDelay: bmcFailureResetDelay, + ResetWaitTime: bmcResetWaitingInterval, + ClientRetryInterval: bmcResetResyncInterval, + ManagerNamespace: managerNamespace, + EventURL: eventURL, + DNSRecordTemplate: dnsRecordTemplate, + Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), + Options: bmc.Options{ BasicAuth: true, }, }).SetupWithManager(mgr); err != nil { diff --git a/config/crd/bases/metal.ironcore.dev_bmcs.yaml b/config/crd/bases/metal.ironcore.dev_bmcs.yaml index 3503c3097..391a5215b 100644 --- a/config/crd/bases/metal.ironcore.dev_bmcs.yaml +++ b/config/crd/bases/metal.ironcore.dev_bmcs.yaml @@ -291,10 +291,8 @@ spec: description: SKU is the stock keeping unit identifier for the BMC. type: string state: - default: Pending - description: |- - State represents the current state of the BMC. - kubebuilder:validation:Enum=Enabled;Error;Pending + description: State represents the current state of the BMC as reported + by Redfish. type: string type: object type: object diff --git a/dist/chart/templates/crd/bmcs.metal.ironcore.dev.yaml b/dist/chart/templates/crd/bmcs.metal.ironcore.dev.yaml index 00cfc597f..aeaf5245e 100644 --- a/dist/chart/templates/crd/bmcs.metal.ironcore.dev.yaml +++ b/dist/chart/templates/crd/bmcs.metal.ironcore.dev.yaml @@ -305,10 +305,8 @@ spec: description: SKU is the stock keeping unit identifier for the BMC. type: string state: - default: Pending - description: |- - State represents the current state of the BMC. - kubebuilder:validation:Enum=Enabled;Error;Pending + description: State represents the current state of the BMC as reported + by Redfish. type: string type: object type: object diff --git a/dist/install.yaml b/dist/install.yaml index 256c91668..e4f0fcf58 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -1344,10 +1344,8 @@ spec: description: SKU is the stock keeping unit identifier for the BMC. type: string state: - default: Pending - description: |- - State represents the current state of the BMC. - kubebuilder:validation:Enum=Enabled;Error;Pending + description: State represents the current state of the BMC as reported + by Redfish. type: string type: object type: object diff --git a/docs/api-reference/api.md b/docs/api-reference/api.md index d7da7319b..c8f00a0d4 100644 --- a/docs/api-reference/api.md +++ b/docs/api-reference/api.md @@ -670,8 +670,6 @@ _Appears in:_ | Field | Description | | --- | --- | | `Enabled` | BMCStateEnabled indicates that the BMC is enabled and functioning correctly.
| -| `Error` | BMCStateError indicates that there is an error with the BMC.
| -| `Pending` | BMCStatePending indicates that there is an error connecting with the BMC.
| #### BMCStatus @@ -694,7 +692,7 @@ _Appears in:_ | `sku` _string_ | SKU is the stock keeping unit identifier for the BMC. | | | | `serialNumber` _string_ | SerialNumber is the serial number of the BMC. | | | | `firmwareVersion` _string_ | FirmwareVersion is the version of the firmware currently running on the BMC. | | | -| `state` _[BMCState](#bmcstate)_ | State represents the current state of the BMC.
kubebuilder:validation:Enum=Enabled;Error;Pending | Pending | | +| `state` _[BMCState](#bmcstate)_ | State represents the current state of the BMC as reported by Redfish. | | | | `powerState` _[BMCPowerState](#bmcpowerstate)_ | PowerState represents the current power state of the BMC. | | | | `lastResetTime` _[Time](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#time-v1-meta)_ | LastResetTime is the timestamp of the last reset operation performed on the BMC. | | | | `metricsReportSubscriptionLink` _string_ | MetricsReportSubscriptionLink is the link to the metrics report subscription of the bmc. | | | diff --git a/internal/controller/bmc_controller.go b/internal/controller/bmc_controller.go index 09abbc281..8fd8e199e 100644 --- a/internal/controller/bmc_controller.go +++ b/internal/controller/bmc_controller.go @@ -45,21 +45,17 @@ const ( // BMCReconciler reconciles a BMC object type BMCReconciler struct { client.Client - Scheme *runtime.Scheme - DefaultProtocol metalv1alpha1.ProtocolScheme - SkipCertValidation bool - // BMCFailureResetDelay defines the duration after which a BMC will be reset upon repeated connection failures. - BMCFailureResetDelay time.Duration - BMCOptions bmc.Options - ManagerNamespace string - EventURL string - // BMCResetWaitTime defines the duration to wait after a BMC reset before attempting reconciliation again. - BMCResetWaitTime time.Duration - // BMCClientRetryInterval defines the duration to requeue reconciliation after a BMC client error/reset/unavailablility. - BMCClientRetryInterval time.Duration - // DNSRecordTemplatePath is the path to the file containing the DNSRecord template. - DNSRecordTemplate string - Conditions *conditionutils.Accessor + Scheme *runtime.Scheme + DefaultProtocol metalv1alpha1.ProtocolScheme + SkipCertValidation bool + FailureResetDelay time.Duration + Options bmc.Options + ManagerNamespace string + EventURL string + ResetWaitTime time.Duration + ClientRetryInterval time.Duration + DNSRecordTemplate string + Conditions *conditionutils.Accessor } // +kubebuilder:rbac:groups=metal.ironcore.dev,resources=endpoints,verbs=get;list;watch @@ -83,6 +79,7 @@ func (r *BMCReconciler) reconcileExists(ctx context.Context, bmcObj *metalv1alph if !bmcObj.DeletionTimestamp.IsZero() { return r.delete(ctx, bmcObj) } + return r.reconcile(ctx, bmcObj) } @@ -99,7 +96,7 @@ func (r *BMCReconciler) delete(ctx context.Context, bmcObj *metalv1alpha1.BMC) ( } } - bmcClient, err := bmcutils.GetBMCClientFromBMC(ctx, r.Client, bmcObj, r.DefaultProtocol, r.SkipCertValidation, r.BMCOptions) + bmcClient, err := bmcutils.GetBMCClientFromBMC(ctx, r.Client, bmcObj, r.DefaultProtocol, r.SkipCertValidation, r.Options) if err == nil { defer bmcClient.Logout() if err := r.deleteEventSubscription(ctx, bmcClient, bmcObj); err != nil { @@ -122,16 +119,21 @@ func (r *BMCReconciler) reconcile(ctx context.Context, bmcObj *metalv1alpha1.BMC log.V(1).Info("Skipped BMC reconciliation") return ctrl.Result{}, nil } - if r.waitForBMCReset(bmcObj, r.BMCResetWaitTime) { + + switch r.resetWaitState(bmcObj, r.ResetWaitTime) { + case resetWaitPending: log.V(1).Info("Skipped BMC reconciliation while waiting for BMC reset to complete") - if err := r.patchBMCStatePending(ctx, bmcObj); err != nil { + return ctrl.Result{RequeueAfter: r.ClientRetryInterval}, nil + case resetWaitExpired: + // The reset wait window elapsed but ConditionReset is still True. Clear + // it so shouldResetBMC can schedule future auto-resets; otherwise the + // stuck True condition blocks all recovery while the BMC stays down. + if err := r.clearResetCondition(ctx, bmcObj); err != nil { return ctrl.Result{}, err } - return ctrl.Result{ - RequeueAfter: r.BMCClientRetryInterval, - }, nil } - bmcClient, err := bmcutils.GetBMCClientFromBMC(ctx, r.Client, bmcObj, r.DefaultProtocol, r.SkipCertValidation, r.BMCOptions, bmcutils.BMCConnectivityCheckOption) + + bmcClient, err := bmcutils.GetBMCClientFromBMC(ctx, r.Client, bmcObj, r.DefaultProtocol, r.SkipCertValidation, r.Options, bmcutils.BMCConnectivityCheckOption) if err != nil { if r.shouldResetBMC(bmcObj) { log.V(1).Info("BMC needs reset, resetting", "BMC", bmcObj.Name) @@ -140,30 +142,44 @@ func (r *BMCReconciler) reconcile(ctx context.Context, bmcObj *metalv1alpha1.BMC } log.V(1).Info("BMC reset initiated", "BMC", bmcObj.Name) return ctrl.Result{ - RequeueAfter: r.BMCClientRetryInterval, + RequeueAfter: r.ClientRetryInterval, }, nil } - return ctrl.Result{RequeueAfter: r.BMCClientRetryInterval}, r.updateReadyConditionOnBMCFailure(ctx, bmcObj, err) + return ctrl.Result{RequeueAfter: r.ClientRetryInterval}, r.updateReadyConditionOnBMCFailure(ctx, bmcObj, err) } defer bmcClient.Logout() - // if BMC reset was issued and is successful, ensure to remove previous reset annotation - if modified, err := r.handlePreviousBMCResetAnnotations(ctx, bmcObj); err != nil || modified { - return ctrl.Result{}, err - } - if modified, err := r.handleAnnotationOperations(ctx, bmcObj, bmcClient); err != nil || modified { return ctrl.Result{}, err } - if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionTrue, ReasonConnected, "BMC is connected"); err != nil { + // Mark BMC connected and clear any in-flight reset in a single status patch. + bmcBase := bmcObj.DeepCopy() + if err := r.Conditions.UpdateSlice( + &bmcObj.Status.Conditions, + ConditionReady, + conditionutils.UpdateStatus(corev1.ConditionTrue), + conditionutils.UpdateReason(ReasonConnected), + conditionutils.UpdateMessage("BMC is connected"), + ); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set BMC connected condition: %w", err) } - if err := r.updateConditions(ctx, bmcObj, false, ConditionReset, corev1.ConditionFalse, "ResetComplete", "BMC reset is complete"); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set BMC reset complete condition: %w", err) + if found, _ := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, &metav1.Condition{}); found { + if err := r.Conditions.UpdateSlice( + &bmcObj.Status.Conditions, + ConditionReset, + conditionutils.UpdateStatus(corev1.ConditionFalse), + conditionutils.UpdateReason(ReasonResetComplete), + conditionutils.UpdateMessage("BMC reset is complete"), + ); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to set BMC reset complete condition: %w", err) + } + } + if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to patch BMC conditions: %w", err) } - if err := r.updateBMCStatusDetails(ctx, bmcClient, bmcObj); err != nil { + if err := r.patchBMCStatus(ctx, bmcClient, bmcObj); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update BMC status: %w", err) } log.V(1).Info("Updated BMC status", "State", bmcObj.Status.State) @@ -188,7 +204,7 @@ func (r *BMCReconciler) reconcile(ctx context.Context, bmcObj *metalv1alpha1.BMC return ctrl.Result{}, nil } -func (r *BMCReconciler) updateBMCStatusDetails(ctx context.Context, bmcClient bmc.BMC, bmcObj *metalv1alpha1.BMC) error { +func (r *BMCReconciler) patchBMCStatus(ctx context.Context, bmcClient bmc.BMC, bmcObj *metalv1alpha1.BMC) error { log := ctrl.LoggerFrom(ctx) var ( ip metalv1alpha1.IP @@ -359,171 +375,136 @@ func (r *BMCReconciler) handleAnnotationOperations(ctx context.Context, bmcObj * log.V(1).Info("Unknown operation annotation, ignoring", "Operation", operation, "Supported Operations", schemas.GracefulRestartResetType) return false, nil } - switch value { - case schemas.GracefulRestartResetType: - log.V(1).Info("Handling operation", "Operation", operation, "RedfishResetType", value) - if err := r.resetBMC(ctx, bmcObj, bmcClient, ReasonUserReset, bmcUserResetMessage); err != nil { - return false, fmt.Errorf("failed to reset BMC: %w", err) - } - log.Info("Handled operation", "Operation", operation) - default: + if value != schemas.GracefulRestartResetType { log.V(1).Info("Unsupported operation annotation", "Operation", operation, "RedfishResetType", value) return false, nil } + + // Consume the annotation before issuing the side-effecting reset, so a + // crash or failed Redfish call cannot leave it behind as stale state. Once + // resetBMC sets ConditionReset, the condition is the sole source of truth + // for the in-flight reset. bmcBase := bmcObj.DeepCopy() metautils.DeleteAnnotation(bmcObj, metalv1alpha1.OperationAnnotation) if err := r.Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil { return false, fmt.Errorf("failed to remove operation annotation: %w", err) } log.V(1).Info("Removed operation annotation", "Operation", operation) + + log.V(1).Info("Handling operation", "Operation", operation, "RedfishResetType", value) + if err := r.resetBMC(ctx, bmcObj, bmcClient, ReasonUserReset, bmcUserResetMessage); err != nil { + return false, fmt.Errorf("failed to reset BMC: %w", err) + } + log.Info("Handled operation", "Operation", operation) return true, nil } func (r *BMCReconciler) updateReadyConditionOnBMCFailure(ctx context.Context, bmcObj *metalv1alpha1.BMC, err error) error { - httpErr := &schemas.Error{} - if errors.As(err, &httpErr) { - // only handle 5xx errors + reason, message := ReasonUnknownError, fmt.Sprintf("BMC connection error: %v", err) + if httpErr, ok := errors.AsType[*schemas.Error](err); ok { switch httpErr.HTTPReturnedStatusCode { case 401: - // Unauthorized error, likely due to bad credentials - if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonAuthenticationFailed, "BMC credentials are invalid"); err != nil { - return fmt.Errorf("failed to set BMC unauthorized condition: %w", err) - } - + reason, message = ReasonAuthenticationFailed, "BMC credentials are invalid" case 500: - // Internal Server Error, might be transient - if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonInternalError, "BMC internal server error"); err != nil { - return fmt.Errorf("failed to set BMC internal server error condition: %w", err) - } + reason, message = ReasonInternalError, "BMC internal server error" case 503: - // Service Unavailable, might be transient - if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonConnectionFailed, "BMC service unavailable"); err != nil { - return fmt.Errorf("failed to set BMC service unavailable condition: %w", err) - } - default: - if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonUnknownError, fmt.Sprintf("BMC connection error: %v", err)); err != nil { - return fmt.Errorf("failed to set BMC error condition: %w", err) - } - } - } else { - if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonUnknownError, fmt.Sprintf("BMC connection error: %v", err)); err != nil { - return fmt.Errorf("failed to set BMC error condition: %w", err) + reason, message = ReasonConnectionFailed, "BMC service unavailable" } } + if err := r.patchCondition(ctx, bmcObj, ConditionReady, corev1.ConditionFalse, reason, message); err != nil { + return fmt.Errorf("failed to set BMC ready condition: %w", err) + } return err } -func (r *BMCReconciler) waitForBMCReset(bmcObj *metalv1alpha1.BMC, delay time.Duration) bool { - condition := &metav1.Condition{} - found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, condition) - if err != nil || !found { - return false - } - if condition.Status == metav1.ConditionTrue { - // give bmc some time to start the reset process - if time.Since(condition.LastTransitionTime.Time) < delay { - return true - } - } - return false -} +// resetWaitState reports the state of an in-flight BMC reset. +type resetWaitState int -func (r *BMCReconciler) handlePreviousBMCResetAnnotations(ctx context.Context, bmcObj *metalv1alpha1.BMC) (bool, error) { - log := ctrl.LoggerFrom(ctx) +const ( + resetWaitNone resetWaitState = iota // no ConditionReset, or it is not True + resetWaitPending // ConditionReset is True and within the wait window + resetWaitExpired // ConditionReset is True but the wait window has elapsed +) + +func (r *BMCReconciler) resetWaitState(bmcObj *metalv1alpha1.BMC, delay time.Duration) resetWaitState { condition := &metav1.Condition{} found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, condition) - if err != nil || !found { - return false, nil + if err != nil || !found || condition.Status != metav1.ConditionTrue { + return resetWaitNone } - if condition.Status == metav1.ConditionTrue { - if operation, ok := bmcObj.GetAnnotations()[metalv1alpha1.OperationAnnotation]; ok && operation == metalv1alpha1.GracefulRestartBMC { - bmcBase := bmcObj.DeepCopy() - metautils.DeleteAnnotation(bmcObj, metalv1alpha1.OperationAnnotation) - if err := r.Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil { - return false, fmt.Errorf("failed to remove operation annotation from previous reset: %w", err) - } - log.V(1).Info("Removed operation annotation from previous reset", "Operation", operation) - return true, nil - } + // give bmc some time to start the reset process + if time.Since(condition.LastTransitionTime.Time) < delay { + return resetWaitPending } - return false, nil + return resetWaitExpired } func (r *BMCReconciler) shouldResetBMC(bmcObj *metalv1alpha1.BMC) bool { - if r.BMCFailureResetDelay == 0 { + if r.FailureResetDelay == 0 { return false } - bmcResetCondition := &metav1.Condition{} - found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, bmcResetCondition) - if err != nil || (found && bmcResetCondition.Status == metav1.ConditionTrue) { + if resetStatus, _ := r.Conditions.FindSliceStatus(bmcObj.Status.Conditions, ConditionReset); resetStatus == corev1.ConditionTrue { return false } readyCondition := &metav1.Condition{} - found, err = r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReady, readyCondition) + found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReady, readyCondition) if err != nil || !found { return false } - if readyCondition.Status == metav1.ConditionFalse && (readyCondition.Reason == ReasonInternalError || readyCondition.Reason == ReasonConnectionFailed) { - if time.Since(readyCondition.LastTransitionTime.Time) > r.BMCFailureResetDelay { - return true - } + if readyCondition.Status == metav1.ConditionFalse && + (readyCondition.Reason == ReasonInternalError || readyCondition.Reason == ReasonConnectionFailed) && + time.Since(readyCondition.LastTransitionTime.Time) > r.FailureResetDelay { + return true } return false } -func (r *BMCReconciler) patchBMCStatePending(ctx context.Context, bmcObj *metalv1alpha1.BMC) error { - if bmcObj.Status.State == metalv1alpha1.BMCStatePending { - return nil - } - bmcBase := bmcObj.DeepCopy() - bmcObj.Status.State = metalv1alpha1.BMCStatePending - if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil { - return fmt.Errorf("failed to patch BMC state to Pending: %w", err) - } - return nil -} - func (r *BMCReconciler) resetBMC(ctx context.Context, bmcObj *metalv1alpha1.BMC, bmcClient bmc.BMC, reason, message string) error { log := ctrl.LoggerFrom(ctx) - if err := r.updateConditions(ctx, bmcObj, true, ConditionReset, corev1.ConditionTrue, reason, message); err != nil { + if err := r.patchCondition(ctx, bmcObj, ConditionReset, corev1.ConditionTrue, reason, message); err != nil { return fmt.Errorf("failed to set BMC resetting condition: %w", err) } if bmcClient == nil { - // No client connection at all (e.g. auto-reset reached with a nil - // client from GetBMCClientFromBMC). No Redfish call possible — surface - // it instead of logging a nil error. return errors.Join( - r.patchBMCStatePending(ctx, bmcObj), + r.clearResetCondition(ctx, bmcObj), fmt.Errorf("could not reset BMC %s: no client connection", bmcObj.Name), ) } if err := bmcClient.ResetManager(ctx, bmcObj.Spec.BMCUUID, schemas.GracefulRestartResetType); err == nil { log.Info("Successfully reset BMC via Redfish", "BMC", bmcObj.Name) - return r.patchBMCStatePending(ctx, bmcObj) + return nil } else { if httpErr, ok := errors.AsType[*schemas.Error](err); ok { // only retryable on 5xx; anything else is a permanent failure for this attempt if httpErr.HTTPReturnedStatusCode < 500 || httpErr.HTTPReturnedStatusCode >= 600 { - return errors.Join(r.patchBMCStatePending(ctx, bmcObj), fmt.Errorf("could not reset BMC: %w", err)) + return errors.Join( + r.clearResetCondition(ctx, bmcObj), + fmt.Errorf("could not reset BMC: %w", err), + ) } } else { - return fmt.Errorf("could not reset BMC, unknown error: %w", err) + return errors.Join( + r.clearResetCondition(ctx, bmcObj), + fmt.Errorf("could not reset BMC, unknown error: %w", err), + ) } } log.V(1).Info("BMC reset returned a retryable 5xx, leaving reset condition set", "BMC", bmcObj.Name) return nil } -func (r *BMCReconciler) updateConditions(ctx context.Context, bmcObj *metalv1alpha1.BMC, createIfNotFound bool, conditionType string, status corev1.ConditionStatus, reason, message string) error { - condition := &metav1.Condition{} - ok, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, conditionType, condition) +func (r *BMCReconciler) clearResetCondition(ctx context.Context, bmcObj *metalv1alpha1.BMC) error { + found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, &metav1.Condition{}) if err != nil { - return fmt.Errorf("failed to find condition %s: %w", conditionType, err) + return fmt.Errorf("failed to find condition %s: %w", ConditionReset, err) } - if !ok && !createIfNotFound { - // condition not found and not allowed to create + if !found { return nil } + return r.patchCondition(ctx, bmcObj, ConditionReset, corev1.ConditionFalse, ReasonResetFailed, "BMC reset failed") +} + +func (r *BMCReconciler) patchCondition(ctx context.Context, bmcObj *metalv1alpha1.BMC, conditionType string, status corev1.ConditionStatus, reason, message string) error { bmcBase := bmcObj.DeepCopy() if err := r.Conditions.UpdateSlice( &bmcObj.Status.Conditions, diff --git a/internal/controller/bmc_controller_test.go b/internal/controller/bmc_controller_test.go index c19dc1087..267cb0294 100644 --- a/internal/controller/bmc_controller_test.go +++ b/internal/controller/bmc_controller_test.go @@ -673,7 +673,7 @@ var _ = Describe("BMC Conditions", func() { SatisfyAll( HaveField("Type", ConditionReset), HaveField("Status", metav1.ConditionFalse), - HaveField("Reason", "ResetComplete"), + HaveField("Reason", ReasonResetComplete), ), )), ) diff --git a/internal/controller/conditions.go b/internal/controller/conditions.go index fbb34302d..f3684686a 100644 --- a/internal/controller/conditions.go +++ b/internal/controller/conditions.go @@ -54,9 +54,13 @@ const ( ReasonVersionUpgradePending = "VersionUpgradePending" // ReasonResetIssued indicates a reset has been issued. ReasonResetIssued = "ResetIssued" - // ReasonResetRequired indicates a BMC reset is needed but not yet issued. + // ReasonResetComplete indicates a reset has completed. + ReasonResetComplete = "ResetComplete" + // ReasonResetFailed indicates a reset attempt failed. + ReasonResetFailed = "ResetFailed" + // ReasonResetRequired indicates a reset is needed but not yet issued. ReasonResetRequired = "ResetRequired" - // ReasonNoResetRequired indicates no BMC reset is needed for this apply cycle. + // ReasonNoResetRequired indicates no reset is needed for this apply cycle. ReasonNoResetRequired = "NoResetRequired" // ReasonAuthenticationFailed indicates authentication has failed. ReasonAuthenticationFailed = "AuthenticationFailed" diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 4c6837750..08599b9e3 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -178,17 +178,17 @@ func SetupTest(redfishMockServers []netip.AddrPort) *corev1.Namespace { Expect(err).NotTo(HaveOccurred()) Expect((&BMCReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - DefaultProtocol: metalv1alpha1.HTTPProtocolScheme, - SkipCertValidation: true, - ManagerNamespace: ns.Name, - BMCResetWaitTime: 400 * time.Millisecond, - BMCClientRetryInterval: 25 * time.Millisecond, - EventURL: "http://localhost:8008", - DNSRecordTemplate: dnsTemplate, - Conditions: accessor, - BMCOptions: bmc.Options{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + DefaultProtocol: metalv1alpha1.HTTPProtocolScheme, + SkipCertValidation: true, + ManagerNamespace: ns.Name, + ResetWaitTime: 400 * time.Millisecond, + ClientRetryInterval: 25 * time.Millisecond, + EventURL: "http://localhost:8008", + DNSRecordTemplate: dnsTemplate, + Conditions: accessor, + Options: bmc.Options{ PowerPollingInterval: 50 * time.Millisecond, PowerPollingTimeout: 200 * time.Millisecond, BasicAuth: true, From 7a9b7281c83765aec1b24a29a58ca2a9c60fcaeb Mon Sep 17 00:00:00 2001 From: Andreas Fritzler Date: Thu, 30 Jul 2026 13:12:05 +0200 Subject: [PATCH 2/2] Distinguish BMC reset timeout from failure in ConditionReset reason Signed-off-by: Andreas Fritzler --- cmd/main.go | 26 ++++----- internal/controller/bmc_controller.go | 81 +++++++++++++-------------- internal/controller/conditions.go | 3 + internal/controller/suite_test.go | 20 +++---- 4 files changed, 66 insertions(+), 64 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 1810e0f58..0b7f268db 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -100,7 +100,7 @@ func main() { // nolint: gocyclo discoveryTimeout time.Duration biosSettingsApplyTimeout time.Duration bmcFailureResetDelay time.Duration - bmcResetResyncInterval time.Duration + bmcReconnectInterval time.Duration bmcResetWaitingInterval time.Duration serverMaxConcurrentReconciles int serverClaimMaxConcurrentReconciles int @@ -129,7 +129,7 @@ func main() { // nolint: gocyclo "Defines the interval at which the server is polled.") flag.DurationVar(&bmcFailureResetDelay, "bmc-failure-reset-delay", 0, "Reset the BMC after this duration of consecutive failures. 0 to disable.") - flag.DurationVar(&bmcResetResyncInterval, "bmc-reset-resync-interval", 2*time.Minute, + flag.DurationVar(&bmcReconnectInterval, "bmc-reconnect-interval", 2*time.Minute, "Defines the interval at which the BMC is requeued after a connection failure or while a reset is in progress.") flag.DurationVar(&bmcResetWaitingInterval, "bmc-reset-waiting-interval", 2*time.Minute, "Defines the duration which the bmc waits before reconciling again when bmc has been reset.") @@ -425,17 +425,17 @@ func main() { // nolint: gocyclo os.Exit(1) } if err = (&controller.BMCReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - DefaultProtocol: effectiveProtocol, - SkipCertValidation: effectiveSkipCert, - FailureResetDelay: bmcFailureResetDelay, - ResetWaitTime: bmcResetWaitingInterval, - ClientRetryInterval: bmcResetResyncInterval, - ManagerNamespace: managerNamespace, - EventURL: eventURL, - DNSRecordTemplate: dnsRecordTemplate, - Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + DefaultProtocol: effectiveProtocol, + SkipCertValidation: effectiveSkipCert, + FailureResetDelay: bmcFailureResetDelay, + ResetWaitTime: bmcResetWaitingInterval, + ReconnectInterval: bmcReconnectInterval, + ManagerNamespace: managerNamespace, + EventURL: eventURL, + DNSRecordTemplate: dnsRecordTemplate, + Conditions: conditionutils.NewAccessor(conditionutils.AccessorOptions{}), Options: bmc.Options{ BasicAuth: true, }, diff --git a/internal/controller/bmc_controller.go b/internal/controller/bmc_controller.go index 8fd8e199e..278040040 100644 --- a/internal/controller/bmc_controller.go +++ b/internal/controller/bmc_controller.go @@ -45,17 +45,17 @@ const ( // BMCReconciler reconciles a BMC object type BMCReconciler struct { client.Client - Scheme *runtime.Scheme - DefaultProtocol metalv1alpha1.ProtocolScheme - SkipCertValidation bool - FailureResetDelay time.Duration - Options bmc.Options - ManagerNamespace string - EventURL string - ResetWaitTime time.Duration - ClientRetryInterval time.Duration - DNSRecordTemplate string - Conditions *conditionutils.Accessor + Scheme *runtime.Scheme + DefaultProtocol metalv1alpha1.ProtocolScheme + SkipCertValidation bool + FailureResetDelay time.Duration + Options bmc.Options + ManagerNamespace string + EventURL string + ResetWaitTime time.Duration + ReconnectInterval time.Duration + DNSRecordTemplate string + Conditions *conditionutils.Accessor } // +kubebuilder:rbac:groups=metal.ironcore.dev,resources=endpoints,verbs=get;list;watch @@ -120,15 +120,15 @@ func (r *BMCReconciler) reconcile(ctx context.Context, bmcObj *metalv1alpha1.BMC return ctrl.Result{}, nil } - switch r.resetWaitState(bmcObj, r.ResetWaitTime) { + switch r.resetWaitState(bmcObj) { case resetWaitPending: log.V(1).Info("Skipped BMC reconciliation while waiting for BMC reset to complete") - return ctrl.Result{RequeueAfter: r.ClientRetryInterval}, nil + return ctrl.Result{RequeueAfter: r.ReconnectInterval}, nil case resetWaitExpired: // The reset wait window elapsed but ConditionReset is still True. Clear // it so shouldResetBMC can schedule future auto-resets; otherwise the // stuck True condition blocks all recovery while the BMC stays down. - if err := r.clearResetCondition(ctx, bmcObj); err != nil { + if err := r.clearResetCondition(ctx, bmcObj, ReasonResetTimeout, "BMC reset timed out waiting for reconnect"); err != nil { return ctrl.Result{}, err } } @@ -142,10 +142,10 @@ func (r *BMCReconciler) reconcile(ctx context.Context, bmcObj *metalv1alpha1.BMC } log.V(1).Info("BMC reset initiated", "BMC", bmcObj.Name) return ctrl.Result{ - RequeueAfter: r.ClientRetryInterval, + RequeueAfter: r.ReconnectInterval, }, nil } - return ctrl.Result{RequeueAfter: r.ClientRetryInterval}, r.updateReadyConditionOnBMCFailure(ctx, bmcObj, err) + return ctrl.Result{RequeueAfter: r.ReconnectInterval}, r.updateReadyConditionOnBMCFailure(ctx, bmcObj, err) } defer bmcClient.Logout() @@ -426,14 +426,14 @@ const ( resetWaitExpired // ConditionReset is True but the wait window has elapsed ) -func (r *BMCReconciler) resetWaitState(bmcObj *metalv1alpha1.BMC, delay time.Duration) resetWaitState { +func (r *BMCReconciler) resetWaitState(bmcObj *metalv1alpha1.BMC) resetWaitState { condition := &metav1.Condition{} found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, condition) if err != nil || !found || condition.Status != metav1.ConditionTrue { return resetWaitNone } // give bmc some time to start the reset process - if time.Since(condition.LastTransitionTime.Time) < delay { + if time.Since(condition.LastTransitionTime.Time) < r.ResetWaitTime { return resetWaitPending } return resetWaitExpired @@ -465,35 +465,34 @@ func (r *BMCReconciler) resetBMC(ctx context.Context, bmcObj *metalv1alpha1.BMC, return fmt.Errorf("failed to set BMC resetting condition: %w", err) } if bmcClient == nil { - return errors.Join( - r.clearResetCondition(ctx, bmcObj), - fmt.Errorf("could not reset BMC %s: no client connection", bmcObj.Name), - ) + return r.failReset(ctx, bmcObj, fmt.Errorf("could not reset BMC %s: no client connection", bmcObj.Name)) } - if err := bmcClient.ResetManager(ctx, bmcObj.Spec.BMCUUID, schemas.GracefulRestartResetType); err == nil { + err := bmcClient.ResetManager(ctx, bmcObj.Spec.BMCUUID, schemas.GracefulRestartResetType) + if err == nil { log.Info("Successfully reset BMC via Redfish", "BMC", bmcObj.Name) return nil - } else { - if httpErr, ok := errors.AsType[*schemas.Error](err); ok { - // only retryable on 5xx; anything else is a permanent failure for this attempt - if httpErr.HTTPReturnedStatusCode < 500 || httpErr.HTTPReturnedStatusCode >= 600 { - return errors.Join( - r.clearResetCondition(ctx, bmcObj), - fmt.Errorf("could not reset BMC: %w", err), - ) - } - } else { - return errors.Join( - r.clearResetCondition(ctx, bmcObj), - fmt.Errorf("could not reset BMC, unknown error: %w", err), - ) + } + if httpErr, ok := errors.AsType[*schemas.Error](err); ok { + // only retryable on 5xx; anything else is a permanent failure for this attempt + if httpErr.HTTPReturnedStatusCode >= 500 && httpErr.HTTPReturnedStatusCode < 600 { + log.V(1).Info("BMC reset returned a retryable 5xx, leaving reset condition set", "BMC", bmcObj.Name) + return nil } + return r.failReset(ctx, bmcObj, fmt.Errorf("could not reset BMC: %w", err)) } - log.V(1).Info("BMC reset returned a retryable 5xx, leaving reset condition set", "BMC", bmcObj.Name) - return nil + return r.failReset(ctx, bmcObj, fmt.Errorf("could not reset BMC, unknown error: %w", err)) +} + +// failReset clears the in-flight reset condition and joins the clearing error +// with the reset failure. +func (r *BMCReconciler) failReset(ctx context.Context, bmcObj *metalv1alpha1.BMC, err error) error { + return errors.Join( + r.clearResetCondition(ctx, bmcObj, ReasonResetFailed, "BMC reset failed"), + err, + ) } -func (r *BMCReconciler) clearResetCondition(ctx context.Context, bmcObj *metalv1alpha1.BMC) error { +func (r *BMCReconciler) clearResetCondition(ctx context.Context, bmcObj *metalv1alpha1.BMC, reason, message string) error { found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, &metav1.Condition{}) if err != nil { return fmt.Errorf("failed to find condition %s: %w", ConditionReset, err) @@ -501,7 +500,7 @@ func (r *BMCReconciler) clearResetCondition(ctx context.Context, bmcObj *metalv1 if !found { return nil } - return r.patchCondition(ctx, bmcObj, ConditionReset, corev1.ConditionFalse, ReasonResetFailed, "BMC reset failed") + return r.patchCondition(ctx, bmcObj, ConditionReset, corev1.ConditionFalse, reason, message) } func (r *BMCReconciler) patchCondition(ctx context.Context, bmcObj *metalv1alpha1.BMC, conditionType string, status corev1.ConditionStatus, reason, message string) error { diff --git a/internal/controller/conditions.go b/internal/controller/conditions.go index f3684686a..82a083bb6 100644 --- a/internal/controller/conditions.go +++ b/internal/controller/conditions.go @@ -58,6 +58,9 @@ const ( ReasonResetComplete = "ResetComplete" // ReasonResetFailed indicates a reset attempt failed. ReasonResetFailed = "ResetFailed" + // ReasonResetTimeout indicates a reset was issued but the BMC did not + // reconnect within the reset wait window. + ReasonResetTimeout = "ResetTimeout" // ReasonResetRequired indicates a reset is needed but not yet issued. ReasonResetRequired = "ResetRequired" // ReasonNoResetRequired indicates no reset is needed for this apply cycle. diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 08599b9e3..16957d392 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -178,16 +178,16 @@ func SetupTest(redfishMockServers []netip.AddrPort) *corev1.Namespace { Expect(err).NotTo(HaveOccurred()) Expect((&BMCReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - DefaultProtocol: metalv1alpha1.HTTPProtocolScheme, - SkipCertValidation: true, - ManagerNamespace: ns.Name, - ResetWaitTime: 400 * time.Millisecond, - ClientRetryInterval: 25 * time.Millisecond, - EventURL: "http://localhost:8008", - DNSRecordTemplate: dnsTemplate, - Conditions: accessor, + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + DefaultProtocol: metalv1alpha1.HTTPProtocolScheme, + SkipCertValidation: true, + ManagerNamespace: ns.Name, + ResetWaitTime: 400 * time.Millisecond, + ReconnectInterval: 25 * time.Millisecond, + EventURL: "http://localhost:8008", + DNSRecordTemplate: dnsTemplate, + Conditions: accessor, Options: bmc.Options{ PowerPollingInterval: 50 * time.Millisecond, PowerPollingTimeout: 200 * time.Millisecond,