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

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)

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.
 

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

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