VEP4: flags, pflag, optional viper #8

ajm188 · 2021-04-08T15:56:42Z

This VEP introduces a series of projects to improve the global flag "situation" in various Vitess binaries to make the user experience and documentation for each of those binaries better.

derekperkins · 2021-04-08T20:56:21Z

veps/vep-4.md

+(NB: I'm not convinced this is a good idea at this point, but I wanted to put
+it out there for criticism).


Not sure how much complexity this adds, but this seems like a very solid goal to me, given that in vitessio/vitess#7471 we're looking at having a server and a client binary.

derekperkins · 2021-04-08T21:00:29Z

I know your proposal is in favor of hiding flag implementations, but given that we all seem to be in favor of moving to cobra, regardless of how the binary consolidation RFC turns out, I wonder if we shouldn't be more prescriptive about using https://github.com/spf13/pflag

ajm188 · 2021-04-08T23:24:38Z

That makes sense to me; we can prescribe by outlining that all the AddFlags(...) functions take a pflag.FlagSet rather than a flag.FlagSet (and, vtadmin actually already does this for parsing discovery options: https://github.com/vitessio/vitess/blob/master/go/vt/vtadmin/cluster/discovery/discovery.go#L81)

systay · 2021-05-19T06:37:04Z

👋 I'm about to open a new PR here, and I was wondering if the new VEP I write should be VEP4 or VEP5

In other words - is this PR still en route to be merged?

ajm188 · 2021-05-28T02:23:14Z

Sorry Andres! You should take VEP4, I will rename

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 · 2021-11-03T19:10:45Z

@derekperkins (and others!) i am baaaaack (sorry for letting this sit on a backburner for so long) with a new version that does the restructure/encapsulation + pflag + viper-if-you-want-to all in one VEP. let me know what you think!

derekperkins · 2021-11-03T20:51:37Z

I like it a lot. While I'm all for landing it in v13, and I don't think we've ever advertised -- for flags, we might need at least one version where it's deprecated.

I know that there's already some support for setting parameters via yaml, though I've never tried it and don't know much about it, so maybe this question isn't valid. Would native viper yaml/json support replace what we have today?

ajm188 · 2021-11-03T22:34:53Z

I don't think we've ever advertised -- for flags

This is true, but I don't think the next part necessarily follows:

we might need at least one version where it's deprecated.

maybe! It's unclear to me how strictly our compatibility guarantees define "breaking change". It doesn't cross RPC boundaries, and you can make preparations now (or in v11, or v10, ..., or v1) to not be broken when we make this change. so, with a looser interpretation, we can drop it now and communicate over the monthly calls or other non—"code-prints-a-warning" channels the migration path. i can also see the argument for the stricter interpretation where it's "yeah, technically you can get away with it, but you shouldn't / that's not how we do things". so, i think it's at the very least an interesting gap in the specificity of the compatibility VEP (which focuses primarily on cross-component RPCs - i.e. vttablet{N} can talk to vtgate{N-1, N, N+1} and not as much on other pieces of the codebase)

I know that there's already some support for setting parameters via yaml, though I've never tried it and don't know much about it, so maybe this question isn't valid.

Just vttablet and vtadmin:

$ git grep -E "yaml2?\.Unmarshal"
go/cmd/vttablet/vttablet.go:            if err := yaml2.Unmarshal(bytes, config); err != nil {
go/vt/dbconfigs/dbconfigs_test.go:      err = yaml2.Unmarshal(inBytes, &gotdb)
go/vt/vtadmin/cluster/config.go:// UnmarshalYAML implements the yaml.Unmarshaler interface.
go/vt/vtadmin/cluster/config_test.go:                   err := yaml.Unmarshal([]byte(tt.yaml), &cfg)
go/vt/vtadmin/cluster/file_config.go:// UnmarshalYAML is part of the yaml.Unmarshaler interface.
go/vt/vtadmin/cluster/file_config.go:   return yaml.Unmarshal(data, fc)
go/vt/vtadmin/cluster/file_config_test.go:                      err := yaml.Unmarshal([]byte(tt.yaml), &cfg)
go/vt/vttablet/tabletserver/tabletenv/config_test.go:   err = yaml2.Unmarshal(inBytes, &gotCfg)
go/vt/vttablet/tabletserver/tabletenv/seconds_test.go:  err = yaml2.Unmarshal([]byte(wantBytes), &gotts)
go/yaml2/yaml.go:       Unmarshal = yaml.Unmarshal

I haven't used the tablet ones ever before, but I have used the vtadmin ones (heh). VTAdmin could easily switch to viper (in fact, its rbac config is viper-backed), it just hasn't been a top priority for me. A quick skim of the vttablet yaml config looks to me like it would also be easy to switch.

Would native viper yaml/json support replace what we have today?

To me, yes; it's better to have one way that all file configs get loaded, so you learn/debug viper once and now you're good to go for any component vs vttablets use thing1, vtadmin uses thing2, and so on.

derekperkins · 2021-11-03T23:03:33Z

yes; it's better to have one way that all file configs get loaded

That's what I assumed, and I think that should be stated in the VEP. Other than that and pending a decision on the deprecation cycle, I think this is great.

Signed-off-by: Andrew Mason <amason@slack-corp.com>

deepthi

Great proposal!
I have some comments/questions inline.

deepthi · 2021-11-08T22:51:18Z

veps/vep-4.md

+
+    Users can actually already update the way they pass flags and subcommands in a way that won't break, without any code changes needed in Vitess.
+
+    The question: **Do we need to do a formal deprecation cycle?**


I think we do need a formal deprecation cycle. More comments below.

deepthi · 2021-11-08T22:54:10Z

veps/vep-4.md

+
+Unfortunately, we cannot simply drop-in `pflag` as a replacement for `flag`, despite its advertisement to support exactly that.
+
+Since the very beginning, Vitess documentation has encouraged the use of the single-dash form of flags (e.g. `-my_flag`).


This is the reason we need a deprecation notice. Quite likely everyone is using the single-dash form of flags and changing all of your scripts is non-trivial.
Luckily vitess-operator uses the double-dash form so at least we won't need to update that.

My understanding of a deprecation cycle (for example, changing a flag name) is:

version 1: old version exists

version 2: old and new versions exist, old "marked" (for some definition of marked) as deprecated. this allows people to switch from the old to the new before going to a version where the old won't work

version 3: old is removed, only new version exists

For flags, we're already in step 2(...ish) because people can migrate their scripts in their current vitess version. The only thing we're missing is the "marking". If we think it's important that happens in the code (vs in release notes / announcements / monthly call), then I totally get it and we'll do the deprecation notice.

deepthi · 2021-11-08T22:54:34Z

veps/vep-4.md

+	flag.Visit(func(f *flag.Flag) {
+		for _, arg := range argv {
+			if strings.HasPrefix(arg, "-"+f.Name) {
+				log.Warningf("Use of single-dash long flags is deprecated and will be removed in 14.0. Please use --%s instead", f.Name)


deepthi · 2021-11-08T22:55:06Z

veps/vep-4.md

+Now, other packages which declare flags can begin moving to `pflag`.
+Once all packages are adding their flags directly to the `pflag.CommandLine` flagset, we can remove the "TODO" interop line.
+Now, other packages can gradually move over to `pflag`, and once everything is adding their flags directly to the `pflag` flagset, we can remove the interop line.


Is there some duplicated text here that should be cleaned up?

yeeeeeeep 😅

deepthi · 2021-11-08T22:55:45Z

veps/vep-4.md

+
+Note that there are other places that call `flag.Parse` directly (most in `go/test`, some in `go/cmd`) that will need the same treatment.
+In addition, places with subcommands will need to update callsites to take a `pflag.FlagSet`, but the changes there should be fairly minimal.
+For example, modulo running `goimports`, [this][mysqlctl_pflag_diff] is the full diff required to compile `mysqlctl`.


Should that link be to a gist or something like that? Also I couldn't really parse this sentence.

It does (easier to see in the Rich Diff view); the gist it links to is defined allll the way down here with the rest of the links.

I can remove the "modulo running goimports" bit (I'm guessing that's where this gets wordy/confusing?); the only reason I added it is it's technically not the actual diff because if you run goimports it (correctly) moves the pflag import down to the 3rd-party section which makes the diff slightly larger/less clear to read. (To be clear: not suggesting we ship a diff that breaks goimports for this work, only that doctoring the diff a bit makes the clarity of the change easier to see for the purpose of the RFC proposal)

deepthi · 2021-11-08T22:56:46Z

veps/vep-4.md

+
+For binaries that have many options (like `vttablet`), specifying all of those on the command-line is cumbersome.
+In addition, we currently don't support live-reloading of configuration variables.
+Live-reload is a huge usability win; consider for example being able to adjust an operation timeout without having to restart the component ... :chefs-kiss:


Agreed. We will still need to cautious about which flags can actually take effect on a running instance. What if someone wants to change a connection pool size, can we actually support that?

Right, I think that's a separate design/rfc/project (i.e. one for "tablet connpool live reload" and another for "vreplication timeouts live reload" and so on). Viper only makes this possible/easy to implement, but we still have to add the watch call and reload logic (which is necessarily app-specific and therefore not provided by viper) ourselves explicitly. I will add some language to clarify that here!

Signed-off-by: Andrew Mason <amason@slack-corp.com>

deepthi

Assuming all feedback has been addressed 👍

deepthi

Can you change this section before merge?

Status: In Review
 Created: 2021-04-07 (second major revision 2021-11-03)

deepthi · 2022-02-18T18:02:53Z

veps/vep-4.md

+Status: In Review
+Created: 2021-04-07 (second major revision 2021-11-03)


Suggested change

Status: In Review

Created: 2021-04-07 (second major revision 2021-11-03)

Status: Approved

Created: 2021-04-07 (second major revision 2021-11-03)

Signed-off-by: Andrew Mason <amason@hey.com>

derekperkins reviewed Apr 8, 2021

View reviewed changes

ajm188 added 3 commits November 3, 2021 15:08

Very rough initial thoughts on flags

3b8aced

Signed-off-by: Andrew Mason <amason@slack-corp.com>

pflag

d0c80a3

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Take 2, virtually unrecognizable (except the intro)

0d74014

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 force-pushed the am_flags_vep branch from 8338970 to 0d74014 Compare November 3, 2021 19:09

ajm188 changed the title ~~Very rough initial thoughts on flags~~ VEP4: flags, pflag, optional viper Nov 3, 2021

ajm188 marked this pull request as ready for review November 3, 2021 19:11

derekperkins approved these changes Nov 3, 2021

View reviewed changes

Updates from review

6e31982

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 mentioned this pull request Nov 6, 2021

[vtadmin] viper config support vitessio/vitess#9154

Merged

3 tasks

deepthi reviewed Nov 8, 2021

View reviewed changes

ajm188 added 2 commits November 11, 2021 06:37

Remove duplicate sentences

d0f5497

Signed-off-by: Andrew Mason <amason@slack-corp.com>

PR feedback: remove duplication, improve clarity (hopefully!)

c766ea9

Signed-off-by: Andrew Mason <amason@slack-corp.com>

deepthi mentioned this pull request Feb 11, 2022

Fixes #8277: vtctld logs leak pii http headers vitessio/vitess#9669

Merged

3 tasks

deepthi approved these changes Feb 18, 2022

View reviewed changes

deepthi reviewed Feb 18, 2022

View reviewed changes

Updating metadata with approval

3afbf55

Signed-off-by: Andrew Mason <amason@hey.com>

deepthi merged commit 637919b into vitessio:main Feb 18, 2022

ajm188 deleted the am_flags_vep branch February 18, 2022 18:20

ajm188 mentioned this pull request Feb 18, 2022

Restructure flag usage across vitess #7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VEP4: flags, pflag, optional viper #8

VEP4: flags, pflag, optional viper #8

ajm188 commented Apr 8, 2021 •

edited

derekperkins Apr 8, 2021

derekperkins commented Apr 8, 2021

ajm188 commented Apr 8, 2021

systay commented May 19, 2021

ajm188 commented May 28, 2021

ajm188 commented Nov 3, 2021

derekperkins commented Nov 3, 2021

ajm188 commented Nov 3, 2021

derekperkins commented Nov 3, 2021

deepthi left a comment

deepthi Nov 8, 2021

deepthi Nov 8, 2021

ajm188 Nov 11, 2021

deepthi Nov 8, 2021

deepthi Nov 8, 2021

ajm188 Nov 11, 2021

deepthi Nov 8, 2021

ajm188 Nov 11, 2021

deepthi Nov 8, 2021

ajm188 Nov 11, 2021

deepthi left a comment

deepthi left a comment

deepthi Feb 18, 2022 •

edited

ajm188 Feb 18, 2022

		(NB: I'm not convinced this is a good idea at this point, but I wanted to put
		it out there for criticism).


		Users can actually already update the way they pass flags and subcommands in a way that won't break, without any code changes needed in Vitess.

		The question: Do we need to do a formal deprecation cycle?


		Unfortunately, we cannot simply drop-in `pflag` as a replacement for `flag`, despite its advertisement to support exactly that.

		Since the very beginning, Vitess documentation has encouraged the use of the single-dash form of flags (e.g. `-my_flag`).

		Status: In Review
		Created: 2021-04-07 (second major revision 2021-11-03)

VEP4: flags, pflag, optional viper #8

VEP4: flags, pflag, optional viper #8

Conversation

ajm188 commented Apr 8, 2021 • edited

Choose a reason for hiding this comment

derekperkins commented Apr 8, 2021

ajm188 commented Apr 8, 2021

systay commented May 19, 2021

ajm188 commented May 28, 2021

ajm188 commented Nov 3, 2021

derekperkins commented Nov 3, 2021

ajm188 commented Nov 3, 2021

derekperkins commented Nov 3, 2021

deepthi left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

deepthi Feb 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajm188 commented Apr 8, 2021 •

edited

deepthi Feb 18, 2022 •

edited