From 8325e107b65d007722d750f5d7ad5cbb1ccc5b08 Mon Sep 17 00:00:00 2001
From: abetomo <abe@enzou.tokyo>
Date: Sat, 25 Feb 2023 11:17:34 +0900
Subject: [PATCH 1/4] Fix value of `system_config.workers` at `run_configure`

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 <abe@enzou.tokyo>
---
 lib/fluent/plugin/base.rb |  12 ++--
 test/plugin/test_base.rb  | 144 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+), 7 deletions(-)

diff --git a/lib/fluent/plugin/base.rb b/lib/fluent/plugin/base.rb
index 3bd8f44224..fc2455f31a 100644
--- a/lib/fluent/plugin/base.rb
+++ b/lib/fluent/plugin/base.rb
@@ -52,14 +52,12 @@ def fluentd_worker_id
         @_fluentd_worker_id
       end
 
+      # @param conf [Fluent::Config::Element, Hash] configuration
       def configure(conf)
-        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
-          system_config_override(workers: workers)
+        if conf.is_a?(Fluent::Config::Element)
+          if (Fluent::Engine.supervisor_mode && !conf.target_worker_ids.empty?) || conf.for_this_worker?
+            system_config_override(workers: conf.target_worker_ids.size)
+          end
         end
         super(conf, system_config.strict_config_value)
         @_state ||= State.new(false, false, false, false, false, false, false, false, false)
diff --git a/test/plugin/test_base.rb b/test/plugin/test_base.rb
index b9567c9228..ffb5eb7667 100644
--- a/test/plugin/test_base.rb
+++ b/test/plugin/test_base.rb
@@ -146,4 +146,148 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
       end
     end
   end
+
+  sub_test_case 'system_config.workers value after configure' do
+    sub_test_case 'with <system> workers 3 </system>' do
+      setup do
+        system_config = Fluent::SystemConfig.new
+        system_config.workers = 3
+        stub(Fluent::Engine).system_config { system_config }
+      end
+
+      sub_test_case 'supervisor_mode is true' do
+        setup do
+          stub(Fluent::Engine).supervisor_mode { true }
+          stub(Fluent::Engine).worker_id { -1 }
+        end
+
+        test 'without <worker> directive' do
+          conf = config_element()
+          conf.set_target_worker_ids([])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 3 }
+        end
+
+        test 'with <worker 0>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 1 }
+        end
+
+        test 'with <worker 0-1>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0, 1])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 2 }
+        end
+
+        test 'with <worker 0-2>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0, 1, 2])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 3 }
+        end
+
+        test '`conf` is `Hash`' do
+          @p.configure({})
+          assert{ @p.system_config.workers == 3 }
+        end
+      end
+
+      sub_test_case 'supervisor_mode is false' do
+        setup do
+          stub(Fluent::Engine).supervisor_mode { false }
+          stub(Fluent::Engine).worker_id { 0 }
+        end
+
+        test 'without <worker> directive' do
+          conf = config_element()
+          conf.set_target_worker_ids([])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 3 }
+        end
+
+        test 'with <worker 0>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 1 }
+        end
+
+        test 'with <worker 0-1>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0, 1])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 2 }
+        end
+
+        test 'with <worker 0-2>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0, 1, 2])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 3 }
+        end
+
+        test '`conf` is `Hash`' do
+          @p.configure({})
+          assert{ @p.system_config.workers == 3 }
+        end
+      end
+    end
+
+    sub_test_case 'without <system> directive' do
+      sub_test_case 'supervisor_mode is true' do
+        setup do
+          stub(Fluent::Engine).supervisor_mode { true }
+          stub(Fluent::Engine).worker_id { -1 }
+        end
+
+        test 'without <worker> directive' do
+          conf = config_element()
+          conf.set_target_worker_ids([])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 1 }
+        end
+
+        test 'with <worker 0>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 1 }
+        end
+
+        test '`conf` is `Hash`' do
+          @p.configure({})
+          assert{ @p.system_config.workers == 1 }
+        end
+      end
+
+      sub_test_case 'supervisor_mode is false' do
+        setup do
+          stub(Fluent::Engine).supervisor_mode { false }
+          stub(Fluent::Engine).worker_id { 0 }
+        end
+
+        test 'without <worker> directive' do
+          conf = config_element()
+          conf.set_target_worker_ids([])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 1 }
+        end
+
+        test 'with <worker 0>' do
+          conf = config_element()
+          conf.set_target_worker_ids([0])
+          @p.configure(conf)
+          assert{ @p.system_config.workers == 1 }
+        end
+
+        test '`conf` is `Hash`' do
+          @p.configure({})
+          assert{ @p.system_config.workers == 1 }
+        end
+      end
+    end
+  end
 end

From 74c0a86a0c42e6fba0079b58dc57c7f3721c2512 Mon Sep 17 00:00:00 2001
From: abetomo <abe@enzou.tokyo>
Date: Fri, 10 Mar 2023 09:57:01 +0900
Subject: [PATCH 2/4] Change to error if Hash is passed to
 `Fluent::Plugin::Base::configure()`

Signed-off-by: abetomo <abe@enzou.tokyo>
---
 lib/fluent/plugin/base.rb | 10 +++++-----
 test/plugin/test_base.rb  | 26 ++++++--------------------
 2 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/lib/fluent/plugin/base.rb b/lib/fluent/plugin/base.rb
index fc2455f31a..aa1ff16ab9 100644
--- a/lib/fluent/plugin/base.rb
+++ b/lib/fluent/plugin/base.rb
@@ -52,13 +52,13 @@ def fluentd_worker_id
         @_fluentd_worker_id
       end
 
-      # @param conf [Fluent::Config::Element, Hash] configuration
       def configure(conf)
-        if conf.is_a?(Fluent::Config::Element)
-          if (Fluent::Engine.supervisor_mode && !conf.target_worker_ids.empty?) || conf.for_this_worker?
-            system_config_override(workers: conf.target_worker_ids.size)
-          end
+        raise ArgumentError, "BUG: type of conf must be Fluent::Config::Element, but #{conf.class} is passed." unless conf.is_a?(Fluent::Config::Element)
+
+        if (Fluent::Engine.supervisor_mode && !conf.target_worker_ids.empty?) || conf.for_this_worker?
+          system_config_override(workers: conf.target_worker_ids.size)
         end
+
         super(conf, system_config.strict_config_value)
         @_state ||= State.new(false, false, false, false, false, false, false, false, false)
         @_state.configure = true
diff --git a/test/plugin/test_base.rb b/test/plugin/test_base.rb
index ffb5eb7667..ab2274f679 100644
--- a/test/plugin/test_base.rb
+++ b/test/plugin/test_base.rb
@@ -147,6 +147,12 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
     end
   end
 
+  test '`ArgumentError` when `conf` is not `Fluent::Config::Element`' do
+    assert_raise ArgumentError.new('BUG: type of conf must be Fluent::Config::Element, but Hash is passed.') do
+      @p.configure({})
+    end
+  end
+
   sub_test_case 'system_config.workers value after configure' do
     sub_test_case 'with <system> workers 3 </system>' do
       setup do
@@ -188,11 +194,6 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
           @p.configure(conf)
           assert{ @p.system_config.workers == 3 }
         end
-
-        test '`conf` is `Hash`' do
-          @p.configure({})
-          assert{ @p.system_config.workers == 3 }
-        end
       end
 
       sub_test_case 'supervisor_mode is false' do
@@ -228,11 +229,6 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
           @p.configure(conf)
           assert{ @p.system_config.workers == 3 }
         end
-
-        test '`conf` is `Hash`' do
-          @p.configure({})
-          assert{ @p.system_config.workers == 3 }
-        end
       end
     end
 
@@ -256,11 +252,6 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
           @p.configure(conf)
           assert{ @p.system_config.workers == 1 }
         end
-
-        test '`conf` is `Hash`' do
-          @p.configure({})
-          assert{ @p.system_config.workers == 1 }
-        end
       end
 
       sub_test_case 'supervisor_mode is false' do
@@ -282,11 +273,6 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
           @p.configure(conf)
           assert{ @p.system_config.workers == 1 }
         end
-
-        test '`conf` is `Hash`' do
-          @p.configure({})
-          assert{ @p.system_config.workers == 1 }
-        end
       end
     end
   end

From d691d6f33d655d8387743c81f51555edd24182b7 Mon Sep 17 00:00:00 2001
From: abetomo <abe@enzou.tokyo>
Date: Fri, 10 Mar 2023 19:25:22 +0900
Subject: [PATCH 3/4] Use `data` in tests

Signed-off-by: abetomo <abe@enzou.tokyo>
---
 test/plugin/test_base.rb | 176 ++++++++++++++++-----------------------
 1 file changed, 72 insertions(+), 104 deletions(-)

diff --git a/test/plugin/test_base.rb b/test/plugin/test_base.rb
index ab2274f679..479ecc49f6 100644
--- a/test/plugin/test_base.rb
+++ b/test/plugin/test_base.rb
@@ -154,6 +154,18 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
   end
 
   sub_test_case 'system_config.workers value after configure' do
+    def assert_system_config_workers_value(data)
+      conf = config_element()
+      conf.set_target_worker_ids(data[:target_worker_ids])
+      @p.configure(conf)
+      assert{ @p.system_config.workers == data[:expected] }
+    end
+
+    def stub_supervisor_mode
+      stub(Fluent::Engine).supervisor_mode { true }
+      stub(Fluent::Engine).worker_id { -1 }
+    end
+
     sub_test_case 'with <system> workers 3 </system>' do
       setup do
         system_config = Fluent::SystemConfig.new
@@ -161,118 +173,74 @@ class FluentPluginBaseTest::DummyPlugin2 < Fluent::Plugin::TestBase
         stub(Fluent::Engine).system_config { system_config }
       end
 
