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

Support SIGHUP configuration reloading #6000

Merged
merged 5 commits into from
Nov 10, 2022

Conversation

portertech
Copy link
Contributor

Description:

Added SIGHUP UNIX signal handling to reload configuration (internal collector restart). I reused the configuration source notification reloading logic, a very small change.

Link to tracking Issue:

This is a possible solution/implementation for #5966

Testing:

Added a basic SIGHUP signal test to ensure the collector finds itself in a running state.

Manual testing demonstrates that the SIGHUP signal successfully triggers the existing configuration reloading logic.

$ ./bin/otelcorecol_* --config ./examples/local/otel-config.yaml
$ ps aux | grep otel
portert+  194518  0.3  0.2 1263472 36692 pts/3   SNl+ 12:55   0:00 ./bin/otelcorecol_linux_amd64 --config ./examples/local/otel-config.yaml
portert+  194534  0.0  0.0  18676   648 pts/4    SN+  12:55   0:00 grep --color=auto otel
$ kill -HUP 194518

From: https://gist.github.com/portertech/63a33aca67bc0468120abc924eda49e3

2022-08-30T12:01:39.918-0700    info    service/service.go:129  Everything is ready. Begin running and processing data.
2022-08-30T12:04:42.226-0700    info    service/collector.go:201        Received signal from OS {"signal": "hangup"}
2022-08-30T12:04:42.226-0700    warn    service/collector.go:158        Config updated, restart service
2022-08-30T12:04:42.226-0700    info    service/service.go:138  Starting shutdown...

Documentation:

Will need to update documentation to cover the newly supported UNIX signal (SIGHUP). This functionality is very common.

Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
@portertech portertech requested review from a team and bogdandrutu August 30, 2022 19:57
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 30, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Would like to wait for the issue to propose a solution.

Signed-off-by: Sean Porter <portertech@gmail.com>
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Comment on lines +205 to +212
switch s {
case syscall.SIGHUP:
if err := col.reloadConfiguration(ctx); err != nil {
return err
}
default:
break LOOP
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a simple if correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was considering future support for other signals.

Copy link
Member

Choose a reason for hiding this comment

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

So please use if for the moment :)

service/collector.go Show resolved Hide resolved
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 12, 2022
@bogdandrutu
Copy link
Member

@swiatekm-sumo any way you can talk to the author, or pickup this?

@github-actions github-actions bot removed the Stale label Oct 13, 2022
@swiatekm
Copy link
Contributor

@swiatekm-sumo any way you can talk to the author, or pickup this?

@portertech is on vacation until Monday. I can pick it up in the meantime if we're in a big hurry though.

@portertech
Copy link
Contributor Author

@bogdandrutu any other changes required?

@bogdandrutu bogdandrutu self-requested a review October 24, 2022 21:07
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

With the small nit left.

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Base: 92.04% // Head: 92.00% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (d9205d7) compared to base (6313054).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6000      +/-   ##
==========================================
- Coverage   92.04%   92.00%   -0.04%     
==========================================
  Files         219      219              
  Lines       13245    13259      +14     
==========================================
+ Hits        12191    12199       +8     
- Misses        830      834       +4     
- Partials      224      226       +2     
Impacted Files Coverage Δ
service/collector.go 76.76% <50.00%> (-2.15%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants