Add support for enable-cadvisor-json-endpoints with Kubelet

[adrianmoisey]

Kubernetes 1.18 disables this by default.
For backwards compatibility with monitoring tools, it would be nice to
be able to re-enable this.

kubernetes/kubernetes#68522

[k8s-ci-robot]

Hi @adrianmoisey. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[adrianmoisey]

/assign @KashifSaadat

[hakman]

/ok-to-test

[adrianmoisey]

@hakman I can't tell if the failed tests are related to this change. Is there anything I need to do?

[olemarkus]

This flag will be removed in 1.21. We need to consider if this is a good time to add support for that flag.
See kubernetes/kubernetes#68522

[adrianmoisey]

This flag will be removed in 1.21. We need to consider if this is a good time to add support for that flag.

This is something I thought about. Our situation is that we're currently upgrading from Kubernetes 1.17 to Kubernetes 1.18. Our monitoring service (datadog) doesn't have a workaround for the disabled enable-cadvisor-json-endpoints yes, so we're forced to enable this to get some metrics.

I do realise that it's a bit strange adding a deprecated flag currently. I'm unsure what kOps' policy on this is.

[hakman]

Maybe we could add it to 1.20 only?

[olemarkus]

Maybe we could add it to 1.20 only?

If we add this, we should validate the k8s version (i.e from 1.18 up to and including 1.20), but we can put this in any kOps version supported these k8s version.

[adrianmoisey]

For my use-case I'd need it in 1.18.

Is there any other way of manipulating the command line arguments that get sent to kubelet?

[olemarkus]

I think the approach in the PR is fine. Just need that validation 0

[adrianmoisey]

Great! Anything I need to do?

Also, who is responsible for cherry-picking it back into 1.18 and 1.19? Could I do that?

[hakman]

You would have to add something similar to:

kops/pkg/apis/kops/validation/validation.go

Lines 513 to 518 in 9d8dec6

	if k.TopologyManagerPolicy != "" {
	allErrs = append(allErrs, IsValidValue(kubeletPath.Child("topologyManagerPolicy"), &k.TopologyManagerPolicy, []string{"none", "best-effort", "restricted", "single-numa-node"})...)
	if !c.IsKubernetesGTE("1.18") {
	allErrs = append(allErrs, field.Forbidden(kubeletPath.Child("topologyManagerPolicy"), "topologyManagerPolicy requires at least Kubernetes 1.18"))
	}
	}

Regarding cherry-picking, we will take care of that at the end, but it will be to 1.20 and possibly 1.19, but for sure not 1.18.

[adrianmoisey]

You would have to add something similar to:

Done this.

I was unsure of the values at the end of the line in https://github.com/kubernetes/kops/pull/10957/files#diff-ae412ac68b83570fd50e9d5d63873f060eb8f503f5e44be78d710076294c3285R521

Is that correct?

[hakman]

Is that correct?

No need for that, just the version check(s).

[adrianmoisey]

Is there anything else needed from me?

[hakman]

Please also squash all commits.

[adrianmoisey]

Done

[hakman]

Sorry, my mistake. Please change and squash one more time.

Suggested change

	if c.IsKubernetesGTE("1.18") && c.IsKubernetesLT("1.21") {
	allErrs = append(allErrs, field.Forbidden(kubeletPath.Child("enableCadvisorJsonEndpoints"), "enableCadvisorJsonEndpoints requires at least Kubernetes 1.18"))
	if c.IsKubernetesLT("1.18") && c.IsKubernetesGTE("1.21") {
	allErrs = append(allErrs, field.Forbidden(kubeletPath.Child("enableCadvisorJsonEndpoints"), "enableCadvisorJsonEndpoints requires at Kubernetes 1.18-1.20"))

[adrianmoisey]

Sorry, my mistake. Please change and squash one more time.

No worries, I should have caught that.

Fixed

[k8s-ci-robot]

@adrianmoisey: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kops-verify-staticcheck	253b91f	link	/test pull-kops-verify-staticcheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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:

• OWNERS [hakman]

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

[hakman]

I think this was it. Thanks @adrianmoisey.

[adrianmoisey]

Perfect! Thanks for the help! I really appreciate it.