From 8d2b8378ed319d5034bfec845b81250ce4af9825 Mon Sep 17 00:00:00 2001 From: Leonor Oliveira <9090754+leonorfmartins@users.noreply.github.com> Date: Tue, 10 Sep 2024 10:57:28 +0100 Subject: [PATCH] Remove UID on update in dual writer mode 2 (#93032) * Remove UID on update in dual writer mode 2 * WIP: add dashboard tests * Add more crud tests for dashboards in each mode * trigger build * Fix test --- pkg/apiserver/rest/dualwriter.go | 3 + pkg/apiserver/rest/dualwriter_mode2.go | 24 ++-- pkg/apiserver/rest/dualwriter_mode3_test.go | 5 +- pkg/apiserver/rest/dualwriter_mode4_test.go | 2 +- pkg/tests/apis/dashboard/dashboards_test.go | 129 +++++++++++++++++--- 5 files changed, 134 insertions(+), 29 deletions(-) diff --git a/pkg/apiserver/rest/dualwriter.go b/pkg/apiserver/rest/dualwriter.go index 14609439c6a..89541d6c18d 100644 --- a/pkg/apiserver/rest/dualwriter.go +++ b/pkg/apiserver/rest/dualwriter.go @@ -143,6 +143,9 @@ type updateWrapper struct { // May return nil, or a preconditions object containing nil fields, // if no preconditions can be determined from the updated object. func (u *updateWrapper) Preconditions() *metav1.Preconditions { + if u.upstream == nil { + return nil + } return u.upstream.Preconditions() } diff --git a/pkg/apiserver/rest/dualwriter_mode2.go b/pkg/apiserver/rest/dualwriter_mode2.go index 84345f6e154..77ce9f3fc0c 100644 --- a/pkg/apiserver/rest/dualwriter_mode2.go +++ b/pkg/apiserver/rest/dualwriter_mode2.go @@ -287,15 +287,8 @@ func (d *DualWriterMode2) Update(ctx context.Context, name string, objInfo rest. log.Info("object not found for update, creating one") } - // obj can be populated in case it's found or empty in case it's not found - updated, err := objInfo.UpdatedObject(ctx, foundObj) - if err != nil { - log.WithValues("object", updated).Error(err, "could not update or create object") - return nil, false, err - } - startLegacy := time.Now() - obj, created, err := d.Legacy.Update(ctx, name, &updateWrapper{upstream: objInfo, updated: updated}, createValidation, updateValidation, forceAllowCreate, options) + obj, created, err := d.Legacy.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) if err != nil { log.WithValues("object", obj).Error(err, "could not update in legacy storage") d.recordLegacyDuration(true, mode2Str, d.resource, "update", startLegacy) @@ -309,15 +302,20 @@ func (d *DualWriterMode2) Update(ctx context.Context, name string, objInfo rest. if err != nil { return obj, false, err } - - objInfo = &updateWrapper{ - upstream: objInfo, - updated: obj, + } else { + acc, err := meta.Accessor(obj) + if err != nil { + return obj, false, err } + acc.SetResourceVersion("") + acc.SetUID("") + forceAllowCreate = true } startStorage := time.Now() - res, created, err := d.Storage.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) + res, created, err := d.Storage.Update(ctx, name, &updateWrapper{ + updated: obj, // use the objected returned from legacy + }, createValidation, updateValidation, forceAllowCreate, options) if err != nil { log.WithValues("object", res).Error(err, "could not update in storage") d.recordStorageDuration(true, mode2Str, d.resource, "update", startStorage) diff --git a/pkg/apiserver/rest/dualwriter_mode3_test.go b/pkg/apiserver/rest/dualwriter_mode3_test.go index ecbd2724c83..5bab5a94f2e 100644 --- a/pkg/apiserver/rest/dualwriter_mode3_test.go +++ b/pkg/apiserver/rest/dualwriter_mode3_test.go @@ -71,7 +71,7 @@ func TestMode3_Create(t *testing.T) { acc, err := meta.Accessor(obj) assert.NoError(t, err) - assert.Equal(t, acc.GetResourceVersion(), "1") + assert.Equal(t, acc.GetResourceVersion(), "") assert.NotEqual(t, obj, anotherObj) }) } @@ -351,6 +351,9 @@ func TestMode3_Update(t *testing.T) { assert.Equal(t, obj, exampleObj) assert.NotEqual(t, obj, anotherObj) + acc, err := meta.Accessor(obj) + assert.NoError(t, err) + assert.Equal(t, acc.GetResourceVersion(), "") }) } } diff --git a/pkg/apiserver/rest/dualwriter_mode4_test.go b/pkg/apiserver/rest/dualwriter_mode4_test.go index eda09e0790b..8a334239a4a 100644 --- a/pkg/apiserver/rest/dualwriter_mode4_test.go +++ b/pkg/apiserver/rest/dualwriter_mode4_test.go @@ -64,7 +64,7 @@ func TestMode4_Create(t *testing.T) { acc, err := meta.Accessor(obj) assert.NoError(t, err) - assert.Equal(t, acc.GetResourceVersion(), "1") + assert.Equal(t, acc.GetResourceVersion(), "") assert.NotEqual(t, obj, anotherObj) }) } diff --git a/pkg/tests/apis/dashboard/dashboards_test.go b/pkg/tests/apis/dashboard/dashboards_test.go index a5aa7808f36..78e5ce6ae98 100644 --- a/pkg/tests/apis/dashboard/dashboards_test.go +++ b/pkg/tests/apis/dashboard/dashboards_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/apis" "github.com/grafana/grafana/pkg/tests/testinfra" "github.com/grafana/grafana/pkg/tests/testsuite" @@ -41,20 +42,7 @@ func TestIntegrationRequiresDevMode(t *testing.T) { require.Error(t, err) } -func TestIntegrationDashboardsApp(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ - AppModeProduction: false, // required for experimental APIs - DisableAnonymous: true, - EnableFeatureToggles: []string{ - featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, // Required to start the example service - }, - }) - _, err := helper.NewDiscoveryClient().ServerResourcesForGroupVersion("dashboard.grafana.app/v0alpha1") - require.NoError(t, err) - +func runDashboardTest(t *testing.T, helper *apis.K8sTestHelper) { t.Run("simple crud+list", func(t *testing.T) { ctx := context.Background() client := helper.GetResourceClient(apis.ResourceClientArgs{ @@ -98,6 +86,17 @@ func TestIntegrationDashboardsApp(t *testing.T) { // require.Len(t, history.Items, 1) // require.Equal(t, created, history.Items[0].GetName()) + obj.Object["spec"].(map[string]any)["title"] = "Changed title" + + updated, err := client.Resource.Update(context.Background(), + obj, + metav1.UpdateOptions{}, + ) + require.NoError(t, err) + require.Equal(t, obj.GetName(), updated.GetName()) + require.Equal(t, obj.GetUID(), updated.GetUID()) + require.Less(t, obj.GetResourceVersion(), updated.GetResourceVersion()) + // Delete the object err = client.Resource.Delete(ctx, created, metav1.DeleteOptions{}) require.NoError(t, err) @@ -107,6 +106,108 @@ func TestIntegrationDashboardsApp(t *testing.T) { require.NoError(t, err) require.Empty(t, rsp.Items) }) +} + +func TestIntegrationDashboardsApp(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + t.Run("with dual writer mode 0", func(t *testing.T) { + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for experimental APIs + DisableAnonymous: true, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, // Required to start the example service + featuremgmt.FlagKubernetesDashboards, + }, + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + "dashboards.dashboard.grafana.app": { + DualWriterMode: 0, + }, + }, + }) + runDashboardTest(t, helper) + }) + + t.Run("with dual writer mode 1", func(t *testing.T) { + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for experimental APIs + DisableAnonymous: true, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, // Required to start the example service + featuremgmt.FlagKubernetesDashboards, + }, + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + "dashboards.dashboard.grafana.app": { + DualWriterMode: 1, + }, + }, + }) + runDashboardTest(t, helper) + }) + + t.Run("with dual writer mode 2", func(t *testing.T) { + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for experimental APIs + DisableAnonymous: true, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, // Required to start the example service + featuremgmt.FlagKubernetesDashboards, + }, + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + "dashboards.dashboard.grafana.app": { + DualWriterMode: 2, + }, + }, + }) + runDashboardTest(t, helper) + }) + + t.Run("with dual writer mode 3", func(t *testing.T) { + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for experimental APIs + DisableAnonymous: true, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, // Required to start the example service + featuremgmt.FlagKubernetesDashboards, + }, + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + "dashboards.dashboard.grafana.app": { + DualWriterMode: 3, + }, + }, + }) + runDashboardTest(t, helper) + }) + + t.Run("with dual writer mode 4", func(t *testing.T) { + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for experimental APIs + DisableAnonymous: true, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, // Required to start the example service + featuremgmt.FlagKubernetesDashboards, + }, + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + "dashboards.dashboard.grafana.app": { + DualWriterMode: 4, + }, + }, + }) + runDashboardTest(t, helper) + }) + + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for experimental APIs + DisableAnonymous: true, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, // Required to start the example service + }, + }) + + _, err := helper.NewDiscoveryClient().ServerResourcesForGroupVersion("dashboard.grafana.app/v0alpha1") + require.NoError(t, err) t.Run("Check discovery client", func(t *testing.T) { disco := helper.GetGroupVersionInfoJSON("dashboard.grafana.app")