From 0536aa2d529d7e54050ac40ab23c29bc9a2aa4dc Mon Sep 17 00:00:00 2001 From: Mariell Hoversholm Date: Tue, 25 Mar 2025 10:41:38 +0100 Subject: [PATCH] Provisioning: Gracefully handle AlreadyExists errors (#102758) * Provisioning: Gracefully handle AlreadyExists errors When this occurs, the job is already in the store. The vast majority of use-cases (which currently is all of them!) will be fine with just accepting that as an OK. * chore: make update-workspace * fix: return error, but expect it in tests --- .../provisioning/controller/repository.go | 4 ++ .../apis/provisioning/jobs/persistentstore.go | 9 ++-- pkg/tests/apis/provisioning/helper_test.go | 36 +++++++++++++++ .../apis/provisioning/provisioning_test.go | 46 +------------------ 4 files changed, 45 insertions(+), 50 deletions(-) diff --git a/pkg/registry/apis/provisioning/controller/repository.go b/pkg/registry/apis/provisioning/controller/repository.go index 5a2a18e375b..d5d61239cbd 100644 --- a/pkg/registry/apis/provisioning/controller/repository.go +++ b/pkg/registry/apis/provisioning/controller/repository.go @@ -371,6 +371,10 @@ func (rc *RepositoryController) addSyncJob(ctx context.Context, obj *provisionin Pull: syncOptions, }, }) + if apierrors.IsAlreadyExists(err) { + logging.FromContext(ctx).Info("sync job already exists, nothing triggered") + return nil + } if err != nil { // FIXME: should we update the status of the repository if we fail to add the job? return fmt.Errorf("error adding sync job: %w", err) diff --git a/pkg/registry/apis/provisioning/jobs/persistentstore.go b/pkg/registry/apis/provisioning/jobs/persistentstore.go index c8e836efe41..681462772bf 100644 --- a/pkg/registry/apis/provisioning/jobs/persistentstore.go +++ b/pkg/registry/apis/provisioning/jobs/persistentstore.go @@ -57,7 +57,9 @@ var ( type Queue interface { // Insert adds a new job to the queue. // - // This saves it if it is a new job, or fails with ErrJobAlreadyExists if one with the same name already exists. + // The job name is not honoured. It will be overwritten with a name that fits the job. + // + // This saves it if it is a new job, or fails with `apierrors.IsAlreadyExists(err) == true` if one already exists. Insert(ctx context.Context, job *provisioning.Job) (*provisioning.Job, error) } @@ -374,11 +376,6 @@ func (s *persistentStore) cleanupClaims(ctx context.Context) error { return nil } -// Insert adds a new job to the queue. -// -// The job name is not honoured. It will be overwritten with a name that fits the job. -// -// This saves it if it is a new job, or fails with `apierrors.IsAlreadyExists(err) == true` if one already exists. func (s *persistentStore) Insert(ctx context.Context, job *provisioning.Job) (*provisioning.Job, error) { s.generateJobName(job) // Side-effect: updates the job's name. diff --git a/pkg/tests/apis/provisioning/helper_test.go b/pkg/tests/apis/provisioning/helper_test.go index a319f069e26..8100407fc29 100644 --- a/pkg/tests/apis/provisioning/helper_test.go +++ b/pkg/tests/apis/provisioning/helper_test.go @@ -17,6 +17,7 @@ import ( ghmock "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" @@ -52,6 +53,41 @@ type provisioningTestHelper struct { ViewerREST *rest.RESTClient } +func (h *provisioningTestHelper) SyncAndWait(t *testing.T, repo string, options *provisioning.SyncJobOptions) { + t.Helper() + + var opts provisioning.SyncJobOptions + if options != nil { + opts = *options + } + body := asJSON(opts) + + result := h.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("sync"). + Body(body). + SetHeader("Content-Type", "application/json"). + Do(t.Context()) + + if apierrors.IsAlreadyExists(result.Error()) { + // Wait for all jobs to finish as we don't have the name. + h.AwaitJobs(t, repo) + return + } + + obj, err := result.Get() + require.NoError(t, err, "expecting to be able to sync repository") + + unstruct, ok := obj.(*unstructured.Unstructured) + require.True(t, ok, "expecting unstructured object, but got %T", obj) + + name := unstruct.GetName() + require.NotEmpty(t, name, "expecting name to be set") + h.AwaitJobSuccess(t, t.Context(), name) +} + func (h *provisioningTestHelper) AwaitJobSuccess(t *testing.T, ctx context.Context, jobName string) { t.Helper() if !assert.EventuallyWithT(t, func(collect *assert.CollectT) { diff --git a/pkg/tests/apis/provisioning/provisioning_test.go b/pkg/tests/apis/provisioning/provisioning_test.go index 358057dc421..d1e45ccb2c5 100644 --- a/pkg/tests/apis/provisioning/provisioning_test.go +++ b/pkg/tests/apis/provisioning/provisioning_test.go @@ -181,27 +181,7 @@ func TestIntegrationProvisioning_CreatingGitHubRepository(t *testing.T) { ) require.NoError(t, err) - result := helper.AdminREST.Post(). - Namespace("default"). - Resource("repositories"). - Name(repo). - SubResource("sync"). - Body(asJSON(provisioning.SyncJobOptions{ - Incremental: false, - })). - SetHeader("Content-Type", "application/json"). - Do(ctx) - - obj, err := result.Get() - require.NoError(t, err, "expecting to be able to sync repository") - obj2, ok := obj.(*unstructured.Unstructured) - if !ok { - require.Fail(t, "expected unstructured response, %T", obj) - } - job := obj2.GetName() - require.NotEmpty(t, job) - - helper.AwaitJobSuccess(t, ctx, job) + helper.SyncAndWait(t, repo, nil) // By now, we should have synced, meaning we have data to read in the local Grafana instance! @@ -290,29 +270,7 @@ func TestIntegrationProvisioning_ImportAllPanelsFromLocalRepository(t *testing.T require.Error(t, err, "no all-panels dashboard should exist") // Now, we import it, such that it may exist - result := helper.AdminREST.Post(). - Namespace("default"). - Resource("repositories"). - Name(repo). - SubResource("sync"). - SetHeader("Content-Type", "application/json"). - Body(asJSON(provisioning.SyncJobOptions{ - Incremental: false, - })). - Do(ctx) - - obj, err := result.Get() - require.NoError(t, err, "expecting to be able to sync repository") - - obj2, ok := obj.(*unstructured.Unstructured) - if !ok { - require.Fail(t, "expected unstructured response, %T", obj) - } - job := obj2.GetName() - require.NotEmpty(t, job) - - // Wait for the async job to finish - helper.AwaitJobSuccess(t, ctx, job) + helper.SyncAndWait(t, repo, nil) found, err := helper.Dashboards.Resource.List(ctx, metav1.ListOptions{}) require.NoError(t, err, "can list values")