Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Storage: Read desired mode from config instead of feature flags #88353

Merged
merged 8 commits into from
May 31, 2024
Merged

Storage: Read desired mode from config instead of feature flags #88353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5a0cc91
Read desired mode from config
suntala May 27, 2024
14086a2
Fix linting
suntala May 27, 2024
a5c74fa
Update playlist integration tests
suntala May 29, 2024
e30e3a4
Add mode 1 playlist integration tests
suntala May 31, 2024
d5d74e3
Add mode 0 dual writing to playlist integration tests
suntala May 31, 2024
9e9dda2
Add documentation for the different dual writing modes
suntala May 31, 2024
d822240
Fix playlist dual writing integration tests for mode 0
suntala May 31, 2024
b4b9b6d
Update kvstore key
suntala May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .golangci.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ allow = [
"github.com/grafana/grafana/pkg/services/apiserver/utils",
"github.com/grafana/grafana/pkg/services/featuremgmt",
"github.com/grafana/grafana/pkg/infra/kvstore",
"github.com/grafana/grafana/pkg/services/apiserver/options",
"github.com/grafana/grafana/pkg/apis/playlist/v0alpha1",
]
deny = [
{ pkg = "github.com/grafana/grafana/pkg", desc = "apiserver is not allowed to import grafana core" }
Expand Down
10 changes: 6 additions & 4 deletions pkg/apis/playlist/v0alpha1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ import (
)

const (
GROUP = "playlist.grafana.app"
VERSION = "v0alpha1"
APIVERSION = GROUP + "/" + VERSION
GROUP = "playlist.grafana.app"
VERSION = "v0alpha1"
APIVERSION = GROUP + "/" + VERSION
RESOURCE = "playlists"
GROUPRESOURCE = GROUP + "/" + RESOURCE
)

var PlaylistResourceInfo = common.NewResourceInfo(GROUP, VERSION,
"playlists", "playlist", "Playlist",
RESOURCE, "playlist", "Playlist",
func() runtime.Object { return &Playlist{} },
func() runtime.Object { return &PlaylistList{} },
)
Expand Down
8 changes: 7 additions & 1 deletion pkg/apiserver/builder/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package builder
import (
"net/http"

grafanarest "github.com/grafana/grafana/pkg/apiserver/rest"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/runtime/serializer"
Expand All @@ -27,7 +28,7 @@ type APIGroupBuilder interface {
scheme *runtime.Scheme,
codecs serializer.CodecFactory,
optsGetter generic.RESTOptionsGetter,
dualWrite bool,
desiredMode grafanarest.DualWriterMode,
) (*genericapiserver.APIGroupInfo, error)

// Get OpenAPI definitions
Expand All @@ -40,6 +41,11 @@ type APIGroupBuilder interface {
// Standard namespace checking will happen before this is called, specifically
// the namespace must matches an org|stack that the user belongs to
GetAuthorizer() authorizer.Authorizer

// Get the desired dual writing mode. These are modes 1, 2, 3 and 4 if
// the feature flag `unifiedStorage` is enabled and mode 0 if it is not enabled.
// #TODO add type for map[string]grafanarest.DualWriterMode?
GetDesiredDualWriterMode(dualWrite bool, toMode map[string]grafanarest.DualWriterMode) grafanarest.DualWriterMode
}

// Builders that implement OpenAPIPostProcessor are given a chance to modify the schema directly
Expand Down
11 changes: 9 additions & 2 deletions pkg/apiserver/builder/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"k8s.io/kube-openapi/pkg/common"

"github.com/grafana/grafana/pkg/apiserver/endpoints/filters"
"github.com/grafana/grafana/pkg/services/apiserver/options"
)

// TODO: this is a temporary hack to make rest.Connecter work with resource level routes
Expand Down Expand Up @@ -126,10 +127,16 @@ func InstallAPIs(
server *genericapiserver.GenericAPIServer,
optsGetter generic.RESTOptionsGetter,
builders []APIGroupBuilder,
dualWrite bool,
storageOpts *options.StorageOptions,
) error {
// dual writing is only enabled when the storage type is not legacy.
// this is needed to support setting a default RESTOptionsGetter for new APIs that don't
// support the legacy storage type.
dualWriteEnabled := storageOpts.StorageType != options.StorageTypeLegacy

for _, b := range builders {
g, err := b.GetAPIGroupInfo(scheme, codecs, optsGetter, dualWrite)
mode := b.GetDesiredDualWriterMode(dualWriteEnabled, storageOpts.DualWriterDesiredModes)
g, err := b.GetAPIGroupInfo(scheme, codecs, optsGetter, mode)
if err != nil {
return err
}
Expand Down
42 changes: 26 additions & 16 deletions pkg/apiserver/rest/dualwriter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"

"github.com/grafana/grafana/pkg/infra/kvstore"
"github.com/grafana/grafana/pkg/services/featuremgmt"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/registry/rest"
Expand Down Expand Up @@ -82,15 +81,25 @@ type DualWriter interface {
type DualWriterMode int

const (
Mode1 DualWriterMode = iota + 1
// Mode0 represents writing to and reading from solely LegacyStorage. This mode is enabled when the
// `unifiedStorage` feature flag is not set. All reads and writes are made to LegacyStorage. None are made to Storage.
Mode0 DualWriterMode = iota
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make sure this still works when mode is set to 0?
I'm afraid we will end up in a weird situation id DW mode is 0 and we inevitably call NewDualWrite with no option for 0.

I wonder if it would be better to just assume if we have a dualWriterMode defined, we will have to be in one of the 4 (1-4) modes. And if it's not defined, we know we are not in the realm of any sort of dualwriter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I added integration tests for mode 0 to make sure we are taking it into account.

Mode 0 will only function as a way of representing whether unifiedStorage is enabled or not. I think this is fine because it too represents one of the dual writing states, that of not being enabled. To me dual writing doesn't have to mean literally writing to both storages. The advantage of this approach is that it allows us to simplify the signature of GetAPIGroupInfo, which was suggested in #87668 (comment). This was the best way I found to do so but happy to consider other options of course.

// Mode1 represents writing to and reading from LegacyStorage for all primary functionality while additionally
// reading and writing to Storage on a best effort basis for the sake of collecting metrics.
Mode1
// Mode2 is the dual writing mode that represents writing to LegacyStorage and Storage and reading from LegacyStorage.
Mode2
// Mode3 represents writing to LegacyStorage and Storage and reading from Storage.
Mode3
// Mode4 represents writing and reading from Storage.
Mode4
)

// NewDualWriter returns a new DualWriter.
func NewDualWriter(mode DualWriterMode, legacy LegacyStorage, storage Storage) DualWriter {
switch mode {
// It is not possible to initialize a mode 0 dual writer. Mode 0 represents
// writing to legacy storage without `unifiedStorage` enabled.
case Mode1:
// read and write only from legacy storage
return newDualWriterMode1(legacy, storage)
Expand Down Expand Up @@ -129,12 +138,14 @@ func (u *updateWrapper) UpdatedObject(ctx context.Context, oldObj runtime.Object
func SetDualWritingMode(
ctx context.Context,
kvs *kvstore.NamespacedKVStore,
features featuremgmt.FeatureToggles,
entity string,
legacy LegacyStorage,
storage Storage,
entity string,
desiredMode DualWriterMode,
) (DualWriter, error) {
toMode := map[string]DualWriterMode{
// It is not possible to initialize a mode 0 dual writer. Mode 0 represents
// writing to legacy storage without `unifiedStorage` enabled.
"1": Mode1,
"2": Mode2,
"3": Mode3,
Expand Down Expand Up @@ -166,7 +177,7 @@ func SetDualWritingMode(
}

// Desired mode is 2 and current mode is 1
if features.IsEnabledGlobally(featuremgmt.FlagDualWritePlaylistsMode2) && (currentMode == Mode1) {
if (desiredMode == Mode2) && (currentMode == Mode1) {
// This is where we go through the different gates to allow the instance to migrate from mode 1 to mode 2.
// There are none between mode 1 and mode 2
currentMode = Mode2
suntala marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -176,17 +187,16 @@ func SetDualWritingMode(
return nil, errDualWriterSetCurrentMode
suntala marked this conversation as resolved.
Show resolved Hide resolved
}
}
// #TODO enable this check when we have a flag/config for setting mode 1 as the desired mode
// if features.IsEnabledGlobally(featuremgmt.FlagDualWritePlaylistsMode1) && (currentMode == Mode2) {
// // This is where we go through the different gates to allow the instance to migrate from mode 2 to mode 1.
// // There are none between mode 1 and mode 2
// currentMode = Mode1

// err := kvs.Set(ctx, entity, fmt.Sprint(currentMode))
// if err != nil {
// return nil, errDualWriterSetCurrentMode
// }
// }
if (desiredMode == Mode1) && (currentMode == Mode2) {
// This is where we go through the different gates to allow the instance to migrate from mode 2 to mode 1.
// There are none between mode 1 and mode 2
currentMode = Mode1
suntala marked this conversation as resolved.
Show resolved Hide resolved

err := kvs.Set(ctx, entity, fmt.Sprint(currentMode))
if err != nil {
return nil, errDualWriterSetCurrentMode
}
}

// #TODO add support for other combinations of desired and current modes

Expand Down
23 changes: 10 additions & 13 deletions pkg/apiserver/rest/dualwriter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,33 @@ import (
"fmt"
"testing"

playlist "github.com/grafana/grafana/pkg/apis/playlist/v0alpha1"
"github.com/grafana/grafana/pkg/infra/kvstore"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

func TestSetDualWritingMode(t *testing.T) {
type testCase struct {
name string
features []any
stackID string
desiredMode DualWriterMode
expectedMode DualWriterMode
}
tests :=
// #TODO add test cases for kv store failures. Requires adding support in kvstore test_utils.go
[]testCase{
{
name: "should return a mode 1 dual writer when no desired mode is set",
features: []any{},
name: "should return a mode 2 dual writer when mode 2 is set as the desired mode",
stackID: "stack-1",
expectedMode: Mode1,
desiredMode: Mode2,
expectedMode: Mode2,
},
{
name: "should return a mode 2 dual writer when mode 2 is set as the desired mode",
features: []any{featuremgmt.FlagDualWritePlaylistsMode2},
name: "should return a mode 1 dual writer when mode 1 is set as the desired mode",
stackID: "stack-1",
expectedMode: Mode2,
desiredMode: Mode1,
expectedMode: Mode1,
},
}

Expand All @@ -43,17 +43,14 @@ func TestSetDualWritingMode(t *testing.T) {
ls := legacyStoreMock{m, l}
us := storageMock{m, s}

f := featuremgmt.WithFeatures(tt.features...)
kvStore := kvstore.WithNamespace(kvstore.NewFakeKVStore(), 0, "storage.dualwriting."+tt.stackID)

key := "playlist"

dw, err := SetDualWritingMode(context.Background(), kvStore, f, key, ls, us)
dw, err := SetDualWritingMode(context.Background(), kvStore, ls, us, playlist.GROUPRESOURCE, tt.desiredMode)
assert.NoError(t, err)
assert.Equal(t, tt.expectedMode, dw.Mode())

// check kv store
val, ok, err := kvStore.Get(context.Background(), key)
val, ok, err := kvStore.Get(context.Background(), playlist.GROUPRESOURCE)
assert.True(t, ok)
assert.NoError(t, err)
assert.Equal(t, val, fmt.Sprint(tt.expectedMode))
Expand Down
3 changes: 2 additions & 1 deletion pkg/cmd/grafana/apiserver/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ func (o *APIServerOptions) RunAPIServer(config *genericapiserver.RecommendedConf
}

// Install the API Group+version
err = builder.InstallAPIs(grafanaAPIServer.Scheme, grafanaAPIServer.Codecs, server, config.RESTOptionsGetter, o.builders, true)
// #TODO figure out how to configure storage type in o.Options.StorageOptions
err = builder.InstallAPIs(grafanaAPIServer.Scheme, grafanaAPIServer.Codecs, server, config.RESTOptionsGetter, o.builders, o.Options.StorageOptions)
if err != nil {
return err
}
Expand Down
9 changes: 7 additions & 2 deletions pkg/registry/apis/dashboard/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ func (b *DashboardsAPIBuilder) GetGroupVersion() schema.GroupVersion {
return v0alpha1.DashboardResourceInfo.GroupVersion()
}

func (b *DashboardsAPIBuilder) GetDesiredDualWriterMode(dualWrite bool, modeMap map[string]grafanarest.DualWriterMode) grafanarest.DualWriterMode {
// Add required configuration support in order to enable other modes. For an example, see pkg/registry/apis/playlist/register.go
return grafanarest.Mode0
}

func addKnownTypes(scheme *runtime.Scheme, gv schema.GroupVersion) {
scheme.AddKnownTypes(gv,
&v0alpha1.Dashboard{},
Expand Down Expand Up @@ -109,7 +114,7 @@ func (b *DashboardsAPIBuilder) GetAPIGroupInfo(
scheme *runtime.Scheme,
codecs serializer.CodecFactory, // pointer?
optsGetter generic.RESTOptionsGetter,
dualWrite bool,
desiredMode grafanarest.DualWriterMode,
) (*genericapiserver.APIGroupInfo, error) {
apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(v0alpha1.GROUP, scheme, metav1.ParameterCodec, codecs)

Expand All @@ -135,7 +140,7 @@ func (b *DashboardsAPIBuilder) GetAPIGroupInfo(
}

// Dual writes if a RESTOptionsGetter is provided
if dualWrite && optsGetter != nil {
if desiredMode != grafanarest.Mode0 && optsGetter != nil {
options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: grafanaregistry.GetAttrs}
if err := store.CompleteWithOptions(options); err != nil {
return nil, err
Expand Down
8 changes: 7 additions & 1 deletion pkg/registry/apis/dashboardsnapshot/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

dashboardsnapshot "github.com/grafana/grafana/pkg/apis/dashboardsnapshot/v0alpha1"
"github.com/grafana/grafana/pkg/apiserver/builder"
grafanarest "github.com/grafana/grafana/pkg/apiserver/rest"
"github.com/grafana/grafana/pkg/infra/appcontext"
"github.com/grafana/grafana/pkg/infra/db"
"github.com/grafana/grafana/pkg/infra/log"
Expand Down Expand Up @@ -86,6 +87,11 @@ func (b *SnapshotsAPIBuilder) GetGroupVersion() schema.GroupVersion {
return resourceInfo.GroupVersion()
}

func (b *SnapshotsAPIBuilder) GetDesiredDualWriterMode(dualWrite bool, modeMap map[string]grafanarest.DualWriterMode) grafanarest.DualWriterMode {
// Add required configuration support in order to enable other modes. For an example, see pkg/registry/apis/playlist/register.go
return grafanarest.Mode0
}

func addKnownTypes(scheme *runtime.Scheme, gv schema.GroupVersion) {
scheme.AddKnownTypes(gv,
&dashboardsnapshot.DashboardSnapshot{},
Expand Down Expand Up @@ -122,7 +128,7 @@ func (b *SnapshotsAPIBuilder) GetAPIGroupInfo(
scheme *runtime.Scheme,
codecs serializer.CodecFactory, // pointer?
optsGetter generic.RESTOptionsGetter,
dualWrite bool,
_ grafanarest.DualWriterMode,
) (*genericapiserver.APIGroupInfo, error) {
apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(dashboardsnapshot.GROUP, scheme, metav1.ParameterCodec, codecs)
storage := map[string]rest.Storage{}
Expand Down
8 changes: 7 additions & 1 deletion pkg/registry/apis/datasource/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
datasource "github.com/grafana/grafana/pkg/apis/datasource/v0alpha1"
query "github.com/grafana/grafana/pkg/apis/query/v0alpha1"
"github.com/grafana/grafana/pkg/apiserver/builder"
grafanarest "github.com/grafana/grafana/pkg/apiserver/rest"
"github.com/grafana/grafana/pkg/plugins"
"github.com/grafana/grafana/pkg/promlib/models"
"github.com/grafana/grafana/pkg/registry/apis/query/queryschema"
Expand Down Expand Up @@ -151,6 +152,11 @@ func (b *DataSourceAPIBuilder) GetGroupVersion() schema.GroupVersion {
return b.connectionResourceInfo.GroupVersion()
}

func (b *DataSourceAPIBuilder) GetDesiredDualWriterMode(dualWrite bool, modeMap map[string]grafanarest.DualWriterMode) grafanarest.DualWriterMode {
// Add required configuration support in order to enable other modes. For an example, see pkg/registry/apis/playlist/register.go
return grafanarest.Mode0
}

func addKnownTypes(scheme *runtime.Scheme, gv schema.GroupVersion) {
scheme.AddKnownTypes(gv,
&datasource.DataSourceConnection{},
Expand Down Expand Up @@ -198,7 +204,7 @@ func (b *DataSourceAPIBuilder) GetAPIGroupInfo(
scheme *runtime.Scheme,
codecs serializer.CodecFactory, // pointer?
_ generic.RESTOptionsGetter,
_ bool,
_ grafanarest.DualWriterMode,
) (*genericapiserver.APIGroupInfo, error) {
storage := map[string]rest.Storage{}

Expand Down
8 changes: 7 additions & 1 deletion pkg/registry/apis/example/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

example "github.com/grafana/grafana/pkg/apis/example/v0alpha1"
"github.com/grafana/grafana/pkg/apiserver/builder"
grafanarest "github.com/grafana/grafana/pkg/apiserver/rest"
"github.com/grafana/grafana/pkg/infra/appcontext"
"github.com/grafana/grafana/pkg/services/featuremgmt"
)
Expand Down Expand Up @@ -53,6 +54,11 @@ func (b *TestingAPIBuilder) GetGroupVersion() schema.GroupVersion {
return b.gv
}

func (b *TestingAPIBuilder) GetDesiredDualWriterMode(dualWrite bool, modeMap map[string]grafanarest.DualWriterMode) grafanarest.DualWriterMode {
// Add required configuration support in order to enable other modes. For an example, see pkg/registry/apis/playlist/register.go
return grafanarest.Mode0
}

func addKnownTypes(scheme *runtime.Scheme, gv schema.GroupVersion) {
scheme.AddKnownTypes(gv,
&example.RuntimeInfo{},
Expand Down Expand Up @@ -85,7 +91,7 @@ func (b *TestingAPIBuilder) GetAPIGroupInfo(
scheme *runtime.Scheme,
codecs serializer.CodecFactory, // pointer?
_ generic.RESTOptionsGetter,
_ bool,
_ grafanarest.DualWriterMode,
) (*genericapiserver.APIGroupInfo, error) {
b.codecs = codecs
apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(b.gv.Group, scheme, metav1.ParameterCodec, codecs)
Expand Down
8 changes: 7 additions & 1 deletion pkg/registry/apis/featuretoggle/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

"github.com/grafana/grafana/pkg/apis/featuretoggle/v0alpha1"
"github.com/grafana/grafana/pkg/apiserver/builder"
grafanarest "github.com/grafana/grafana/pkg/apiserver/rest"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/setting"
Expand Down Expand Up @@ -49,6 +50,11 @@ func (b *FeatureFlagAPIBuilder) GetGroupVersion() schema.GroupVersion {
return gv
}

func (b *FeatureFlagAPIBuilder) GetDesiredDualWriterMode(dualWrite bool, modeMap map[string]grafanarest.DualWriterMode) grafanarest.DualWriterMode {
// Add required configuration support in order to enable other modes. For an example, see pkg/registry/apis/playlist/register.go
return grafanarest.Mode0
}

func addKnownTypes(scheme *runtime.Scheme, gv schema.GroupVersion) {
scheme.AddKnownTypes(gv,
&v0alpha1.Feature{},
Expand Down Expand Up @@ -82,7 +88,7 @@ func (b *FeatureFlagAPIBuilder) GetAPIGroupInfo(
scheme *runtime.Scheme,
codecs serializer.CodecFactory, // pointer?
_ generic.RESTOptionsGetter,
_ bool,
_ grafanarest.DualWriterMode,
) (*genericapiserver.APIGroupInfo, error) {
apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(v0alpha1.GROUP, scheme, metav1.ParameterCodec, codecs)

Expand Down