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.

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

COSI-55: improve graceful shutdown in main.go #84

Merged

Conversation

anurag4DSB
Copy link
Collaborator

@anurag4DSB anurag4DSB commented Jan 2, 2025

Enhanced signal handling for SIGINT/SIGTERM to allow for a graceful shutdown process. Added timeout-based force exit to prevent indefinite waiting during shutdown scenarios.

note to reviewers: Unit test suite is needed for main package, for now relying on e2e tests.

@anurag4DSB anurag4DSB requested a review from fredmnl January 2, 2025 12:08
@anurag4DSB anurag4DSB marked this pull request as ready for review January 2, 2025 12:08
Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.67%. Comparing base (2c61eca) to head (49ea6c2).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

Components Coverage Δ
🏠 Main Package ∅ <ø> (∅)
🚗 Driver Package 92.22% <ø> (ø)
📡 gRPC Factory Package 86.25% <ø> (ø)
🔐 IAM Client Package 100.00% <ø> (ø)
🌐 S3 Client Package 100.00% <ø> (ø)
🔧 Util Package 100.00% <ø> (ø)
🔖 Constants Package ∅ <ø> (∅)
@@           Coverage Diff           @@
##             main      #84   +/-   ##
=======================================
  Coverage   93.67%   93.67%           
=======================================
  Files          10       10           
  Lines         680      680           
=======================================
  Hits          637      637           
  Misses         37       37           
  Partials        6        6           

case <-ctx.Done():
klog.InfoS("Scality COSI driver shutdown initiated successfully, context canceled")
case sig = <-sigs:
klog.ErrorS(nil, "Initiating graceful shutdown, repeat signal to force shutdown", "type", sig)
Copy link

@fredmnl fredmnl Jan 2, 2025

Choose a reason for hiding this comment

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

Suggested change
klog.ErrorS(nil, "Initiating graceful shutdown, repeat signal to force shutdown", "type", sig)
klog.ErrorS(nil, "Forcing shutdown due to repeated signal", "type", sig)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we discusssed here that it will be that error meaage
I might be understanding wrong
@fredmnl

Copy link

Choose a reason for hiding this comment

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

🤔 The same line of log is printed twice here. I agree with the first one which is when one signal is sent Initiating graceful shutdown, repeat signal to force shutdown. The second one should say that we received a second signal and that we will exit forcefully. What do you think?

Copy link

Choose a reason for hiding this comment

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

👍
Also I would change Force to Forcing in both messages.

Copy link

Choose a reason for hiding this comment

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

Yep, good point!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After thinking more, I took your suggestions and made the change. I want to keep the log messages in past tense where possible.
Let me know if anyone is not good with this.

This is the change I made:

klog.InfoS("Graceful shutdown initiated; repeat signal to force immediate shutdown")
		select {
		case sig = <-sigs:
			klog.ErrorS(nil, "Second signal received—forcing shutdown", "type", sig)
			os.Exit(1)
		case <-time.After(30 * time.Second):
			klog.ErrorS(nil, "Forcing shutdown due to timeout", "timeout", 30*time.Second)
			os.Exit(1)
		}

Copy link

Choose a reason for hiding this comment

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

LGTM

case <-ctx.Done():
klog.InfoS("Scality COSI driver shutdown initiated successfully, context canceled")
case sig = <-sigs:
klog.ErrorS(nil, "Initiating graceful shutdown, repeat signal to force shutdown", "type", sig)
Copy link

Choose a reason for hiding this comment

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

👍
Also I would change Force to Forcing in both messages.

Enhanced signal handling for SIGINT/SIGTERM to allow for a
graceful shutdown process. Added timeout-based force exit
to prevent indefinite waiting during shutdown scenarios.
@anurag4DSB anurag4DSB force-pushed the improvement/COSI-55-refactor-main.go-signals-for-shutdown branch from de1933e to 49ea6c2 Compare January 7, 2025 17:24
@anurag4DSB anurag4DSB merged commit 90be3ed into main Jan 7, 2025
8 checks passed
@anurag4DSB anurag4DSB deleted the improvement/COSI-55-refactor-main.go-signals-for-shutdown branch January 7, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants