[RFC] Standardized viper framework for vitess configuration parameters #11456

ajm188 · 2022-10-09T14:04:12Z

Signed-off-by: Andrew Mason andrew@planetscale.com

Description

This PR outlines, in docs and working code, a framework for a standardized way of interacting with viper to power configuration parameters for all Vitess binaries. (the docs also try to provide some context as to why we want to introduce a framework like this).

Please feel free to comment, ask questions, poke holes, etc and just generally provide feedback on the approach. We can then (pending changes that come up during the RFC period) merge this in and start moving modules to use this approach to take their configurations from places other than command-line flags.

Related Issue(s)

#11788

Checklist

"Backport me!" label has been added if this change should be backported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

vitess-bot · 2022-10-09T14:04:14Z

mattlord

I like it! I only had a few very minor questions.

go/trace/plugin_datadog.go

go/viperutil/viper.go

ajm188 · 2022-10-14T14:05:32Z

doc/viper/viper.md

+
+Broadly speaking, there's two approaches for Vitess to using viper.
+
+### Approach 1: Everything in the global registry.


@mattlord absolutely not urgent, but curious if you have any strong reactions (positive or negative!) to the 2(and a half) approaches outlined here

I'm gonna noodle on this in the background but broadly turn my attention today to working out how to best use config files (also, if you have any good examples of configs that we would want to be dynamic (query timeouts, and other "threshold-y" type things, probably?) let me know!

https://github.com/vitessio/vitess/blob/main/go/vt/vttablet/tabletserver/debugenv.go
https://github.com/vitessio/vitess/blob/main/go/vt/vtgate/debugenv.go

Those are the things that can be changed at runtime as of today.

deepthi · 2022-11-18T19:24:04Z

First comment is that we need an issue with a task breakdown!

ajm188 · 2022-11-22T11:27:16Z

First comment is that we need an issue with a task breakdown!