-      sub_test_case 'supervisor_mode is true' do
-        setup do
-          stub(Fluent::Engine).supervisor_mode { true }
-          stub(Fluent::Engine).worker_id { -1 }
-        end
-
-        test 'without <worker> directive' do
-          conf = config_element()
-          conf.set_target_worker_ids([])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 3 }
-        end
-
-        test 'with <worker 0>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 1 }
-        end
-
-        test 'with <worker 0-1>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0, 1])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 2 }
-        end
-
-        test 'with <worker 0-2>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0, 1, 2])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 3 }
-        end
+      data(
+        'without <worker> directive',
+        {
+          target_worker_ids: [],
+          expected: 3
+        },
+        keep: true
+      )
+      data(
+        'with <worker 0>',
+        {
+          target_worker_ids: [0],
+          expected: 1
+        },
+        keep: true
+      )
+      data(
+        'with <worker 0-1>',
+        {
+          target_worker_ids: [0, 1],
+          expected: 2
+        },
+        keep: true
+      )
+      data(
+        'with <worker 0-2>',
+        {
+          target_worker_ids: [0, 1, 2],
+          expected: 3
+        },
+        keep: true
+      )
+
+      test 'system_config.workers value after configure' do
+        assert_system_config_workers_value(data)
       end
 
-      sub_test_case 'supervisor_mode is false' do
-        setup do
-          stub(Fluent::Engine).supervisor_mode { false }
-          stub(Fluent::Engine).worker_id { 0 }
-        end
-
-        test 'without <worker> directive' do
-          conf = config_element()
-          conf.set_target_worker_ids([])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 3 }
-        end
-
-        test 'with <worker 0>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 1 }
-        end
-
-        test 'with <worker 0-1>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0, 1])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 2 }
-        end
-
-        test 'with <worker 0-2>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0, 1, 2])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 3 }
-        end
+      test 'system_config.workers value after configure  with supervisor_mode' do
+        stub_supervisor_mode
+        assert_system_config_workers_value(data)
       end
     end
 
     sub_test_case 'without <system> directive' do
-      sub_test_case 'supervisor_mode is true' do
-        setup do
-          stub(Fluent::Engine).supervisor_mode { true }
-          stub(Fluent::Engine).worker_id { -1 }
-        end
-
-        test 'without <worker> directive' do
-          conf = config_element()
-          conf.set_target_worker_ids([])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 1 }
-        end
-
-        test 'with <worker 0>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 1 }
-        end
+      data(
+        'without <worker> directive',
+        {
+          target_worker_ids: [],
+          expected: 1
+        },
+        keep: true
+      )
+      data(
+        'with <worker 0>',
+        {
+          target_worker_ids: [0],
+          expected: 1
+        },
+        keep: true
+      )
+
+      test 'system_config.workers value after configure' do
+        assert_system_config_workers_value(data)
       end
 
-      sub_test_case 'supervisor_mode is false' do
-        setup do
-          stub(Fluent::Engine).supervisor_mode { false }
-          stub(Fluent::Engine).worker_id { 0 }
-        end
-
-        test 'without <worker> directive' do
-          conf = config_element()
-          conf.set_target_worker_ids([])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 1 }
-        end
-
-        test 'with <worker 0>' do
-          conf = config_element()
-          conf.set_target_worker_ids([0])
-          @p.configure(conf)
-          assert{ @p.system_config.workers == 1 }
-        end
+      test 'system_config.workers value after configure  with supervisor_mode' do
+        stub_supervisor_mode
+        assert_system_config_workers_value(data)
       end
     end
   end

From a1b9913a8b93d6700588f6e013e9a119557f3c5d Mon Sep 17 00:00:00 2001
From: abetomo <abe@enzou.tokyo>
Date: Fri, 10 Mar 2023 19:56:34 +0900
Subject: [PATCH 4/4] Improve conditions

* Change of order
* Use `conf.for_every_workers?` instead of `conf.target_worker_ids.empty?`.

Signed-off-by: abetomo <abe@enzou.tokyo>
---
 lib/fluent/plugin/base.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fluent/plugin/base.rb b/lib/fluent/plugin/base.rb
index aa1ff16ab9..b0cee6ca03 100644
--- a/lib/fluent/plugin/base.rb
+++ b/lib/fluent/plugin/base.rb
@@ -55,7 +55,7 @@ def fluentd_worker_id
       def configure(conf)
         raise ArgumentError, "BUG: type of conf must be Fluent::Config::Element, but #{conf.class} is passed." unless conf.is_a?(Fluent::Config::Element)
 
-        if (Fluent::Engine.supervisor_mode && !conf.target_worker_ids.empty?) || conf.for_this_worker?
+        if conf.for_this_worker? || (Fluent::Engine.supervisor_mode && !conf.for_every_workers?)
           system_config_override(workers: conf.target_worker_ids.size)
         end