Improvements on Retry Logic

Author: hardikdr

internal/controller/serverbootconfig_helpers.go

@@ -27,6 +27,7 @@ import (
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -145,24 +146,27 @@ func PatchServerBootConfigWithError(
 }
 
 // PatchServerBootConfigCondition patches a single condition on the ServerBootConfiguration status.
-// Callers should only set condition types they own.
+// Callers should only set condition types they own. Retries on conflict so concurrent condition
+// writes from HTTP and PXE controllers do not lose each other's updates.
 func PatchServerBootConfigCondition(
 	ctx context.Context,
 	c client.Client,
 	namespacedName types.NamespacedName,
 	condition metav1.Condition,
 ) error {
-	var cur metalv1alpha1.ServerBootConfiguration
-	if fetchErr := c.Get(ctx, namespacedName, &cur); fetchErr != nil {
-		return fmt.Errorf("failed to fetch ServerBootConfiguration: %w", fetchErr)
-	}
-	base := cur.DeepCopy()
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		var cur metalv1alpha1.ServerBootConfiguration
+		if fetchErr := c.Get(ctx, namespacedName, &cur); fetchErr != nil {
+			return fmt.Errorf("failed to fetch ServerBootConfiguration: %w", fetchErr)
+		}
+		base := cur.DeepCopy()
 
-	// Default to current generation if caller didn't set it.
-	if condition.ObservedGeneration == 0 {
-		condition.ObservedGeneration = cur.Generation
-	}
-	apimeta.SetStatusCondition(&cur.Status.Conditions, condition)
+		// Default to current generation if caller didn't set it.
+		if condition.ObservedGeneration == 0 {
+			condition.ObservedGeneration = cur.Generation
+		}
+		apimeta.SetStatusCondition(&cur.Status.Conditions, condition)
 
-	return c.Status().Patch(ctx, &cur, client.MergeFrom(base))
+		return c.Status().Patch(ctx, &cur, client.MergeFrom(base))
+	})
 }

internal/controller/serverbootconfiguration_http_controller.go

@@ -24,6 +24,7 @@ import (
 	bootv1alpha1 "github.com/ironcore-dev/boot-operator/api/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -153,33 +154,35 @@ func (r *ServerBootConfigurationHTTPReconciler) reconcile(ctx context.Context, l
 
 func (r *ServerBootConfigurationHTTPReconciler) patchHTTPBootReadyConditionFromHTTPState(ctx context.Context, httpBootConfig *bootv1alpha1.HTTPBootConfig, cfg *metalv1alpha1.ServerBootConfiguration) error {
 	key := types.NamespacedName{Name: cfg.Name, Namespace: cfg.Namespace}
-	var cur metalv1alpha1.ServerBootConfiguration
-	if err := r.Get(ctx, key, &cur); err != nil {
-		return err
-	}
-	base := cur.DeepCopy()
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		var cur metalv1alpha1.ServerBootConfiguration
+		if err := r.Get(ctx, key, &cur); err != nil {
+			return err
+		}
+		base := cur.DeepCopy()
 
-	cond := metav1.Condition{
-		Type:               HTTPBootReadyConditionType,
-		ObservedGeneration: cur.Generation,
-	}
-	switch httpBootConfig.Status.State {
-	case bootv1alpha1.HTTPBootConfigStateReady:
-		cond.Status = metav1.ConditionTrue
-		cond.Reason = "BootConfigReady"
-		cond.Message = "HTTP boot configuration is ready."
-	case bootv1alpha1.HTTPBootConfigStateError:
-		cond.Status = metav1.ConditionFalse
-		cond.Reason = "BootConfigError"
-		cond.Message = "HTTPBootConfig reported an error."
-	default:
-		cond.Status = metav1.ConditionUnknown
-		cond.Reason = "BootConfigPending"
-		cond.Message = "Waiting for HTTPBootConfig to become Ready."
-	}
+		cond := metav1.Condition{
+			Type:               HTTPBootReadyConditionType,
+			ObservedGeneration: cur.Generation,
+		}
+		switch httpBootConfig.Status.State {
+		case bootv1alpha1.HTTPBootConfigStateReady:
+			cond.Status = metav1.ConditionTrue
+			cond.Reason = "BootConfigReady"
+			cond.Message = "HTTP boot configuration is ready."
+		case bootv1alpha1.HTTPBootConfigStateError:
+			cond.Status = metav1.ConditionFalse
+			cond.Reason = "BootConfigError"
+			cond.Message = "HTTPBootConfig reported an error."
+		default:
+			cond.Status = metav1.ConditionUnknown
+			cond.Reason = "BootConfigPending"
+			cond.Message = "Waiting for HTTPBootConfig to become Ready."
+		}
 
-	apimeta.SetStatusCondition(&cur.Status.Conditions, cond)
-	return r.Status().Patch(ctx, &cur, client.MergeFrom(base))
+		apimeta.SetStatusCondition(&cur.Status.Conditions, cond)
+		return r.Status().Patch(ctx, &cur, client.MergeFrom(base))
+	})
 }
 
 // getSystemUUIDFromServer fetches the UUID from the referenced Server object.

internal/controller/serverbootconfiguration_pxe_controller.go

