fix: mark ur retry on conflict (#3961)

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>

cmd/kyverno/main.go

@@ -284,7 +284,7 @@ func main() {
 		stopCh,
 		log.Log.WithName("UpdateRequestGenerator"))
 
-	urc, err := background.NewController(
+	urc := background.NewController(
 		kubeClient,
 		kyvernoClient,
 		dynamicClient,
@@ -296,10 +296,6 @@ func main() {
 		log.Log.WithName("BackgroundController"),
 		configuration,
 	)
-	if err != nil {
-		setupLog.Error(err, "Failed to create generate controller")
-		os.Exit(1)
-	}
 
 	grcc, err := generatecleanup.NewController(
 		kubeClient,

pkg/background/request_process.go

@@ -15,7 +15,7 @@ import (
 	"k8s.io/client-go/util/retry"
 )
 
-func (c *Controller) ProcessUR(ur *kyvernov1beta1.UpdateRequest) error {
+func (c *Controller) processUR(ur *kyvernov1beta1.UpdateRequest) error {
 	switch ur.Spec.Type {
 	case kyvernov1beta1.Mutate:
 		ctrl, _ := mutate.NewMutateExistingController(c.kyvernoClient, c.client,
@@ -31,29 +31,22 @@ func (c *Controller) ProcessUR(ur *kyvernov1beta1.UpdateRequest) error {
 	return nil
 }
 
-func (c *Controller) MarkUR(ur *kyvernov1beta1.UpdateRequest) (*kyvernov1beta1.UpdateRequest, bool, error) {
+func (c *Controller) markUR(ur *kyvernov1beta1.UpdateRequest) (*kyvernov1beta1.UpdateRequest, bool, error) {
-	handler := ur.Status.Handler
+	ur = ur.DeepCopy()
-	if handler != "" {
+	if ur.Status.Handler != "" {
-		if handler != config.KyvernoPodName() {
+		return ur, ur.Status.Handler == config.KyvernoPodName(), nil
-			return ur, false, nil
 	}
-		return ur, true, nil
-	}
-	handler = config.KyvernoPodName()
-	ur.Status.Handler = handler
-	var updateRequest *kyvernov1beta1.UpdateRequest
-
 	err := retry.RetryOnConflict(common.DefaultRetry, func() error {
 		var retryError error
-		updateRequest, retryError = c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{})
+		ur.Status.Handler = config.KyvernoPodName()
+		ur, retryError = c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{})
 		return retryError
 	})
-
+	return ur, true, err
-	return updateRequest, true, err
 }
 
-func (c *Controller) UnmarkUR(ur *kyvernov1beta1.UpdateRequest) error {
+func (c *Controller) unmarkUR(ur *kyvernov1beta1.UpdateRequest) error {
-	if _, err := c.PatchHandler(ur, ""); err != nil {
+	if _, err := c.patchHandler(ur, ""); err != nil {
 		return err
 	}
 	if ur.Spec.Type == kyvernov1beta1.Mutate && ur.Status.State == kyvernov1beta1.Completed {
@@ -62,7 +55,7 @@ func (c *Controller) UnmarkUR(ur *kyvernov1beta1.UpdateRequest) error {
 	return nil
 }
 
-func (c *Controller) PatchHandler(ur *kyvernov1beta1.UpdateRequest, val string) (*kyvernov1beta1.UpdateRequest, error) {
+func (c *Controller) patchHandler(ur *kyvernov1beta1.UpdateRequest, val string) (*kyvernov1beta1.UpdateRequest, error) {
 	patch := jsonutils.NewPatch(
 		"/status/handler",
 		"replace",

pkg/background/update_request_controller.go

@@ -81,7 +81,7 @@ func NewController(
 	namespaceInformer corev1informers.NamespaceInformer,
 	log logr.Logger,
 	dynamicConfig config.Configuration,
-) (*Controller, error) {
+) *Controller {
 	c := Controller{
 		client:         client,
 		kyvernoClient:  kyvernoClient,
@@ -90,21 +90,18 @@ func NewController(
 		queue:          workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "generate-request"),
 		log:            log,
 		Config:         dynamicConfig,
+		statusControl:  common.StatusControl{Client: kyvernoClient},
+		policyLister:   policyInformer.Lister(),
+		npolicyLister:  npolicyInformer.Lister(),
+		urLister:       urInformer.Lister().UpdateRequests(config.KyvernoNamespace()),
+		nsLister:       namespaceInformer.Lister(),
 	}
-
-	c.statusControl = common.StatusControl{Client: kyvernoClient}
 	urInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc:    c.addUR,
 		UpdateFunc: c.updateUR,
 		DeleteFunc: c.deleteUR,
 	})
-
+	return &c
-	c.policyLister = policyInformer.Lister()
-	c.npolicyLister = npolicyInformer.Lister()
-	c.urLister = urInformer.Lister().UpdateRequests(config.KyvernoNamespace())
-	c.nsLister = namespaceInformer.Lister()
-
-	return &c, nil
 }
 
 // Run starts workers
@@ -178,39 +175,32 @@ func (c *Controller) syncUpdateRequest(key string) error {
 	defer func() {
 		logger.V(4).Info("completed sync update request", "key", key, "processingTime", time.Since(startTime).String())
 	}()
-
 	_, urName, err := cache.SplitMetaNamespaceKey(key)
 	if err != nil {
 		return err
 	}
-
 	ur, err := c.urLister.Get(urName)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			return nil
 		}
-
 		return fmt.Errorf("failed to fetch update request %s: %v", key, err)
 	}
-
+	ur, ok, err := c.markUR(ur)
-	ur, ok, err := c.MarkUR(ur)
+	if err != nil {
+		return fmt.Errorf("failed to mark handler for UR %s: %v", key, err)
+	}
 	if !ok {
 		logger.V(3).Info("another instance is handling the UR", "handler", ur.Status.Handler)
 		return nil
 	}
-	if err != nil {
-		return fmt.Errorf("failed to mark handler for UR %s: %v", key, err)
-	}
-
 	logger.V(3).Info("UR is marked successfully", "ur", ur.GetName(), "resourceVersion", ur.GetResourceVersion())
-	if err := c.ProcessUR(ur); err != nil {
+	if err := c.processUR(ur); err != nil {
 		return fmt.Errorf("failed to process UR %s: %v", key, err)
 	}
-
+	if err = c.unmarkUR(ur); err != nil {
-	if err = c.UnmarkUR(ur); err != nil {
 		return fmt.Errorf("failed to unmark UR %s: %v", key, err)
 	}
-
 	return nil
 }