Issue 8803: use deterministic name to create backupRepository

changelogs/unreleased/8808-Lyndon-Li

@@ -0,0 +1 @@
+Fix issue #8803, use deterministic name to create backupRepository

pkg/repository/backup_repo_op.go

@@ -94,9 +94,9 @@ func GetBackupRepository(ctx context.Context, cli client.Client, namespace strin
 func NewBackupRepository(namespace string, key BackupRepositoryKey) *velerov1api.BackupRepository {
 	return &velerov1api.BackupRepository{
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace:    namespace,
-			GenerateName: fmt.Sprintf("%s-%s-%s-", key.VolumeNamespace, key.BackupLocation, key.RepositoryType),
-			Labels:       repoLabelsFromKey(key),
+			Namespace: namespace,
+			Name:      fmt.Sprintf("%s-%s-%s", key.VolumeNamespace, key.BackupLocation, key.RepositoryType),
+			Labels:    repoLabelsFromKey(key),
 		},
 		Spec: velerov1api.BackupRepositorySpec{
 			VolumeNamespace:       key.VolumeNamespace,

pkg/repository/ensurer.go

@@ -28,7 +28,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
-	veleroclient "github.com/vmware-tanzu/velero/pkg/client"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 )
 
 // Ensurer ensures that backup repositories are created and ready.
@@ -61,17 +62,15 @@ func (r *Ensurer) EnsureRepo(ctx context.Context, namespace, volumeNamespace, ba
 
 	log := r.log.WithField("volumeNamespace", volumeNamespace).WithField("backupLocation", backupLocation).WithField("repositoryType", repositoryType)
 
-	// It's only safe to have one instance of this method executing concurrently for a
-	// given BackupRepositoryKey, so synchronize based on that. It's fine
-	// to run concurrently for *different* BackupRepositoryKey. If you had 2 goroutines
-	// running this for the same inputs, both might find no BackupRepository exists, then
-	// both would create new ones for the same BackupRepositoryKey.
+	// The BackupRepository is labeled with BackupRepositoryKey.
+	// This function searches for an existing BackupRepository by BackupRepositoryKey label.
+	// If it doesn't exist, it creates a new one and wait for its readiness.
 	//
-	// This issue could probably be avoided if we had a deterministic name for
-	// each BackupRepository and we just tried to create it, checked for an
-	// AlreadyExists err, and then waited for it to be ready. However, there are
-	// already repositories in the wild with non-deterministic names (i.e. using
-	// GenerateName) which poses a backwards compatibility problem.
+	// The BackupRepository is also named as BackupRepositoryKey.
+	// It creates a BackupRepository with deterministic name
+	// so that this function could support multiple thread calling by leveraging API server's optimistic lock mechanism.
+	// Therefore, the name must be unique for a BackupRepository.
+	// Don't use name to filter/search BackupRepository, since it may be changed in future, use label instead.
 	log.Debug("Acquiring lock")
 
 	repoMu := r.repoLock(backupRepoKey)
@@ -108,7 +107,8 @@ func (r *Ensurer) repoLock(key BackupRepositoryKey) *sync.Mutex {
 
 func (r *Ensurer) createBackupRepositoryAndWait(ctx context.Context, namespace string, backupRepoKey BackupRepositoryKey) (*velerov1api.BackupRepository, error) {
 	toCreate := NewBackupRepository(namespace, backupRepoKey)
-	if err := veleroclient.CreateRetryGenerateName(r.repoClient, ctx, toCreate); err != nil {
+
+	if err := r.repoClient.Create(ctx, toCreate, &client.CreateOptions{}); err != nil && !apierrors.IsAlreadyExists(err) {
 		return nil, errors.Wrap(err, "unable to create backup repository resource")
 	}
 

pkg/repository/ensurer_test.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -138,13 +139,38 @@ func TestEnsureRepo(t *testing.T) {
 }
 
 func TestCreateBackupRepositoryAndWait(t *testing.T) {
-	bkRepoObj := NewBackupRepository(velerov1.DefaultNamespace, BackupRepositoryKey{
+	existingRepoReady := NewBackupRepository(velerov1.DefaultNamespace, BackupRepositoryKey{
 		VolumeNamespace: "fake-ns",
 		BackupLocation:  "fake-bsl",
 		RepositoryType:  "fake-repo-type",
 	})
 
-	bkRepoObj.Status.Phase = velerov1.BackupRepositoryPhaseReady
+	existingRepoReady.Status.Phase = velerov1.BackupRepositoryPhaseReady
+
+	existingRepoNotReady := NewBackupRepository(velerov1.DefaultNamespace, BackupRepositoryKey{
+		VolumeNamespace: "fake-ns",
+		BackupLocation:  "fake-bsl",
+		RepositoryType:  "fake-repo-type",
+	})
+
+	key := BackupRepositoryKey{
+		VolumeNamespace: "fake-ns",
+		BackupLocation:  "fake-bsl",
+		RepositoryType:  "fake-repo-type",
+	}
+
+	existingRepoWithUnexpectedName := &velerov1.BackupRepository{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: velerov1.DefaultNamespace,
+			Name:      "ake-ns-fake-bsl-fake-repo-type-xxx00",
+			Labels:    repoLabelsFromKey(key),
+		},
+		Spec: velerov1.BackupRepositorySpec{
+			VolumeNamespace:       key.VolumeNamespace,
+			BackupStorageLocation: key.BackupLocation,
+			RepositoryType:        key.RepositoryType,
+		},
+	}
 
 	scheme := runtime.NewScheme()
 	velerov1.AddToScheme(scheme)
@@ -172,7 +198,7 @@ func TestCreateBackupRepositoryAndWait(t *testing.T) {
 			bsl:            "fake-bsl",
 			repositoryType: "fake-repo-type",
 			kubeClientObj: []runtime.Object{
-				bkRepoObj,
+				existingRepoWithUnexpectedName,
 			},
 			runtimeScheme: scheme,
 			err:           "failed to wait BackupRepository, errored early: more than one BackupRepository found for workload namespace \"fake-ns\", backup storage location \"fake-bsl\", repository type \"fake-repo-type\"",
@@ -185,6 +211,28 @@ func TestCreateBackupRepositoryAndWait(t *testing.T) {
 			runtimeScheme:  scheme,
 			err:            "failed to wait BackupRepository, timeout exceeded: backup repository not provisioned",
 		},
+		{
+			name:           "repo already exists and ready",
+			namespace:      "fake-ns",
+			bsl:            "fake-bsl",
+			repositoryType: "fake-repo-type",
+			kubeClientObj: []runtime.Object{
+				existingRepoReady,
+			},
+			runtimeScheme: scheme,
+			expectedRepo:  existingRepoReady,
+		},
+		{
+			name:           "repo already exists but not ready",
+			namespace:      "fake-ns",
+			bsl:            "fake-bsl",
+			repositoryType: "fake-repo-type",
+			kubeClientObj: []runtime.Object{
+				existingRepoNotReady,
+			},
+			runtimeScheme: scheme,
+			err:           "failed to wait BackupRepository, timeout exceeded: backup repository not provisioned",
+		},
 	}
 
 	for _, test := range tests {