@@ -31,6 +31,7 @@ import (
 	"github.com/ironcore-dev/boot-operator/internal/registry"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/go-logr/logr"
@@ -172,33 +173,35 @@ func (r *ServerBootConfigurationPXEReconciler) reconcile(ctx context.Context, lo
 
 func (r *ServerBootConfigurationPXEReconciler) patchIPXEBootReadyConditionFromIPXEState(ctx context.Context, config *v1alpha1.IPXEBootConfig, bootConfig *metalv1alpha1.ServerBootConfiguration) error {
 	key := types.NamespacedName{Name: bootConfig.Name, Namespace: bootConfig.Namespace}
-	var cur metalv1alpha1.ServerBootConfiguration
-	if err := r.Get(ctx, key, &cur); err != nil {
-		return err
-	}
-	base := cur.DeepCopy()
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		var cur metalv1alpha1.ServerBootConfiguration
+		if err := r.Get(ctx, key, &cur); err != nil {
+			return err
+		}
+		base := cur.DeepCopy()
 
-	cond := metav1.Condition{
-		Type:               IPXEBootReadyConditionType,
-		ObservedGeneration: cur.Generation,
-	}
-	switch config.Status.State {
-	case v1alpha1.IPXEBootConfigStateReady:
-		cond.Status = metav1.ConditionTrue
-		cond.Reason = "BootConfigReady"
-		cond.Message = "IPXE boot configuration is ready."
-	case v1alpha1.IPXEBootConfigStateError:
-		cond.Status = metav1.ConditionFalse
-		cond.Reason = "BootConfigError"
-		cond.Message = "IPXEBootConfig reported an error."
-	default:
-		cond.Status = metav1.ConditionUnknown
-		cond.Reason = "BootConfigPending"
-		cond.Message = "Waiting for IPXEBootConfig to become Ready."
-	}
+		cond := metav1.Condition{
+			Type:               IPXEBootReadyConditionType,
+			ObservedGeneration: cur.Generation,
+		}
+		switch config.Status.State {
+		case v1alpha1.IPXEBootConfigStateReady:
+			cond.Status = metav1.ConditionTrue
+			cond.Reason = "BootConfigReady"
+			cond.Message = "IPXE boot configuration is ready."
+		case v1alpha1.IPXEBootConfigStateError:
+			cond.Status = metav1.ConditionFalse
+			cond.Reason = "BootConfigError"
+			cond.Message = "IPXEBootConfig reported an error."
+		default:
+			cond.Status = metav1.ConditionUnknown
+			cond.Reason = "BootConfigPending"
+			cond.Message = "Waiting for IPXEBootConfig to become Ready."
+		}
 
-	apimeta.SetStatusCondition(&cur.Status.Conditions, cond)
-	return r.Status().Patch(ctx, &cur, client.MergeFrom(base))
+		apimeta.SetStatusCondition(&cur.Status.Conditions, cond)
+		return r.Status().Patch(ctx, &cur, client.MergeFrom(base))
+	})
 }
 
 func (r *ServerBootConfigurationPXEReconciler) getSystemUUIDFromBootConfig(ctx context.Context, config *metalv1alpha1.ServerBootConfiguration) (string, error) {

internal/controller/serverbootconfiguration_readiness_controller.go

@@ -9,6 +9,7 @@ import (
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -47,12 +48,42 @@ func (r *ServerBootConfigurationReadinessReconciler) Reconcile(ctx context.Conte
 		return ctrl.Result{}, nil
 	}
 
+	desired := computeDesiredState(cfg, r.RequireHTTPBoot, r.RequireIPXEBoot)
+
+	if cfg.Status.State == desired {
+		return ctrl.Result{}, nil
+	}
+
+	// Re-fetch immediately before patching so that we use the freshest resourceVersion and do not
+	// overwrite conditions that HTTP/PXE controllers may have written since our initial Get above.
+	if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		var fresh metalv1alpha1.ServerBootConfiguration
+		if err := r.Get(ctx, req.NamespacedName, &fresh); err != nil {
+			return err
+		}
+		// Recompute desired from the freshest conditions so we never apply a stale decision.
+		freshDesired := computeDesiredState(&fresh, r.RequireHTTPBoot, r.RequireIPXEBoot)
+		if fresh.Status.State == freshDesired {
+			return nil
+		}
+		base := fresh.DeepCopy()
+		fresh.Status.State = freshDesired
+		return r.Status().Patch(ctx, &fresh, client.MergeFrom(base))
+	}); err != nil {
+		return ctrl.Result{}, err
+	}
+
+	return ctrl.Result{}, nil
+}
+
+// computeDesiredState derives the ServerBootConfiguration state from the mode-specific conditions.
+func computeDesiredState(cfg *metalv1alpha1.ServerBootConfiguration, requireHTTP, requireIPXE bool) metalv1alpha1.ServerBootConfigurationState {
 	desired := metalv1alpha1.ServerBootConfigurationStatePending
 
 	allReady := true
 	hasError := false
 
-	if r.RequireHTTPBoot {
+	if requireHTTP {
 		c := apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType)
 		switch {
 		case c == nil:
@@ -64,7 +95,7 @@ func (r *ServerBootConfigurationReadinessReconciler) Reconcile(ctx context.Conte
 		}
 	}
 
-	if r.RequireIPXEBoot {
+	if requireIPXE {
 		c := apimeta.FindStatusCondition(cfg.Status.Conditions, IPXEBootReadyConditionType)
 		switch {
 		case c == nil:
@@ -83,17 +114,7 @@ func (r *ServerBootConfigurationReadinessReconciler) Reconcile(ctx context.Conte
 		desired = metalv1alpha1.ServerBootConfigurationStateReady
 	}
 
-	if cfg.Status.State == desired {
-		return ctrl.Result{}, nil
-	}
-
-	base := cfg.DeepCopy()
-	cfg.Status.State = desired
-	if err := r.Status().Patch(ctx, cfg, client.MergeFrom(base)); err != nil {
-		return ctrl.Result{}, err
-	}
-
-	return ctrl.Result{}, nil
+	return desired
 }
 
 func (r *ServerBootConfigurationReadinessReconciler) SetupWithManager(mgr ctrl.Manager) error {