-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Wire server run options to flags. #1560
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
Wire server run options to flags. #1560
Conversation
This issue is currently awaiting triage. If metrics-server contributors determine this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
The motivating use case for these options is to configure graceful shutdown so that kube-aggregators using endpoint-based apiservice resolution (--enable-aggregator-routing) have time to stop routing to a given metrics-server instance before it stops.
ea54768
to
a4fc038
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, dgrisonnet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Version format could only be major.minor, for example: '--emulated-version=wardle=1.2,kube=1.31'. Options are: | ||
kube=1.31..1.31 (default=1.31)If the component is not specified, defaults to "kube" | ||
--external-hostname string The hostname to use when generating externalized URLs for this master (e.g. Swagger API Docs or OpenID Discovery). | ||
--feature-gates colonSeparatedMultimapStringString Comma-separated list of component:key=value pairs that describe feature gates for alpha/experimental features of different components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support most of the feature flags listed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting that we should override the flag to change the description to only include the feature gates that metrics-server's code uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From user perspective yes, we should not expose nor document flags that don't work or are not supported in our documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, I'll try to figure out how we can have a curated list that is kept in sync with k/k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --feature-gates
are inherited from ServerRunOptions which uses the default component global registry. I guess we can build the server options for metrics-server instead of using the default one which is used for kube-components.
I tried to build the option as follows:
diff --git a/cmd/metrics-server/app/options/options.go b/cmd/metrics-server/app/options/options.go
index 2cdc98b7..96912fff 100644
--- a/cmd/metrics-server/app/options/options.go
+++ b/cmd/metrics-server/app/options/options.go
@@ -100,7 +100,7 @@ func (o *Options) Flags() (fs flag.NamedFlagSets) {
// NewOptions constructs a new set of default options for metrics-server.
func NewOptions() *Options {
return &Options{
- GenericServerRunOptions: genericoptions.NewServerRunOptions(),
+ GenericServerRunOptions: genericoptions.NewServerRunOptionsForComponent("metrics server", utilversion.NewComponentGlobalsRegistry()),
This result in all features gates are gone, but the flag --feature-gates
still added:
--feature-gates colonSeparatedMultimapStringString
Comma-separated list of component:key=value pairs that describe feature gates for alpha/experimental features of different components.
If the component is not specified, defaults to "kube". This flag can be repeatedly invoked. For example: --feature-gates 'wardle:featureA=true,wardle:featureB=false' --feature-gates
'kube:featureC=true'Options are:
--goaway-chance float
I haven't tested the functionality yet. Any thoughts? Is this the right direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, feature flags is ok, as I expect it by default it doesn't include kube features and allows us to define our own feature flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with you. Currently, we don't have any feature gates, right?
What this PR does / why we need it:
Exposes the server run options set via cli flags.
The motivating use case for these options is to configure graceful shutdown so that kube-aggregators using endpoint-based apiservice resolution (--enable-aggregator-routing) have time to stop routing to a given metrics-server instance before it stops.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #