(C)UDN Controller: use rate limiting instead of delayed addition, do not update conditions unnecessarily

Replace custom condition update logic with standard k8s meta.SetStatusCondition
to avoid updating condtions even if they haven't changed.
Use rate limiting instead of fixed interval requeuing for network-in-use errors.

Signed-off-by: Patryk Diak <[email protected]>

Author: kyrtapz

go-controller/pkg/clustermanager/userdefinednetwork/controller.go

@@ -15,6 +15,7 @@ import (
 	netv1lister "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/listers/k8s.cni.cncf.io/v1"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -87,8 +88,6 @@ type Controller struct {
 	eventRecorder               record.EventRecorder
 }
 
-const defaultNetworkInUseCheckInterval = 1 * time.Minute
-
 func New(
 	nadClient netv1clientset.Interface,
 	nadInfomer netv1infomer.NetworkAttachmentDefinitionInformer,
@@ -104,18 +103,17 @@ func New(
 	udnLister := udnInformer.Lister()
 	cudnLister := cudnInformer.Lister()
 	c := &Controller{
-		nadClient:                   nadClient,
-		nadLister:                   nadInfomer.Lister(),
-		udnClient:                   udnClient,
-		udnLister:                   udnLister,
-		cudnLister:                  cudnLister,
-		renderNadFn:                 renderNadFn,
-		podInformer:                 podInformer,
-		namespaceInformer:           namespaceInformer,
-		networkManager:              networkManager,
-		networkInUseRequeueInterval: defaultNetworkInUseCheckInterval,
-		namespaceTracker:            map[string]sets.Set[string]{},
-		eventRecorder:               eventRecorder,
+		nadClient:         nadClient,
+		nadLister:         nadInfomer.Lister(),
+		udnClient:         udnClient,
+		udnLister:         udnLister,
+		cudnLister:        cudnLister,
+		renderNadFn:       renderNadFn,
+		podInformer:       podInformer,
+		namespaceInformer: namespaceInformer,
+		networkManager:    networkManager,
+		namespaceTracker:  map[string]sets.Set[string]{},
+		eventRecorder:     eventRecorder,
 	}
 	udnCfg := &controller.ControllerConfig[userdefinednetworkv1.UserDefinedNetwork]{
 		RateLimiter:    workqueue.DefaultTypedControllerRateLimiter[string](),
@@ -389,7 +387,8 @@ func (c *Controller) reconcileUDN(key string) error {
 
 	var networkInUse *networkInUseError
 	if errors.As(syncErr, &networkInUse) {
-		c.udnController.ReconcileAfter(key, c.networkInUseRequeueInterval)
+		// Call ReconcileRateLimited directly to ensure retries without the default limits
+		c.udnController.ReconcileRateLimited(key)
 		return updateStatusErr
 	}
 
@@ -452,34 +451,34 @@ func (c *Controller) updateUserDefinedNetworkStatus(udn *userdefinednetworkv1.Us
 
 	networkCreatedCondition := newNetworkCreatedCondition(nad, syncError)
 
-	conditions, updated := updateCondition(udn.Status.Conditions, networkCreatedCondition)
+	updated := meta.SetStatusCondition(&udn.Status.Conditions, *networkCreatedCondition)
+	if !updated {
+		return nil
+	}
 
-	if updated {
-		var err error
-		conditionsApply := make([]*metaapplyv1.ConditionApplyConfiguration, len(conditions))
-		for i := range conditions {
-			conditionsApply[i] = &metaapplyv1.ConditionApplyConfiguration{
-				Type:               &conditions[i].Type,
-				Status:             &conditions[i].Status,
-				LastTransitionTime: &conditions[i].LastTransitionTime,
-				Reason:             &conditions[i].Reason,
-				Message:            &conditions[i].Message,
-			}
+	var err error
+	conditionsApply := make([]*metaapplyv1.ConditionApplyConfiguration, len(udn.Status.Conditions))
+	for i, condition := range udn.Status.Conditions {
+		conditionsApply[i] = &metaapplyv1.ConditionApplyConfiguration{
+			Type:               &condition.Type,
+			Status:             &condition.Status,
+			LastTransitionTime: &condition.LastTransitionTime,
+			Reason:             &condition.Reason,
+			Message:            &condition.Message,
 		}
-		udnApplyConf := udnapplyconfkv1.UserDefinedNetwork(udn.Name, udn.Namespace).
-			WithStatus(udnapplyconfkv1.UserDefinedNetworkStatus().
-				WithConditions(conditionsApply...))
-		opts := metav1.ApplyOptions{FieldManager: "user-defined-network-controller"}
-		udn, err = c.udnClient.K8sV1().UserDefinedNetworks(udn.Namespace).ApplyStatus(context.Background(), udnApplyConf, opts)
-		if err != nil {
-			if apierrors.IsNotFound(err) {
-				return nil
-			}
-			return fmt.Errorf("failed to update UserDefinedNetwork status: %w", err)
+	}
+	udnApplyConf := udnapplyconfkv1.UserDefinedNetwork(udn.Name, udn.Namespace).
+		WithStatus(udnapplyconfkv1.UserDefinedNetworkStatus().
+			WithConditions(conditionsApply...))
+	opts := metav1.ApplyOptions{FieldManager: "user-defined-network-controller"}
+	udn, err = c.udnClient.K8sV1().UserDefinedNetworks(udn.Namespace).ApplyStatus(context.Background(), udnApplyConf, opts)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
 		}
-		klog.Infof("Updated status UserDefinedNetwork [%s/%s]", udn.Namespace, udn.Name)
+		return fmt.Errorf("failed to update UserDefinedNetwork status: %w", err)
 	}
-
+	klog.Infof("Updated status UserDefinedNetwork [%s/%s]", udn.Namespace, udn.Name)
 	return nil
 }
 
@@ -507,21 +506,6 @@ func newNetworkCreatedCondition(nad *netv1.NetworkAttachmentDefinition, syncErro
 	return networkCreatedCondition
 }
 
-func updateCondition(conditions []metav1.Condition, cond *metav1.Condition) ([]metav1.Condition, bool) {
-	if len(conditions) == 0 {
-		return append(conditions, *cond), true
-	}
-
-	idx := slices.IndexFunc(conditions, func(c metav1.Condition) bool {
-		return (c.Type == cond.Type) &&
-			(c.Status != cond.Status || c.Reason != cond.Reason || c.Message != cond.Message)
-	})
-	if idx != -1 {
-		return slices.Replace(conditions, idx, idx+1, *cond), true
-	}
-	return conditions, false
-}
-
 func (c *Controller) cudnNeedUpdate(_ *userdefinednetworkv1.ClusterUserDefinedNetwork, _ *userdefinednetworkv1.ClusterUserDefinedNetwork) bool {
 	return true
 }
@@ -549,7 +533,8 @@ func (c *Controller) reconcileCUDN(key string) error {
 
 	var networkInUse *networkInUseError
 	if errors.As(syncErr, &networkInUse) {
-		c.cudnController.ReconcileAfter(key, c.networkInUseRequeueInterval)
+		// Call ReconcileRateLimited directly to ensure retries without the default limits
+		c.cudnController.ReconcileRateLimited(key)
 		return updateStatusErr
 	}
 
@@ -688,18 +673,18 @@ func (c *Controller) updateClusterUDNStatus(cudn *userdefinednetworkv1.ClusterUs
 
 	networkCreatedCondition := newClusterNetworCreatedCondition(nads, syncError)
 
-	conditions, updated := updateCondition(cudn.Status.Conditions, networkCreatedCondition)
+	updated := meta.SetStatusCondition(&cudn.Status.Conditions, networkCreatedCondition)
 	if !updated {
 		return nil
 	}
-	conditionsApply := make([]*metaapplyv1.ConditionApplyConfiguration, len(conditions))
-	for i := range conditions {
+	conditionsApply := make([]*metaapplyv1.ConditionApplyConfiguration, len(cudn.Status.Conditions))
+	for i, condition := range cudn.Status.Conditions {
 		conditionsApply[i] = &metaapplyv1.ConditionApplyConfiguration{
-			Type:               &conditions[i].Type,
-			Status:             &conditions[i].Status,
-			LastTransitionTime: &conditions[i].LastTransitionTime,
-			Reason:             &conditions[i].Reason,
-			Message:            &conditions[i].Message,
+			Type:               &condition.Type,
+			Status:             &condition.Status,
+			LastTransitionTime: &condition.LastTransitionTime,
+			Reason:             &condition.Reason,
+			Message:            &condition.Message,
 		}
 	}
 	var err error
@@ -720,15 +705,15 @@ func (c *Controller) updateClusterUDNStatus(cudn *userdefinednetworkv1.ClusterUs
 	return nil
 }
 
-func newClusterNetworCreatedCondition(nads []netv1.NetworkAttachmentDefinition, syncError error) *metav1.Condition {
+func newClusterNetworCreatedCondition(nads []netv1.NetworkAttachmentDefinition, syncError error) metav1.Condition {
 	var namespaces []string
 	for _, nad := range nads {
 		namespaces = append(namespaces, nad.Namespace)
 	}
 	affectedNamespaces := strings.Join(namespaces, ", ")
 
 	now := metav1.Now()
-	condition := &metav1.Condition{
+	condition := metav1.Condition{
 		Type:               conditionTypeNetworkCreated,
 		Status:             metav1.ConditionTrue,
 		Reason:             "NetworkAttachmentDefinitionCreated",

go-controller/pkg/clustermanager/userdefinednetwork/nad.go

@@ -3,6 +3,7 @@ package userdefinednetwork
 import (
 	"encoding/json"
 	"fmt"
+	"slices"
 
 	netv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 
@@ -31,6 +32,8 @@ func NetAttachDefNotInUse(nad *netv1.NetworkAttachmentDefinition, pods []*corev1
 		}
 	}
 	if len(connectedPods) > 0 {
+		// Sort the connected pods to ensure a consistent error
+		slices.Sort(connectedPods)
 		return fmt.Errorf("network in use by the following pods: %v", connectedPods)
 	}
 	return nil

go-controller/pkg/controller/controller.go

@@ -27,6 +27,7 @@ const (
 // to reconcile through its Reconcile method
 type Reconciler interface {
 	Reconcile(key string)
+	ReconcileRateLimited(key string)
 	ReconcileAfter(key string, duration time.Duration)
 	addHandler() error
 	startWorkers() error
@@ -272,6 +273,10 @@ func (c *controller[T]) Reconcile(key string) {
 	c.queue.Add(key)
 }
 
+func (c *controller[T]) ReconcileRateLimited(key string) {
+	c.queue.AddRateLimited(key)
+}
+
 func (c *controller[T]) ReconcileAfter(key string, duration time.Duration) {
 	c.queue.AddAfter(key, duration)
 }