APB: Replace logic to update status to check first if there's a chang…

…e in status to avoid hitting the KAPI server for any pod status change

Signed-off-by: Jordi Gil <jgil@redhat.com>

go-controller/pkg/ovn/controller/apbroute/master_controller.go

@@ -1,16 +1,19 @@
 package apbroute
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"net"
 	"reflect"
+	"strings"
 	"sync"
 	"time"
 
 	nettypes "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 	libovsdbclient "github.com/ovn-org/libovsdb/client"
 	v1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -19,6 +22,7 @@ import (
 	"k8s.io/client-go/kubernetes"
 	corev1listers "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
+	"k8s.io/client-go/util/retry"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
 
@@ -29,6 +33,7 @@ import (
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/factory"
 	libovsdbutil "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdb/util"
 	addressset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/address_set"
+	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 )
 
 const (
@@ -494,32 +499,43 @@ func (c *ExternalGatewayMasterController) processNextPodWorkItem(wg *sync.WaitGr
 // updateStatusAPBExternalRoute updates the CR with the current status of the CR instance, including errors captured while processing the CR during its lifetime
 func (c *ExternalGatewayMasterController) updateStatusAPBExternalRoute(policyName string, gwIPs sets.Set[string],
 	processedError error) error {
-	// if gwIPs == nil {
-	// 	// policy doesn't exist anymore, nothing to do
-	// 	return nil
-	// }
-
-	// resultErr := retry.RetryOnConflict(util.OvnConflictBackoff, func() error {
-	// 	routePolicy, err := c.apbRoutePolicyClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.TODO(), policyName, metav1.GetOptions{})
-	// 	if err != nil {
-	// 		if apierrors.IsNotFound(err) {
-	// 			// policy doesn't exist, no need to update status
-	// 			return nil
-	// 		}
-	// 		return err
-	// 	}
-
-	// 	updateStatus(routePolicy, strings.Join(sets.List(gwIPs), ","), processedError)
-
-	// 	_, err = c.apbRoutePolicyClient.K8sV1().AdminPolicyBasedExternalRoutes().UpdateStatus(context.TODO(), routePolicy, metav1.UpdateOptions{})
-	// 	if !apierrors.IsNotFound(err) {
-	// 		return err
-	// 	}
-	// 	return nil
-	// })
-	// if resultErr != nil {
-	// 	return fmt.Errorf("failed to update AdminPolicyBasedExternalRoutes %s status: %v", policyName, resultErr)
-	// }
+	if gwIPs == nil {
+		// policy doesn't exist anymore, nothing to do
+		return nil
+	}
+	var lastMessage string
+	var err error
+	status, err := c.GetAPBRoutePolicyStatus(policyName)
+	if err != nil && !apierrors.IsNotFound(err) {
+		klog.Warningf("AdminPolicyBasedExternalRoute %s has been deleted before being able to update its status", policyName)
+		return nil
+	}
+	if !status.LastTransitionTime.Time.IsZero() {
+		lastMessage = status.Messages[len(status.Messages)-1]
+	}
+	expectedMessage := fmt.Sprintf("Configured external gateway IPs: %s", strings.Join(sets.List(gwIPs), ","))
+	if processedError != nil || len(lastMessage) == 0 || (len(lastMessage) > 0 && lastMessage != expectedMessage) {
+		resultErr := retry.RetryOnConflict(util.OvnConflictBackoff, func() error {
+			routePolicy, err := c.apbRoutePolicyClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.TODO(), policyName, metav1.GetOptions{})
+			if err != nil {
+				if apierrors.IsNotFound(err) {
+					// policy doesn't exist, no need to update status
+					return nil
+				}
+				return err
+			}
+			updateStatus(routePolicy, expectedMessage, processedError)
+
+			_, err = c.apbRoutePolicyClient.K8sV1().AdminPolicyBasedExternalRoutes().UpdateStatus(context.TODO(), routePolicy, metav1.UpdateOptions{})
+			if !apierrors.IsNotFound(err) {
+				return err
+			}
+			return nil
+		})
+		if resultErr != nil {
+			return fmt.Errorf("failed to update AdminPolicyBasedExternalRoutes %s status: %v", policyName, resultErr)
+		}
+	}
 	return nil
 }
 
@@ -531,15 +547,15 @@ func (c *ExternalGatewayMasterController) GetStaticGatewayIPsForTargetNamespace(
 	return c.mgr.getStaticGatewayIPsForTargetNamespace(namespaceName)
 }
 
-func updateStatus(route *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, gwIPs string, err error) {
+func updateStatus(route *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, message string, err error) {
 	route.Status.LastTransitionTime = metav1.Time{Time: time.Now()}
 	if err != nil {
 		route.Status.Status = adminpolicybasedrouteapi.FailStatus
 		route.Status.Messages = append(route.Status.Messages, fmt.Sprintf("Failed to apply policy: %v", err.Error()))
 		return
 	}
 	route.Status.Status = adminpolicybasedrouteapi.SuccessStatus
-	route.Status.Messages = append(route.Status.Messages, fmt.Sprintf("Configured external gateway IPs: %s", gwIPs))
+	route.Status.Messages = append(route.Status.Messages, message)
 	klog.V(4).Infof("Updating Admin Policy Based External Route %s with Status: %s, Message: %s", route.Name, route.Status.Status, route.Status.Messages[len(route.Status.Messages)-1])
 }
 
@@ -568,10 +584,18 @@ func (c *ExternalGatewayMasterController) DeletePodSNAT(nodeName string, extIPs,
 	return c.nbClient.deletePodSNAT(nodeName, extIPs, podIPNets)
 }
 
+// GetAPBRoutePolicyStatus retrieves the CR status field from the current copy in the informer within a lock to prevent
+// concurrency issues. It returns an error when the informer call fails with an error different that object not found.
 func (c *ExternalGatewayMasterController) GetAPBRoutePolicyStatus(policyName string) (*adminpolicybasedrouteapi.AdminPolicyBasedRouteStatus, error) {
-	pol, err := c.routeLister.Get(policyName)
-	if err != nil {
-		return nil, err
-	}
-	return &pol.Status, nil
+	var policyStatus adminpolicybasedrouteapi.AdminPolicyBasedRouteStatus
+
+	err := c.mgr.routePolicySyncCache.DoWithLock(policyName, func(policyName string) error {
+		policyObj, err := c.mgr.routeLister.Get(policyName)
+		if err != nil && !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to retrieve AdminPolicyBasedExternalRoute before status update: %w", err)
+		}
+		policyStatus = policyObj.Status
+		return nil
+	})
+	return &policyStatus, err
 }