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

broker: log content store errors to LOG_CRIT #4526

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Aug 26, 2022

Problem: In the event of content store errors, such as ENOSPC
if the disk is full, error messages are logged to LOG_ERR. This
does not properly signify the severity of the error, as something
like ENOSPC would mean no data is being backed up to disk.

Solution: Log to LOG_CRIT, to signify the importance of this
issue and the need for it to be resolved.

@chu11 chu11 force-pushed the issue4472_broker_log_crit_content_store_failure branch from 764f3c8 to fb747e5 Compare August 26, 2022 22:26
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #4526 (764f3c8) into master (ba4c510) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 764f3c8 differs from pull request most recent head fb747e5. Consider uploading reports for the commit fb747e5 to get more accurate results

@@            Coverage Diff             @@
##           master    #4526      +/-   ##
==========================================
- Coverage   83.38%   83.36%   -0.02%     
==========================================
  Files         403      402       -1     
  Lines       67736    67664      -72     
==========================================
- Hits        56480    56409      -71     
+ Misses      11256    11255       -1     
Impacted Files Coverage Δ
src/broker/content-cache.c 85.80% <0.00%> (ø)
src/shell/info.c 78.19% <0.00%> (-2.08%) ⬇️
src/modules/job-exec/bulk-exec.c 79.35% <0.00%> (-1.48%) ⬇️
src/common/libsubprocess/server.c 60.54% <0.00%> (-0.55%) ⬇️
src/modules/job-info/guest_watch.c 76.21% <0.00%> (-0.55%) ⬇️
src/cmd/flux-job.c 87.37% <0.00%> (-0.16%) ⬇️
src/common/libjob/unwrap.c
src/common/libterminus/terminus.c 86.06% <0.00%> (+0.24%) ⬆️
src/common/libsubprocess/subprocess.c 88.19% <0.00%> (+0.29%) ⬆️
src/common/libsubprocess/local.c 84.39% <0.00%> (+0.48%) ⬆️

@@ -505,7 +505,7 @@ static void cache_store_continuation (flux_future_t *f, void *arg)
flux_log (cache->h, LOG_DEBUG, "content store: %s",
"backing store service unavailable");
else
flux_log_error (cache->h, "content store");
flux_log (cache->h, LOG_CRIT, "content store");
Copy link
Member

Choose a reason for hiding this comment

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

Should be

flux_log (cache->h, LOG_CRIT, "content store: %s", strerror (errno));

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yeah, no strerror output when you take out the error ... my bad, sloppy :P

@chu11 chu11 force-pushed the issue4472_broker_log_crit_content_store_failure branch from fb747e5 to b27c5ea Compare August 26, 2022 23:13
@chu11
Copy link
Member Author

chu11 commented Aug 26, 2022

re-pushed, fixing up the 1 line ... in my 1 line patch :P

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

👍

Problem: In the event of content store errors, such as ENOSPC
if the disk is full, error messages are logged to LOG_ERR.  This
does not properly signify the severity of the error, as something
like ENOSPC would mean no data is being backed up to disk.

Solution: Log to LOG_CRIT, to signify the importance of this
issue and the need for it to be resolved.
@jameshcorbett jameshcorbett force-pushed the issue4472_broker_log_crit_content_store_failure branch from b27c5ea to e6c152b Compare August 31, 2022 19:05
@mergify mergify bot merged commit 5178d0d into flux-framework:master Aug 31, 2022
@chu11 chu11 deleted the issue4472_broker_log_crit_content_store_failure branch September 1, 2022 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants