public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Provisioning: Fix migration behavior for folder-type repositories (#109518)

Browse Source 
Fix migration issues for folder sync
This commit is contained in:
Roberto Jiménez Sánchez
2025-08-12 16:48:13 +02:00
committed by GitHub
parent 5f6abae81b
commit 5c5729a25d
6 changed files with 413 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/registry/apis/provisioning/jobs/migrate/clean.go
+17
View File
@@ -4,6 +4,7 @@ import (
"context"
"fmt"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs"
"github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
"github.com/grafana/grafana/pkg/registry/apis/provisioning/resources"
@@ -45,6 +46,22 @@ func (c *namespaceCleaner) Clean(ctx context.Context, namespace string, progress
Action: repository.FileActionDeleted,
}
// Skip provisioned resources - only delete unprovisioned (unmanaged) resources
meta, err := utils.MetaAccessor(item)
if err != nil {
result.Error = fmt.Errorf("extracting meta accessor for resource %s: %w", result.Name, err)
progress.Record(ctx, result)
return nil // Continue with next resource
}
manager, _ := meta.GetManagerProperties()
// Skip if resource is managed by any provisioning system
if manager.Identity != "" {
result.Action = repository.FileActionIgnored
progress.Record(ctx, result)
return nil // Skip this resource
}
if err := client.Delete(ctx, item.GetName(), metav1.DeleteOptions{}); err != nil {
result.Error = fmt.Errorf("deleting resource %s/%s %s: %w", result.Group, result.Resource, result.Name, err)
progress.Record(ctx, result)
pkg/registry/apis/provisioning/jobs/migrate/clean_test.go
+95 -7
View File
@@ -139,8 +139,8 @@ func TestNamespaceCleaner_Clean(t *testing.T) {
progress.AssertExpectations(t)
})
t.Run("should successfully clean namespace", func(t *testing.T) {
// Create a mock dynamic client that returns a list with multiple items
t.Run("should only delete unprovisioned resources", func(t *testing.T) {
// Create a mock dynamic client that returns a list with mixed provisioned and unprovisioned items
mockDynamicClient := &mockDynamicInterface{
items: []unstructured.Unstructured{
{
@@ -148,7 +148,8 @@ func TestNamespaceCleaner_Clean(t *testing.T) {
"apiVersion": "folder.grafana.app/v1alpha1",
"kind": "Folder",
"metadata": map[string]interface{}{
"name": "folder-1",
"name": "unprovisioned-folder",
// No manager annotations - this is unprovisioned
},
},
},
@@ -157,7 +158,21 @@ func TestNamespaceCleaner_Clean(t *testing.T) {
"apiVersion": "dashboard.grafana.app/v1alpha1",
"kind": "Dashboard",
"metadata": map[string]interface{}{
"name": "dashboard-1",
"name": "provisioned-dashboard",
"annotations": map[string]interface{}{
"grafana.app/managerKind": "repo",
"grafana.app/managerId": "test-repo",
},
},
},
},
{
Object: map[string]interface{}{
"apiVersion": "dashboard.grafana.app/v1alpha1",
"kind": "Dashboard",
"metadata": map[string]interface{}{
"name": "unprovisioned-dashboard",
// No manager annotations - this is unprovisioned
},
},
},
@@ -177,15 +192,88 @@ func TestNamespaceCleaner_Clean(t *testing.T) {
progress.On("SetMessage", mock.Anything, "remove unprovisioned folders").Return()
progress.On("SetMessage", mock.Anything, "remove unprovisioned dashboards").Return()
// Expect two successful deletions
// Expect only unprovisioned resources to be deleted (2 deletions)
progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool {
return result.Action == repository.FileActionDeleted &&
result.Name == "dashboard-1" &&
result.Name == "unprovisioned-folder" &&
result.Error == nil
})).Return()
progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool {
return result.Action == repository.FileActionDeleted &&
result.Name == "folder-1" &&
result.Name == "unprovisioned-dashboard" &&
result.Error == nil
})).Return()
// Expect provisioned resource to be ignored (1 ignore)
progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool {
return result.Action == repository.FileActionIgnored &&
result.Name == "provisioned-dashboard" &&
result.Error == nil
})).Return()
err := cleaner.Clean(context.Background(), "test-namespace", progress)
require.NoError(t, err)
mockClientFactory.AssertExpectations(t)
clients.AssertExpectations(t)
progress.AssertExpectations(t)
})
t.Run("should skip all provisioned resources", func(t *testing.T) {
// Create a mock dynamic client with only provisioned resources
mockDynamicClient := &mockDynamicInterface{
items: []unstructured.Unstructured{
{
Object: map[string]interface{}{
"apiVersion": "dashboard.grafana.app/v1alpha1",
"kind": "Dashboard",
"metadata": map[string]interface{}{
"name": "repo-managed-dashboard",
"annotations": map[string]interface{}{
"grafana.app/managerKind": "repo",
"grafana.app/managerId": "my-repo",
},
},
},
},
{
Object: map[string]interface{}{
"apiVersion": "folder.grafana.app/v1alpha1",
"kind": "Folder",
"metadata": map[string]interface{}{
"name": "file-provisioned-folder",
"annotations": map[string]interface{}{
"grafana.app/managerKind": "classic-file-provisioning",
"grafana.app/managerId": "file-provisioner",
},
},
},
},
},
}
clients := &mockClients{}
clients.On("ForResource", mock.Anything).
Return(mockDynamicClient, schema.GroupVersionKind{}, nil)
mockClientFactory := resources.NewMockClientFactory(t)
mockClientFactory.On("Clients", mock.Anything, "test-namespace").
Return(clients, nil)
cleaner := NewNamespaceCleaner(mockClientFactory)
progress := jobs.NewMockJobProgressRecorder(t)
progress.On("SetMessage", mock.Anything, "remove unprovisioned folders").Return()
progress.On("SetMessage", mock.Anything, "remove unprovisioned dashboards").Return()
// Expect both resources to be ignored (no deletions)
progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool {
return result.Action == repository.FileActionIgnored &&
result.Name == "repo-managed-dashboard" &&
result.Error == nil
})).Return()
progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool {
return result.Action == repository.FileActionIgnored &&
result.Name == "file-provisioned-folder" &&
result.Error == nil
})).Return()
pkg/registry/apis/provisioning/jobs/migrate/unifiedstorage.go
+19 -2
View File
@@ -32,6 +32,24 @@ func NewUnifiedStorageMigrator(
func (m *UnifiedStorageMigrator) Migrate(ctx context.Context, repo repository.ReaderWriter, options provisioning.MigrateJobOptions, progress jobs.JobProgressRecorder) error {
namespace := repo.Config().GetNamespace()
// For folder-type repositories, only run sync (skip export and cleaner)
if repo.Config().Spec.Sync.Target == provisioning.SyncTargetTypeFolder {
progress.SetMessage(ctx, "pull resources")
syncJob := provisioning.Job{
Spec: provisioning.JobSpec{
Pull: &provisioning.SyncJobOptions{
Incremental: false,
},
},
}
if err := m.syncWorker.Process(ctx, repo, syncJob, progress); err != nil {
return fmt.Errorf("pull resources: %w", err)
}
return nil
}
// For instance-type repositories, run the full workflow: export -> sync -> clean
progress.SetMessage(ctx, "export resources")
progress.StrictMaxErrors(1) // strict as we want the entire instance to be managed
@@ -48,8 +66,8 @@ func (m *UnifiedStorageMigrator) Migrate(ctx context.Context, repo repository.Re
// Reset the results after the export as pull will operate on the same resources
progress.ResetResults()
progress.SetMessage(ctx, "pull resources")
progress.SetMessage(ctx, "pull resources")
syncJob := provisioning.Job{
Spec: provisioning.JobSpec{
Pull: &provisioning.SyncJobOptions{
@@ -57,7 +75,6 @@ func (m *UnifiedStorageMigrator) Migrate(ctx context.Context, repo repository.Re
},
},
}
if err := m.syncWorker.Process(ctx, repo, syncJob, progress); err != nil {
return fmt.Errorf("pull resources: %w", err)
}
pkg/registry/apis/provisioning/jobs/migrate/unifiedstorage_test.go
+166 -1
View File
@@ -28,6 +28,11 @@ func TestUnifiedStorageMigrator_Migrate(t *testing.T) {
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
pr.On("SetMessage", mock.Anything, "export resources").Return()
pr.On("StrictMaxErrors", 1).Return()
@@ -45,6 +50,11 @@ func TestUnifiedStorageMigrator_Migrate(t *testing.T) {
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
pr.On("SetMessage", mock.Anything, "export resources").Return()
pr.On("StrictMaxErrors", 1).Return()
@@ -67,6 +77,11 @@ func TestUnifiedStorageMigrator_Migrate(t *testing.T) {
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
pr.On("SetMessage", mock.Anything, "export resources").Return()
pr.On("StrictMaxErrors", 1).Return()
@@ -93,6 +108,11 @@ func TestUnifiedStorageMigrator_Migrate(t *testing.T) {
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
pr.On("SetMessage", mock.Anything, "export resources").Return()
pr.On("StrictMaxErrors", 1).Return()
@@ -113,6 +133,144 @@ func TestUnifiedStorageMigrator_Migrate(t *testing.T) {
},
expectedError: "",
},
{
name: "should only run sync for folder-type repositories",
setupMocks: func(nc *MockNamespaceCleaner, ew *jobs.MockWorker, sw *jobs.MockWorker, pr *jobs.MockJobProgressRecorder, rw *repository.MockRepository) {
rw.On("Config").Return(&provisioning.Repository{
ObjectMeta: metav1.ObjectMeta{
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeFolder,
},
},
})
// Export should be skipped - no export-related mocks
// Cleaner should also be skipped - no cleaner-related mocks
// Only sync job should run
pr.On("SetMessage", mock.Anything, "pull resources").Return()
sw.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Pull != nil && !job.Spec.Pull.Incremental
}), pr).Return(nil)
},
expectedError: "",
},
{
name: "should fail when sync job fails for folder-type repositories",
setupMocks: func(nc *MockNamespaceCleaner, ew *jobs.MockWorker, sw *jobs.MockWorker, pr *jobs.MockJobProgressRecorder, rw *repository.MockRepository) {
rw.On("Config").Return(&provisioning.Repository{
ObjectMeta: metav1.ObjectMeta{
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeFolder,
},
},
})
// Only sync job should run and fail
pr.On("SetMessage", mock.Anything, "pull resources").Return()
sw.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Pull != nil && !job.Spec.Pull.Incremental
}), pr).Return(errors.New("folder sync failed"))
},
expectedError: "pull resources: folder sync failed",
},
{
name: "should run complete workflow for instance-type repositories",
setupMocks: func(nc *MockNamespaceCleaner, ew *jobs.MockWorker, sw *jobs.MockWorker, pr *jobs.MockJobProgressRecorder, rw *repository.MockRepository) {
rw.On("Config").Return(&provisioning.Repository{
ObjectMeta: metav1.ObjectMeta{
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
// Export should run for instance repositories
pr.On("SetMessage", mock.Anything, "export resources").Return()
pr.On("StrictMaxErrors", 1).Return()
ew.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Push != nil
}), pr).Return(nil)
pr.On("ResetResults").Return()
// Sync job and cleanup should also run
pr.On("SetMessage", mock.Anything, "pull resources").Return()
sw.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Pull != nil && !job.Spec.Pull.Incremental
}), pr).Return(nil)
pr.On("SetMessage", mock.Anything, "clean namespace").Return()
nc.On("Clean", mock.Anything, "test-namespace", pr).Return(nil)
},
expectedError: "",
},
{
name: "should handle empty target type as instance",
setupMocks: func(nc *MockNamespaceCleaner, ew *jobs.MockWorker, sw *jobs.MockWorker, pr *jobs.MockJobProgressRecorder, rw *repository.MockRepository) {
rw.On("Config").Return(&provisioning.Repository{
ObjectMeta: metav1.ObjectMeta{
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: "", // Empty target should default to instance behavior
},
},
})
// Should run full workflow like instance type
pr.On("SetMessage", mock.Anything, "export resources").Return()
pr.On("StrictMaxErrors", 1).Return()
ew.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Push != nil
}), pr).Return(nil)
pr.On("ResetResults").Return()
pr.On("SetMessage", mock.Anything, "pull resources").Return()
sw.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Pull != nil && !job.Spec.Pull.Incremental
}), pr).Return(nil)
pr.On("SetMessage", mock.Anything, "clean namespace").Return()
nc.On("Clean", mock.Anything, "test-namespace", pr).Return(nil)
},
expectedError: "",
},
{
name: "should pass migrate options to export job",
setupMocks: func(nc *MockNamespaceCleaner, ew *jobs.MockWorker, sw *jobs.MockWorker, pr *jobs.MockJobProgressRecorder, rw *repository.MockRepository) {
rw.On("Config").Return(&provisioning.Repository{
ObjectMeta: metav1.ObjectMeta{
Name: "test-repo",
Namespace: "test-namespace",
},
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
pr.On("SetMessage", mock.Anything, "export resources").Return()
pr.On("StrictMaxErrors", 1).Return()
// Verify that the export job receives the migrate message
ew.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Push != nil && job.Spec.Push.Message == "test migration message"
}), pr).Return(nil)
pr.On("ResetResults").Return()
pr.On("SetMessage", mock.Anything, "pull resources").Return()
sw.On("Process", mock.Anything, rw, mock.MatchedBy(func(job provisioning.Job) bool {
return job.Spec.Pull != nil && !job.Spec.Pull.Incremental
}), pr).Return(nil)
pr.On("SetMessage", mock.Anything, "clean namespace").Return()
nc.On("Clean", mock.Anything, "test-namespace", pr).Return(nil)
},
expectedError: "",
},
}
for _, tt := range tests {
@@ -129,7 +287,14 @@ func TestUnifiedStorageMigrator_Migrate(t *testing.T) {
migrator := NewUnifiedStorageMigrator(mockNamespaceCleaner, exportWorker, syncWorker)
err := migrator.Migrate(context.Background(), readerWriter, provisioning.MigrateJobOptions{}, progressRecorder)
var migrateOptions provisioning.MigrateJobOptions
if tt.name == "should pass migrate options to export job" {
migrateOptions = provisioning.MigrateJobOptions{
Message: "test migration message",
}
}
err := migrator.Migrate(context.Background(), readerWriter, migrateOptions, progressRecorder)
if tt.expectedError != "" {
require.Error(t, err)
pkg/registry/apis/provisioning/jobs/migrate/worker.go
+7
View File
@@ -55,6 +55,13 @@ func (w *MigrationWorker) Process(ctx context.Context, repo repository.Repositor
}
}
// Block migrate for legacy resources if repository type is folder
if repo.Config().Spec.Sync.Target == provisioning.SyncTargetTypeFolder {
if dualwrite.IsReadingLegacyDashboardsAndFolders(ctx, w.storageStatus) {
return errors.New("migration of legacy resources is not supported for folder-type repositories")
}
}
if dualwrite.IsReadingLegacyDashboardsAndFolders(ctx, w.storageStatus) {
return w.legacyMigrator.Migrate(ctx, rw, *options, progress)
}
pkg/registry/apis/provisioning/jobs/migrate/worker_test.go
+109
View File
@@ -114,6 +114,7 @@ func TestMigrationWorker_Process(t *testing.T) {
tests := []struct {
name string
setupMocks func(*MockMigrator, *MockMigrator, *dualwrite.MockService, *jobs.MockJobProgressRecorder)
setupRepo func(*repository.MockRepository)
job provisioning.Job
expectedError string
isLegacyActive bool
@@ -128,6 +129,9 @@ func TestMigrationWorker_Process(t *testing.T) {
},
setupMocks: func(lm *MockMigrator, um *MockMigrator, ds *dualwrite.MockService, pr *jobs.MockJobProgressRecorder) {
},
setupRepo: func(repo *repository.MockRepository) {
// No Config() call expected since we fail before that
},
expectedError: "missing migrate settings",
},
{
@@ -144,6 +148,15 @@ func TestMigrationWorker_Process(t *testing.T) {
ds.On("ReadFromUnified", mock.Anything, mock.Anything).Return(false, nil)
lm.On("Migrate", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
},
setupRepo: func(repo *repository.MockRepository) {
repo.On("Config").Return(&provisioning.Repository{
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
},
},
{
name: "should use unified storage migrator when legacy storage is not active",
@@ -159,6 +172,15 @@ func TestMigrationWorker_Process(t *testing.T) {
ds.On("ReadFromUnified", mock.Anything, mock.Anything).Return(true, nil)
um.On("Migrate", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
},
setupRepo: func(repo *repository.MockRepository) {
repo.On("Config").Return(&provisioning.Repository{
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
},
},
{
name: "should propagate migrator errors",
@@ -174,8 +196,92 @@ func TestMigrationWorker_Process(t *testing.T) {
ds.On("ReadFromUnified", mock.Anything, mock.Anything).Return(false, nil)
lm.On("Migrate", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errors.New("migration failed"))
},
setupRepo: func(repo *repository.MockRepository) {
repo.On("Config").Return(&provisioning.Repository{
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
},
expectedError: "migration failed",
},
{
name: "should block migration of legacy resources for folder-type repositories",
job: provisioning.Job{
Spec: provisioning.JobSpec{
Action: provisioning.JobActionMigrate,
Migrate: &provisioning.MigrateJobOptions{},
},
},
isLegacyActive: true,
setupMocks: func(lm *MockMigrator, um *MockMigrator, ds *dualwrite.MockService, pr *jobs.MockJobProgressRecorder) {
pr.On("SetTotal", mock.Anything, 10).Return()
ds.On("ReadFromUnified", mock.Anything, mock.Anything).Return(false, nil)
// legacyMigrator should not be called as we block before reaching it
},
setupRepo: func(repo *repository.MockRepository) {
repo.On("Config").Return(&provisioning.Repository{
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeFolder,
},
},
})
},
expectedError: "migration of legacy resources is not supported for folder-type repositories",
},
{
name: "should allow migration of legacy resources for instance-type repositories",
job: provisioning.Job{
Spec: provisioning.JobSpec{
Action: provisioning.JobActionMigrate,
Migrate: &provisioning.MigrateJobOptions{},
},
},
isLegacyActive: true,
setupMocks: func(lm *MockMigrator, um *MockMigrator, ds *dualwrite.MockService, pr *jobs.MockJobProgressRecorder) {
pr.On("SetTotal", mock.Anything, 10).Return()
ds.On("ReadFromUnified", mock.Anything, mock.Anything).Return(false, nil)
lm.On("Migrate", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
},
setupRepo: func(repo *repository.MockRepository) {
repo.On("Config").Return(&provisioning.Repository{
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeInstance,
},
},
})
},
expectedError: "",
},
{
name: "should allow migration for folder-type repositories when legacy storage is not active",
job: provisioning.Job{
Spec: provisioning.JobSpec{
Action: provisioning.JobActionMigrate,
Migrate: &provisioning.MigrateJobOptions{},
},
},
isLegacyActive: false,
setupMocks: func(lm *MockMigrator, um *MockMigrator, ds *dualwrite.MockService, pr *jobs.MockJobProgressRecorder) {
pr.On("SetTotal", mock.Anything, 10).Return()
ds.On("ReadFromUnified", mock.Anything, mock.Anything).Return(true, nil)
um.On("Migrate", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
},
setupRepo: func(repo *repository.MockRepository) {
repo.On("Config").Return(&provisioning.Repository{
Spec: provisioning.RepositorySpec{
Sync: provisioning.SyncOptions{
Target: provisioning.SyncTargetTypeFolder,
},
},
})
},
expectedError: "",
},
}
for _, tt := range tests {
@@ -192,6 +298,9 @@ func TestMigrationWorker_Process(t *testing.T) {
}
rw := repository.NewMockRepository(t)
if tt.setupRepo != nil {
tt.setupRepo(rw)
}
err := worker.Process(context.Background(), rw, tt.job, progressRecorder)
if tt.expectedError != "" {