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.

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

Storage: Customise setting dual writing modes #87668

Merged
merged 5 commits into from May 14, 2024

Conversation

suntala
Copy link
Contributor

@suntala suntala commented May 10, 2024

What is this feature?

Adds the ability to update the dual writing mode based on entity using feature flags.

Why do we need this feature?

We need to be able to customise mode based on the following factors:

  • entity/kind
  • instance
  • deployment wave

Who is this feature for?

k8s storage operators

Which issue(s) does this PR fix?:

Partial https://github.com/grafana/search-and-storage-team/issues/15

Special notes for your reviewer:

  • Corresponding PRs in https://github.com/grafana/hosted-grafana/ and https://github.com/grafana/deployment_tools to follow.
  • Went with a different approach than the one suggested in the ticket. Customising modes using configuration doesn't seem flexible enough. As far as I know, there isn't a way to provide a different configuration based on the deployment wave without relying on feature flags. HG API doesn't provide that customisation.
  • This setup is in no way perfect. For instance, we need to manage multiple modes being defined using feature flags for a given entity. For example, both mode 2 and mode 3 could be set for playlists. The idea would be to transition to Feature Toggles 2.0 when they are available. The new toggles should solve that issue. We require more complex rules, in particular support for non-boolean flag values. Need to double check but the new toggle version could allow us to update the mode for an instance without restarting it.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone May 10, 2024
@suntala suntala changed the title Storage: Make dual writing modes customisable Storage: Customise setting dual writing modes May 10, 2024
@suntala suntala force-pushed the suntala/configurable-dualwriting-modes branch from dfa06b8 to 5124629 Compare May 10, 2024 21:23
@suntala suntala added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels May 10, 2024
@suntala suntala force-pushed the suntala/configurable-dualwriting-modes branch from 5124629 to a8e2fbe Compare May 10, 2024 21:43
@suntala suntala marked this pull request as ready for review May 10, 2024 21:57
@suntala suntala requested review from grafanabot and a team as code owners May 10, 2024 21:57
@suntala suntala force-pushed the suntala/configurable-dualwriting-modes branch from 3c95a6a to 5e0c4b5 Compare May 13, 2024 09:57
@ryantxu
Copy link
Member

ryantxu commented May 13, 2024

I think this approach is fine for now -- since we only have playlists, and progress is good!

However -- given that this is a pattern we will want to apply incrementatlly to all the resources that migrate, i think it makes more sense to add a section to the config that can handle this:
https://github.com/grafana/grafana/blob/main/pkg/services/apiserver/config.go#L53

and then use it to pass in a mode rather than a bool here:
https://github.com/grafana/grafana/blob/main/pkg/services/apiserver/service.go#L314

@leonorfmartins
Copy link
Contributor

also, one thing I was considering is that all functions newDualWriter<number> should be private in the future, so that each entity just needs to care about getting a dualwriter on which the mode is passed by argument. What do you think?

@suntala
Copy link
Contributor Author

suntala commented May 13, 2024

However -- given that this is a pattern we will want to apply incrementatlly to all the resources that migrate, i think it makes more sense to add a section to the config that can handle this:
https://github.com/grafana/grafana/blob/main/pkg/services/apiserver/config.go#L53

and then use it to pass in a mode rather than a bool here:
https://github.com/grafana/grafana/blob/main/pkg/services/apiserver/service.go#L314

Totally agree about updating the boolean parameter for dualwriting (second link you included) so that we replace it with the dual writing mode.

About using config instead of feature flags though, I don't see a straightforward way of doing this currently. As far as I know, there is no way to apply different configs based on the deployment wave. That means if we want to use config to set the desired mode, we still need all the feature flags I already created and added. That's why I thought to use the feature flags directly.

Next steps I want to try out:

  • replace the boolean parameter with a mode value instead
  • do this much earlier in the process, in the applyGrafanaConfig function. Even if we are using a feature flag for now, we can still capture the value earlier than we currently do. Adding a StorageOption for the mode would make sense.
    Not sure yet how much work this will be so might try it out in a different PR.

@suntala
Copy link
Contributor Author

suntala commented May 13, 2024

also, one thing I was considering is that all functions newDualWriter should be private in the future, so that each entity just needs to care about getting a dualwriter on which the mode is passed by argument. What do you think?

That could be helpful. It would take a bunch of refactoring though so might be better to do that as a separate PR.

@hairyhenderson
Copy link
Member

As far as I know, there is no way to apply different configs based on the deployment wave.

One approach that we've used in cloud is to use an "implicit" (i.e. not defined/registered in the Grafana codebase) feature toggle to trigger configuration to render differently. So, you could use config for this after all. It'd still be effectively using a feature toggle, just not a grafana feature toggle.

@suntala
Copy link
Contributor Author

suntala commented May 13, 2024

One approach that we've used in cloud is to use an "implicit" (i.e. not defined/registered in the Grafana codebase) feature toggle to trigger configuration to render differently.

Good to know! I can see how that keep the list of Grafana feature toggles more manageable to read. Does it come with any other advantages (cost for example)?

I'm going to try to get this merged as soon as possible so we can start trying out dual writing in ops but that could be an improvement to make along with some others I already have in mind.

@suntala suntala force-pushed the suntala/configurable-dualwriting-modes branch from 576c6f9 to 2e73c4f Compare May 13, 2024 18:58
@suntala suntala merged commit 6836bfe into main May 14, 2024
19 checks passed
@suntala suntala deleted the suntala/configurable-dualwriting-modes branch May 14, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes type/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants