-
Notifications
You must be signed in to change notification settings - Fork 19
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
Allow chunking of stdout output files. #234
Conversation
Thanks a lot for this! Can you take a look at the CI output please? |
Yup, I somehow lost the ending brace of a function when staging the diff. This should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments, overall looks very good thanks a lot!
I agree unit-tests are complicated to write for this case but It should be possible to write an integration test instead.
You could create a unit test using this as a base like in your test:
command = "nfsstat -v -s -Z3"
stdout = "/tmp/foo-stdout.log"
stdout-rotate-size = 2048
working-directory = "/tmp/"
the log redirection is tested here: https://github.com/FedericoPonzi/Horust/blob/master/tests/section_general.rs#L51 but if you take a look at the other tests you should be able to quickly write an integ test for this case given you already have the service code.
DOCUMENTATION.md
Outdated
@@ -47,6 +47,7 @@ If service `a` should start after service `b`, then `a` will be started as soon | |||
If `b` goes in a `FinishedFailed` state (finished in an unsuccessful manner), `a` might not start at all. | |||
* **`start-delay` = `time`**: Start this service with the specified delay. Check how to specify times [here](https://github.com/tailhook/humantime/blob/49f11fdc2a59746085d2457cb46bce204dec746a/src/duration.rs#L338) | |||
* **`stdout` = `STDOUT|STDERR|file-path`**: Redirect stdout of this service. STDOUT and STDERR are special strings, pointing to stdout and stderr respectively. Otherwise, a file path is assumed. | |||
* **`stdout-rotate-size` = `bytes`**: Chunk size of the file specified in `stdout` in bytes. Once the file grows above the specified size it will be closed and a new file will be created with a suffix `.1`. Once the new file also grows above the specified size it will also be closed and a next one will be created with the next suffix `.2`. This allows adding external log rotation script, which can compress the old logs and maybe move them out to a different storage location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add the default value and how it behaves with the default? Also, it might be more convenient if these were kilobytes or even megabytes? I don't think users need bytes granulation level for logs, it depends on the use case but I would probably use 100MB as a default for my horust configs.
Actually, should we set the default to some sane value? I think in the majority of the use cases, they won't want to set this to 0. Maybe we should put 50 or 100 megs as default, and let users pick 0 if they really want to. What do you think? (I'm aware it's a change of behavior, but it feels like a desirable change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break the logic of having simple config files like:
stdout = "STDOUT"
Not specifying the stdout-rotate-size
would default it to some value and this contradicts the STDOUT
, which can't be chunked. So this behavior change would break a bunch of integration tests and potentially a bunch of existing user configs.
Ah cool, you already have a bunch of test harnesses! Please take another look 😸 Also if you prefer having a "clean" PR with fewer commits, I can squash my fixes into the initial commits and force re-push it. |
Can you also add the new parameter to the sample service? https://github.com/FedericoPonzi/Horust/blob/31c829792b86d28d220084d2c5f8620108984d9c/example_services/sample_service.toml this is used as source for |
9a11f32
to
eb86837
Compare
Adjusted the sample service - stderr now goes to stderr, while stdout goes to a rotated file. |
The deserialize test will also need to be updated: Horust/src/horust/formats/service.rs Line 672 in 31c8297
|
This allows rotating and maintaining the logs by some external program - they can be compressed and/or moved to storage.
Yup, just noticed and fixed it 😄 |
Thanks a lot for your contribution! |
Motivation and Context
When Horust runs for longer periods of time and the supervised programs generate a lot of output, it would be beneficial to introduce a simple log rotation system. A full-blown solution would have Horust have a complicated log rotation and compression system - but that would be an overkill. A much simpler (and chosen) approach is to just chunk the output file and then let the user customize the rotation with a small shell script.
Description
This change adds a parameter
stdout-rotation-size
which allows setting the maximum size of the stdout output file in bytes. For example: if this is set to 10240 (10 kb) and the stdout is redirected to/tmp/out.txt
, then as soon as this file grows to 10kb it will be closed and a new file/tmp/out.txt.1
will be created and the output will continue there.Having these separate output files allows compressing the, for example with gzip.
How Has This Been Tested?
The changes are tightly bound to spawning threads, duping descriptors and creating files - so it is very hard to test them with unit tests. But they were tested with a simple configuration file:
This correctly created a bunch of
/tmp/foo-stdout*
files after some time, each 2kb large.Types of changes
Checklist: