diff --git a/changelogs/fragments/65422-fix-throttle-with-linear-strategy.yml b/changelogs/fragments/65422-fix-throttle-with-linear-strategy.yml new file mode 100644 index 00000000000..a9877897d82 --- /dev/null +++ b/changelogs/fragments/65422-fix-throttle-with-linear-strategy.yml @@ -0,0 +1,2 @@ +bugfixes: +- "throttle: the linear strategy didn't always stuck with the throttle limit" diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py index 2275e5590d1..d3ac58cf764 100644 --- a/lib/ansible/plugins/strategy/__init__.py +++ b/lib/ansible/plugins/strategy/__init__.py @@ -325,9 +325,26 @@ class StrategyBase: # and then queue the new task try: + # Determine the "rewind point" of the worker list. This means we start + # iterating over the list of workers until the end of the list is found. + # Normally, that is simply the length of the workers list (as determined + # by the forks or serial setting), however a task/block/play may "throttle" + # that limit down. + rewind_point = len(self._workers) + if throttle > 0 and self.ALLOW_BASE_THROTTLING: + if task.run_once: + display.debug("Ignoring 'throttle' as 'run_once' is also set for '%s'" % task.get_name()) + else: + if throttle <= rewind_point: + display.debug("task: %s, throttle: %d" % (task.get_name(), throttle)) + rewind_point = throttle + queued = False starting_worker = self._cur_worker while True: + if self._cur_worker >= rewind_point: + self._cur_worker = 0 + worker_prc = self._workers[self._cur_worker] if worker_prc is None or not worker_prc.is_alive(): self._queued_task_cache[(host.name, task._uuid)] = { @@ -346,19 +363,6 @@ class StrategyBase: self._cur_worker += 1 - # Determine the "rewind point" of the worker list. This means we start - # iterating over the list of workers until the end of the list is found. - # Normally, that is simply the length of the workers list (as determined - # by the forks or serial setting), however a task/block/play may "throttle" - # that limit down. - rewind_point = len(self._workers) - if throttle > 0 and self.ALLOW_BASE_THROTTLING: - if task.run_once: - display.debug("Ignoring 'throttle' as 'run_once' is also set for '%s'" % task.get_name()) - else: - if throttle <= rewind_point: - display.debug("task: %s, throttle: %d" % (task.get_name(), throttle)) - rewind_point = throttle if self._cur_worker >= rewind_point: self._cur_worker = 0 diff --git a/test/integration/targets/throttle/group_vars/all.yml b/test/integration/targets/throttle/group_vars/all.yml new file mode 100644 index 00000000000..b04b2aae112 --- /dev/null +++ b/test/integration/targets/throttle/group_vars/all.yml @@ -0,0 +1,4 @@ +--- +throttledir: '{{ base_throttledir }}/{{ subdir }}' +base_throttledir: "{{ lookup('env', 'OUTPUT_DIR') }}/throttle.dir" +subdir: "{{ test_id if lookup('env', 'SELECTED_STRATEGY') in ['free', 'host_pinned'] else '' }}" diff --git a/test/integration/targets/throttle/runme.sh b/test/integration/targets/throttle/runme.sh index 3a2a3da5331..0db5098db74 100755 --- a/test/integration/targets/throttle/runme.sh +++ b/test/integration/targets/throttle/runme.sh @@ -3,5 +3,5 @@ set -eux # https://github.com/ansible/ansible/pull/42528 -ANSIBLE_STRATEGY='linear' ansible-playbook test_throttle.yml -vv -i inventory --forks 12 "$@" -ANSIBLE_STRATEGY='free' ansible-playbook test_throttle.yml -vv -i inventory --forks 12 "$@" +SELECTED_STRATEGY='linear' ansible-playbook test_throttle.yml -vv -i inventory --forks 12 "$@" +SELECTED_STRATEGY='free' ansible-playbook test_throttle.yml -vv -i inventory --forks 12 "$@" diff --git a/test/integration/targets/throttle/test_throttle.py b/test/integration/targets/throttle/test_throttle.py index d108bd73b43..3ee8424eb75 100755 --- a/test/integration/targets/throttle/test_throttle.py +++ b/test/integration/targets/throttle/test_throttle.py @@ -24,6 +24,7 @@ try: if len(throttlelist) > max_throttle: print(throttlelist) raise ValueError("Too many concurrent tasks: %d/%d" % (len(throttlelist), max_throttle)) + time.sleep(1.5) finally: # remove the file, then wait to make sure it's gone os.unlink(throttlefile) diff --git a/test/integration/targets/throttle/test_throttle.yml b/test/integration/targets/throttle/test_throttle.yml index a2993ddc6cf..8990ea2f21a 100644 --- a/test/integration/targets/throttle/test_throttle.yml +++ b/test/integration/targets/throttle/test_throttle.yml @@ -1,59 +1,84 @@ --- - hosts: localhosts gather_facts: false - vars: - throttledir: "{{ lookup('env', 'OUTPUT_DIR') }}/throttle.dir/" + strategy: linear + run_once: yes tasks: - - name: Clean throttledir '{{ throttledir }}' + - name: Clean base throttledir '{{ base_throttledir }}' file: state: absent - path: '{{ throttledir }}' + path: '{{ base_throttledir }}' ignore_errors: yes - run_once: yes + - name: Create throttledir '{{ throttledir }}' file: state: directory path: '{{ throttledir }}' - run_once: yes + loop: "{{ range(1, test_count|int)|list }}" + loop_control: + loop_var: test_id + vars: + test_count: "{{ 9 if lookup('env', 'SELECTED_STRATEGY') in ['free', 'host_pinned'] else 2 }}" + +- hosts: localhosts + gather_facts: false + strategy: "{{ lookup('env', 'SELECTED_STRATEGY') }}" + tasks: - block: - name: "Test 1 (max throttle: 3)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 3" + vars: + test_id: 1 throttle: 3 - block: - name: "Test 2 (max throttle: 5)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 5" throttle: 5 + vars: + test_id: 2 - block: - name: "Test 3 (max throttle: 8)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 8" throttle: 8 throttle: 6 + vars: + test_id: 3 - block: - block: - name: "Test 4 (max throttle: 8)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 8" throttle: 8 + vars: + test_id: 4 throttle: 6 throttle: 12 throttle: 15 - block: - - name: "Test 1 (max throttle: 3)" + - name: "Teat 5 (max throttle: 3)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 3" + vars: + test_id: 5 throttle: 3 - block: - - name: "Test 2 (max throttle: 5)" + - name: "Test 6 (max throttle: 5)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 5" throttle: 5 + vars: + test_id: 6 - block: - - name: "Test 3 (max throttle: 6)" + - name: "Test 7 (max throttle: 6)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 6" throttle: 6 + vars: + test_id: 7 throttle: 3 - block: - block: - - name: "Test 4 (max throttle: 8)" + - name: "Test 8 (max throttle: 8)" script: "test_throttle.py {{throttledir}} {{inventory_hostname}} 8" throttle: 8 + vars: + test_id: 8 throttle: 6 throttle: 4 throttle: 2 diff --git a/test/units/plugins/strategy/test_strategy_base.py b/test/units/plugins/strategy/test_strategy_base.py index 33776ba54f1..1516deadab0 100644 --- a/test/units/plugins/strategy/test_strategy_base.py +++ b/test/units/plugins/strategy/test_strategy_base.py @@ -201,6 +201,7 @@ class TestStrategyBase(unittest.TestCase): mock_task = MagicMock() mock_task._uuid = 'abcd' + mock_task.throttle = 0 try: strategy_base = StrategyBase(tqm=tqm)