afuser: support time-reopen() #3585
Conversation
|
Build SUCCESS |
alltilla
changed the title
alltilla
force-pushed
the
usertty_time_reopen
branch
from
54d2bba
to
402a458
Compare
|
I'm expecting KIRA to fail, because this probably needs some internal changes. Please do not merge, until we have prepared the internal changes. |
alltilla
changed the title
|
Build SUCCESS |
|
Well this is awkward, because KIRA did not fail.. I will check if I am wrong, or we are missing some |
alltilla
force-pushed
the
usertty_time_reopen
branch
from
402a458
to
d8dc6cc
Compare
|
Build FAILURE |
We should always have cfg. If we do not, that is a coding error, so an assert fits better for this purpose. The motivation of doing this now is the time_reopen's inherited value. Previously the code was prepared in a way, that when we did not have a cfg, the time_reopen could stay -1, which could cause issues in the timers using it. In reality cfg was always available, but this recurring code snippet was misleading. Signed-off-by: Attila Szakacs <[email protected]>
It already had a support for time reopen, and even for the possibility to use a value other than the one inherited from the global options (see log_threaded_fetcher_driver_init_method), but the config option itself was not introduced to the grammar. This grammar rule has an effect on all drivers, whose grammar utilize `threaded_fetcher_driver_option`, such as `threaded-diskq-source` and `python`. Signed-off-by: Attila Szakacs <[email protected]>
It already had a support for time reopen, and even for the possibility to use a value other than the one inherited from the global options (see log_threaded_dest_driver_init_method), but the config option itself was not introduced to the grammar. This grammar rule has an effect on all drivers, whose grammar utilize `threaded_dest_driver_option`, such as `afamqp`, `afmongodb`, `afsmtp`, `afsql`, `afstomp`, `example_destination`, `http`, `java`, `python`, `redis` and `riemann`. Signed-off-by: Attila Szakacs <[email protected]>
It already had a support for time reopen, and even for the possibility to use a value other than the one inherited from the global options (see log_writer_options_init), but the config option itself was not introduced to the grammar. This grammar rule has an effect on all drivers, whose grammar utilize `dest_writer_option`, such as `affile`, `afprog`, `afsocket`. Signed-off-by: Attila Szakacs <[email protected]>
`affile` seemed to have a `time_reopen` on its own, additionally to the `logwriter_option`'s `time_reopen` value. The previous commit cleaned up the usage of `time_reopen` in `logwriter_options`, so it is better for these drivers to use that, instead of implementing their own logic again. In reality they could not even to that, even if they wanted, because the `KW_TIME_REOPEN` grammar rule, inherited from `dest_writer_option` would make it impossible to a driver, such as `affile`, to create their own `KW_TIME_REOPEN` rule, because it would cause a yacc conflict. Signed-off-by: Attila Szakacs <[email protected]>
`afsocket` seemed to have a `time_reopen` on its own, additionally to the `logwriter_option`'s `time_reopen` value. An earlier commit cleaned up the usage of `time_reopen` in `logwriter_options`, so it is better for these drivers to use that, instead of implementing their own logic again. In reality they could not even to that, even if they wanted, because the `KW_TIME_REOPEN` grammar rule, inherited from `dest_writer_option` would make it impossible to a driver, such as `afsocket`, to create their own `KW_TIME_REOPEN` rule, because it would cause a yacc conflict. Signed-off-by: Attila Szakacs <[email protected]>
`pseudofile` always used the global option's `time_reopen` value. As `pseudofile` does not have `logwriter_options`, and it does not inherit from neither `logthrfetcher`, nor `logthrdest`, it has to implement its own `time_reopen` logic. Signed-off-by: Attila Szakacs <[email protected]>
`afuser` always used a default 600 value to disconnect from the tty. It is better to make it configurable. `time_reopen` is an option which has a similar behavior in other drivers, so it is logical to make this new option `time-reopen()`. This introduces a new problem, however, as the default `time-reopen()` is 60 and not 600. We decided to let it stay 60, and make a "breaking" change with in in `afuser`. We think, that it is not a big issue, and as we give an option to change it back to 600, the original behavior can be achieved with config modification. As `afuser` does not have `logwriter_options`, and it does not inherit from neither `logthrfetcher`, nor `logthrdest`, it has to implement its own `time_reopen` logic. Signed-off-by: Attila Szakacs <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
alltilla
force-pushed
the
usertty_time_reopen
branch
from
d8dc6cc
to
7403d43
Compare
|
Build SUCCESS |
|
Internal MR !317 is prepared. This PR is now ready to review and merge. |
|
@alltilla Do you know when this fix will be released? Which release and date? |
|
We will release 3.32.1 approximately 2 months from now, if everything goes well. This code will most probably be shipped with that. From where do you obtain your syslog-ng package? Is it okay for you to use an unreleased package? As you can see it from the "Binary packages" CI run, we generate packages for every PR, and anyone can do that by themselves also with the help of If you need an installer sooner, I would suggest to wait until this PR gets its 2nd approve, and gets merged, then you can generate a package from the master branch. |
There was a problem hiding this comment.
There's one note I was thinking about, namely the fact that we are peeking into LogWriterOptions in unrelated code, with expressions like "owner->writer_options->time_reopen". With that we implement an assumption that a driver is actually using LogWriter to implement its functionality.
With this we couple with an implementation detail in unrelated code.
This does not necessarily increase coupling today, as we might have the same assumption in other parts of the same code. Also, it might be a safe assumption, as I can't see why we would want to change this implementation in the future.
I just wanted to mention it. Approve on my part, just consider the above. Thanks.
|
@alltilla Thanks, I will next available release |
|
Hey @alltilla, I already saw that syslog-ng 3.32.1 was released. |
|
Hi @amdrsantos, It will probably be done in 1-2 weeks, we will send an email to the syslog-ng mail list, when it is ready. Attila |
|
@alltilla do you know if Debian 10 (buster) packaging of syslog-ng 3.32.1 will be released shortly? |
|
Hi @amdrsantos You might have seen on the mail list, that the OBS packages are discontinued. I would advise to use I generated one for you with the following commands, but you can do it manually, too: We will work on providing packages in the future somewhere, but we have not decided the details yet. syslog-ng-3.32.1-debian-buster-packages.zip Cheers, |
|
@alltilla Is it possible to use dbld to generate the packages for arm64? |
|
If you are running on arm64 it should work out of the box. Cross compiling
should also be possible but is not supported atm.
…On Fri, Jun 4, 2021, 15:45 Alexandre Santos ***@***.***> wrote:
@alltilla <https://github.com/alltilla> Is it possible to use dbld to
generate the packages for arm64?
If yes, do you know how?
Thanks in advance,
Alex
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3585 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFOK5UKH3AJTLZBLVND4VDTRDKF7ANCNFSM4YETONPQ>
.
|
Support
time-reopen()option.syslog-ngwill waittime-reopen()seconds (instead of the previously hard-coded 600), when it could not connect to the usertty terminal, then try to connect again.Add per driver
time-reopen()option, for drivers, which were previously supporting it, but was not able to set it per driver.Signed-off-by: Attila Szakacs [email protected]