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,44 +138,52 @@ func (u *CRDUtils) CreateAgentCR(ctx context.Context, log logrus.FieldLogger, ho
 			}
 		}
 
-		updatedhost := &aiv1beta1.Agent{
-			Spec: aiv1beta1.AgentSpec{
-				Approved:                false,
-				Hostname:                "",
-				MachineConfigPool:       "",
-				Role:                    "",
-				InstallationDiskID:      "",
-				InstallerArgs:           "",
-				IgnitionConfigOverrides: "",
-			},
-			ObjectMeta: metav1.ObjectMeta{
-				Name:            hostId,
-				Namespace:       infraEnvCR.Namespace,
-				Labels:          labels,
-				ResourceVersion: host.ResourceVersion,
-				OwnerReferences: []metav1.OwnerReference{
-					{
-						APIVersion: infraEnvCR.APIVersion,
-						Kind:       infraEnvCR.Kind,
-						Name:       infraEnvCR.Name,
-						UID:        infraEnvCR.UID,
+		if err := 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,
+					Hostname:                "",
+					MachineConfigPool:       "",
+					Role:                    "",
+					InstallationDiskID:      "",
+					InstallerArgs:           "",
+					IgnitionConfigOverrides: "",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            hostId,
+					Namespace:       infraEnvCR.Namespace,
+					Labels:          labels,
+					ResourceVersion: current.ResourceVersion,
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: infraEnvCR.APIVersion,
+							Kind:       infraEnvCR.Kind,
+							Name:       infraEnvCR.Name,
+							UID:        infraEnvCR.UID,
+						},
 					},
 				},
-			},
+			}
+			if cluster != nil && cluster.KubeKeyNamespace != "" {
+				updatedhost.Spec.ClusterDeploymentName = &aiv1beta1.ClusterReference{
+					Name:      cluster.KubeKeyName,
+					Namespace: cluster.KubeKeyNamespace,
+				}
+			}
+			return u.client.Update(ctx, updatedhost)
+		}); err != nil {
+			return err
 		}
-
 		// Update 'kube_key_namespace' in Host
-		if err1 := u.hostApi.UpdateKubeKeyNS(ctx, hostId, infraEnvCR.Namespace); err1 != nil {
+		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)
 		}
-		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)
+		log.Infof("Updated Agent CR. Namespace: %s, Cluster: %s, HostID: %s", infraEnvCR.Namespace, clusterName, hostId)
 	}
 
 	return nil

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())