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.

Sign up for GitHub

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

Jump to bottom

Balk at using Kubernetes versions too new to be supported #8700

Merged
merged 2 commits into from
Apr 5, 2020
Merged

Balk at using Kubernetes versions too new to be supported #8700

merged 2 commits into from
Apr 5, 2020

Conversation

johngmyers
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 9, 2020
@johngmyers
Copy link
Member Author

/retest

@johngmyers
Copy link
Member Author

e2e test is failing because it is trying to use a k8s 1.19 alpha, which the added code prevents.

@johngmyers johngmyers mentioned this pull request Mar 9, 2020
Merged
justinsb
justinsb reviewed Mar 10, 2020
View reviewed changes
upup/pkg/fi/cloudup/apply_cluster.go
tooNewVersion.Minor += 1
tooNewVersion.Pre = nil
tooNewVersion.Build = nil
if util.IsKubernetesGTE(tooNewVersion.String(), *parsed) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder about GT vs GTE. I think this means that e.g. kops 1.18 won't be able to run k8s 1.19 alpha.1 ?

It's probably fine but you know me - super careful!

Shall we discuss in office hours?

Copy link
Member Author

@johngmyers johngmyers Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I intended for kops 1.18 to balk at running k8s 1.19.0-alpha.1 (unless the environment variable was set)

And it won't, thus the e2e failure.

Copy link
Member

@justinsb justinsb Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I saw that after I commented :-)

It does mean that whatever tests we're still running against k8s HEAD will break at some random point in the cycle when k/k cuts the next alpha. OTOH that might be a good prompt for us to cut the branch for the next release - though it is typically pretty early in the cycle meaning we'd have to do a lot of cherry picks.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW this test-infra PR will fix the prow tests: https://github.com/kubernetes/test-infra/pull/16678/files

@justinsb
Copy link
Member

Another option we could do is to set these guidelines in the channel file (channels/stable). Though I do agree this is a nice way to do it - thanks for starting us off here!

@johngmyers
Copy link
Member Author

Would we expect to change this guideline post-release? If not, I don't see the benefit of setting this in the channel file.

@hakman
Copy link
Member

hakman commented Mar 10, 2020

Any special reason this is desired? I would rather not have this behaviour at all, especially not another env var. Anyone that chooses a newer Kubernetes version does this already at its own risk by setting it manually in the cluster spec.

@johngmyers
Copy link
Member Author

I've seen several issues filed by users who were using older versions of kops than the Kubernetes they were running, with no idea that wasn't supported. It would be to the users' benefit if kops declined to perform actions that are so risky to merit an explicit disclaimer of support.

@hakman
Copy link
Member

hakman commented Mar 10, 2020

We have to find a balance between users not reading any docs and develoment pain. Having env vars required to test newer k8s versions is not pleasant. I think a good balance would be to keep only the warning.

@johngmyers
Copy link
Member Author

Setting an environment variable is easy, particularly compared to having to host your own assets. I believe this is a case where the needs of the users outweigh convenience to developers. The environment variable override is the pattern established by the code handling k8s versions marked as obsolete in the channel.

@hakman
Copy link
Member

hakman commented Mar 10, 2020

The environment variable override is the pattern established by the code handling k8s versions marked as obsolete in the channel.

This is an old piece of code that will probably be replaced by the new approach of not supporting old releases in 1.19 or 1.20.

I think this is something on which we will disagree. From my point of view, env vars are unfriendly. Even the message is annoying to see every time you run a command.

This is not a tool for the casual user. If someone doesn't spend the time to even check the compatibility matrix, there is a very slim chance that it will be able to host its own cluster.
There will be similar questions when the user will not read the release notes and so on.

@olemarkus
Copy link
Member

FWIW the place I have seen problems here is where one admin forgets to update kops locally and accidentally causes problems. Say what you want about such mistakes, but it does happen.

@hakman
Copy link
Member

hakman commented Mar 13, 2020

FWIW the place I have seen problems here is where one admin forgets to update kops locally and accidentally causes problems. Say what you want about such mistakes, but it does happen.

OK. Can we do it in such a way that it only applies to official releases?

@johngmyers
Copy link
Member Author

johngmyers commented Mar 13, 2020 •
edited
Loading

Heh, our wrapper around kops puts its version in an annotation on the cluster spec. It then requires a flag to run an older version on that cluster.

@johngmyers
Copy link
Member Author

/retest

@rifelpet
Copy link
Member

rifelpet commented Apr 5, 2020

we decided during last office hours that this is good to merge. developers can set the env var in their shell, and CI jobs will do the same according to the test-infra PR linked above.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, rifelpet

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit 502aaac into kubernetes:master Apr 5, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Apr 5, 2020
@johngmyers johngmyers deleted the version-tweaks branch April 5, 2020 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@rifelpet rifelpet rifelpet left review comments

@joshbranham joshbranham Awaiting requested review from joshbranham

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@rifelpet rifelpet

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.18
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@johngmyers @justinsb @hakman @olemarkus @rifelpet @k8s-ci-robot