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

kubeadm: better error handling for unknown phases and commands #127096

Merged
merged 1 commit into from
Sep 10, 2024
Merged

kubeadm: better error handling for unknown phases and commands #127096

merged 1 commit into from
Sep 10, 2024

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Sep 3, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

If an unknown command or a phase is called consistently
return the same error.

If a command that has subcommands is called
return an error.

To achieve the above add a new util function
RequireSubcommand() that sets NoArgs and RunE for
regular commands or a "phase" command.

Remove MacroCommandLongDescription and just return an
error that a subcommand is required from the phase runner.

Fix minor comments capitalization.

Perform other minor fixes in util/error.go.

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#3093 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: if an unknown command name is passed to any parent command such as 'kubeadm init phase' return an error. If 'kubeadm init phase' or another command that has subcommands is called without subcommand name, print the available commands and also return an error.

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

@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/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 3, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Sep 3, 2024
@neolit123
Copy link
Member Author

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 3, 2024
neolit123
neolit123 commented Sep 3, 2024
View reviewed changes
cmd/kubeadm/app/cmd/phases/workflow/runner.go Outdated
Short: fmt.Sprintf("Use this command to invoke single phase of the %s workflow", cmd.Name()),
Args: cobra.NoArgs,
Copy link
Member Author

Choose a reason for hiding this comment

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

this tells the cmd that anything after e.g. kubeadm init phase ... is not an arg but a subcommand.

neolit123
neolit123 commented Sep 3, 2024
View reviewed changes
cmd/kubeadm/app/cmd/phases/workflow/runner.go Outdated
Short: fmt.Sprintf("Use this command to invoke single phase of the %s workflow", cmd.Name()),
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
return errors.Errorf("%s\n\nphase name is required", e.Help(phaseCommandUse))
Copy link
Member Author

Choose a reason for hiding this comment

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

RunE here just returns and error that has the constructed Help() screen.
it means that one should be calling the parent phase command without a subcommand.

neolit123
neolit123 commented Sep 3, 2024
View reviewed changes
Copy link
Member Author

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

please double check my understanding in case this might break some odd behavior that we have.

/hold for review

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2024
@pacoxu
Copy link
Member

pacoxu commented Sep 4, 2024

A similar case shows a different message, but both looks OK.

[root@node ~]# ./kubeadm config images  list2
invalid subcommand "list2"
See 'kubeadm config images -h' for help and examples

[root@node ~]# ./kubeadm init phase foo
unknown command "foo" for "kubeadm init phase"
To see the stack trace of this error execute with --v=5 or higher

@neolit123
Copy link
Member Author

that's because these is a custom cmdutil.SubCmdRun() function. it's supposed to also print the help screen (-h) but it doesn't work. i can try adding a second commit to this PR that aligns all errors to do the same.

i might drop printing the help screen on unknown commands as that seems hacky.

@pacoxu
Copy link
Member

pacoxu commented Sep 4, 2024

/lgtm

i can try adding a second commit to this PR that aligns all errors to do the same.

Not have to.

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

LGTM label has been added.

Git tree hash: 357739cd7664c5d2f5f3f4e399fdf55b69fcb99e

@carlory
Copy link
Member

carlory commented Sep 4, 2024

LGTM
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2024
@neolit123
Copy link
Member Author

/hold
will address later today or tomorrow

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2024
@neolit123 neolit123 force-pushed the 1.32-fix-unknown-phase-error branch from 851a70f to 5749564 Compare September 5, 2024 16:37
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 5, 2024
@k8s-ci-robot k8s-ci-robot requested a review from pacoxu September 5, 2024 16:37
@neolit123 neolit123 force-pushed the 1.32-fix-unknown-phase-error branch from 5749564 to 1529e5a Compare September 5, 2024 16:37
@neolit123 neolit123 changed the title kubeadm: show error if unknown phase is provided kubeadm: better error handling for unknown phases and commands Sep 5, 2024
@neolit123
Copy link
Member Author

neolit123 commented Sep 5, 2024 •
edited
Loading

@pacoxu @carlory
i rescoped the PR to have better fixes across the board. now it also cleans some tech debt around cmd / error handling and makes everything consistent.

@neolit123
kubeadm: better error handling for unknown phases and commands
b497d28 
If an unknown command or a phase is called consistently
return the same error.

If a command that has subcommands is called
return an error.

To achieve the above add a new util function
RequireSubcommand() that sets NoArgs and RunE for
regular commands or a "phase" command.

Remove MacroCommandLongDescription and just return an
error that a subcommand is required from the phase runner.

Fix minor comments capitalization.

Perform other minor fixes in util/error.go.
@neolit123 neolit123 force-pushed the 1.32-fix-unknown-phase-error branch from 1529e5a to b497d28 Compare September 5, 2024 16:41
neolit123
neolit123 commented Sep 5, 2024
View reviewed changes
cmd/kubeadm/app/cmd/util/documentation.go

// MacroCommandLongDescription provide a standard description for "macro" commands
MacroCommandLongDescription = LongDesc(`
This command is not meant to be run on its own. See list of available subcommands.
Copy link
Member Author

Choose a reason for hiding this comment

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

@SataQiu re: 'upgrade apply' phases, your PR can hopefully merge first and will make this PR to remove also the MacroCommandLongDescription from new phases.

/hold

Copy link
Member Author

Choose a reason for hiding this comment

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

@SataQiu re: 'upgrade apply' phases, your PR can hopefully merge first and will make this PR to remove also the MacroCommandLongDescription from new phases.

/hold

seems no longer needed. the macro description is removed without conflicts.

@neolit123
Copy link
Member Author

neolit123 commented Sep 5, 2024 •
edited
Loading

/retest
reported #127163

@neolit123
Copy link
Member Author

/retest
another flake
#127168

@neolit123 neolit123 mentioned this pull request Sep 9, 2024
Merged
@neolit123
Copy link
Member Author

looking for review/LGTM

SataQiu
SataQiu reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@SataQiu SataQiu left a comment

Choose a reason for hiding this comment

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

/lgtm

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

LGTM label has been added.

Git tree hash: 982c7b55a0914e4346674dbf8893296c31acb5bf

@neolit123
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2024
@k8s-ci-robot k8s-ci-robot merged commit d913914 into kubernetes:master Sep 10, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SataQiu SataQiu SataQiu left review comments

@carlory carlory Awaiting requested review from carlory

@pacoxu pacoxu Awaiting requested review from pacoxu

Assignees

@pacoxu pacoxu

@SataQiu SataQiu

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/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.32
Development

Successfully merging this pull request may close these issues.
5 participants
@neolit123 @k8s-ci-robot @pacoxu @carlory @SataQiu