MGMT-23738: Add RetryOnConflict to CreateAgentCR to fix flaky agent move between infraenvs

Author: shay23bra

internal/controller/controllers/crd_utils.go

@@ -13,6 +13,7 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -137,6 +138,26 @@ func (u *CRDUtils) CreateAgentCR(ctx context.Context, log logrus.FieldLogger, ho
 			}
 		}
 
+		if err := u.updateAgentCR(ctx, namespacedName, labels, infraEnvCR, cluster); err != nil {
+			return err
+		}
+		// Update 'kube_key_namespace' in Host
+		if err := u.hostApi.UpdateKubeKeyNS(ctx, hostId, infraEnvCR.Namespace); err != nil {
+			return errors.Wrapf(err, "Failed to update 'kube_key_namespace' in host %s", hostId)
+		}
+		log.Infof("Updated Agent CR. Namespace: %s, Cluster: %s, HostID: %s", infraEnvCR.Namespace, clusterName, hostId)
+	}
+
+	return nil
+}
+
+func (u *CRDUtils) updateAgentCR(ctx context.Context, namespacedName types.NamespacedName, labels map[string]string, infraEnvCR *aiv1beta1.InfraEnv, cluster *common.Cluster) error {
+	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		current := &aiv1beta1.Agent{}
+		if err := u.client.Get(ctx, namespacedName, current); err != nil {
+			return err
+		}
+
 		updatedhost := &aiv1beta1.Agent{
 			Spec: aiv1beta1.AgentSpec{
 				Approved:                false,
@@ -148,10 +169,10 @@ func (u *CRDUtils) CreateAgentCR(ctx context.Context, log logrus.FieldLogger, ho
 				IgnitionConfigOverrides: "",
 			},
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            hostId,
-				Namespace:       infraEnvCR.Namespace,
+				Name:            namespacedName.Name,
+				Namespace:       namespacedName.Namespace,
 				Labels:          labels,
-				ResourceVersion: host.ResourceVersion,
+				ResourceVersion: current.ResourceVersion,
 				OwnerReferences: []metav1.OwnerReference{
 					{
 						APIVersion: infraEnvCR.APIVersion,
@@ -162,22 +183,14 @@ func (u *CRDUtils) CreateAgentCR(ctx context.Context, log logrus.FieldLogger, ho
 				},
 			},
 		}
-
-		// Update 'kube_key_namespace' in Host
-		if err1 := u.hostApi.UpdateKubeKeyNS(ctx, hostId, infraEnvCR.Namespace); err1 != nil {
-			return errors.Wrapf(err, "Failed to update 'kube_key_namespace' in host %s", hostId)
-		}
 		if cluster != nil && cluster.KubeKeyNamespace != "" {
 			updatedhost.Spec.ClusterDeploymentName = &aiv1beta1.ClusterReference{
 				Name:      cluster.KubeKeyName,
 				Namespace: cluster.KubeKeyNamespace,
 			}
 		}
-		log.Infof("Updating Agent CR. Namespace: %s, Cluster: %s, HostID: %s", infraEnvCR.Namespace, clusterName, hostId)
 		return u.client.Update(ctx, updatedhost)
-	}
-
-	return nil
+	})
 }
 
 type DummyCRDUtils struct{}

internal/controller/controllers/crd_utils_test.go

@@ -2,6 +2,7 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 
 	strfmt "github.com/go-openapi/strfmt"
 	"github.com/google/uuid"
@@ -13,10 +14,13 @@ import (
 	"github.com/openshift/assisted-service/models"
 	hivev1 "github.com/openshift/hive/apis/hive/v1"
 	"go.uber.org/mock/gomock"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 )
 
 var _ = Describe("create agent CR", func() {
@@ -176,6 +180,68 @@ var _ = Describe("create agent CR", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		It("Already existing agent update retries on conflict", func() {
+			clusterDeployment := newClusterDeployment(clusterName, testNamespace, defaultClusterSpec)
+			Expect(c.Create(ctx, clusterDeployment)).ShouldNot(HaveOccurred())
+			infraEnvImage := newInfraEnvImage(infraEnvName, infraEnvNamespace, v1beta1.InfraEnvSpec{
+				ClusterRef: &v1beta1.ClusterReference{
+					Name:      clusterDeployment.Name,
+					Namespace: clusterDeployment.Namespace,
+				},
+			})
+			Expect(c.Create(ctx, infraEnvImage)).ShouldNot(HaveOccurred())
+
+			hostId := uuid.New().String()
+			agent := newAgent(hostId, infraEnvNamespace, v1beta1.AgentSpec{})
+			Expect(c.Create(ctx, agent)).ShouldNot(HaveOccurred())
+
+			id := strfmt.UUID(hostId)
+			otherClusterId := strfmt.UUID(uuid.New().String())
+			infraEnvId2 := strfmt.UUID(uuid.New().String())
+			h := common.Host{
+				Host: models.Host{
+					ID:         &id,
+					ClusterID:  &otherClusterId,
+					InfraEnvID: infraEnvId,
+				},
+			}
+			infraEnv2 := &common.InfraEnv{
+				KubeKeyNamespace: infraEnvNamespace,
+				InfraEnv: models.InfraEnv{
+					ID:   &infraEnvId2,
+					Name: &infraEnvName,
+				},
+			}
+
+			mockHostApi.EXPECT().GetHostByKubeKey(gomock.Any()).Return(&h, nil).Times(1)
+			mockHostApi.EXPECT().UnRegisterHost(ctx, gomock.Any()).Return(nil).Times(1)
+			mockHostApi.EXPECT().UpdateKubeKeyNS(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1)
+
+			updateCount := 0
+			conflictClient := fakeclient.NewClientBuilder().
+				WithScheme(scheme.Scheme).
+				WithObjects(clusterDeployment, infraEnvImage, agent).
+				WithInterceptorFuncs(interceptor.Funcs{
+					Update: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
+						updateCount++
+						if updateCount == 1 {
+							return k8serrors.NewConflict(
+								schema.GroupResource{Group: "agent-install.openshift.io", Resource: "agents"},
+								obj.GetName(),
+								fmt.Errorf("the object has been modified"),
+							)
+						}
+						return cl.Update(ctx, obj, opts...)
+					},
+				}).
+				Build()
+
+			conflictCrdUtils := NewCRDUtils(conflictClient, mockHostApi)
+			err := conflictCrdUtils.CreateAgentCR(ctx, log, hostId, infraEnv2, cluster)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(updateCount).To(Equal(2))
+		})
+
 		It("Already existing agent different infraenv same namespace", func() {
 			clusterDeployment := newClusterDeployment(clusterName, testNamespace, defaultClusterSpec)
 			Expect(c.Create(ctx, clusterDeployment)).ShouldNot(HaveOccurred())