From 047c51be01cc4aa95f8b6b4445df1408bde7446e Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Wed, 8 Oct 2025 05:25:57 -0600 Subject: [PATCH] Provisioning: Do full sync on resync period when needed (#112144) --- .../pkg/repository/github/webhook.go | 24 ++------ apps/provisioning/pkg/repository/workflows.go | 32 +++++++++++ .../pkg/repository/workflows_test.go | 55 +++++++++++++++++++ .../provisioning/controller/repository.go | 29 +++++++++- .../controller/repository_test.go | 35 ++++++++++++ 5 files changed, 152 insertions(+), 23 deletions(-) diff --git a/apps/provisioning/pkg/repository/github/webhook.go b/apps/provisioning/pkg/repository/github/webhook.go index 40f87ddc93d..0da826fe9de 100644 --- a/apps/provisioning/pkg/repository/github/webhook.go +++ b/apps/provisioning/pkg/repository/github/webhook.go @@ -7,7 +7,6 @@ import ( "log/slog" "net/http" "slices" - "strings" "github.com/google/go-github/v70/github" "github.com/google/uuid" @@ -16,7 +15,6 @@ import ( "github.com/grafana/grafana-app-sdk/logging" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/apps/provisioning/pkg/repository" - "github.com/grafana/grafana/apps/provisioning/pkg/safepath" common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" ) @@ -125,27 +123,13 @@ func (r *githubWebhookRepository) parsePushEvent(event *github.PushEvent) (*prov // however, if we get an event where just a .keep file is being deleted, and no other files in the folder // are being deleted, the folder could be gone from git, but not from grafana and we do not have a way // to get the grafana uid to delete the folder. so, instead, we will queue a full sync to clean things up. - dirsWithKeepDeletes := make(map[string]struct{}) - dirsWithOtherDeletes := make(map[string]struct{}) + var deletedPaths []string for _, change := range event.GetCommits() { - for _, removedFile := range change.Removed { - dir := safepath.Dir(removedFile) - if strings.HasSuffix(removedFile, ".keep") { - dirsWithKeepDeletes[dir] = struct{}{} - } else { - dirsWithOtherDeletes[dir] = struct{}{} - } - } - } - // if there are any keep files deleted that do not have other files deleted in the same folder, we need to queue a full sync - incremental := true - for dir := range dirsWithKeepDeletes { - if _, exists := dirsWithOtherDeletes[dir]; !exists { - incremental = false - break - } + deletedPaths = append(deletedPaths, change.Removed...) } + incremental := repository.CanUseIncrementalSync(deletedPaths) + return &provisioning.WebhookResponse{ Code: http.StatusAccepted, Job: &provisioning.JobSpec{ diff --git a/apps/provisioning/pkg/repository/workflows.go b/apps/provisioning/pkg/repository/workflows.go index 48ba1319a62..5657de9dc5e 100644 --- a/apps/provisioning/pkg/repository/workflows.go +++ b/apps/provisioning/pkg/repository/workflows.go @@ -1,9 +1,12 @@ package repository import ( + "strings" + apierrors "k8s.io/apimachinery/pkg/api/errors" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/apps/provisioning/pkg/safepath" ) func IsWriteAllowed(repo *provisioning.Repository, ref string) error { @@ -40,3 +43,32 @@ func IsWriteAllowed(repo *provisioning.Repository, ref string) error { return nil } } + +// CanUseIncrementalSync checks if an incremental sync can be performed or if a full sync is needed, +// given a list of deleted file paths. It will return true if a .keep file is deleted without +// other files being deleted in the same directory. This is because the folder will not be a part of the +// deleted files, and the .keep file is not a resource in grafana, so we can't get the folder uid. +// A full sync will clean that up. +func CanUseIncrementalSync(deletedPaths []string) bool { + dirsWithKeepDeletes := make(map[string]struct{}) + dirsWithOtherDeletes := make(map[string]struct{}) + + for _, path := range deletedPaths { + dir := safepath.Dir(path) + if strings.HasSuffix(path, ".keep") { + dirsWithKeepDeletes[dir] = struct{}{} + } else { + dirsWithOtherDeletes[dir] = struct{}{} + } + } + + // if there are any .keep files deleted that don't have other files deleted in the same folder, + // we need to do a full sync + for dir := range dirsWithKeepDeletes { + if _, exists := dirsWithOtherDeletes[dir]; !exists { + return false + } + } + + return true +} diff --git a/apps/provisioning/pkg/repository/workflows_test.go b/apps/provisioning/pkg/repository/workflows_test.go index 995c85e0eb5..91a69daf35a 100644 --- a/apps/provisioning/pkg/repository/workflows_test.go +++ b/apps/provisioning/pkg/repository/workflows_test.go @@ -345,3 +345,58 @@ func TestIsWriteAllowed(t *testing.T) { }) } } + +func TestCanUseIncrementalSync(t *testing.T) { + tests := []struct { + name string + deletedPaths []string + want bool + }{ + { + name: "no deleted paths", + deletedPaths: []string{}, + want: true, + }, + { + name: "no keep file deletions", + deletedPaths: []string{"test.json"}, + want: true, + }, + { + name: "keep file deletion at root without other deletions", + deletedPaths: []string{".keep"}, + want: false, + }, + { + name: "keep file deletion with other deletions in same folder", + deletedPaths: []string{"test/.keep", "test/test.json"}, + want: true, + }, + { + name: "multiple keep files in different folders without other deletions", + deletedPaths: []string{"folder1/.keep", "folder2/.keep"}, + want: false, + }, + { + name: "nested folder with only keep file deleted", + deletedPaths: []string{"parent/child/.keep"}, + want: false, + }, + { + name: "some folders with only keep, some with other files", + deletedPaths: []string{"folder1/.keep", "folder2/.keep", "folder2/dashboard.json"}, + want: false, + }, + { + name: "only regular files deleted from multiple folders", + deletedPaths: []string{"folder1/file1.json", "folder2/file2.json", "folder3/file3.json"}, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := CanUseIncrementalSync(tt.deletedPaths) + require.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/registry/apis/provisioning/controller/repository.go b/pkg/registry/apis/provisioning/controller/repository.go index b8301f784df..70e5c3b4a87 100644 --- a/pkg/registry/apis/provisioning/controller/repository.go +++ b/pkg/registry/apis/provisioning/controller/repository.go @@ -340,7 +340,6 @@ func (rc *RepositoryController) determineSyncStrategy(ctx context.Context, obj * logger.Info("full sync on interval for non-versioned repository") return &provisioning.SyncJobOptions{} } - latestRef, err := versioned.LatestRef(ctx) if err != nil { logger.Warn("incremental sync on interval without knowing if ref has actually changed", "error", err) @@ -353,13 +352,37 @@ func (rc *RepositoryController) determineSyncStrategy(ctx context.Context, obj * return nil } - logger.Info("incremental sync on interval") - return &provisioning.SyncJobOptions{Incremental: true} + // Whenever possible, we try to keep it as an incremental sync to keep things performant. + // However, if there are any .keep file deletions inside a folder with no other deletions, we need + // to do a full sync to see if the folder was deleted as well in git. + incremental, err := shouldUseIncrementalSync(ctx, versioned, obj, latestRef) + if err != nil { + logger.Warn("unable to compare files for incremental sync, doing full sync", "error", err) + return &provisioning.SyncJobOptions{} + } + + logger.Info("sync on interval", "incremental", incremental) + return &provisioning.SyncJobOptions{Incremental: incremental} default: return nil } } +func shouldUseIncrementalSync(ctx context.Context, versioned repository.Versioned, obj *provisioning.Repository, latestRef string) (bool, error) { + changes, err := versioned.CompareFiles(ctx, obj.Status.Sync.LastRef, latestRef) + if err != nil { + return false, err + } + var deletedPaths []string + for _, change := range changes { + if change.Action == repository.FileActionDeleted { + deletedPaths = append(deletedPaths, change.Path) + } + } + + return repository.CanUseIncrementalSync(deletedPaths), nil +} + func (rc *RepositoryController) addSyncJob(ctx context.Context, obj *provisioning.Repository, syncOptions *provisioning.SyncJobOptions) error { job, err := rc.jobs.Insert(ctx, obj.Namespace, provisioning.JobSpec{ Repository: obj.GetName(), diff --git a/pkg/registry/apis/provisioning/controller/repository_test.go b/pkg/registry/apis/provisioning/controller/repository_test.go index 69f4275fe9e..e5789672e9d 100644 --- a/pkg/registry/apis/provisioning/controller/repository_test.go +++ b/pkg/registry/apis/provisioning/controller/repository_test.go @@ -303,3 +303,38 @@ func TestRepositoryController_handleDelete(t *testing.T) { }) } } + +func TestShouldUseIncrementalSync(t *testing.T) { + versioned := repository.NewMockVersioned(t) + obj := &provisioning.Repository{ + Status: provisioning.RepositoryStatus{ + Sync: provisioning.SyncStatus{ + LastRef: "123", + }, + }, + } + latestRef := "456" + t.Run("should use incremental sync", func(t *testing.T) { + versioned.On("CompareFiles", context.Background(), obj.Status.Sync.LastRef, latestRef).Return([]repository.VersionedFileChange{ + { + Action: repository.FileActionDeleted, + Path: "test.json", + }, + }, nil).Once() + got, err := shouldUseIncrementalSync(context.Background(), versioned, obj, latestRef) + assert.NoError(t, err) + assert.True(t, got) + }) + + t.Run("should not use incremental sync", func(t *testing.T) { + versioned.On("CompareFiles", context.Background(), obj.Status.Sync.LastRef, latestRef).Return([]repository.VersionedFileChange{ + { + Action: repository.FileActionDeleted, + Path: "test/.keep", + }, + }, nil).Once() + got, err := shouldUseIncrementalSync(context.Background(), versioned, obj, latestRef) + assert.NoError(t, err) + assert.False(t, got) + }) +}