fix: Compression in log-rotate plugin exceeds the default timeout of shell.run #8620
Conversation
Fixes: Compression in log-rotate plugin exceeds the default timeout of shell.run
…shell.run Fixes: Compression in log-rotate plugin exceeds the default timeout of shell.run
alptugay
changed the title
apisix/plugins/log-rotate.lua
Outdated
| @@ -277,6 +277,7 @@ local function rotate() | |||
| max_size = attr.max_size or max_size | |||
| enable_compression = attr.enable_compression or enable_compression | |||
| end | |||
| local timeout = interval * 100 -- Timeout for compression is interval in milliseconds | |||
There was a problem hiding this comment.
We'd better add a new configuration item to control the timeout parameter.
There was a problem hiding this comment.
I don't agree that timeout should be a user defined parameter. Because, setting a timeout greater than interval may cause undefined behaviour (eg. second rotation starts but first compression is still running), and setting the value too low also causes the mentioned undesired behaviour (so doesn't fix the issue in the first place). So, this value should be automatically set based on interval and shoud not be user defined imho.
There was a problem hiding this comment.
@alptugay, I agree with you, however setting a fixed timeout is inappropriate. What if the compression starts just before the next rotation?
The correct way would be to provide a timeout such that timeout occurs before the next log-rotation. And if the compression still gets a timeout, we can reschedule the compression later using ngx_timer. WDYT?
There was a problem hiding this comment.
What if the compression starts just before the next rotation?
Rotation is done in intervals. Execution step is like below in simple terms:
- Rename current log file to logfile.date
- Send signal to process so new file opens
- compress logfile.date with a timeout that is not bigger than the interval,
So I don't think step 3 will start just before next rotation.
But your second argument is valid, compression can still get a timeout if it can't be finished in the specified interval. We can schedule it with ngx_timer but this means those timers will add up. Assume the interval is 5 minutes and compression takes 10 minutes, after 1 hour we will have done 12 intervals jobs but only 6 compression will be completed and 6 will be waiting. So this will add up and probably make the system unresponsive
There was a problem hiding this comment.
Makes sense. LGTM.
There was a problem hiding this comment.
I suggest adding a configuration to control the timeout parameter, the default timeout is 10s, it's enough for most people, if you use interval as timeout, it could be 1min or 1 hour, maybe someone don't need such long time.
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
There was a problem hiding this comment.
@lingsamuel, @monkeyDluffy6017, this PR LGTM. Please review and approve.
monkeyDluffy6017
added
the
wait for update
|
@shreemaan-abhishek please fix #8620 (comment) if it's a typo, then merge this PR, thanks |
|
@shreemaan-abhishek Pay attention to this comment: #8620 (comment) |
|
@alptugay can I take over this PR? You can invite me to collaborate on your forked repo. |
Co-authored-by: Liu Wei <[email protected]>
|
@alptugay are you still on it? |
|
I will take over from here. |
Revolyssup
removed
the
wait for update
|
@Revolyssup Could you add this attribute in |
monkeyDluffy6017
added
the
wait for update
Signed-off-by: Ashish Tiwari <[email protected]>
Signed-off-by: Ashish Tiwari <[email protected]>
Revolyssup
removed
the
wait for update
|
@monkeyDluffy6017 Added in the config-default.yaml |
Description
This issue fixes the bug: "Compression in log-rotate plugin exceeds the default timeout of shell.run" #8619
The interval parameter is passed to shell.run() function
Fixes #8619
Checklist