Skip to content

rgw: reopen ops log file on sighup#44488

Merged
cbodley merged 1 commit intoceph:masterfrom
cfsnyder:wip-53788-reopen-ops-log-sighup
Mar 8, 2022
Merged

rgw: reopen ops log file on sighup#44488
cbodley merged 1 commit intoceph:masterfrom
cfsnyder:wip-53788-reopen-ops-log-sighup

Conversation

@cfsnyder
Copy link
Contributor

@cfsnyder cfsnyder commented Jan 6, 2022

Handles radosgw SIGHUP such that ops log file is reopened if applicable.

Fixes: https://tracker.ceph.com/issues/53788
Signed-off-by: Cory Snyder csnyder@iland.com

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)

@github-actions github-actions bot added the rgw label Jan 6, 2022
mattbenjamin
mattbenjamin reviewed Jan 6, 2022
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

yup, this is needed; I'm a bit concerned about the signal delivery semantics here; @cbodley is it safe because it's registered in the thread managing the logs? i thought posix signal delivery was to the pgrp leader; I didn't refresh my memory...

@mattbenjamin
Copy link
Contributor

mattbenjamin commented Jan 6, 2022

yup, this is needed; I'm a bit concerned about the signal delivery semantics here; @cbodley is it safe because it's registered in the thread managing the logs? i thought posix signal delivery was to the pgrp leader; I didn't refresh my memory...

well, in fact, some type of handling of the rotation case is needed. another approach I think I've used is to ignore the signal and notice (with a periodic stat of the path) whether there is a new ino (in which event you must reopen); if it actually is being truncated, you can I think just retry reads on the same timer

@cfsnyder
Copy link
Contributor Author

cfsnyder commented Jan 7, 2022

jenkins test make check

@cfsnyder
Copy link
Contributor Author

cfsnyder commented Jan 7, 2022

jenkins test api

@cfsnyder cfsnyder force-pushed the wip-53788-reopen-ops-log-sighup branch from 9d77cea to fed15b8 Compare January 26, 2022 14:18
@cfsnyder
rgw: reopen ops log file on sighup
f26523f 
Handles radosgw SIGHUP such that ops log file is reopened if applicable.

Fixes: https://tracker.ceph.com/issues/53788
Signed-off-by: Cory Snyder <csnyder@iland.com>
@cfsnyder cfsnyder force-pushed the wip-53788-reopen-ops-log-sighup branch from fed15b8 to f26523f Compare January 26, 2022 14:23
@cbodley
Copy link
Contributor

cbodley commented Feb 23, 2022

yup, this is needed; I'm a bit concerned about the signal delivery semantics here; @cbodley is it safe because it's registered in the thread managing the logs? i thought posix signal delivery was to the pgrp leader; I didn't refresh my memory...

well, in fact, some type of handling of the rotation case is needed. another approach I think I've used is to ignore the signal and notice (with a periodic stat of the path) whether there is a new ino (in which event you must reopen); if it actually is being truncated, you can I think just retry reads on the same timer

@mattbenjamin sorry i didn't reply here; i'm not sure i understand your concern. does it matter which thread is running the signal handler, if it's just setting std::atomic_bool need_reopen?

@mattbenjamin
Copy link
Contributor

mattbenjamin commented Feb 23, 2022

yup, this is needed; I'm a bit concerned about the signal delivery semantics here; @cbodley is it safe because it's registered in the thread managing the logs? i thought posix signal delivery was to the pgrp leader; I didn't refresh my memory...

well, in fact, some type of handling of the rotation case is needed. another approach I think I've used is to ignore the signal and notice (with a periodic stat of the path) whether there is a new ino (in which event you must reopen); if it actually is being truncated, you can I think just retry reads on the same timer

@mattbenjamin sorry i didn't reply here; i'm not sure i understand your concern. does it matter which thread is running the signal handler, if it's just setting std::atomic_bool need_reopen?

Probably not? Just worrying.
Matt

cbodley
cbodley approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

looks ok to me. ops_log_file is guaranteed to exist the whole time this rgw_sighup_handler is registered, and ops_log_file->reopen() is atomic

@cbodley
Copy link
Contributor

cbodley commented Mar 8, 2022

passed qa https://pulpito.ceph.com/cbodley-2022-03-07_22:02:15-rgw-wip-cbodley-testing-distro-default-smithi/

@cbodley
Copy link
Contributor

cbodley commented Mar 8, 2022

jenkins test api

@cbodley cbodley merged commit 7eaed47 into ceph:master Mar 8, 2022
This was referenced Jun 10, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattbenjamin mattbenjamin mattbenjamin left review comments

@cbodley cbodley cbodley approved these changes

Assignees

No one assigned

Labels

needs-qa rgw wip-cbodley-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cfsnyder @mattbenjamin @cbodley