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

fix kubelet flushlogs not working together with exit #104774

Merged
merged 2 commits into from Jan 6, 2022
Merged

fix kubelet flushlogs not working together with exit #104774

merged 2 commits into from Jan 6, 2022

Conversation

kerthcet
Copy link
Member

@kerthcet kerthcet commented Sep 5, 2021 •
edited

Signed-off-by: kerthcet kerthcet@gmail.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubelet flushlogs not working together with exit

Which issue(s) this PR fixes:

Part of #102231

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix failed flushing logs in defer function when kubelet cmd exit 1.

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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 5, 2021
@kerthcet
Copy link
Member Author

kerthcet commented Sep 5, 2021

/ok-to-test

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 5, 2021
@kerthcet
Copy link
Member Author

kerthcet commented Sep 5, 2021

/retest

@endocrimes
Copy link
Member

/triage accepted
/priority important-longterm

@kerthcet Might be worth adding a release note entry as this might be useful to folks debugging deployments.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. 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 7, 2021
@endocrimes endocrimes added this to Needs Approver in SIG Node PR Triage Sep 7, 2021
@endocrimes
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added 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. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 7, 2021
sanposhiho
sanposhiho reviewed Sep 8, 2021
View reviewed changes
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

Thanks for helping the issue!
Also, in this PR, please check the below point.

#104775 (comment)

On this PR, you have to consider, for example, this point
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubelet/app/server.go#L160

@kerthcet kerthcet force-pushed the bug/fix-kubelet-log-defer-conflict-with-exit branch from d803761 to e7ebe9e Compare September 9, 2021 05:47
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 9, 2021
@ehashman ehashman moved this from Needs Approver to Needs Reviewer in SIG Node PR Triage Oct 6, 2021
@kerthcet
Copy link
Member Author

kerthcet commented Oct 8, 2021

kindly ping @endocrimes , @sanposhiho

@sanposhiho
Copy link
Member

Sorry for the late reply. LGTM :)
/lgtm

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

kindly ping @pacoxu

@pacoxu
Copy link
Member

pacoxu commented Oct 28, 2021

/assign @mrunalp

for approval

@ehashman ehashman moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Nov 1, 2021
@kerthcet
fix kubelet flushlogs not working together with os.Exit()
9600e8d 
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet kerthcet force-pushed the bug/fix-kubelet-log-defer-conflict-with-exit branch from beef8c3 to 9600e8d Compare November 16, 2021 05:54
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2021
@kerthcet
Copy link
Member Author

No changes applied, just rebase the master branch.
/remove-kind bug
/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Nov 16, 2021
@ehashman
Copy link
Member

/remove-kind feature
/kind cleanup

ehashman
ehashman reviewed Nov 29, 2021
View reviewed changes
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

Please update your first comment, this does not fix entirely fix the linked issue and GitHub will auto-close it if this merges.

cmd/kubelet/app/server.go
@@ -152,31 +153,25 @@ HTTP server: The kubelet can also listen for HTTP and respond to a simple API
// DisableFlagParsing=true provides the full set of flags passed to the kubelet in the
// `args` arg to Run, without Cobra's interference.
DisableFlagParsing: true,
Run: func(cmd *cobra.Command, args []string) {
RunE: func(cmd *cobra.Command, args []string) error {
Copy link
Member

Choose a reason for hiding this comment

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

https://pkg.go.dev/github.com/spf13/cobra#Command.RunE

cmd/kubelet/app/server.go
Comment on lines -159 to -160
cmd.Usage()
os.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

After this change we no longer print usage info. I suspect this is okay because the kubelet usage info is massively long and makes it hard to see the error, but we should note this in the release note.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will still print usage info here

cmd/kubelet/app/server.go Outdated Show resolved Hide resolved
@ehashman ehashman moved this from Needs Approver to Waiting on Author in SIG Node PR Triage Nov 29, 2021
@kerthcet
prefer %w to unwrap errors
185e0a0 
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet
Copy link
Member Author

/retest

@kerthcet
Copy link
Member Author

kindly ping @mrunalp

@dims
Copy link
Member

dims commented Jan 5, 2022

/assign @sjenning @klueska

NOTE: there are 3 people in favor of this change (lgtm got lost!)

@sjenning
Copy link
Contributor

sjenning commented Jan 6, 2022

/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 Jan 6, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kerthcet, pohly, sjenning

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 Jan 6, 2022
@ehashman ehashman moved this from Waiting on Author to Done in SIG Node PR Triage Jan 6, 2022
@k8s-ci-robot k8s-ci-robot merged commit b1c204a into kubernetes:master Jan 6, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Jan 6, 2022
@kerthcet kerthcet deleted the bug/fix-kubelet-log-defer-conflict-with-exit branch January 7, 2022 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp mrunalp left review comments

@pacoxu pacoxu pacoxu left review comments

@ehashman ehashman ehashman left review comments

@sanposhiho sanposhiho sanposhiho left review comments

@pohly pohly pohly approved these changes

@vishh vishh Awaiting requested review from vishh

@yifan-gu yifan-gu Awaiting requested review from yifan-gu

Assignees

@mrunalp mrunalp

@klueska klueska

@sjenning sjenning

@endocrimes endocrimes

@pacoxu pacoxu

@sanposhiho sanposhiho

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 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
SIG Node PR Triage
Done
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

None yet