OCPBUGS-59311: must-gather: Handle signals properly by tchap · Pull Request #2237 · openshift/oc

tchap · 2026-03-23T09:50:23Z

Since RunLogsContext is available now after rebase, I am finishing this issue.

openshift-ci-robot · 2026-03-23T09:50:31Z

@tchap: This pull request references Jira Issue OCPBUGS-59311, which is invalid:

expected the bug to target either version "4.22." or "openshift-4.22.", but it targets "4.21" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

coderabbitai · 2026-03-23T09:50:33Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 2e8d721d-ff48-4a79-8238-92dbfc56246b

📥 Commits

Reviewing files that changed from the base of the PR and between 5fc4a6b and d10aa25.

📒 Files selected for processing (1)

pkg/cli/admin/mustgather/mustgather.go

Walkthrough

Modified signal handling and context management in the mustgather operation. Introduced OS signal-aware context using signal.NotifyContext to gracefully handle interrupt and termination signals. Refined error handling to suppress logging of context cancellation errors and updated container log retrieval to use context-aware methods.

Changes

Cohort / File(s)	Summary
Signal-aware context management `pkg/cli/admin/mustgather/mustgather.go`	Added `signal.NotifyContext` for `os.Interrupt` and `syscall.SIGTERM` handling in `MustGatherOptions.Run()`. Removed warning message when context is already canceled. Modified `processNextWorkItem()` to suppress logging of `context.Canceled` errors from `getGatherContainerLogs()`. Updated container log retrieval to call `opts.RunLogsContext(ctx)` instead of `opts.RunLogs()`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2026-03-23T09:50:41Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2026-03-23T09:53:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tchap
Once this PR has been reviewed and has the lgtm label, please assign ingvagabund for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tchap · 2026-03-23T10:48:59Z

/jira refresh

openshift-ci-robot · 2026-03-23T10:49:05Z

@tchap: This pull request references Jira Issue OCPBUGS-59311, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.22.0) matches configured target version for branch (4.22.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (yinzhou@redhat.com), skipping review request.

Details

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2026-03-23T14:59:51Z

@tchap: all tests passed!

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

tchap · 2026-03-23T15:18:06Z

/verified by @tchap

openshift-ci-robot · 2026-03-23T15:18:21Z

@tchap: This PR has been marked as verified by @tchap.

Details

In response to this:

/verified by @tchap

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 openshift-eng/jira-lifecycle-plugin repository.

tchap · 2026-03-23T15:18:42Z

pkg/cli/admin/mustgather/mustgather.go

 	}
 	// stream gather container logs
 	if err := o.getGatherContainerLogs(ctx, pod); err != nil {
-		log("gather logs unavailable: %v", err)


I found this message extra when sending SIGINT.

ardaguclu · 2026-03-23T15:30:00Z

/hold
K8s bump is reverted

ardaguclu · 2026-03-24T19:52:19Z

Once #2241 merges, this PR can be ready for review

ardaguclu · 2026-03-24T19:54:10Z

pkg/cli/admin/mustgather/mustgather.go

 	defer func() {
 		// Shortcircuit this in case the context is already cancelled.
 		if ctx.Err() != nil {
-			klog.Warning("Reprinting cluster state skipped, terminating...")


Is this removal necessary?

It's not necessary, but when you actually send an interrupt, this message doesn't feel like it adds any value.

Is it possible that we check the error type in ctx.Err and ignore this log only when user interrupts?

ctx.Err is not gonna give enough information, it's canceled on a signal and there is no other way it can be canceled. If you want to particularly skip on os.Interrupt, it can be done.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 23, 2026

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2026

must-gather: Handle signals properly

d10aa25

tchap force-pushed the must-gather-handle-signals branch from 089ce25 to d10aa25 Compare March 23, 2026 10:41

tchap marked this pull request as ready for review March 23, 2026 10:42

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2026

openshift-ci bot requested review from ardaguclu and ingvagabund March 23, 2026 10:43

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 23, 2026

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Mar 23, 2026

tchap commented Mar 23, 2026

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2026

ardaguclu reviewed Mar 24, 2026

View reviewed changes

Conversation

tchap commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 23, 2026

Uh oh!

coderabbitai bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

openshift-ci bot commented Mar 23, 2026

Uh oh!

openshift-ci bot commented Mar 23, 2026

Uh oh!

tchap commented Mar 23, 2026

Uh oh!

openshift-ci-robot commented Mar 23, 2026

Uh oh!

openshift-ci bot commented Mar 23, 2026

Uh oh!

tchap commented Mar 23, 2026

Uh oh!

openshift-ci-robot commented Mar 23, 2026

Uh oh!

tchap Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented Mar 23, 2026

Uh oh!

ardaguclu commented Mar 24, 2026

Uh oh!

ardaguclu Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

tchap Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

ardaguclu Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

tchap Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tchap commented Mar 23, 2026 •

edited

Loading

coderabbitai bot commented Mar 23, 2026 •

edited

Loading