diff --git a/pkg/services/dashboards/dashboard.go b/pkg/services/dashboards/dashboard.go index 5959d07b4ed..c0cc9cf2d23 100644 --- a/pkg/services/dashboards/dashboard.go +++ b/pkg/services/dashboards/dashboard.go @@ -35,7 +35,7 @@ type DashboardService interface { CountInFolders(ctx context.Context, orgID int64, folderUIDs []string, user identity.Requester) (int64, error) GetAllDashboards(ctx context.Context) ([]*Dashboard, error) GetAllDashboardsByOrgId(ctx context.Context, orgID int64) ([]*Dashboard, error) - CleanUpDashboard(ctx context.Context, dashboardUID string, orgId int64) error + CleanUpDashboard(ctx context.Context, dashboardUID string, dashboardId int64, orgId int64) error CountDashboardsInOrg(ctx context.Context, orgID int64) (int64, error) SetDefaultPermissions(ctx context.Context, dto *SaveDashboardDTO, dash *Dashboard, provisioned bool) UnstructuredToLegacyDashboard(ctx context.Context, item *unstructured.Unstructured, orgID int64) (*Dashboard, error) diff --git a/pkg/services/dashboards/dashboard_service_mock.go b/pkg/services/dashboards/dashboard_service_mock.go index d24985eecf8..4a044d20431 100644 --- a/pkg/services/dashboards/dashboard_service_mock.go +++ b/pkg/services/dashboards/dashboard_service_mock.go @@ -48,17 +48,17 @@ func (_m *FakeDashboardService) BuildSaveDashboardCommand(ctx context.Context, d return r0, r1 } -// CleanUpDashboard provides a mock function with given fields: ctx, dashboardUID, orgId -func (_m *FakeDashboardService) CleanUpDashboard(ctx context.Context, dashboardUID string, orgId int64) error { - ret := _m.Called(ctx, dashboardUID, orgId) +// CleanUpDashboard provides a mock function with given fields: ctx, dashboardUID, dashboardId, orgId +func (_m *FakeDashboardService) CleanUpDashboard(ctx context.Context, dashboardUID string, dashboardId int64, orgId int64) error { + ret := _m.Called(ctx, dashboardUID, dashboardId, orgId) if len(ret) == 0 { panic("no return value specified for CleanUpDashboard") } var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, int64) error); ok { - r0 = rf(ctx, dashboardUID, orgId) + if rf, ok := ret.Get(0).(func(context.Context, string, int64, int64) error); ok { + r0 = rf(ctx, dashboardUID, dashboardId, orgId) } else { r0 = ret.Error(0) } diff --git a/pkg/services/dashboards/service/dashboard_service.go b/pkg/services/dashboards/service/dashboard_service.go index 80895cc95a7..a1af9d09580 100644 --- a/pkg/services/dashboards/service/dashboard_service.go +++ b/pkg/services/dashboards/service/dashboard_service.go @@ -95,6 +95,7 @@ type DashboardServiceImpl struct { publicDashboardService publicdashboards.ServiceWrapper serverLockService *serverlock.ServerLockService kvstore kvstore.KVStore + dual dualwrite.Service dashboardPermissionsReady chan struct{} } @@ -149,6 +150,12 @@ func (dr *DashboardServiceImpl) cleanupK8sDashboardResources(ctx context.Context return nil } + readingFromLegacy := dualwrite.IsReadingLegacyDashboardsAndFolders(ctx, dr.dual) + if readingFromLegacy { + // Legacy does its own cleanup + return nil + } + // Create a timeout context to ensure we complete before the lock expires ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -344,7 +351,7 @@ func (dr *DashboardServiceImpl) processDashboardBatch(ctx context.Context, orgID "deletionTimestamp", deletionTimestamp, "resourceVersion", resourceVersion) - if err = dr.CleanUpDashboard(ctx, dash.UID, orgID); err != nil { + if err = dr.CleanUpDashboard(ctx, dash.UID, dash.ID, orgID); err != nil { errs = append(errs, fmt.Errorf("failed to clean up dashboard %s: %w", dash.UID, err)) } itemsProcessed++ @@ -394,6 +401,7 @@ func ProvideDashboardServiceImpl( publicDashboardService: publicDashboardService, serverLockService: serverLockService, kvstore: kvstore, + dual: dual, } defaultLimits, err := readQuotaConfig(cfg) @@ -1779,7 +1787,7 @@ func (dr *DashboardServiceImpl) DeleteInFolders(ctx context.Context, orgID int64 func (dr *DashboardServiceImpl) Kind() string { return entity.StandardKindDashboard } -func (dr *DashboardServiceImpl) CleanUpDashboard(ctx context.Context, dashboardUID string, orgId int64) error { +func (dr *DashboardServiceImpl) CleanUpDashboard(ctx context.Context, dashboardUID string, dashboardID int64, orgId int64) error { ctx, span := tracer.Start(ctx, "dashboards.service.CleanUpDashboard") defer span.End() @@ -1789,7 +1797,7 @@ func (dr *DashboardServiceImpl) CleanUpDashboard(ctx context.Context, dashboardU return err } - return dr.dashboardStore.CleanupAfterDelete(ctx, &dashboards.DeleteDashboardCommand{OrgID: orgId, UID: dashboardUID}) + return dr.dashboardStore.CleanupAfterDelete(ctx, &dashboards.DeleteDashboardCommand{OrgID: orgId, UID: dashboardUID, ID: dashboardID}) } // ----------------------------------------------------------------------------------------- diff --git a/pkg/services/dashboards/service/dashboard_service_test.go b/pkg/services/dashboards/service/dashboard_service_test.go index 83e6eca02a7..ce9d97e650e 100644 --- a/pkg/services/dashboards/service/dashboard_service_test.go +++ b/pkg/services/dashboards/service/dashboard_service_test.go @@ -43,6 +43,7 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/storage/legacysql/dualwrite" "github.com/grafana/grafana/pkg/storage/unified/resource" ) @@ -2577,6 +2578,7 @@ func TestCleanUpDashboard(t *testing.T) { ctx := context.Background() dashboardUID := "dash-uid" + dashboardID := int64(1) orgID := int64(1) // Setup mocks @@ -2586,11 +2588,12 @@ func TestCleanUpDashboard(t *testing.T) { fakeStore.On("CleanupAfterDelete", mock.Anything, &dashboards.DeleteDashboardCommand{ OrgID: orgID, UID: dashboardUID, + ID: dashboardID, }).Return(tc.cleanupError).Maybe() } // Execute - err := service.CleanUpDashboard(ctx, dashboardUID, orgID) + err := service.CleanUpDashboard(ctx, dashboardUID, dashboardID, orgID) // Assert if tc.expectedError != nil { @@ -2608,11 +2611,12 @@ func TestCleanUpDashboard(t *testing.T) { func TestK8sDashboardCleanupJob(t *testing.T) { tests := []struct { - name string - featureEnabled bool - batchSize int - setupFunc func(*DashboardServiceImpl, context.Context, *client.MockK8sHandler) - verifyFunc func(*testing.T, *DashboardServiceImpl, context.Context, *client.MockK8sHandler, *kvstore.FakeKVStore) + name string + featureEnabled bool + readFromUnified bool + batchSize int + setupFunc func(*DashboardServiceImpl, context.Context, *client.MockK8sHandler) + verifyFunc func(*testing.T, *DashboardServiceImpl, context.Context, *client.MockK8sHandler, *kvstore.FakeKVStore) }{ { name: "Should not run cleanup when feature flag is disabled", @@ -2620,9 +2624,16 @@ func TestK8sDashboardCleanupJob(t *testing.T) { batchSize: 10, }, { - name: "Should process dashboard cleanup for all orgs", - featureEnabled: true, - batchSize: 10, + name: "Should not run cleanup when feature flag is enabled but we're reading from legacy", + featureEnabled: true, + readFromUnified: false, + batchSize: 10, + }, + { + name: "Should process dashboard cleanup for all orgs", + featureEnabled: true, + readFromUnified: true, + batchSize: 10, setupFunc: func(service *DashboardServiceImpl, ctx context.Context, k8sCliMock *client.MockK8sHandler) { // Test organizations fakeOrgService := service.orgService.(*orgtest.FakeOrgService) @@ -2690,9 +2701,10 @@ func TestK8sDashboardCleanupJob(t *testing.T) { }, }, { - name: "Should handle pagination and batching when processing large sets of dashboards", - featureEnabled: true, - batchSize: 3, + name: "Should handle pagination and batching when processing large sets of dashboards", + featureEnabled: true, + readFromUnified: true, + batchSize: 3, setupFunc: func(service *DashboardServiceImpl, ctx context.Context, k8sCliMock *client.MockK8sHandler) { // Test organization fakeOrgService := service.orgService.(*orgtest.FakeOrgService) @@ -2776,6 +2788,8 @@ func TestK8sDashboardCleanupJob(t *testing.T) { sqlStore, _ := sqlstore.InitTestDB(t) lockService := serverlock.ProvideService(sqlStore, tracing.InitializeTracerForTest()) kv := kvstore.NewFakeKVStore() + dual := dualwrite.NewMockService(t) + dual.On("ReadFromUnified", mock.Anything, mock.Anything).Return(tc.readFromUnified, nil) fakeStore := dashboards.FakeDashboardStore{} fakePublicDashboardService := publicdashboards.NewFakePublicDashboardServiceWrapper(t) @@ -2795,6 +2809,7 @@ func TestK8sDashboardCleanupJob(t *testing.T) { serverLockService: lockService, kvstore: kv, features: features, + dual: dual, } ctx, k8sCliMock := setupK8sDashboardTests(service)