Fix value of `system_config.workers` at `run_configure` #4066

abetomo · 2023-02-20T23:40:04Z

Which issue(s) this PR fixes:
Fixes #4051

What this PR does / why we need it:

Override system_config only if conf.target_worker_ids.size >= 1.

If <worker N> or <worker 0-N> is not set, conf.target_worker_ids is empty.
If <system> workers N </system> is not set, system_config.workers defaults to 1.

Docs Changes:

Release Note:

Fix value of system_config.workers at run_configure
- (Fix bug that system_config.workers value can be wrongly 1 at config check at startup.)
Change argument type of Fluent::Plugin::Base::configure() to Fluent::Config::Element only.
- (Note: This doesn't change the specification. It was a bug that a variable whose type is not Fluent::Config::Element could be passed to Fluent::Plugin::Base::configure().)

daipom · 2023-02-21T14:13:10Z

Thanks for the fix!

The status of this PR is Draft. Please let me know which part is TODO.

abetomo · 2023-02-21T14:20:03Z

Thanks for your comments!
I will add some tests.

daipom · 2023-02-21T14:24:25Z

I will add some tests.

Thanks! We need tests for this issue!

In addition, this problem is difficult. Hard to know what was wrong and how we should fix it.
I want to clarify it.

The specification history related to this is as follows.

What went wrong in which process?

abetomo · 2023-02-21T15:23:12Z

#2651 is related.

If there is no <worker> directive, conf.for_this_worker? is false.
However, if Fluent::Engine.supervisor_mode is true, it will overwrite system_config.workers regardless of conf.for_this_worker?.

Since conf.target_worker_ids is empty when there is no <worker> directive, this fix checks for it and does not override system_config.workers.

daipom · 2023-02-21T16:10:48Z

Thanks so much!
It certainly seems that #2651 has something to do with this problem.

There was nothing wrong at the point of #1507 and #2292.
(The method of rewriting the value of the system config in the process seems a bit too complicated, but I guess it was inevitable since SystemConfig.workers was used to determine if the plugin works in the multi-worker or not.
It seems to me that this complicated specification could be the cause of bugs in the future..., anyway, it is not directly related to this issue.)

I will see #2651!

daipom · 2023-02-22T02:17:30Z

It is hard to understand the purpose of code changes in #2651...

I'm concerned about the following points.

About Support multi workers assign syntax on <worker> section. Fix #2289 #2292
- If conf.for_this_worker? is true, then conf.target_worker_ids appears to have at least one id. Why was the condition if conf.target_worker_ids && !conf.target_worker_ids.empty? needed?
  - https://github.com/fluent/fluentd/pull/2292/files#diff-601c8bc645b7008fa857b253a5295ef4b796c6236d0428ed8d72cecd572be6edR243-R245
  - https://github.com/fluent/fluentd/pull/2292/files#diff-a011588d74f931c0c2154891220af14f9181f28eb70b978d2e017866dfba3081R54-R59
- It seems to me that the following code is enough at this point.
```
if conf.for_this_worker?
  system_config_override(workers: conf.target_worker_ids.size)
end
```
About Fix dry run mode #2651
- Why does Fluent::Engine::run_configure() run before launching the supervisor process even when dry_run is false?
  - Shouldn't it be run only when dry_run is true?
  - https://github.com/fluent/fluentd/pull/2651/files#diff-6042a07157b33b2d729ab3d10ed6bba89ad5f71aaed2be1aeb22a179a45ebbaeR499-R508
- When Fluent::Engine.supervisor_mode is true, what is the expected value of workers?
  - I think we have to validate the config in one process in this case, but how many workers each plugin will work with will change depending on the setting.
    For example, plugin-A works with worker-1 and worker-2, and plugin-B works with only worker-3.
    With the current structure, would it be difficult to properly reflect and validate the number of workers?

I don't think all the points should be solved now.
I just noted points of concern as I work on this issue.
If you have noticed anything in the process of making this fix, I'd like to know.

The current fix may be enough to solve the issue #4051, but it may be better to leave some code comments for the future.

abetomo · 2023-02-22T12:36:31Z

Thanks for your comment!
I'll look into it.

abetomo · 2023-02-23T12:42:44Z

About Support multi workers assign syntax on <worker> section. Fix #2289 #2292

As for #2292 condition, I too think @daipom's code is enough.

daipom · 2023-02-23T15:35:10Z

Thanks!

When Fluent::Engine.supervisor_mode is true, what is the expected value of workers?
* I think we have to validate the config in one process in this case, but how many workers each plugin will work with will change depending on the setting.
For example, plugin-A works with worker-1 and worker-2, and plugin-B works with only worker-3.
With the current structure, would it be difficult to properly reflect and validate the number of workers?

I have checked this, and I understand this fix allows us to validate the config with the correct workers value!

I used this config.

<system>
  workers 8
</system>

<worker 0>
  <source>
    @id A
    @type sample
    tag test
  </source>
</worker>

<worker 1-2>
  <source>
    @id B
    @type sample
    tag test
  </source>
</worker>

<source>
  @id C
  @type sample
  tag test
</source>

<match test.**>
  @type stdout
</match>

I added a log for debugging to in_sample.

--- a/lib/fluent/plugin/in_sample.rb
+++ b/lib/fluent/plugin/in_sample.rb
@@ -63,6 +63,7 @@ module Fluent::Plugin
 
     def configure(conf)
       super
+      log.info id: @id, workers: system_config.workers
       @sample_index = 0
       config = conf.elements.select{|e| e.name == 'storage' }.first

The result of master branch:

2023-02-24 00:20:14 +0900 [info]: init supervisor logger path=nil rotate_age=nil rotate_size=nil
2023-02-24 00:20:14 +0900 [info]: parsing config file is succeeded path="../work/fluent.conf"
...
2023-02-24 00:20:14 +0900 [info]: [C]  id="C" workers=1
2023-02-24 00:20:14 +0900 [info]: [A]  id="A" workers=1
2023-02-24 00:20:14 +0900 [info]: [B]  id="B" workers=2
...
2023-02-24 00:20:14 +0900 [info]: starting fluentd-1.15.3 pid=1286518 ruby="3.2.0"
...
2023-02-24 00:20:15 +0900 [info]: #7 [C]  id="C" workers=8
...
2023-02-24 00:20:15 +0900 [info]: #1 [B]  id="B" workers=2
...
2023-02-24 00:20:15 +0900 [info]: #0 [A]  id="A" workers=1
...

Before starting Fluentd, workers value of C is wrong.

The expected workers values before stating Fluentd are as follows.

[C]  id="C" workers=8
[A]  id="A" workers=1
[B]  id="B" workers=2

When applying this fix, these values are as expected!

daipom · 2023-02-23T15:50:21Z

Summarize what we've learned so far.

About Support multi workers assign syntax on <worker> section. Fix #2289 #2292

As for #2292 condition, I too think @daipom's code is enough.

fluentd/lib/fluent/config/element.rb

Lines 256 to 258 in 6c649d1

    
           def for_this_worker? 
        
             @target_worker_ids.include?(Fluent::Engine.worker_id) 
        
           end

fluentd/lib/fluent/plugin/base.rb

Lines 56 to 61 in 6c649d1

    
           if Fluent::Engine.supervisor_mode || (conf.respond_to?(:for_this_worker?) && conf.for_this_worker?) 
        
             workers = if conf.target_worker_ids && !conf.target_worker_ids.empty? 
        
                         conf.target_worker_ids.size 
        
                       else 
        
                         1 
        
                       end

Originally, there was no need to overwrite 1.

fluentd/lib/fluent/engine.rb

Lines 213 to 216 in 6c649d1

    
           def worker_id 
        
             if @supervisor_mode 
        
               return -1 
        
             end

fluentd/lib/fluent/plugin/base.rb

Line 56 in 6c649d1

    
           if Fluent::Engine.supervisor_mode || (conf.respond_to?(:for_this_worker?) && conf.for_this_worker?)

We need Fluent::Engine.supervisor_mode condition to validate the config before starting Fluentd.

However, with supervisor_mode == true, the current logic overwrites the workers of all plugins that are not inside the worker directive.
This is the bug we have to fix.

After starting Fluentd, the for_this_worker? condition prevented this bug, so it was hard to notice.

This PR completely fixes this problem.

lib/fluent/plugin/base.rb

abetomo · 2023-02-25T02:26:28Z

conf of Fluent::TextParser.configure(conf) is Hash.
Thus, I added a conf.is_a?(Fluent::Config::Element) check.

!conf.target_worker_ids.empty? check is only necessary for Fluent::Engine.supervisor_mode == true.
So I changed to (Fluent::Engine.supervisor_mode && !conf.target_worker_ids.empty?).

daipom · 2023-02-26T04:18:06Z

Thanks for the fix and adding tests! I will see this soon.

lib/fluent/plugin/base.rb

abetomo · 2023-02-28T23:20:17Z

Thanks for the review!
I fixed the use of Hash in the test with another PR.
#4074

Override `system_config` only if `conf.target_worker_ids.size >= 1`. * If `<worker N>` or `<worker 0-N>` is not set, `conf.target_worker_ids` is empty. * If `<system> workers N </system>` is not set, `system_config.workers` defaults to 1. Signed-off-by: abetomo <[email protected]>

lib/fluent/plugin/base.rb

Signed-off-by: abetomo <[email protected]>

test/plugin/test_base.rb

Signed-off-by: abetomo <[email protected]>

lib/fluent/plugin/base.rb

* Change of order * Use `conf.for_every_workers?` instead of `conf.target_worker_ids.empty?`. Signed-off-by: abetomo <[email protected]>

daipom

LGTM.
Thanks for fixing this difficult issue!

ashie

LGTM

ashie · 2023-03-13T01:40:53Z

Thanks!

daipom self-requested a review February 20, 2023 23:45

daipom reviewed Feb 23, 2023

View reviewed changes

lib/fluent/plugin/base.rb Outdated Show resolved Hide resolved

abetomo force-pushed the fix-value-of-system_config_workers-at-run_configure branch from 355ca5f to 6438674 Compare February 25, 2023 02:22

abetomo marked this pull request as ready for review February 25, 2023 02:26

daipom reviewed Feb 27, 2023

View reviewed changes

lib/fluent/plugin/base.rb Outdated Show resolved Hide resolved

abetomo mentioned this pull request Feb 28, 2023

test: use Fluent::Config::Element in test #4074

Merged

abetomo mentioned this pull request Mar 9, 2023

Use driver for RegexpParser tests #4085

Merged

abetomo force-pushed the fix-value-of-system_config_workers-at-run_configure branch from 6438674 to b3a3aef Compare March 10, 2023 00:30

daipom reviewed Mar 10, 2023

View reviewed changes

lib/fluent/plugin/base.rb Outdated Show resolved Hide resolved

lib/fluent/plugin/base.rb Outdated Show resolved Hide resolved

Change to error if Hash is passed to Fluent::Plugin::Base::configure()

74c0a86

Signed-off-by: abetomo <[email protected]>

abetomo force-pushed the fix-value-of-system_config_workers-at-run_configure branch from b3a3aef to 74c0a86 Compare March 10, 2023 00:57

daipom reviewed Mar 10, 2023

View reviewed changes

test/plugin/test_base.rb Outdated Show resolved Hide resolved

Use data in tests

d691d6f

Signed-off-by: abetomo <[email protected]>

abetomo force-pushed the fix-value-of-system_config_workers-at-run_configure branch from 7b3315d to d691d6f Compare March 10, 2023 10:25

daipom reviewed Mar 10, 2023

View reviewed changes

lib/fluent/plugin/base.rb Outdated Show resolved Hide resolved

Improve conditions

a1b9913

* Change of order * Use `conf.for_every_workers?` instead of `conf.target_worker_ids.empty?`. Signed-off-by: abetomo <[email protected]>

daipom approved these changes Mar 10, 2023

View reviewed changes

ashie approved these changes Mar 13, 2023

View reviewed changes

ashie merged commit bbb304b into fluent:master Mar 13, 2023

abetomo deleted the fix-value-of-system_config_workers-at-run_configure branch March 13, 2023 02:04

ashie added this to the v1.16.0 milestone Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix value of `system_config.workers` at `run_configure` #4066

Fix value of `system_config.workers` at `run_configure` #4066

abetomo commented Feb 20, 2023 •

edited by daipom

Loading

daipom commented Feb 21, 2023

abetomo commented Feb 21, 2023

daipom commented Feb 21, 2023

abetomo commented Feb 21, 2023

daipom commented Feb 21, 2023 •

edited

Loading

daipom commented Feb 22, 2023

abetomo commented Feb 22, 2023

abetomo commented Feb 23, 2023

daipom commented Feb 23, 2023

daipom commented Feb 23, 2023

abetomo commented Feb 25, 2023

daipom commented Feb 26, 2023

abetomo commented Feb 28, 2023 •

edited

Loading

daipom left a comment

ashie left a comment

ashie commented Mar 13, 2023

Fix value of system_config.workers at run_configure #4066

Fix value of system_config.workers at run_configure #4066

Conversation

abetomo commented Feb 20, 2023 • edited by daipom Loading

daipom commented Feb 21, 2023

abetomo commented Feb 21, 2023

daipom commented Feb 21, 2023

abetomo commented Feb 21, 2023

daipom commented Feb 21, 2023 • edited Loading

daipom commented Feb 22, 2023

abetomo commented Feb 22, 2023

abetomo commented Feb 23, 2023

daipom commented Feb 23, 2023

daipom commented Feb 23, 2023

abetomo commented Feb 25, 2023

daipom commented Feb 26, 2023

abetomo commented Feb 28, 2023 • edited Loading

daipom left a comment

Choose a reason for hiding this comment

ashie left a comment

Choose a reason for hiding this comment

ashie commented Mar 13, 2023

Fix value of `system_config.workers` at `run_configure` #4066

Fix value of `system_config.workers` at `run_configure` #4066

abetomo commented Feb 20, 2023 •

edited by daipom

Loading

daipom commented Feb 21, 2023 •

edited

Loading

abetomo commented Feb 28, 2023 •

edited

Loading