Enable ExperimentalCriticalPodAnnotation feature gate

[a-chernykh]

Otherwise, it is possible that critical system components will be evicted

#3194
kubernetes/kubernetes#51432

[k8s-ci-robot]

Hi @andreychernih. 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.

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

[a-chernykh]

/assign @justinsb

[resouer]

/ok-to-test

[resouer]

If you enabled a experimental feature, it worth to document it somewhere.

[a-chernykh]

/ping @justinsb

[justinsb]

@andreychernih thanks for fixing... this puts us in a messy situation; we don't really want to get into the habit of enabling experimental features by default because they are (by definition) experimental. But in this case it seems that the behaviour when we don't have the flag on is pretty pathological.

One option might be to enable it on k8s 1.8 clusters, for example, and document that, and then hopefully this will eventually become a non-experimental feature.

But this is bad enough that I think it's worth forcing, as you have done. I think at this point we'll cut kops 1.8-alpha as our next release, and then we can just document this as a potentially breaking change in kops 1.8 (which will work for earlier k8s versions also).

I think we need to make sure of a few things:

• That the user can still set this to false and we won't override that
• That the user can still set other featureGates
• That this featureGate doesn't become part of the manifest files that users edit (which I believe is the case)

I think this is achieved by tweaking the logic to be:

if clusterSpec.Kubelet.FeatureGates == nil {
clusterSpec.Kubelet.FeatureGates = make(map[string]string)
}
if _, found := clusterSpec.Kubelet.FeatureGates["ExperimentalCriticalPodAnnotation"]; !found {
clusterSpec.Kubelet.FeatureGates["ExperimentalCriticalPodAnnotation"] = "true"
}

Technically this was only introduced in 1.5.2 IIUC, but people running 1.5 should be running later revisions anyway. I'm not sure whether we want to skip this on 1.4 - again nobody should be running it, but we don't want to break anyone.

But I can try this out and send a PR if you don't get there first!

@andreychernih you are probably very aware of the trade-offs here - do you agree with this analysis?

We have some 1.8 release notes WIP here: https://github.com/kubernetes/kops/blob/master/docs/releases/1.8-NOTES.md

[a-chernykh]

@justinsb yeah, this makes sense, thanks for looking. I've submitted the changes, can you give it another look, please?

[justinsb]

Ah - so this is annoying...

The problem is that 1.8.0-alpha.1 is considered to be less than 1.8.0. So 1.8.0-alpha.1 wouldn't match "1.8".

That said, we could just mask out the alpha.1 version before the comparison - I do like this approach!

[justinsb]

/lgtm

I'm going to mask out the alpha-1, as this is actually great and I want this change in the first kops 1.8 alpha :-)

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue. .