From 3c5ebbadd3c536d87122a11eb3dc31ebce928c69 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Mon, 24 Mar 2025 16:18:38 +0800 Subject: [PATCH] issue 8803: use deterministic name to create backupRepository Signed-off-by: Lyndon-Li --- changelogs/unreleased/8808-Lyndon-Li | 1 + pkg/repository/backup_repo_op.go | 6 ++-- pkg/repository/ensurer.go | 24 ++++++------- pkg/repository/ensurer_test.go | 54 ++++++++++++++++++++++++++-- 4 files changed, 67 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/8808-Lyndon-Li diff --git a/changelogs/unreleased/8808-Lyndon-Li b/changelogs/unreleased/8808-Lyndon-Li new file mode 100644 index 0000000000..1b242d960c --- /dev/null +++ b/changelogs/unreleased/8808-Lyndon-Li @@ -0,0 +1 @@ +Fix issue #8803, use deterministic name to create backupRepository \ No newline at end of file diff --git a/pkg/repository/backup_repo_op.go b/pkg/repository/backup_repo_op.go index ff253e6038..36356e7aff 100644 --- a/pkg/repository/backup_repo_op.go +++ b/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, diff --git a/pkg/repository/ensurer.go b/pkg/repository/ensurer.go index db1e596479..91cdb0d9e3 100644 --- a/pkg/repository/ensurer.go +++ b/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") } diff --git a/pkg/repository/ensurer_test.go b/pkg/repository/ensurer_test.go index d480ba1a47..de5892bb7c 100644 --- a/pkg/repository/ensurer_test.go +++ b/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 {