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

kubeadm: Fix upgrade plan for air-gapped setups #94421

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Sep 2, 2020

What type of PR is this?
/kind bug
/kind regression

What this PR does / why we need it:

A bug was discovered in the enforceRequirements func for upgrade plan.
If a command line argument that specifies the target Kubernetes version is
supplied, the returned ClusterConfiguration by enforceRequirements will
have its KubernetesVersion field set to the new version.
If no version was specified, the returned KubernetesVersion points to the
currently installed one.

This remained undetected for a couple of reasons

  • It's only upgrade plan that allows for the version command line argument to
    be optional (in upgrade plan it's mandatory)
  • Prior to 1.19, the implementation of upgrade plan did not make use of the
    KubernetesVersion returned by enforceRequirements.

upgrade plan supports this optional command line argument to enable
air-gapped setups (as not specifying a version on the command line will end up
looking for the latest version over the Interned).

Hence, the only option is to make enforceRequirements consistent in the
upgrade plan case and always return the currently installed version in the
KubernetesVersion field.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2274

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-soon
/assign @fabriziopandini @neolit123

Does this PR introduce a user-facing change?:

Fix a regression in 1.19 where kubeadm bails out with a fatal error when an optional version command line argument is supplied to the "kubeadm upgrade plan" command

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


A bug was discovered in the `enforceRequirements` func for `upgrade plan`.
If a command line argument that specifies the target Kubernetes version is
supplied, the returned `ClusterConfiguration` by `enforceRequirements` will
have its `KubernetesVersion` field set to the new version.
If no version was specified, the returned `KubernetesVersion` points to the
currently installed one.

This remained undetected for a couple of reasons
- It's only `upgrade plan` that allows for the version command line argument to
  be optional (in `upgrade plan` it's mandatory)
- Prior to 1.19, the implementation of `upgrade plan` did not make use of the
  `KubernetesVersion` returned by `enforceRequirements`.

`upgrade plan` supports this optional command line argument to enable
air-gapped setups (as not specifying a version on the command line will end up
looking for the latest version over the Interned).

Hence, the only option is to make `enforceRequirements` consistent in the
`upgrade plan` case and always return the currently installed version in the
`KubernetesVersion` field.

Signed-off-by: Rostislav M. Georgiev <[email protected]>
@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 2, 2020
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/regression Categorizes issue or PR as related to a regression from a prior release. area/kubeadm priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 2, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rosti

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 Sep 2, 2020
// This also makes the KubernetesVersion value returned for `upgrade plan` consistent as that command
// allows to not specify a target version in which case KubernetesVersion will always hold the currently
// installed one.
cfg.KubernetesVersion = newK8sVersion
Copy link
Member

@neolit123 neolit123 Sep 2, 2020

Choose a reason for hiding this comment

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

Just to double check. After this change, if the user passes to plan 1.19.2, while the cluster version is at 1.19.1, would the table for control plane component upgrade still print .2 as the planned upgrade target in a case where the latest stable is not 1.19.2 (eg newer than that)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The tables are generated using the version getter (initialized at the return of this func with the value of newK8sVersion).

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks!

@neolit123
Copy link
Member

/lgtm
Do you have time to cherry pick for 1.19?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@neolit123
Copy link
Member

/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Sep 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0b92e8b into kubernetes:master Sep 3, 2020
k8s-ci-robot added a commit that referenced this pull request Sep 4, 2020
…upstream-release-1.19

Automated cherry pick of #94421: kubeadm: Fix `upgrade plan` for air-gapped setups
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. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade plan with a version has stopped working on 1.19.0
5 participants