Provisioning: Do full sync on resync period when needed (#112144)

2025-10-08 05:25:57 -06:00
parent 40dde1d4aa
commit 047c51be01
5 changed files with 152 additions and 23 deletions
@@ -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{
@@ -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
+}
@@ -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)
+		})
+	}
+}
@@ -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(),
@@ -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)
+	})
+}