(it's empty now, but! 😂) I'll be putting in details here as I go!

ajm188

TODOs:

[RFC] Standardized viper framework for vitess configuration parameters #11456 (comment)
[RFC] Standardized viper framework for vitess configuration parameters #11456 (comment)
[RFC] Standardized viper framework for vitess configuration parameters #11456 (comment)

Other more general merge-blocking TODOs:

fill out issue with more details
update local example (at least, do the operator later) to demonstrate file-based configs
at least attempt (but do not include in this change) a dynamic Value to power either vtgate's or vttablet's debugenv to make sure the API works well/ergonomically

.golangci.yml

go/flagutil/enum.go

go/vt/topo/locks.go

deepthi

Nice write-up. Looks like they're a bunch of TODOs. We can get this reviewed at the code level once those are resolved.

doc/viper/viper.md

ajm188 · 2022-11-29T19:53:56Z

Nice write-up. Looks like they're a bunch of TODOs. We can get this reviewed at the code level once those are resolved.

well, at least some of the TODOs (example => this one) can potentially be obviated via code review, so i think at least a first-pass should happen before doing that

rohit-nayak-ps · 2022-12-05T16:28:28Z

go/vt/topo/locks.go

@@ -66,10 +76,29 @@ func init() {
 	for _, cmd := range FlagBinaries {
 		servenv.OnParseFor(cmd, registerTopoLockFlags)
 	}
+


Will each module that supports dynamic configuration have to (need to?) write such a block of code? Or will it (can it?) be delegated to a helper that can take some metadata and callback?

you would need to write your own code to consume the update notifications (if you want to do something like "log when the value changes" or "only check at most N seconds for a new value otherwise use the previous value").

if you want to just always take the current value in the config file (and also always pay the potential cost of the RWMutex), you can just call myValue.Get().

i think if we tried to create a single helper for this, it would be extremely tricky to support every use case (or turn into a mess of spaghetti code as we add edge case after edge case 😂)

I think we should always be calling myValue.Get() unless there is a very good reason to only poll periodically.

strongly agree that should be the (rare) exception, and not the norm

go/vt/mysqlctl/azblobbackupstorage/azblob.go

doc/viper/viper.md

harshit-gangal · 2022-12-06T14:17:06Z

I have looked through the doc. Looks good to me

go/viperutil/config.go

doc/viper/viper.md

go/viperutil/debug/debug.go

deepthi · 2022-12-06T21:18:51Z

go/viperutil/funcs/decode.go

+
+// TODO: this creates an import cycle ... IMO tabletenv should not define Seconds,
+// it should be in something more akin to flagutil with the other values like
+// TabletTypeFlag and friends.


Going to file an issue to follow up on this one, as it'll involve some more moving stuff around

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Co-authored-by: Matt Lord <mattalord@gmail.com> Signed-off-by: Andrew Mason <andrew@planetscale.com>

Signed-off-by: Andrew Mason <andrew@planetscale.com>

rohit-nayak-ps · 2023-05-25T11:09:01Z

go/vt/mysqlctl/azblobbackupstorage/azblob.go

-	fs.StringVar(&containerName, "azblob_backup_container_name", "", "Azure Blob Container Name.")
-	fs.StringVar(&storageRoot, "azblob_backup_storage_root", "", "Root prefix for all backup-related Azure Blobs; this should exclude both initial and trailing '/' (e.g. just 'a/b' not '/a/b/').")
-	fs.IntVar(&azBlobParallelism, "azblob_backup_parallelism", 1, "Azure Blob operation parallelism (requires extra memory when increased).")
+	fs.String("azblob_backup_account_name", accountName.Default(), "Azure Storage Account name for backups; if this flag is unset, the environment variable VT_AZBLOB_ACCOUNT_NAME will be used.")


All this looks great.

Wondering if we could further simplify this by allowing a FlagDesc atrribute to viperutil.Configure so that we could just call a helper viperutil.BindAndRegisterAllFlags(configKeyPrefix) . We already specify the types, flag names and defaults in Configure().

I wanted that so badly, the issue is it's very hard to generalize the underlying function calls to pflag.{String,Int,...}Var and pflag.Var

…ardized viper framework for vitess configuration parameters (vitessio#2407) * backport of 2263 * resolve conflicts Signed-off-by: Andrew Mason <andrew@planetscale.com> --------- Signed-off-by: Andrew Mason <andrew@planetscale.com> Co-authored-by: Andrew Mason <andrew@planetscale.com>

…tess configuration parameters (vitessio#2263) * cherry pick of 11456 * resolve go.mod Signed-off-by: Andrew Mason <andrew@planetscale.com> * resolve flagdata conflicts Signed-off-by: Andrew Mason <andrew@planetscale.com> --------- Signed-off-by: Andrew Mason <andrew@planetscale.com> Co-authored-by: Andrew Mason <andrew@planetscale.com>

ajm188 added the Do Not Merge label Oct 9, 2022

mattlord reviewed Oct 10, 2022

View reviewed changes

go/trace/plugin_datadog.go Outdated Show resolved Hide resolved

go/viperutil/viper.go Outdated Show resolved Hide resolved

go/viperutil/viper.go Outdated Show resolved Hide resolved

go/viperutil/viper.go Outdated Show resolved Hide resolved

ajm188 commented Oct 14, 2022

View reviewed changes

ajm188 force-pushed the andrew/viper-rfc branch from b3c61a0 to f952af6 Compare November 8, 2022 20:56

ajm188 force-pushed the andrew/viper-rfc branch from 9788a28 to 1a13f9f Compare November 21, 2022 19:45

ajm188 added Type: Feature Request Type: RFC Request For Comment Component: General Changes throughout the code base labels Nov 21, 2022

ajm188 changed the title ~~[WIP] Taking a stab at a way to standardize viper usage~~ [RFC] Standardized viper framework for vitess configuration parameters Nov 21, 2022

ajm188 marked this pull request as ready for review November 21, 2022 19:53

ajm188 requested review from rsajwani, frouioui, GuptaManan100, harshit-gangal, rohit-nayak-ps and systay as code owners November 21, 2022 19:53

ajm188 commented Nov 27, 2022

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

go/flagutil/enum.go Show resolved Hide resolved

go/vt/topo/locks.go Outdated Show resolved Hide resolved

deepthi reviewed Nov 29, 2022

View reviewed changes

doc/viper/viper.md Show resolved Hide resolved

deepthi requested a review from a team November 29, 2022 20:36

rohit-nayak-ps reviewed Dec 5, 2022

View reviewed changes

ajm188 force-pushed the andrew/viper-rfc branch from 8aac968 to 91689f1 Compare December 5, 2022 16:31

rohit-nayak-ps reviewed Dec 5, 2022

View reviewed changes

go/vt/mysqlctl/azblobbackupstorage/azblob.go Show resolved Hide resolved

harshit-gangal reviewed Dec 6, 2022

View reviewed changes

doc/viper/viper.md Show resolved Hide resolved

deepthi reviewed Dec 6, 2022

View reviewed changes

go/viperutil/config.go Show resolved Hide resolved

deepthi reviewed Dec 6, 2022

View reviewed changes

Andrew Mason and others added 15 commits May 23, 2023 16:08

better newline handling to avoid double-newline at end

a00b9a4

Signed-off-by: Andrew Mason <andrew@planetscale.com>

refactor to colocate persistence with the sync viper

67cd23d

Signed-off-by: Andrew Mason <andrew@planetscale.com>

forgot to copy over non-blocking send

ad52e4e

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Update doc/viper/viper.md

2ee8df9

Co-authored-by: Matt Lord <mattalord@gmail.com> Signed-off-by: Andrew Mason <andrew@planetscale.com>

net.JoinHostPort

2c9f4be

Signed-off-by: Andrew Mason <andrew@planetscale.com>

reorder imports

88def22

Signed-off-by: Andrew Mason <andrew@planetscale.com>

cleanup TODO panic

21e0350

Signed-off-by: Andrew Mason <andrew@planetscale.com>

error string formatting

cb80b7b

Signed-off-by: Andrew Mason <andrew@planetscale.com>

update comments

0598cbd

Signed-off-by: Andrew Mason <andrew@planetscale.com>

copyright year update

be0c895

Signed-off-by: Andrew Mason <andrew@planetscale.com>

delete empty file

554c38d

Signed-off-by: Andrew Mason <andrew@planetscale.com>

extract function for initializing the viper config

63ba912

Signed-off-by: Andrew Mason <andrew@planetscale.com>

update example package

52f94b4

Signed-off-by: Andrew Mason <andrew@planetscale.com>

more docs

098f0d2

Signed-off-by: Andrew Mason <andrew@planetscale.com>

vterrors

dee064f

Signed-off-by: Andrew Mason <andrew@planetscale.com>

ajm188 force-pushed the andrew/viper-rfc branch from 2324853 to dee064f Compare May 23, 2023 20:09

bigger time interval

0da240e

Signed-off-by: Andrew Mason <andrew@planetscale.com>

ajm188 mentioned this pull request May 24, 2023

viper website docs vitessio/website#1482

Merged

ajm188 removed the Do Not Merge label May 24, 2023

rohit-nayak-ps reviewed May 25, 2023

View reviewed changes

ajm188 merged commit 1ef59e2 into vitessio:main May 25, 2023
112 of 113 checks passed

ajm188 deleted the andrew/viper-rfc branch May 25, 2023 13:17

ajm188 mentioned this pull request May 25, 2023

[ci] add generator for templated flag testdata #13150

Merged

4 tasks

ajm188 mentioned this pull request Jun 1, 2023

gentler warning message on config-not-found #13215

Merged

4 tasks

robherley mentioned this pull request Jun 16, 2023

Add lightstep tracer plugin github/vitess-gh#64

Closed

3 tasks

ajm188 mentioned this pull request Jul 6, 2023

Viper migrations - go/acl go/mysql go/stats go/streamlog #13452

Closed

4 tasks

deepthi mentioned this pull request Jul 15, 2023

add option for warming reads to mirror primary read queries onto replicas from vtgates to warm bufferpools #13206

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Standardized viper framework for vitess configuration parameters #11456

[RFC] Standardized viper framework for vitess configuration parameters #11456

ajm188 commented Oct 9, 2022 •

edited by mattlord

vitess-bot bot commented Oct 9, 2022 •

edited by deepthi

mattlord left a comment

ajm188 Oct 14, 2022

deepthi Nov 18, 2022

deepthi commented Nov 18, 2022

ajm188 commented Nov 22, 2022

ajm188 left a comment •

edited

deepthi left a comment •

edited

ajm188 commented Nov 29, 2022

rohit-nayak-ps Dec 5, 2022

ajm188 Dec 5, 2022

deepthi Dec 6, 2022

ajm188 Dec 6, 2022

harshit-gangal commented Dec 6, 2022

deepthi Dec 6, 2022

ajm188 Dec 6, 2022

ajm188 Dec 6, 2022

rohit-nayak-ps May 25, 2023

ajm188 May 25, 2023


		Broadly speaking, there's two approaches for Vitess to using viper.

		### Approach 1: Everything in the global registry.

[RFC] Standardized viper framework for vitess configuration parameters #11456

[RFC] Standardized viper framework for vitess configuration parameters #11456

Conversation

ajm188 commented Oct 9, 2022 • edited by mattlord

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Oct 9, 2022 • edited by deepthi

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

mattlord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi commented Nov 18, 2022

ajm188 commented Nov 22, 2022

ajm188 left a comment • edited

Choose a reason for hiding this comment

deepthi left a comment • edited

Choose a reason for hiding this comment

ajm188 commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshit-gangal commented Dec 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 commented Oct 9, 2022 •

edited by mattlord

vitess-bot bot commented Oct 9, 2022 •

edited by deepthi

ajm188 left a comment •

edited

deepthi left a comment •

edited