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

Improve wording in kubeadm upgrade plan #98728

Merged
merged 1 commit into from Feb 3, 2021
Merged

Improve wording in kubeadm upgrade plan #98728

merged 1 commit into from Feb 3, 2021

Conversation

stmcginnis
Copy link
Contributor

@stmcginnis stmcginnis commented Feb 3, 2021 •
edited

Originally raised as an issue with invalid versions to plan, but it has
been determined with air gapped environments and development versions it
is not possible to fully address that issue.

But one thing that was identified was that we can do a better job in how
we output the upgrade plan information. Kubeadm outputs the requested
version as "Latest stable version", though that may not actually be the
case. For this instance, we want to change this to "Target version" to
be a little more accurate.

Then in the component upgrade table that is emitted, the last column of
AVAILABLE isn't quite right either. Also changing this to TARGET to
reflect that this is the version we are targeting to upgrade to,
regardless of its availability.

There could be some improvements in checking available versions,
particularly in air gapped environments, to make sure we actually have
access to the requested version. But this at least clarifies some of the
output a bit.

Fixes kubernetes/kubeadm#2311

/kind bug
/kind cleanup

kubeadm: Some text in the `kubeadm upgrade plan` output has changed. If you have scripts or other automation that parses this output, please review these changes and update your scripts to account for the new output.

@stmcginnis
Improve wording in kubeadm upgrade plan
874ceb0 
Originally raised as an issue with invalid versions to plan, but it has
been determined with air gapped environments and development versions it
is not possible to fully address that issue.

But one thing that was identified was that we can do a better job in how
we output the upgrade plan information. Kubeadm outputs the requested
version as "Latest stable version", though that may not actually be the
case. For this instance, we want to change this to "Target version" to
be a little more accurate.

Then in the component upgrade table that is emitted, the last column of
AVAILABLE isn't quite right either. Also changing this to TARGET to
reflect that this is the version we are targetting to upgrade to,
regardless of its availability.

There could be some improvements in checking available versions,
particularly in air gapped environments, to make sure we actually have
access to the requested version. But this at least clarifies some of the
output a bit.

Signed-off-by: Sean McGinnis <sean.mcginnis@gmail.com>
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 3, 2021
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 3, 2021
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 3, 2021
@neolit123
Copy link
Member

thanks for the PR.
it seems to contain the changes i suggested on the issue, so it LGTM.

Some text in the kubeadm upgrade plan output has changed. If you have scripts or other automation that parses this output, please review these changes and update your scripts to account for the new output.

the release note seems fine, but please prefix it with kubeadm: Some text...

as a side note we really give no guarantees when this output changes.
the PR that wanted to add machine readable output for plan got stale:
#83941

/ok-to-test
/priority backlog
/triage accepted

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 3, 2021
@stmcginnis
Copy link
Contributor Author

Thanks @neolit123, release note text updated. Guess I should have thought of that.

"as a side note we really give no guarantees when this output changes."

Yeah, that makes a lot of sense. Just thought it might be useful to point it out just in case it might help anyone. But I agree, parsing command line output should be avoided in most cases. ;)

@neolit123
Copy link
Member

pull-kubernetes-bazel-test — Job failed.

looks like a flake in another package.

/retest

@stmcginnis
Copy link
Contributor Author

Looks like it hit #98697

@neolit123
Copy link
Member

/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 Feb 3, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, stmcginnis

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 Feb 3, 2021
@k8s-ci-robot k8s-ci-robot merged commit c884bf1 into kubernetes:master Feb 3, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Feb 3, 2021
@melodychn
Copy link

@stmcginnis hello from 1.21 release notes team!

If you have scripts or other automation that parses this output, please review these changes and update your scripts to account for the new output.

Can you clarify that this requires users / operators to take action / warrants a sport in "Urgent upgrade notes"? If so, please prepend the release note with [ACTION REQUIRED], otherwise confirm that users / operators do not need to take action when upgrading to 1.21.

Thank you! (CC: @wilsonehusin)

@neolit123
Copy link
Member

hi @melodychn i'd say "action required" is not needed here.
the output of the command that the PR changes, should not be parsed and this is unsupported.

the sentence is for users that are doing this unsupported action.

@stmcginnis stmcginnis deleted the kubeadm-plan branch February 12, 2021 21:58
@stmcginnis
Copy link
Contributor Author

Exactly as @neolit123 says. No action required. It's really just an informational message in case anyone has been relying on something they shouldn't be.

@CaoDonghui123 CaoDonghui123 mentioned this pull request Feb 19, 2021
Merged
@chokhareganesh
Copy link

to upgrade to exact version try with providing version at end
kubeadm upgrade plan exact-version

kubeadm upgrade plan 1.28.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothysc timothysc Awaiting requested review from timothysc

@yagonobre yagonobre Awaiting requested review from yagonobre

Assignees

@neolit123 neolit123

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

kubeadm upgrade plan allows invalid versions
5 participants
@stmcginnis @k8s-ci-robot @neolit123 @melodychn @chokhareganesh