Allow show hidden metrics in kube-apiserver #84292

RainbowMango · 2019-10-24T12:56:48Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add a flag to show hidden metrics.

On a subsequent release (when the metric's deprecatedVersion is equal to current_kubernetes_version - 1)), a deprecated metric will become a hidden metric. Unlike their deprecated counterparts, hidden metrics will no longer be automatically registered to the metrics endpoint (hence hidden). However, they can be explicitly enabled through a command line flag on the binary (i.e. '--enable-hidden-metrics=really_deprecated_metric'). This is to provide cluster admins an escape hatch to properly migrate off of a deprecated metric, if they were not able to react to the earlier deprecation warnings. Hidden metrics should be deleted after one release.

Which issue(s) this PR fixes:

Special notes for your reviewer:
It's a lightweight implementation against the KEP.

Does this PR introduce a user-facing change?:

New flag `--show-hidden-metrics-for-version` in kube-apiserver can be used to show all hidden metrics that deprecated in the previous minor release.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-instrumentation/20190404-kubernetes-control-plane-metrics-stability.md

fedebongio · 2019-10-24T20:16:00Z

/assign @logicalhan

logicalhan

/cc @ehashman

This looks reasonable to me.

@lavalamp, does this flag look okay to you?

ehashman · 2019-10-24T21:10:16Z

There's no tests included in this PR, we should really include a unit test at minimum.

I'm assuming this is sort of a follow-up to #81970.

@logicalhan we previously discussed that we wanted this to be a feature flag rather than a CLI option as adding a CLI option removes the possibility of removing it/changing schema in the future, and the KEP specifies that we should be able to list specific hidden metrics to enable/disable (rather than a blanket disable for all of them). I think for now it would make more sense to go with the feature flag approach as I previously looked at?

ehashman · 2019-10-24T21:10:32Z

/sig instrumentation

lavalamp · 2019-10-24T21:41:08Z

cmd/kube-apiserver/app/options/options.go

@@ -153,6 +154,9 @@ func (s *ServerRunOptions) Flags() (fss cliflag.NamedFlagSets) {
 	fs.BoolVar(&s.AllowPrivileged, "allow-privileged", s.AllowPrivileged,
 		"If true, allow privileged containers. [default=false]")

+	fs.BoolVar(&s.ShowHiddenMetrics, "show-hidden-metrics", s.ShowHiddenMetrics,


What is to prevent an admin from:

in version X, turn this on to get back deprecated metric M

in version Y, not turn this off, and therefore fail to notice metric N is being deprecated

in version Z, metric N is removed with (effectively) no warning.

To prevent this scenario, I suggest forcing the user to specify the version for which they want to show metrics as the value of this flag rather than "true".

I like the suggestion, it seems reasonable to me.

I think this is a good idea (and I also wouldn't mind getting rid of the KEP specification to be able to turn on/off individual metrics and just make it binary-wide for hidden) so perhaps we should update the KEP to include this?

Personally I think we should be able to turning off individual metrics, since we have had memory leak issues in the past. These don't tend to manifest except under certain cardinality conditions, so it's possible that metrics like that will get sneak by and get released at some point. I'm thinking of that as a beta/GA feature though. Always good to have that safety net of being able to turn off a memory leak without having to patch and release kubernetes.

I like the suggestion, it seems reasonable to me.

+1
It's a really good point!

logicalhan · 2019-10-24T22:50:26Z

@logicalhan we previously discussed that we wanted this to be a feature flag rather than a CLI option as adding a CLI option removes the possibility of removing it/changing schema in the future, and the KEP specifies that we should be able to list specific hidden metrics to enable/disable (rather than a blanket disable for all of them). I think for now it would make more sense to go with the feature flag approach as I previously looked at?

I was thinking maybe we should just separate the functionality. A generic toggle for deprecated/hidden metrics but we should still eventually have the ability to disable metric[s] on demand. I want the safety net to be able to get rid of a metric memory leak without having to release a new version of kubernetes.

ehashman · 2019-10-24T23:38:18Z

So from this discussion I'm hearing we should specify two things:

what baseline version we want base unhiding metrics against
what specific metrics we want to unhide

Sounds like we now need two CLI flags... can we have a quick design discussion somewhere, maybe on the KEP, before moving forward with this?

/hold

RainbowMango · 2019-10-25T02:08:47Z

@ehashman

what specific metrics we want to unhide

If I understand correctly, here @logicalhan want to hide specific metrics, not unhide. :)

I doubt if it is convincing enough to add a flag(hide metrics) just to prevent memory leak?

RainbowMango · 2019-10-25T02:22:36Z

From this discussion, personally I think we should provide a feature gate as in #81970 which can provide stability guarantees for the flag(s).

e.g

in 1.17 flag(alpha) feature gate(default off)
in 1.18 flag(beta) feature gate(default on)
in 1.19 flag(stable) feature gate(removed)

logicalhan · 2019-10-25T02:29:35Z

From this discussion, personally I think we should provide a feature gate as in #81970 which can provide stability guarantees for the flag(s).

e.g

in 1.17 flag(alpha) feature gate(default off)

in 1.18 flag(beta) feature gate(default on)

in 1.19 flag(stable) feature gate(removed)

This is basically not at all the conclusion I am coming to. How are you arriving at feature gates from this conversation?

logicalhan · 2019-10-25T02:40:18Z

I doubt if it is convincing enough to add a flag(hide metrics) just to prevent memory leak?

We have already had to do this: #74636. We should not require a new release of kubernetes in order to turn off a bad metric.

RainbowMango · 2019-10-25T02:44:30Z

How are you arriving at feature gates from this conversation?

I like @lavalamp 's idea. I am a little worried we will lose the possibility of improving this flag when we get another good idea.

Am I over-cautious?

logicalhan · 2019-10-25T02:57:31Z

@lavalamp’s idea (as I understand it from talking to him about it today) is that the flag takes in a string parameter, a version for which we will show what would otherwise be hidden metrics. This eliminates the danger of what happens when someone forgets to untoggle a flag after migrating a set of deprecated metrics.

Being able to hide metrics is something which we can address orthogonally, using another flag.

RainbowMango · 2019-10-25T03:15:58Z

@lavalamp’s idea (as I understand it from talking to him about it today) is that the flag takes in a string parameter, a version for which we will show what would otherwise be hidden metrics. This eliminates the danger of what happens when someone forgets to untoggle a flag after migrating a set of deprecated metrics.

Being able to hide metrics is something which we can address orthogonally, using another flag.

OK. Copy that. as well as

We have already had to do this: #74636. We should not require a new release of kubernetes in order to turn off a bad metric.

I will push a change according @lavalamp's comments.

RainbowMango · 2019-11-11T11:47:36Z

Note that we will also need this flag implemented in the rest of the kube binaries, not just the apiserver (e.g. kubelet, scheduler, etc.)

Yeah, I know that. I intended to deal with them after this PR. Because they will share the validation logic from the component base.

sttts · 2019-11-11T12:22:45Z

cmd/kube-apiserver/app/options/options.go

+	// TODO(RainbowMango): move it to genericoptions before next flag comes.
+	mfs := fss.FlagSet("metrics")
+	mfs.StringVar(&s.ShowHiddenMetricsForVersion, "show-hidden-metrics-for-version", s.ShowHiddenMetricsForVersion,
+		"The previous version(x.y) for which you want to show hidden metrics. Only the previous minor version is allowed.")


What is this version? Which formats are accepted? What does the last sentence mean?

Why not --show-hidden-metrics without any argument?

This kind of flag is incompatible by design to the previous version on each version change.

Why not --show-hidden-metrics without any argument?

See the discussion above:
#84292 (comment)

Also updated to KEP:
why not --show-hidden-metrics

Ok, got that.

Another point: I have not read the KEP. I have no clue what this flag does. What does "show" mean? What does "hidden" mean? This must be super clear from the flag description.

That was the question I asked here on the KEP update: kubernetes/enhancements#1358 (comment)

Why didn't we stick with the original plan of allowing enabling a specific deprecated metric?

Because we want to reserve the ability to disable a metric (regardless of whether it is stable or not) at application boot and that functionality is orthogonal to metrics being auto-disabled by metrics stability framework.

The original plan dictates providing a mechanism for turning off individual metrics via the command line.

..all metrics should be able to be individually disabled by the cluster administrator, regardless of stability class. By default, all non-deprecated metrics will be automatically registered to the metrics endpoint unless explicitly blacklisted via a command line flag (i.e. '--disable-metrics=somebrokenmetric,anothermetric')

I suggest: The previous version for which you want to show hidden metrics. Only the previous minor version is meaningful, other values will be ignored. The format is <major>.<minor>, e.g.: 1.16

Given the discussion, I think it makes sense to add a comment here explaining the purpose of this (to avoid giving the admin a surprise at v N+2). This could also be said in the help text, e.g.: The purpose of this format is make sure you have the opportunity to notice if the next release hides additional metrics, rather than being surprised when they are permanently removed in the release after that.

cmd/kube-apiserver/app/options/validation.go

cmd/kube-apiserver/app/server.go

RainbowMango · 2019-11-12T03:25:03Z

I suggest: The previous version for which you want to show hidden metrics. Only the previous minor version is meaningful, other values will be ignored. The format is ., e.g.: 1.16

New push with a minor change: other values will be ignored --> other values will not be allowed according to the logic of validation.

lavalamp · 2019-11-12T17:04:13Z

Does "not be allowed" mean apiserver won't start? I can't decide if that's good or not.

liggitt · 2019-11-12T17:09:09Z

silently ignoring an explicit option they asked for seems not good, especially for something like this where that would mean alerting/monitoring configurations could be making incorrect decisions based on absent metrics

lavalamp · 2019-11-12T17:13:49Z

staging/src/k8s.io/component-base/metrics/registry.go

+
+	targetVersion, err := semver.Parse(pathVersion)
+	if err != nil {
+		return fmt.Errorf("specified --show-hidden-metrics-for-version '%v' not follows the version format x.y", targetVersionStr)


s/not follows/does not follow/

lavalamp · 2019-11-12T17:15:18Z

staging/src/k8s.io/component-base/metrics/registry.go

+		return fmt.Errorf("specified --show-hidden-metrics-for-version '%v' not follows the version format x.y", targetVersionStr)
+	}
+
+	maxAllowedVersion, err := semver.Make(fmt.Sprintf("%d.%d.0", currentVersion.Major, currentVersion.Minor))


I think you don't need any of this semver code. Only the previous minor version is allowed, so you can print it to a string and compare directly.

I think that can handle the above error condition, too. The error message can say, --show-hidden-metrics-for-version must be omitted or have the value "%v". Only the previous minor version is allowed.

lavalamp · 2019-11-12T17:17:22Z

OK, if @liggitt agrees then I'm sold, but that means the error handling code can be drastically simplified.

- Update bazel by hack/update-bazel.sh

RainbowMango · 2019-11-13T02:42:04Z

OK, if @liggitt agrees then I'm sold, but that means the error handling code can be drastically simplified.

Thanks, much clear now.

brancz · 2019-11-13T10:37:40Z

/lgtm

lavalamp · 2019-11-13T19:19:41Z

staging/src/k8s.io/component-base/metrics/registry.go

+		return nil
+	}
+
+	validVersionStr := fmt.Sprintf("%d.%d", currentVersion.Major, currentVersion.Minor-1)


I don't think we're ever going to have a v2, but if we did, it would print 2.-1 which is not right :)

Feel free to fix in a follow-up if you want.

lavalamp · 2019-11-13T19:20:04Z

/lgtm
/approve

k8s-ci-robot · 2019-11-13T19:22:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, lavalamp, logicalhan, RainbowMango

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:

~~cmd/kube-apiserver/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/component-base/metrics/OWNERS~~ [brancz,lavalamp,logicalhan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from deads2k and smarterclayton October 24, 2019 12:58

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 24, 2019

k8s-ci-robot assigned logicalhan Oct 24, 2019

logicalhan reviewed Oct 24, 2019

View reviewed changes

k8s-ci-robot requested a review from ehashman October 24, 2019 20:50

k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Oct 24, 2019

lavalamp reviewed Oct 24, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2019

sttts reviewed Nov 11, 2019

View reviewed changes

cmd/kube-apiserver/app/options/validation.go Outdated Show resolved Hide resolved

sttts reviewed Nov 11, 2019

View reviewed changes

cmd/kube-apiserver/app/server.go Outdated Show resolved Hide resolved

RainbowMango force-pushed the pr_add_metrics_flag_to_apiserver branch from 22dd973 to 1290698 Compare November 12, 2019 03:16

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019

lavalamp reviewed Nov 12, 2019

View reviewed changes

RainbowMango added 3 commits November 13, 2019 10:32

Add common flag validation functionality shared by kube-binaries

0efb642

Add metrics flag to show hidden metrics to kube-apiserver

ac0562b

Deal with auto-generated files.

b2fbdee

- Update bazel by hack/update-bazel.sh

RainbowMango force-pushed the pr_add_metrics_flag_to_apiserver branch from 1290698 to b2fbdee Compare November 13, 2019 02:33

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 13, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2019

lavalamp reviewed Nov 13, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2019

k8s-ci-robot merged commit 21df24c into kubernetes:master Nov 13, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 13, 2019

RainbowMango mentioned this pull request Nov 14, 2019

Allow show hidden metrics #85270

Closed

5 tasks

RainbowMango mentioned this pull request Nov 27, 2019

Automated cherry pick of #85444: Provided a mechanism to re-register hidden metrics. #85627

Merged

RainbowMango deleted the pr_add_metrics_flag_to_apiserver branch May 30, 2020 09:04

Allow show hidden metrics in kube-apiserver #84292

Allow show hidden metrics in kube-apiserver #84292

Conversation

RainbowMango commented Oct 24, 2019 • edited

fedebongio commented Oct 24, 2019

logicalhan left a comment

Choose a reason for hiding this comment

ehashman commented Oct 24, 2019

ehashman commented Oct 24, 2019

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

logicalhan commented Oct 24, 2019

ehashman commented Oct 24, 2019

RainbowMango commented Oct 25, 2019

RainbowMango commented Oct 25, 2019

logicalhan commented Oct 25, 2019

logicalhan commented Oct 25, 2019

RainbowMango commented Oct 25, 2019

logicalhan commented Oct 25, 2019

RainbowMango commented Oct 25, 2019

RainbowMango commented Nov 11, 2019

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

RainbowMango commented Nov 12, 2019

lavalamp commented Nov 12, 2019

liggitt commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Nov 12, 2019

RainbowMango commented Nov 13, 2019

brancz commented Nov 13, 2019

Choose a reason for hiding this comment

lavalamp commented Nov 13, 2019

k8s-ci-robot commented Nov 13, 2019

RainbowMango commented Oct 24, 2019 •

edited