TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 5 Projects Releases 1 Activity Actions

node/ir: Handle double SIGHUP correctly #1197

Merged
fyrchik merged 1 commit from elebedeva/frostfs-node:fix/double-sighup into master 2024-06-25 15:20:00 +00:00
Conversation 0 Commits 1 Files changed 2 +60 -22
2 changed files with 60 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
cmd/frostfs-ir/config.go
View file

@ -45,18 +45,41 @@ func reloadConfig() error {
func watchForSignal(cancel func()) {
ch := make(chan os.Signal, 1)
signal.Notify(ch, syscall.SIGHUP, syscall.SIGINT, syscall.SIGTERM)
signal.Notify(ch, syscall.SIGINT, syscall.SIGTERM)
sighupCh := make(chan os.Signal, 1)
signal.Notify(sighupCh, syscall.SIGHUP)
for {
select {
case err := <-intErr:
// signals causing application to shut down should have priority over
// reconfiguration signal
case <-ch:
log.Info(logs.FrostFSNodeTerminationSignalHasBeenReceivedStopping)
cancel()
shutdown()
fyrchik commented 2024-06-24 13:11:23 +00:00
Outdated
Review
Copy link

In your implementation, we now have a race condition between ch and sighupCh.
However, if we received SIGTERM there is no need to SIGHUP, because we will eventually shutdown.
I suggest make SIGTERM/SIGINT having priority over SIGHUP.

In your implementation, we now have a race condition between `ch` and `sighupCh`. However, if we received SIGTERM there is no need to SIGHUP, because we will eventually shutdown. I suggest make SIGTERM/SIGINT having priority over SIGHUP.
acid-ant commented 2024-06-25 06:14:47 +00:00
Outdated
Review
Copy link

Also, there are is no guarantee that we receive all signals, even if we have buffered channel.

Also, there are is no [guarantee](https://github.com/golang/go/blob/b3b4556c245c8f21872910ee866133428bbb5a60/src/os/signal/signal.go#L109) that we receive all signals, even if we have buffered channel.
fyrchik commented 2024-06-25 06:42:30 +00:00
Outdated
Review
Copy link

Also, there are is no guarantee

It was the problem of the old implementation. Now we have 2 separate channels and SIGTERM / SIGINT are handled similarly, so we should not lose anything useful.

>Also, there are is no guarantee It was the problem of the old implementation. Now we have 2 separate channels and SIGTERM / SIGINT are handled similarly, so we should not lose _anything useful_.
👍 1
elebedeva commented 2024-06-25 09:27:14 +00:00
Outdated
Review
Copy link

fixed

fixed
log.Info(logs.FrostFSNodeTerminationSignalProcessingIsComplete)
return
case err := <-intErr: // internal application error
log.Info(logs.FrostFSIRInternalError, zap.String("msg", err.Error()))
cancel()
shutdown()
return
case sig := <-ch:
switch sig {
case syscall.SIGHUP:
default:
// block until any signal is receieved
select {
case <-ch:
log.Info(logs.FrostFSNodeTerminationSignalHasBeenReceivedStopping)
cancel()
shutdown()
log.Info(logs.FrostFSNodeTerminationSignalProcessingIsComplete)
return
case err := <-intErr: // internal application error
log.Info(logs.FrostFSIRInternalError, zap.String("msg", err.Error()))
cancel()
shutdown()
return
case <-sighupCh:
log.Info(logs.FrostFSNodeSIGHUPHasBeenReceivedRereadingConfiguration)
err := reloadConfig()
if err != nil {
@ -70,12 +93,6 @@ func watchForSignal(cancel func()) {
log.Error(logs.FrostFSNodeConfigurationReading, zap.Error(err))
}
log.Info(logs.FrostFSNodeConfigurationHasBeenReloadedSuccessfully)
case syscall.SIGTERM, syscall.SIGINT:
log.Info(logs.FrostFSNodeTerminationSignalHasBeenReceivedStopping)
cancel()
shutdown()
log.Info(logs.FrostFSNodeTerminationSignalProcessingIsComplete)
return
}
}
}

35
cmd/frostfs-node/config.go
View file

@ -1224,22 +1224,22 @@ type dCmp struct {
func (c *cfg) signalWatcher(ctx context.Context) {
ch := make(chan os.Signal, 1)
signal.Notify(ch, syscall.SIGHUP, syscall.SIGINT, syscall.SIGTERM)
signal.Notify(ch, syscall.SIGINT, syscall.SIGTERM)
sighupCh := make(chan os.Signal, 1)
signal.Notify(sighupCh, syscall.SIGHUP)
for {
select {
case sig := <-ch:
switch sig {
case syscall.SIGHUP:
c.reloadConfig(ctx)
case syscall.SIGTERM, syscall.SIGINT:
// signals causing application to shut down should have priority over
// reconfiguration signal
case <-ch:
c.log.Info(logs.FrostFSNodeTerminationSignalHasBeenReceivedStopping)
c.shutdown()
c.log.Info(logs.FrostFSNodeTerminationSignalProcessingIsComplete)
return
}
case err := <-c.internalErr: // internal application error
c.log.Warn(logs.FrostFSNodeInternalApplicationError,
zap.String("message", err.Error()))
@ -1248,6 +1248,27 @@ func (c *cfg) signalWatcher(ctx context.Context) {
c.log.Info(logs.FrostFSNodeInternalErrorProcessingIsComplete)
return
default:
// block until any signal is receieved
select {
case <-sighupCh:
c.reloadConfig(ctx)
case <-ch:
c.log.Info(logs.FrostFSNodeTerminationSignalHasBeenReceivedStopping)
c.shutdown()
c.log.Info(logs.FrostFSNodeTerminationSignalProcessingIsComplete)
return
case err := <-c.internalErr: // internal application error
c.log.Warn(logs.FrostFSNodeInternalApplicationError,
zap.String("message", err.Error()))
c.shutdown()
c.log.Info(logs.FrostFSNodeInternalErrorProcessingIsComplete)
return
}
}
}
}