Logstash: Safe default time value string for config.reload.interval in 6.x

[jakommo]

Starting from 6.0 config.reload.interval requires a time value string. Prior to 6.0 the value was interpreted as seconds (The breaking changes suggest it was ms before, but I'm pretty certain it was seconds).

Now it seem that we do not check for a time value / unit and if config.reload.interval: 50 is used in 6.x, this will actually be interpreted as 50ms rather than 50s as it was in < 6.0.

Running Logstash 6.x with config.reload.interval: 50 will cause 100% CPU usage without processing any events.

I propose we have a check if a time value / unit was used on the setting and throw an error/warning otherwise. Users that miss the breaking change and have config.reload.interval: 5 in their settings file will be in trouble otherwise.

I also feel like we should use a default of seconds rather than ms as I don't see a use case where reloading in ms would make sense as being the default.

cc @guyboertje as we talked about this.

[PhaedrusTheGreek]

+1 on this. This matter has been responsible for more than one Logstash performance issue for me. Numeric values should not be allowed or interpreted as seconds.

Also the docs say "Seconds", but this is not true - if a numeric value is given, it's milliseconds.

[hackery]

Just bitten by this as well - in our case where a pipeline had a syntax error and repeatedly reloads, logging a long error line until /var/log is full.
The docs for 7.6 still say value is in seconds and makes no mention of a units qualifier.

[kares]

this is quite unfortunate, as its not even millis but rather nanos, default is 3_000_000_000 (3s)
we'll need to document and potentially at least warn users for setting anything < 1_000_000_000

[karenzone]

PR #11771 updates docs to indicate that the unit qualifier is required

[kares]

nanosecond 'default' is really an internal detail leaking down to users,
its easy to set 60 instead of 60s given the config yaml

the settings object treats integer values like nanos. internally, we convert back to seconds

[dharada]

this is quite unfortunate, as its not even millis but rather nanos, default is 3_000_000_000 (3s)
we'll need to document and potentially at least warn users for setting anything < 1_000_000_000

I think it is better to add a validation logic (e.g < 1s) in initialize method to prevent this kind of CPU usage saturation issue. It means "_warn to users for setting_" is not enough to prevent. (i.e) a mechanism(implementation) is needed, not only document.

Resource saturation issues are not easy to analyze/know what the root-cause is after incident. How about to define minimum value for that setting and validate in initialization ?

[Tarasovych]

Same issue here, was a bit confused with

By default, Logstash checks for configuration changes every 3 seconds.

and

--config.reload.interval RELOAD_INTERVAL How frequently to poll the configuration location
                             for changes, in seconds.
                              (default: 3000000000)

How should I set RELOAD_INTERVAL? I've tried to pass --config.reload.interval 3000000000 and --config.reload.interval "3000000000", error was

ERROR: option '--config.reload.interval': invalid time unit: "3000000000"

in both cases. I suppose, that neither int nor string is suitable as an argument value?