NO-JIRA: Handle SIGTERM in all of the commands - #6710
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@smrtrfszm: This pull request explicitly references no jira issue. DetailsIn 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. |
WalkthroughReplaced custom OS signal handling with controller-runtime’s SetupSignalHandler to obtain a cancellable context in NewRunCommand. Updated imports accordingly and passed the derived context to opts.run(ctx). Error handling and overall invocation remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Cmd as NewRunCommand
participant CTRL as controller-runtime (SetupSignalHandler)
participant Run as opts.run(ctx)
User->>Cmd: execute command
Cmd->>CTRL: SetupSignalHandler()
CTRL-->>Cmd: context with cancel on SIGTERM/SIGINT
Cmd->>Run: run(ctx)
alt success
Run-->>Cmd: nil
Cmd-->>User: exit 0
else error
Run-->>Cmd: error
Cmd-->>User: exit with error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: smrtrfszm The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @smrtrfszm. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
sync-fg-configmap/update.go (5)
5-19: Reorder imports: stdlib, external, internal (per repo guidelines).cmdutil (internal) is currently grouped before external deps. Reorder to satisfy “stdlib, external, internal”.
import ( - "context" - "fmt" - "log" - "os" - - cmdutil "github.com/openshift/hypershift/cmd/util" - - corev1 "k8s.io/api/core/v1" - - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - "github.com/spf13/cobra" + "context" + "fmt" + "log" + "os" + + corev1 "k8s.io/api/core/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/spf13/cobra" + + cmdutil "github.com/openshift/hypershift/cmd/util" )
48-51: Remove redundant os.Exit after log.Fatal.log.Fatal already calls os.Exit(1); the explicit os.Exit(1) is unreachable.
- if err := opts.run(ctx); err != nil { - log.Fatal(err) - os.Exit(1) - } + if err := opts.run(ctx); err != nil { + log.Fatal(err) + }
45-52: Optional: prefer RunE and return errors instead of exiting.Let Cobra handle error propagation and exit codes; it improves composability and testability.
- cmd.Run = func(cmd *cobra.Command, args []string) { - ctx := ctrl.SetupSignalHandler() - - if err := opts.run(ctx); err != nil { - log.Fatal(err) - os.Exit(1) - } - } + cmd.RunE = func(cmd *cobra.Command, args []string) error { + ctx := ctrl.SetupSignalHandler() + return opts.run(ctx) + }
41-41: Fix flag help typo.“The path path …” → “The path …”.
- cmd.Flags().StringVar(&opts.File, "file", opts.File, "The path path to the file that contains the feature gate YAML to apply.") + cmd.Flags().StringVar(&opts.File, "file", opts.File, "The path to the file that contains the feature gate YAML to apply.")
57-60: Validate required inputs early (namespace).Fail fast if namespace isn’t set (env or flag), avoiding confusing API errors.
func (o *syncFGConfigMapOptions) run(ctx context.Context) error { + if o.Namespace == "" { + return fmt.Errorf("namespace is required (set --namespace or NAMESPACE env var)") + } content, err := os.ReadFile(o.File)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
sync-fg-configmap/update.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/!(*.pb).go
📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)
**/!(*.pb).go: Avoid variable shadowing
Do not over-nest control flow (e.g., nested if or for blocks)
Avoid init() functions unless absolutely necessary
Keep functions small and focused
Prefer composition over inheritance (via embedding)
Use the functional options pattern for constructors where flexibility is needed
Avoid defining interfaces until you need them
Do not return interfaces from constructors or public APIs
Define interfaces on the consumer side, not the producer side
Keep interfaces small and focused (generally 1–2 methods)
Avoid embedding pointer types unless necessary
Don’t overuse getters/setters — prefer public fields when it makes sense
Use value receivers when the method doesn't mutate state or require pointer semantics
Do not use util, common, or similarly vague package names
Avoid package name collisions by using clear, unique names
Do not expose unnecessary symbols (keep exported API minimal)
Distinguish between nil and empty slices
Avoid memory leaks from slicing large arrays
Always check the capacity when copying or appending slices
Preallocate slice capacity when size is known ahead of time
Always initialize maps before use
Check existence with the two-value assignment (val, ok := m[key])
Be aware that ranging over a map is in random order
Always check errors — don’t ignore them
Wrap errors with context when rethrowing
Avoid panics except in truly unrecoverable cases
Use errors.Is and errors.As for error comparison in Go 1.20+
Always defer cancel() when using context.WithCancel
Do not leak goroutines — ensure they exit cleanly
Avoid data races — use mutexes or channels appropriately
Never close a channel from the receiving side
Keep imports grouped and ordered: stdlib, external, internal
Avoid magic numbers — use named constants
Prefer explicit over implicit — especially in exported APIs
Only use generics when they simplify code or add real flexibility
Avoid over-engineering with type parameters
Be cautious with constraint complexity — keep...
Files:
sync-fg-configmap/update.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)
Use
make lint-fixafter writing Go code to automatically fix most linting issuesFollow the rules defined in @100-go-mistakes.mdc for Go code
Files:
sync-fg-configmap/update.go
🔇 Additional comments (2)
sync-fg-configmap/update.go (2)
15-16: Good switch to controller-runtime for signal handling.Using ctrl.SetupSignalHandler standardizes SIGINT/SIGTERM handling and aligns this command with others. LGTM.
46-46: LGTM: context from ctrl.SetupSignalHandler().This ensures cancellation on both SIGINT and SIGTERM without custom signal plumbing.
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
This will fail |
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 8
... and 3 more failed tests |
c5d96ca to
9812fed
Compare
|
/ok-to-test |
|
/retest-required |
Use `ctrl.SetupSignalHandler()` to handle signals. This function returns a context that is canceled on SIGINT and also on SIGTERM. Kubernetes sends a SIGTERM signal to allow graceful shutdowns, so this will allow these commands as well to gracefully shut down. Most of the commands use this already.
9812fed to
a1913cd
Compare
openshift-ci
Bot
added
area/control-plane-operator
smrtrfszm
changed the title
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
|
@smrtrfszm: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
I now have all the evidence needed. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 7 failing CI jobs are caused by a git merge conflict in Root CauseThe PR branch What the PR does to
What changed on
Both sets of changes touch lines adjacent to or overlapping with the PR's modifications, making git unable to auto-merge. The 4 older e2e jobs (build IDs The Note on verify-workflows (build 2045129791597711360): The build log shows it checked out PR #6871 instead of #6710, though its prowjob.json references PR #6710. This build failed with merge conflicts in Recommendations
Evidence
|
|
Rotten PRs close after 7d of inactivity. Reopen the PR by commenting /close |
|
@openshift-ci[bot]: Closed this PR. DetailsIn 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 kubernetes-sigs/prow repository. |
5 participants
What this PR does / why we need it:
Kubernetes sends a SIGTERM on termination, so pods can gracefully shut down. A few commands doesn't handle this.
This PR switches the signal handling to use
ctrl.SetupSignalHandler()everywhere. This function returns a context that is canceled on SIGINT and also on SIGTERM. Most other commands use this.Checklist
Summary by CodeRabbit