Skip to content
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

Add HA feature gate and minVersion validation #54539

Merged

Conversation

jamiehannaford
Copy link
Contributor

@jamiehannaford jamiehannaford commented Oct 25, 2017

What this PR does / why we need it:

As we add more feature gates, there might be occasions where a feature is only available on newer releases of K8s. If a user makes a mistake, we should notify them as soon as possible in the init procedure and not them go down the path of hard-to-debug component issues.

Specifically with HA, we ideally need the new TaintNodesByCondition (added in v1.8.0 but working in v1.9.0).

Which issue this PR fixes:

kubernetes/kubeadm#261
kubernetes/kubeadm#277

Release note:

Feature gates now check minimum versions

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews @luxas @timothysc

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 25, 2017
@jamiehannaford
Copy link
Contributor Author

/test pull-kubernetes-unit

Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM but some nits.
Interested in why you cut that dep... seems useful to have by me

@@ -22,22 +22,66 @@ import (
"strconv"
"strings"

utilfeature "k8s.io/apiserver/pkg/util/feature"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this dep?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I asked that myself. But we're not actually using that much from it, just a few string types, so it doesn't add much value TBH. If we embedded those structs, we'd need to start doing stuff like

HighAvailability:    {utilfeature.FeatureSpec{Default: false, PreRelease: alpha}, MinimumVersion: v190},

which looks a bit weird imo.

Copy link
Contributor Author

@jamiehannaford jamiehannaford Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really mind either way, we can keep on using it if somebody has a strong preference

Copy link
Member

@luxas luxas Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to stay close in sync as possible with upstream, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will add back those types. However I don't think there is consistent tooling around this for "upstream". Here we're just cribbing off what apiserver does.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, can be generally improved indeed, yes

)

var v190, _ = version.ParseSemantic("v1.9.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use MustParseSemantic

if requestedVersion == "" {
return nil
}
parsedExpVersion, err := version.ParseGeneric(requestedVersion)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParseSemantic

@jamiehannaford
Copy link
Contributor Author

/test pull-kubernetes-unit

@jamiehannaford
Copy link
Contributor Author

/test pull-kubernetes-unit

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2017
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

You might wanna fix the nit though, otherwise we can't test HA until it's out :)

)

var v190 = version.MustParseSemantic("v1.9.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh that should probably be v1.9.0-alpha.1 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luxas Yep, I'll update this in a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks!

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jamiehannaford, luxas

Associated issue: 261

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

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

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 54593, 54607, 54539, 54105). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 633ca56 into kubernetes:master Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants