Skip to content

Fix SIGHUP - #4113

Open
End-rey wants to merge 2 commits into
masterfrom
fix-sighup
Open

Fix SIGHUP#4113
End-rey wants to merge 2 commits into
masterfrom
fix-sighup

Conversation

@End-rey

@End-rey End-rey commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Noticed some issues when adding a new configuration to gRPC.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.18%. Comparing base (36e3cf4) to head (771b309).

Files with missing lines Patch % Lines
cmd/neofs-node/config.go 0.00% 34 Missing ⚠️
cmd/neofs-node/attributes.go 0.00% 7 Missing ⚠️
cmd/neofs-node/netmap.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4113      +/-   ##
==========================================
- Coverage   27.19%   27.18%   -0.01%     
==========================================
  Files         677      677              
  Lines       46206    46210       +4     
==========================================
- Hits        12564    12562       -2     
- Misses      32442    32447       +5     
- Partials     1200     1201       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@roman-khimov roman-khimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Create an issue for neofs-testcases to test these scenarios.

Comment thread cmd/neofs-node/config.go
return fmt.Errorf("read configuration: %w", err)
}
if err := validateConfig(newCfg); err != nil {
return fmt.Errorf("validate configuration: %w", err)

@carpawell carpawell Aug 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not sure if running with an incorrect config is ok. maybe app should die? the only feedback we have there is log message, otherwise: app is healthy, and systemd thinks we are "Ready"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aren't we running with an old config in this case?

@carpawell carpawell Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, but we have a new config file (envs), and the app is still running with already unknown old config

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree. A failed reload can leave the runtime state out of sync with the configuration, so reporting READY=1 is misleading. I think stopping the node on reload failure would be a good idea.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i am fully ok with stopping

@carpawell carpawell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Conflicts

@roman-khimov roman-khimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Create an issue for neofs-testcases to test these scenarios.

Comment thread CHANGELOG.md Outdated
@End-rey
End-rey force-pushed the fix-sighup branch 2 times, most recently from 8848cd1 to 771b309 Compare August 10, 2026 17:08
Validate the newly read configuration before applying it. Invalid configuration
now aborts reload before services are modified.

Stop the node when configuration reload fails, preventing it from running with
a runtime state that can differ from the configuration file or environment.
Systemd restarts the node according to the service restart policy.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
Handle node attribute parsing errors through the common graceful shutdown path.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
@End-rey

End-rey commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

The gRPC-specific fixes no longer make sense because any SIGHUP reload failure now triggers a graceful node shutdown.

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.

3 participants