fix: support human-readable sizes for fluentd-buffer-limit log option#4856
Conversation
shouhei
force-pushed
the
fix/fluentd-buffer-limit-human-readable
branch
2 times, most recently
from
15c6e5c to
ff70efa
Compare
utafrali
left a comment
utafrali
left a comment
There was a problem hiding this comment.
The core change is correct and backward-compatible: swapping strconv.Atoi for units.RAMInBytes is the right approach and the existing test suite validates the happy path. The main issues are a silent int64-to-int narrowing cast and missing error-path test coverage after the parser change.
| if err != nil { | ||
| return result, fmt.Errorf("error occurs %w,invalid buffer limit (%s)", err, config[fluentdBufferLimit]) | ||
| } | ||
| bufferLimit = int(parsedBufferLimit) |
There was a problem hiding this comment.
units.RAMInBytes returns int64, but the result is silently narrowed to int. On 32-bit platforms int is 32 bits, so any value above ~2 GiB would wrap silently without error. First check whether fluent.Config.BufferLimit actually requires int or already accepts int64 (in which case you can drop the cast entirely). If the cast is unavoidable, add an explicit bounds check:
if parsedBufferLimit > int64(math.MaxInt) {
return result, fmt.Errorf("invalid buffer limit: value %d overflows int", parsedBufferLimit)
}
bufferLimit = int(parsedBufferLimit)| Async: false, | ||
| AsyncReconnectInterval: 0, | ||
| SubSecondPrecision: false, | ||
| RequestAck: false}, false}, |
There was a problem hiding this comment.
The new test only covers the happy path. The rejection behavior has changed from strconv.Atoi to units.RAMInBytes, so add at least one wantErr: true case with an invalid value (e.g. "1X" or "not-a-size") to confirm that malformed input still returns an error after the parser swap.
| - :whale: `--log-opt=fluentd-address=<ADDRESS>`: The address of the `fluentd` daemon, tcp(default) and unix sockets are supported.. | ||
| - :whale: `--log-opt=fluentd-async=<true|false>`: Enable async mode for fluentd. The default value is false. | ||
| - :whale: `--log-opt=fluentd-buffer-limit=<LIMIT>`: The buffer limit for fluentd. If the buffer is full, the call to record logs will fail. The default is 8192. (<https://github.com/fluent/fluent-logger-golang/tree/master#bufferlimit>) | ||
| - :whale: `--log-opt=fluentd-buffer-limit=<LIMIT>`: The buffer limit for fluentd. If the buffer is full, the call to record logs will fail. The default is 1MiB. Accepts human-readable sizes (e.g., `1k`, `1m`, `1g`) or raw byte values. (<https://github.com/fluent/fluent-logger-golang/tree/master#bufferlimit>) |
There was a problem hiding this comment.
The examples 1k, 1m, 1g use RAMInBytes (binary units), so 1m = 1,048,576 bytes, not 1,000,000. This surprises users coming from SI conventions. Consider using IEC suffixes in the examples (1KiB, 1MiB, 1GiB) or adding a short note clarifying that the binary map is used, which is consistent with how Docker documents this option.
Signed-off-by: Shouhei <shouhei.yamaguchi.be@gmail.com>
shouhei
force-pushed
the
fix/fluentd-buffer-limit-human-readable
branch
from
ff70efa to
ee94580
Compare
|
@utafrali Thanks for the review. I updated the PR to follow your suggestions:
|
4 participants
Problem
fluentd-buffer-limitcurrently accepts only raw integer values innerdctl.In Docker-compatible workflows, users may specify human-readable values such as:
Fix
Use github.com/docker/go-units to parse fluentd-buffer-limit.
This allows nerdctl to accept human-readable values such as 16M and 16MiB in addition to raw byte values.