Add features package #1849

nrb · 2019-09-05T20:58:44Z

This PR creates the features package and the necessary plumbing for flags to be enabled at the command line and in the configuration file.

Closes #1798

nrb · 2019-09-05T21:07:25Z

Looking at how this is currently written, how one might consume it, and based on the design proposal, I think it might make more sense to do an API like this within the features package:

var flags sets.String

func Enabled(name string) bool {
  flags.Has(name)
}

func Enable(names ...string) {
  flags.Insert(names....)
}

func All() []string {
  return flags.List()
}

This then skips requiring initializing our own flag set struct, and callers can simply use features.Enable("myfeature") and features.Enabled("myfeature").

I can simplify the enabling code by just doing features.Enable(config.Features()...) and features.Enable(cmdFeatures...).

What do others think of that?

nrb · 2019-09-05T21:09:20Z

I'm asking about this slightly different design because passing around a flag set on the client's factory seems a bit unwieldy for something that should be accessible ~anywhere in the code base easily.

I guess the last question I would have is if we're terribly concerned with concurrent access to the flag list, but I'm assuming that is something we don't have to address until later.

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Feature flags must be consumed as the `--features` command line argument. There is currently no plugin API change in order to minimize changes. Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

carlisia

One minor comment for now.

carlisia · 2019-09-10T19:05:07Z

pkg/client/config.go

@@ -76,6 +81,24 @@ func SaveConfig(config map[string]string) error {
 	return json.NewEncoder(configFile).Encode(&config)
 }

+func (c VeleroConfig) Namespace() string {
+	ns, ok := c[ConfigKeyNamespace]
+	if !ok {


Very subjective, but I think this and the next function would read better if the check on the map key was all inlined.

I think we can't do this here since val is used outside the if... block?

nrb · 2019-09-11T16:57:04Z

@skriss @prydonius Curious about your thoughts on this approach before I proceed further.

prydonius · 2019-09-11T18:19:41Z

because passing around a flag set on the client's factory seems a bit unwieldy for something that should be accessible ~anywhere in the code base easily.

I would agree, seems like a global here would be a lot easier. As far as concurrent access, seems like we'll only need to set the flags once at the start of the server, so not sure that we really need the mutexes?

nrb · 2019-09-12T16:27:51Z

As far as concurrent access, seems like we'll only need to set the flags once at the start of the server, so not sure that we really need the mutexes?

True - part of my concern there came when I realized this might be used by plugins. If we think it's overkill I can definitely remove.

prydonius · 2019-09-12T18:25:22Z

Ah okay, do we expect plugins to enable/disable features?

nrb · 2019-09-12T19:00:56Z

I'm thinking they'll have to inside their process. As an example, this code passes the --features argument to any plugin processes, so they'll know what features have been activated by the main server process. For the in-tree plugins, they'll enable them because the root velero command enables them. Other plugins could consume and enable/disable these and they see fit, I presume.

As a concrete example of passing to plugins, I would think any CSI plugin would check for the relevant feature before doing any work, and exit as a no-op if it's not enabled.

skriss

Approach WFM, added some comments.

pkg/client/config.go

pkg/client/factory.go

pkg/features/feature_flags.go

pkg/plugin/clientmgmt/client_builder.go

The client factory isn't where the features flags should get initialized, as it's not guaranteed to be around. Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Users shouldn't have to call NewFeatureFlagSet, so Enable will call it for them if necessary. Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

nrb · 2019-09-17T19:11:43Z

I believe I've addressed all the feedback now. Mutexes were removed since I don't think they provided much value.

I'm thinking it may be useful to add feature information to the ServerStatusRequest struct so that the client may request them along with the server's version; what do others think? If that's acceptable, I think I'd rather open a new issue/PR to add it versus continuing with this PR.

skriss · 2019-09-17T19:38:31Z

I'm thinking it may be useful to add feature information to the ServerStatusRequest struct so that the client may request them along with the server's version; what do others think? If that's acceptable, I think I'd rather open a new issue/PR to add it versus continuing with this PR.

It does seem useful though the only CLI command that exposes the ServerStatusRequest right now is velero version, which wouldn't really make sense for displaying this information. #1094 would probably be the more sensible path.

nrb · 2019-09-17T19:53:31Z

The velero plugins get command uses it, too, but I agree, it's not quite a good fit for velero version. That's part of the reason I'd like to move that to a different issue.

skriss

Mostly LGTM, just one style nit.

Have you been able to manually verify the flag-parsing for both server and CLI commands?

pkg/client/config.go

skriss · 2019-09-17T19:57:19Z

The velero plugins get command uses it, too, but I agree, it's not quite a good fit for velero version. That's part of the reason I'd like to move that to a different issue.

Yep - makes sense to me.

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

nrb · 2019-09-17T20:13:02Z

Yeah, I've tested this with backup commands using the CLI option, CLI option + config file, and both. I've also done the same with the server command. I can see the feature getting passed down to plugins too.

There's one thing that may be an issue - now that we have the root velero command reading in the config, it can be used by velero server. This could lead to a feature being read in unintentionally during local development, though the file won't be present on actual server installs.

It could also be a local development convenience, so I'm unsure if I want to add any sort of conditional loading logic.

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

prydonius

lgtm - one small formatting issue!

pkg/features/feature_flags.go

skriss · 2019-09-18T15:23:09Z

There's one thing that may be an issue - now that we have the root velero command reading in the config, it can be used by velero server. This could lead to a feature being read in unintentionally during local development, though the file won't be present on actual server installs.

I'm not too worried about this atm, guessing it'll be pretty obvious if that's happening.

Ready to merge once adnan's comment is addressed!

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

skriss

LGTM

* upstream/master: (38 commits) sync controller: replace revision file with full diff each interval (vmware-tanzu#1892) Increment logging for item backupper (vmware-tanzu#1904) Add LD_LIBRARY_PATH as an env varible for the use of vsphere plugin (vmware-tanzu#1893) Remove unused flag (vmware-tanzu#1913) Use layers in the builder Dockerfile (vmware-tanzu#1907) Fix for vmware-tanzu#1888: check item's original namespace, not remapped one, for inclusion/exclusion (vmware-tanzu#1909) fail on make verify if generated CRDs differ (vmware-tanzu#1906) velero API type changes for structural schema CRDs (vmware-tanzu#1898) Generate CRDs with structural schema (vmware-tanzu#1885) Plan for moving plugin repos (vmware-tanzu#1870) move plugin proto updating into make update (vmware-tanzu#1887) Add features package (vmware-tanzu#1849) GCP: support specifying Cloud KMS key name for backup storage locations (vmware-tanzu#1879) Adds to website (vmware-tanzu#1882) proposal for generating Velero CRDs with structural schema (vmware-tanzu#1875) Improve contributing docs (vmware-tanzu#1852) [doc] Diagram (image) now mentions velero (vmware-tanzu#1877) AWS: add support for arbitrary SSE algorithms, e.g. AES256 (vmware-tanzu#1869) update restic docs for PR vmware-tanzu#1807 (vmware-tanzu#1867) changelog for PR vmware-tanzu#1864 ...

nrb requested review from carlisia, prydonius and skriss September 5, 2019 20:58

nrb added 3 commits September 8, 2019 15:22

Add features package

97b19ff

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Make config a more generic map with acessors

025794c

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Refactor to make features a global

379854e

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

nrb force-pushed the fix-1798 branch 2 times, most recently from f04a276 to 2d65c2b Compare September 9, 2019 16:37

Provide known feature flags to plugins

c67a97d

Feature flags must be consumed as the `--features` command line argument. There is currently no plugin API change in order to minimize changes. Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

nrb force-pushed the fix-1798 branch from 2d65c2b to c67a97d Compare September 9, 2019 17:57

carlisia reviewed Sep 10, 2019

View reviewed changes

skriss suggested changes Sep 12, 2019

View reviewed changes

nrb added 9 commits September 17, 2019 13:37

Move feature flag argument parsing to command

575641e

The client factory isn't where the features flags should get initialized, as it's not guaranteed to be around. Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Address review feedback and improve usage

79c13a0

Users shouldn't have to call NewFeatureFlagSet, so Enable will call it for them if necessary. Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Improve data type handling on config struct

2c618c6

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Remove mutexes from feature flagset

bb26cc6

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Move config processing into the main command

a80c59e

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Comment on PersistentPreRun function behavior

02f0794

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Only pass features arg to plugins when present

4340013

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Log enabled features on server startup

1f7274b

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Add client feature list to bug template

f522601

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

nrb marked this pull request as ready for review September 17, 2019 19:11

skriss suggested changes Sep 17, 2019

View reviewed changes

pkg/client/config.go Show resolved Hide resolved

Fix spacing

e4428f0

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

Add changelog

c2c365a

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

prydonius approved these changes Sep 17, 2019

View reviewed changes

pkg/features/feature_flags.go Outdated Show resolved Hide resolved

Remove blank line

fec779c

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

nrb mentioned this pull request Sep 18, 2019

Add velero status command #1094

Closed

skriss approved these changes Sep 18, 2019

View reviewed changes

skriss merged commit 8ec1548 into vmware-tanzu:master Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add features package #1849

Add features package #1849

nrb commented Sep 5, 2019 •

edited by skriss

nrb commented Sep 5, 2019 •

edited

nrb commented Sep 5, 2019

carlisia left a comment

carlisia Sep 10, 2019

skriss Sep 18, 2019

nrb commented Sep 11, 2019

prydonius commented Sep 11, 2019

nrb commented Sep 12, 2019

prydonius commented Sep 12, 2019

nrb commented Sep 12, 2019

skriss left a comment

nrb commented Sep 17, 2019

skriss commented Sep 17, 2019

nrb commented Sep 17, 2019

skriss left a comment

skriss commented Sep 17, 2019

nrb commented Sep 17, 2019

prydonius left a comment

skriss commented Sep 18, 2019

skriss left a comment

Add features package #1849

Add features package #1849

Conversation

nrb commented Sep 5, 2019 • edited by skriss

nrb commented Sep 5, 2019 • edited

nrb commented Sep 5, 2019

carlisia left a comment

Choose a reason for hiding this comment

carlisia Sep 10, 2019

Choose a reason for hiding this comment

skriss Sep 18, 2019

Choose a reason for hiding this comment

nrb commented Sep 11, 2019

prydonius commented Sep 11, 2019

nrb commented Sep 12, 2019

prydonius commented Sep 12, 2019

nrb commented Sep 12, 2019

skriss left a comment

Choose a reason for hiding this comment

nrb commented Sep 17, 2019

skriss commented Sep 17, 2019

nrb commented Sep 17, 2019

skriss left a comment

Choose a reason for hiding this comment

skriss commented Sep 17, 2019

nrb commented Sep 17, 2019

prydonius left a comment

Choose a reason for hiding this comment

skriss commented Sep 18, 2019

skriss left a comment

Choose a reason for hiding this comment

nrb commented Sep 5, 2019 •

edited by skriss

nrb commented Sep 5, 2019 •

edited