From d15051b187b1c613a526c39c468a96d4449c0890 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 13 Aug 2024 10:35:50 +0100 Subject: [PATCH 01/21] Begin v0.3.10dev --- docs/changelog.rst | 5 +++++ mitogen/__init__.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 7c6fc96d..0d1a14a2 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -18,6 +18,11 @@ To avail of fixes in an unreleased version, please download a ZIP file `directly from GitHub `_. +Unreleased +---------- + + + v0.3.9 (2024-08-13) ------------------- diff --git a/mitogen/__init__.py b/mitogen/__init__.py index b0c66793..1d483ead 100644 --- a/mitogen/__init__.py +++ b/mitogen/__init__.py @@ -35,7 +35,7 @@ be expected. On the slave, it is built dynamically during startup. #: Library version as a tuple. -__version__ = (0, 3, 9) +__version__ = (0, 3, 10, 'dev') #: This is :data:`False` in slave contexts. Previously it was used to prevent From 06617f8231b4facc0cd7b751b72293a7d8744a5c Mon Sep 17 00:00:00 2001 From: Jonathan Rosser Date: Tue, 16 Jul 2024 15:38:40 +0100 Subject: [PATCH 02/21] ansible_mitogen: Handle unsafe paths in _remote_chmod This is missing from https://github.com/mitogen-hq/mitogen/commit/b822f20007ebe94106b15275962ea4cbbd8a0331 --- ansible_mitogen/mixins.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index 0ba41aad..2cd97a3e 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -280,7 +280,9 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): paths, mode, sudoable) return self.fake_shell(lambda: mitogen.select.Select.all( self._connection.get_chain().call_async( - ansible_mitogen.target.set_file_mode, path, mode + ansible_mitogen.target.set_file_mode, + ansible_mitogen.utils.unsafe.cast(path), + mode, ) for path in paths )) From 5ab872f28930a70e03253d0fb97acbbee2a47430 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Thu, 29 Aug 2024 11:07:35 +0100 Subject: [PATCH 03/21] ansible_mitogen: Add regression test for ActionModuleMixin._remote_chmod() Adapted from Jonathon's reproducer in #1087. --- docs/changelog.rst | 2 + docs/contributors.rst | 1 + tests/ansible/regression/all.yml | 1 + .../issue_1087__template_streamerror.yml | 43 +++++++++++++++++++ tests/ansible/regression/template_test.yml | 28 ++++++++++++ tests/ansible/regression/templates/foo.bar.j2 | 1 + 6 files changed, 76 insertions(+) create mode 100644 tests/ansible/regression/issue_1087__template_streamerror.yml create mode 100644 tests/ansible/regression/template_test.yml create mode 100644 tests/ansible/regression/templates/foo.bar.j2 diff --git a/docs/changelog.rst b/docs/changelog.rst index 0d1a14a2..63b4da12 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -21,6 +21,8 @@ To avail of fixes in an unreleased version, please download a ZIP file Unreleased ---------- +* :gh:issue:`1087` Fix :exc:`mitogen.core.StreamError` when Ansible template + module is called with a ``dest:`` filename that has an extension v0.3.9 (2024-08-13) diff --git a/docs/contributors.rst b/docs/contributors.rst index ed7fef11..1a590869 100644 --- a/docs/contributors.rst +++ b/docs/contributors.rst @@ -125,6 +125,7 @@ sponsorship and outstanding future-thinking of its early adopters.
  • rkrzr
  • jgadling
  • John F Wall — Making Ansible Great with Massive Parallelism
    • +
  • Jonathan Rosser
  • KennethC
  • Luca Berruti
  • Lewis Bellwood — Happy to be apart of a great project.
    • diff --git a/tests/ansible/regression/all.yml b/tests/ansible/regression/all.yml index 31541e9f..a4272805 100644 --- a/tests/ansible/regression/all.yml +++ b/tests/ansible/regression/all.yml @@ -16,3 +16,4 @@ - import_playbook: issue_776__load_plugins_called_twice.yml - import_playbook: issue_952__ask_become_pass.yml - import_playbook: issue_1066__add_host__host_key_checking.yml +- import_playbook: issue_1087__template_streamerror.yml diff --git a/tests/ansible/regression/issue_1087__template_streamerror.yml b/tests/ansible/regression/issue_1087__template_streamerror.yml new file mode 100644 index 00000000..fa950ea4 --- /dev/null +++ b/tests/ansible/regression/issue_1087__template_streamerror.yml @@ -0,0 +1,43 @@ +- name: regression/issue_1087__template_streamerror.yml + # Ansible's template module has been seen to raise mitogen.core.StreamError + # iif there is a with_items loop and the destination path has an extension. + # This printed an error message and left file permissions incorrect, + # but did not cause the task/playbook to fail. + hosts: test-targets + gather_facts: false + become: false + vars: + foos: + - dest: /tmp/foo + - dest: /tmp/foo.txt + foo: Foo + bar: Bar + tasks: + - block: + - name: Test template does not cause StreamError + delegate_to: localhost + run_once: true + environment: + ANSIBLE_VERBOSITY: "{{ ansible_verbosity }}" + command: + cmd: > + ansible-playbook + {% for inv in ansible_inventory_sources %} + -i "{{ inv }}" + {% endfor %} + regression/template_test.yml + chdir: ../ + register: issue_1087_cmd + failed_when: + - issue_1087_cmd is failed + or issue_1087_cmd.stdout is search('ERROR|mitogen\.core\.CallError') + or issue_1087_cmd.stderr is search('ERROR|mitogen\.core\.CallError') + + always: + - name: Cleanup + file: + path: "{{ item.dest }}" + state: absent + with_items: "{{ foos }}" + tags: + - issue_1087 diff --git a/tests/ansible/regression/template_test.yml b/tests/ansible/regression/template_test.yml new file mode 100644 index 00000000..0b7dd36d --- /dev/null +++ b/tests/ansible/regression/template_test.yml @@ -0,0 +1,28 @@ +- name: regression/template_test.yml + # Ansible's template module has been seen to raise mitogen.core.StreamError + # iif there is a with_items loop and the destination path has an extension + hosts: test-targets + gather_facts: false + become: false + vars: + foos: + - dest: /tmp/foo + - dest: /tmp/foo.txt + foo: Foo + bar: Bar + tasks: + - block: + - name: Template files + template: + src: foo.bar.j2 + dest: "{{ item.dest }}" + mode: u=rw,go=r + # This has to be with_items, loop: doesn't trigger the bug + with_items: "{{ foos }}" + + always: + - name: Cleanup + file: + path: "{{ item.dest }}" + state: absent + with_items: "{{ foos }}" diff --git a/tests/ansible/regression/templates/foo.bar.j2 b/tests/ansible/regression/templates/foo.bar.j2 new file mode 100644 index 00000000..ca51a6f4 --- /dev/null +++ b/tests/ansible/regression/templates/foo.bar.j2 @@ -0,0 +1 @@ +A {{ foo }} walks into a {{ bar }}. Ow! From ce1accedbce1d841d2fa88676fabbdaa1487f92f Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Fri, 30 Aug 2024 16:00:52 +0100 Subject: [PATCH 04/21] tests: Refactor Ansible copy integration tests to be loop driven This is in anticipation of #1110, which only exhibits inside a with_items: loop. For this refactor `loop:` is used, to confirm the refactored tests are still correct. A subsequent commit will change them to with_items. The content of the files and their SHA1 checksums are unchanged. --- tests/ansible/integration/action/copy.yml | 125 +++++++++++----------- 1 file changed, 62 insertions(+), 63 deletions(-) diff --git a/tests/ansible/integration/action/copy.yml b/tests/ansible/integration/action/copy.yml index 73f3bd1e..cbebe3ab 100644 --- a/tests/ansible/integration/action/copy.yml +++ b/tests/ansible/integration/action/copy.yml @@ -2,91 +2,90 @@ - name: integration/action/copy.yml hosts: test-targets - tasks: - - name: Create tiny file - copy: - dest: /tmp/copy-tiny-file - content: - this is a tiny file. - delegate_to: localhost - run_once: true + vars: + sourced_files: + - src: /tmp/copy-tiny-file + dest: /tmp/copy-tiny-file.out + content: this is a tiny file. + expected_checksum: f29faa9a6f19a700a941bf2aa5b281643c4ec8a0 + - src: /tmp/copy-large-file + dest: /tmp/copy-large-file.out + content: "{{ 'x' * 200000 }}" + expected_checksum: 62951f943c41cdd326e5ce2b53a779e7916a820d + + inline_files: + - dest: /tmp/copy-tiny-inline-file.out + content: tiny inline content + expected_checksum: b26dd6444595e2bdb342aa0a91721b57478b5029 + - dest: /tmp/copy-large-inline-file.out + content: | + {{ 'y' * 200000 }} + expected_checksum: d675f47e467eae19e49032a2cc39118e12a6ee72 - - name: Create large file + files: "{{ sourced_files + inline_files }}" + tasks: + - name: Create sourced files copy: - dest: /tmp/copy-large-file - # Must be larger than Connection.SMALL_SIZE_LIMIT. - content: "{% for x in range(200000) %}x{% endfor %}" + dest: "{{ item.src }}" + content: "{{ item.content }}" + mode: u=rw,go=r + loop: "{{ sourced_files }}" + loop_control: + label: "{{ item.src }}" delegate_to: localhost run_once: true - - name: Cleanup copied files + - name: Cleanup lingering destination files file: + path: "{{ item.dest }}" state: absent - path: "{{item}}" - with_items: - - /tmp/copy-tiny-file.out - - /tmp/copy-large-file.out - - /tmp/copy-tiny-inline-file.out - - /tmp/copy-large-inline-file.out + loop: "{{ files }}" + loop_control: + label: "{{ item.dest }}" - - name: Copy large file + - name: Copy sourced files copy: - dest: /tmp/copy-large-file.out - src: /tmp/copy-large-file - - - name: Copy tiny file - copy: - dest: /tmp/copy-tiny-file.out - src: /tmp/copy-tiny-file + src: "{{ item.src }}" + dest: "{{ item.dest }}" + mode: u=rw,go=r + loop: "{{ sourced_files }}" + loop_control: + label: "{{ item.dest }}" - - name: Copy tiny inline file + - name: Copy inline files copy: - dest: /tmp/copy-tiny-inline-file.out - content: "tiny inline content" - - - name: Copy large inline file - copy: - dest: /tmp/copy-large-inline-file.out - content: | - {% for x in range(200000) %}y{% endfor %} + dest: "{{ item.dest }}" + content: "{{ item.content }}" + mode: u=rw,go=r + loop: "{{ inline_files }}" + loop_control: + label: "{{ item.dest }}" # stat results - name: Stat copied files stat: - path: "{{item}}" - with_items: - - /tmp/copy-tiny-file.out - - /tmp/copy-large-file.out - - /tmp/copy-tiny-inline-file.out - - /tmp/copy-large-inline-file.out + path: "{{ item.dest }}" + loop: "{{ files }}" + loop_control: + label: "{{ item.dest }}" register: stat - assert: that: - - stat.results[0].stat.checksum == "f29faa9a6f19a700a941bf2aa5b281643c4ec8a0" - - stat.results[1].stat.checksum == "62951f943c41cdd326e5ce2b53a779e7916a820d" - - stat.results[2].stat.checksum == "b26dd6444595e2bdb342aa0a91721b57478b5029" - - stat.results[3].stat.checksum == "d675f47e467eae19e49032a2cc39118e12a6ee72" - fail_msg: stat={{stat}} + - item.stat.checksum == item.item.expected_checksum + quiet: true # Avoid spamming stdout with 400 kB of item.item.content + fail_msg: item={{ item }} + loop: "{{ stat.results }}" + loop_control: + label: "{{ item.stat.path }}" - - name: Cleanup files + - name: Cleanup destination files file: + path: "{{ item.dest }}" state: absent - path: "{{item}}" - with_items: - - /tmp/copy-tiny-file - - /tmp/copy-tiny-file.out - - /tmp/copy-no-mode - - /tmp/copy-no-mode.out - - /tmp/copy-with-mode - - /tmp/copy-with-mode.out - - /tmp/copy-large-file - - /tmp/copy-large-file.out - - /tmp/copy-tiny-inline-file.out - - /tmp/copy-large-inline-file - - /tmp/copy-large-inline-file.out - - # end of cleaning out files (again) + loop: "{{ files }}" + loop_control: + label: "{{ item.dest }}" tags: - copy From 0bd3c6cba50bf9e607aa13432586af8af4fe51ba Mon Sep 17 00:00:00 2001 From: Jonathan Rosser Date: Wed, 28 Aug 2024 08:24:35 +0100 Subject: [PATCH 05/21] Fix AnsibleUnsafeText when copying files larger than SMALL_FILE_LIMIT Small files are carried in-band in the communication between controller and remote, with larger files being copied by falling back to a more traditional ansible put_file mechanism. This large file code path was missed in b822f20. --- ansible_mitogen/connection.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index 6bdf11ba..a3f66eac 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -1129,6 +1129,6 @@ class Connection(ansible.plugins.connection.ConnectionBase): self.get_chain().call( ansible_mitogen.target.transfer_file, context=self.binding.get_child_service_context(), - in_path=in_path, - out_path=out_path + in_path=ansible_mitogen.utils.unsafe.cast(in_path), + out_path=ansible_mitogen.utils.unsafe.cast(out_path) ) From 5af6534a70eb03a71c5a181bd2abcf77e2be903e Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Fri, 30 Aug 2024 17:19:20 +0100 Subject: [PATCH 06/21] tests: Test AnsibleUnsafeText when copying files larger SMALL_FILE_LIMIT The bug was fixed in a previous commit by Jonathan Rosser. This adds testing. The bug is only triggered when the copy module is used inside a `with_items:` loop and the destination filename has an extension. A `loop:` loop is not sufficient. refs #1110 --- docs/changelog.rst | 3 +++ tests/ansible/integration/action/copy.yml | 17 ++++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 63b4da12..4fc177c1 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -23,6 +23,9 @@ Unreleased * :gh:issue:`1087` Fix :exc:`mitogen.core.StreamError` when Ansible template module is called with a ``dest:`` filename that has an extension +* :gh:issue:`1110` Fix :exc:`mitogen.core.StreamError` when Ansible copy + module is called with a file larger than 124 kibibytes + (:data:`ansible_mitogen.connection.Connection.SMALL_FILE_LIMIT`) v0.3.9 (2024-08-13) diff --git a/tests/ansible/integration/action/copy.yml b/tests/ansible/integration/action/copy.yml index cbebe3ab..edaa3e49 100644 --- a/tests/ansible/integration/action/copy.yml +++ b/tests/ansible/integration/action/copy.yml @@ -1,4 +1,6 @@ # Verify copy module for small and large files, and inline content. +# To exercise https://github.com/mitogen-hq/mitogen/pull/1110 destination +# files must have extensions and loops must use `with_items:`. - name: integration/action/copy.yml hosts: test-targets @@ -29,7 +31,7 @@ dest: "{{ item.src }}" content: "{{ item.content }}" mode: u=rw,go=r - loop: "{{ sourced_files }}" + with_items: "{{ sourced_files }}" loop_control: label: "{{ item.src }}" delegate_to: localhost @@ -39,7 +41,7 @@ file: path: "{{ item.dest }}" state: absent - loop: "{{ files }}" + with_items: "{{ files }}" loop_control: label: "{{ item.dest }}" @@ -48,7 +50,7 @@ src: "{{ item.src }}" dest: "{{ item.dest }}" mode: u=rw,go=r - loop: "{{ sourced_files }}" + with_items: "{{ sourced_files }}" loop_control: label: "{{ item.dest }}" @@ -57,7 +59,7 @@ dest: "{{ item.dest }}" content: "{{ item.content }}" mode: u=rw,go=r - loop: "{{ inline_files }}" + with_items: "{{ inline_files }}" loop_control: label: "{{ item.dest }}" @@ -66,7 +68,7 @@ - name: Stat copied files stat: path: "{{ item.dest }}" - loop: "{{ files }}" + with_items: "{{ files }}" loop_control: label: "{{ item.dest }}" register: stat @@ -76,7 +78,7 @@ - item.stat.checksum == item.item.expected_checksum quiet: true # Avoid spamming stdout with 400 kB of item.item.content fail_msg: item={{ item }} - loop: "{{ stat.results }}" + with_items: "{{ stat.results }}" loop_control: label: "{{ item.stat.path }}" @@ -84,8 +86,9 @@ file: path: "{{ item.dest }}" state: absent - loop: "{{ files }}" + with_items: "{{ files }}" loop_control: label: "{{ item.dest }}" tags: - copy + - issue_1110 From 46c9f772d84e6a42c8774ad38a5be6f804f06f39 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 3 Sep 2024 18:25:56 +0100 Subject: [PATCH 07/21] tests: Simplify Ansible ssh password tests, test priority This - Removes the indirection of calling ansible in a sub-shell - Includes vanilla Ansible, which was previously skipped - Tests whether ansible_ssh_pass overrides ansible_password, as it should As a one off I've the new tests against vanilla Ansible 2.10 through Ansible 10, to confirm the baseline priorities have remained unchanged all releases currently supported by Mitogen 0.3.x. --- tests/ansible/integration/ssh/all.yml | 1 + tests/ansible/integration/ssh/password.yml | 51 ++++++++ tests/ansible/integration/ssh/variables.yml | 128 -------------------- 3 files changed, 52 insertions(+), 128 deletions(-) create mode 100644 tests/ansible/integration/ssh/password.yml diff --git a/tests/ansible/integration/ssh/all.yml b/tests/ansible/integration/ssh/all.yml index a8335ab7..1b0f36e4 100644 --- a/tests/ansible/integration/ssh/all.yml +++ b/tests/ansible/integration/ssh/all.yml @@ -1,3 +1,4 @@ - import_playbook: config.yml +- import_playbook: password.yml - import_playbook: timeouts.yml - import_playbook: variables.yml diff --git a/tests/ansible/integration/ssh/password.yml b/tests/ansible/integration/ssh/password.yml new file mode 100644 index 00000000..cf9396e0 --- /dev/null +++ b/tests/ansible/integration/ssh/password.yml @@ -0,0 +1,51 @@ +- name: integration/ssh/password.yml + hosts: test-targets[0] + gather_facts: false + vars: + ansible_user: mitogen__user1 + tasks: + - meta: reset_connection + - name: ansible_password + vars: + ansible_password: user1_password + ping: + + - meta: reset_connection + - name: ansible_ssh_pass + vars: + ansible_ssh_pass: user1_password + ping: + + - meta: reset_connection + - name: absent password should fail + ping: + ignore_errors: true + ignore_unreachable: true + register: ssh_no_password_result + - assert: + that: + - ssh_no_password_result.unreachable == True + fail_msg: ssh_no_password_result={{ ssh_no_password_result }} + + - meta: reset_connection + - name: ansible_ssh_pass should override ansible_password + ping: + vars: + ansible_password: wrong + ansible_ssh_pass: user1_password + + # Tests that ansible_ssh_pass has priority over ansible_password + # and that a wrong password causes a target to be marked unreachable. + - meta: reset_connection + - name: ansible_password should not override + vars: + ansible_password: user1_password + ansible_ssh_pass: wrong + ping: + ignore_errors: true + ignore_unreachable: true + register: ssh_wrong_password_result + - assert: + that: + - ssh_wrong_password_result.unreachable == True + fail_msg: ssh_wrong_password_result={{ ssh_wrong_password_result }} diff --git a/tests/ansible/integration/ssh/variables.yml b/tests/ansible/integration/ssh/variables.yml index d2fa683b..9f5b16bc 100644 --- a/tests/ansible/integration/ssh/variables.yml +++ b/tests/ansible/integration/ssh/variables.yml @@ -13,134 +13,6 @@ -o "ControlPath /tmp/mitogen-ansible-test-{{18446744073709551615|random}}" tasks: - - include_tasks: ../_mitogen_only.yml - - - name: ansible_ssh_user, ansible_ssh_pass - shell: > - ANSIBLE_ANY_ERRORS_FATAL=false - ANSIBLE_STRATEGY=mitogen_linear - ANSIBLE_SSH_ARGS="-o HostKeyAlgorithms=+ssh-rsa -o PubkeyAcceptedKeyTypes=+ssh-rsa" - ANSIBLE_VERBOSITY="{{ ansible_verbosity }}" - ansible -m shell -a whoami - {% for inv in ansible_inventory_sources %} - -i "{{ inv }}" - {% endfor %} - test-targets - -e ansible_ssh_user=mitogen__has_sudo - -e ansible_ssh_pass=has_sudo_password - args: - chdir: ../.. - register: out - - - name: ansible_ssh_user, wrong ansible_ssh_pass - shell: > - ANSIBLE_ANY_ERRORS_FATAL=false - ANSIBLE_STRATEGY=mitogen_linear - ANSIBLE_SSH_ARGS="-o HostKeyAlgorithms=+ssh-rsa -o PubkeyAcceptedKeyTypes=+ssh-rsa" - ANSIBLE_VERBOSITY="{{ ansible_verbosity }}" - ansible -m shell -a whoami - {% for inv in ansible_inventory_sources %} - -i "{{ inv }}" - {% endfor %} - test-targets - -e ansible_ssh_user=mitogen__has_sudo - -e ansible_ssh_pass=wrong_password - -e ansible_python_interpreter=python3000 - args: - chdir: ../.. - register: out - ignore_errors: true - - - assert: - that: - - out.rc == 4 # ansible.executor.task_queue_manager.TaskQueueManager.RUN_UNREACHABLE_HOSTS - fail_msg: out={{out}} - - - - name: ansible_user, ansible_ssh_pass - shell: > - ANSIBLE_ANY_ERRORS_FATAL=false - ANSIBLE_STRATEGY=mitogen_linear - ANSIBLE_SSH_ARGS="-o HostKeyAlgorithms=+ssh-rsa -o PubkeyAcceptedKeyTypes=+ssh-rsa" - ANSIBLE_VERBOSITY="{{ ansible_verbosity }}" - ansible -m shell -a whoami - {% for inv in ansible_inventory_sources %} - -i "{{ inv }}" - {% endfor %} - test-targets - -e ansible_user=mitogen__has_sudo - -e ansible_ssh_pass=has_sudo_password - args: - chdir: ../.. - register: out - - - name: ansible_user, wrong ansible_ssh_pass - shell: > - ANSIBLE_ANY_ERRORS_FATAL=false - ANSIBLE_STRATEGY=mitogen_linear - ANSIBLE_SSH_ARGS="-o HostKeyAlgorithms=+ssh-rsa -o PubkeyAcceptedKeyTypes=+ssh-rsa" - ANSIBLE_VERBOSITY="{{ ansible_verbosity }}" - ansible -m shell -a whoami - {% for inv in ansible_inventory_sources %} - -i "{{ inv }}" - {% endfor %} - test-targets - -e ansible_user=mitogen__has_sudo - -e ansible_ssh_pass=wrong_password - -e ansible_python_interpreter=python3000 - args: - chdir: ../.. - register: out - ignore_errors: true - - - assert: - that: - - out.rc == 4 # ansible.executor.task_queue_manager.TaskQueueManager.RUN_UNREACHABLE_HOSTS - fail_msg: out={{out}} - - - - name: ansible_user, ansible_password - shell: > - ANSIBLE_ANY_ERRORS_FATAL=false - ANSIBLE_STRATEGY=mitogen_linear - ANSIBLE_SSH_ARGS="-o HostKeyAlgorithms=+ssh-rsa -o PubkeyAcceptedKeyTypes=+ssh-rsa" - ANSIBLE_VERBOSITY="{{ ansible_verbosity }}" - ansible -m shell -a whoami - {% for inv in ansible_inventory_sources %} - -i "{{ inv }}" - {% endfor %} - test-targets - -e ansible_user=mitogen__has_sudo - -e ansible_password=has_sudo_password - args: - chdir: ../.. - register: out - - - name: ansible_user, wrong ansible_password - shell: > - ANSIBLE_ANY_ERRORS_FATAL=false - ANSIBLE_STRATEGY=mitogen_linear - ANSIBLE_SSH_ARGS="-o HostKeyAlgorithms=+ssh-rsa -o PubkeyAcceptedKeyTypes=+ssh-rsa" - ANSIBLE_VERBOSITY="{{ ansible_verbosity }}" - ansible -m shell -a whoami - {% for inv in ansible_inventory_sources %} - -i "{{ inv }}" - {% endfor %} - test-targets - -e ansible_user=mitogen__has_sudo - -e ansible_password=wrong_password - -e ansible_python_interpreter=python3000 - args: - chdir: ../.. - register: out - ignore_errors: true - - - assert: - that: - - out.rc == 4 # ansible.executor.task_queue_manager.TaskQueueManager.RUN_UNREACHABLE_HOSTS - fail_msg: out={{out}} - - - name: setup ansible_ssh_private_key_file shell: chmod 0600 ../data/docker/mitogen__has_sudo_pubkey.key args: From be288ad3985787f4e42132da4ede2f68b3fe05a6 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 22 Aug 2022 13:01:51 +0200 Subject: [PATCH 08/21] patch #509 : ansible_ssh_common_args issues --- ansible_mitogen/transport_config.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ansible_mitogen/transport_config.py b/ansible_mitogen/transport_config.py index 3ab623f8..144de563 100644 --- a/ansible_mitogen/transport_config.py +++ b/ansible_mitogen/transport_config.py @@ -498,12 +498,13 @@ class PlayContextSpec(Spec): ) def ssh_args(self): + local_vars = self._task_vars.get("hostvars", {}).get(self._inventory_name, {}) return [ mitogen.core.to_text(term) for s in ( - C.config.get_config_value("ssh_args", plugin_type="connection", plugin_name="ssh", variables=self._task_vars.get("vars", {})), - C.config.get_config_value("ssh_common_args", plugin_type="connection", plugin_name="ssh", variables=self._task_vars.get("vars", {})), - C.config.get_config_value("ssh_extra_args", plugin_type="connection", plugin_name="ssh", variables=self._task_vars.get("vars", {})) + C.config.get_config_value("ssh_args", plugin_type="connection", plugin_name="ssh", variables=local_vars), + C.config.get_config_value("ssh_common_args", plugin_type="connection", plugin_name="ssh", variables=local_vars), + C.config.get_config_value("ssh_extra_args", plugin_type="connection", plugin_name="ssh", variables=local_vars) ) for term in ansible.utils.shlex.shlex_split(s or '') ] @@ -738,12 +739,13 @@ class MitogenViaSpec(Spec): ) def ssh_args(self): + local_vars = self._task_vars.get("hostvars", {}).get(self._inventory_name, {}) return [ mitogen.core.to_text(term) for s in ( - C.config.get_config_value("ssh_args", plugin_type="connection", plugin_name="ssh", variables=self._task_vars.get("vars", {})), - C.config.get_config_value("ssh_common_args", plugin_type="connection", plugin_name="ssh", variables=self._task_vars.get("vars", {})), - C.config.get_config_value("ssh_extra_args", plugin_type="connection", plugin_name="ssh", variables=self._task_vars.get("vars", {})) + C.config.get_config_value("ssh_args", plugin_type="connection", plugin_name="ssh", variables=local_vars), + C.config.get_config_value("ssh_common_args", plugin_type="connection", plugin_name="ssh", variables=local_vars), + C.config.get_config_value("ssh_extra_args", plugin_type="connection", plugin_name="ssh", variables=local_vars) ) for term in ansible.utils.shlex.shlex_split(s) if s From f3097b5743d543720d9416f490f4ce2b7be952d0 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Thu, 5 Sep 2024 17:50:09 +0100 Subject: [PATCH 09/21] ci: Template Ansible test-targets inventory with Jinja2 --- .ci/ansible_tests.py | 44 ++++++++----------------- tests/ansible/templates/test-targets.j2 | 28 ++++++++++++++++ 2 files changed, 42 insertions(+), 30 deletions(-) create mode 100644 tests/ansible/templates/test-targets.j2 diff --git a/.ci/ansible_tests.py b/.ci/ansible_tests.py index 0dd978c4..102eda9c 100755 --- a/.ci/ansible_tests.py +++ b/.ci/ansible_tests.py @@ -6,11 +6,13 @@ import glob import os import signal import sys -import textwrap + +import jinja2 import ci_lib +TEMPLATES_DIR = os.path.join(ci_lib.GIT_ROOT, 'tests/ansible/templates') TESTS_DIR = os.path.join(ci_lib.GIT_ROOT, 'tests/ansible') HOSTS_DIR = os.path.join(ci_lib.TMP, 'hosts') @@ -52,37 +54,19 @@ with ci_lib.Fold('job_setup'): distros[container['distro']].append(container['name']) families[container['family']].append(container['name']) + jinja_env = jinja2.Environment( + loader=jinja2.FileSystemLoader(searchpath=TEMPLATES_DIR), + lstrip_blocks=True, # Remove spaces and tabs from before a block + trim_blocks=True, # Remove first newline after a block + ) + inventory_template = jinja_env.get_template('test-targets.j2') inventory_path = os.path.join(HOSTS_DIR, 'target') + with open(inventory_path, 'w') as fp: - fp.write('[test-targets]\n') - fp.writelines( - "%(name)s " - "ansible_host=%(hostname)s " - "ansible_port=%(port)s " - "ansible_python_interpreter=%(python_path)s " - "ansible_user=mitogen__has_sudo_nopw " - "ansible_password=has_sudo_nopw_password" - "\n" - % container - for container in containers - ) - - for distro, hostnames in sorted(distros.items(), key=lambda t: t[0]): - fp.write('\n[%s]\n' % distro) - fp.writelines('%s\n' % name for name in hostnames) - - for family, hostnames in sorted(families.items(), key=lambda t: t[0]): - fp.write('\n[%s]\n' % family) - fp.writelines('%s\n' % name for name in hostnames) - - fp.write(textwrap.dedent( - ''' - [linux:children] - test-targets - - [linux_containers:children] - test-targets - ''' + fp.write(inventory_template.render( + containers=containers, + distros=distros, + families=families, )) ci_lib.dump_file(inventory_path) diff --git a/tests/ansible/templates/test-targets.j2 b/tests/ansible/templates/test-targets.j2 new file mode 100644 index 00000000..f61a3c78 --- /dev/null +++ b/tests/ansible/templates/test-targets.j2 @@ -0,0 +1,28 @@ +[test-targets] +{% for c in containers %} +{{ c.name }} ansible_host={{ c.hostname }} ansible_port={{ c.port }} ansible_python_interpreter={{ c.python_path }} +{% endfor %} + +[test-targets:vars] +ansible_user=mitogen__has_sudo_nopw +ansible_password=has_sudo_nopw_password + +{% for distro, hostnames in distros | dictsort %} +[{{ distro }}] +{% for hostname in hostnames %} +{{ hostname }} +{% endfor %} +{% endfor %} + +{% for family, hostnames in families | dictsort %} +[{{ family }}] +{% for hostname in hostnames %} +{{ hostname }} +{% endfor %} +{% endfor %} + +[linux:children] +test-targets + +[linux_containers:children] +test-targets From 79ed797badd039f1ba719f99a1ac2ebf8940798c Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Fri, 6 Sep 2024 11:51:58 +0100 Subject: [PATCH 10/21] tests: Test templating of ansible_ssh_common_args et al refs #905 --- docs/changelog.rst | 3 ++ docs/contributors.rst | 1 + tests/ansible/hosts/default.hosts | 7 ++++ tests/ansible/integration/ssh/all.yml | 1 + tests/ansible/integration/ssh/args.yml | 48 +++++++++++++++++++++++++ tests/ansible/templates/test-targets.j2 | 11 ++++++ 6 files changed, 71 insertions(+) create mode 100644 tests/ansible/integration/ssh/args.yml diff --git a/docs/changelog.rst b/docs/changelog.rst index 4fc177c1..8cc24424 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -26,6 +26,9 @@ Unreleased * :gh:issue:`1110` Fix :exc:`mitogen.core.StreamError` when Ansible copy module is called with a file larger than 124 kibibytes (:data:`ansible_mitogen.connection.Connection.SMALL_FILE_LIMIT`) +* :gh:issue:`905` Initial support for templated ``ansible_ssh_args``, + ``ansible_ssh_common_args``, and ``ansible_ssh_extra_args`` variables. + NB: play or task scoped variables will probably still fail. v0.3.9 (2024-08-13) diff --git a/docs/contributors.rst b/docs/contributors.rst index 1a590869..e40607a0 100644 --- a/docs/contributors.rst +++ b/docs/contributors.rst @@ -116,6 +116,7 @@ sponsorship and outstanding future-thinking of its early adopters.
    • Alex Willmer
      • +
    • Christian Bourgeois
    • Dan Dorman — - When I truly understand my enemy … then in that very moment I also love him.
    • Daniel Foerster
    • Deps — Private Maven Repository Hosting for Java, Scala, Groovy, Clojure
      • diff --git a/tests/ansible/hosts/default.hosts b/tests/ansible/hosts/default.hosts index 4f5ea4c6..adc271e2 100644 --- a/tests/ansible/hosts/default.hosts +++ b/tests/ansible/hosts/default.hosts @@ -12,3 +12,10 @@ target ansible_host=localhost ansible_user="{{ lookup('pipe', 'whoami') }}" target [linux_containers] + +[issue905] +ssh-common-args ansible_host=localhost ansible_user="{{ lookup('pipe', 'whoami') }}" + +[issue905:vars] +ansible_ssh_common_args=-o PermitLocalCommand=yes -o LocalCommand="touch {{ ssh_args_canary_file }}" +ssh_args_canary_file=/tmp/ssh_args_{{ inventory_hostname }} diff --git a/tests/ansible/integration/ssh/all.yml b/tests/ansible/integration/ssh/all.yml index 1b0f36e4..5c16f187 100644 --- a/tests/ansible/integration/ssh/all.yml +++ b/tests/ansible/integration/ssh/all.yml @@ -1,3 +1,4 @@ +- import_playbook: args.yml - import_playbook: config.yml - import_playbook: password.yml - import_playbook: timeouts.yml diff --git a/tests/ansible/integration/ssh/args.yml b/tests/ansible/integration/ssh/args.yml new file mode 100644 index 00000000..5892b5fe --- /dev/null +++ b/tests/ansible/integration/ssh/args.yml @@ -0,0 +1,48 @@ +- name: integration/ssh/args.yml + hosts: issue905 + gather_facts: false + tasks: + # Test that ansible_ssh_common_args are templated; ansible_ssh_args & + # ansible_ssh_extra_args aren't directly tested, we assume they're similar. + # FIXME This test currently relies on variables set in the host group. + # Ideally they'd be set here, and the host group eliminated, but + # Mitogen currently fails to template when defined in the play. + # TODO Replace LocalCommand canary with SetEnv canary, to simplify test. + # Requires modification of sshd_config files to add AcceptEnv ... + - name: Test templating of ansible_ssh_common_args et al + block: + - name: Ensure no lingering canary files + file: + path: "{{ ssh_args_canary_file }}" + state: absent + delegate_to: localhost + + - name: Reset connections to force new ssh execution + meta: reset_connection + + - name: Perform SSH connection, to trigger side effect + ping: + + # LocalCommand="touch {{ ssh_args_canary_file }}" in ssh_*_args + - name: Stat for canary file created by side effect + stat: + path: "{{ ssh_args_canary_file }}" + delegate_to: localhost + register: ssh_args_canary_stat + + - assert: + that: + - ssh_args_canary_stat.stat.exists == true + quiet: true + success_msg: "Canary found: {{ ssh_args_canary_file }}" + fail_msg: | + ssh_args_canary_file={{ ssh_args_canary_file }} + ssh_args_canary_stat={{ ssh_args_canary_stat }} + always: + - name: Cleanup canary files + file: + path: "{{ ssh_args_canary_file }}" + state: absent + delegate_to: localhost + tags: + - issue_905 diff --git a/tests/ansible/templates/test-targets.j2 b/tests/ansible/templates/test-targets.j2 index f61a3c78..e2708192 100644 --- a/tests/ansible/templates/test-targets.j2 +++ b/tests/ansible/templates/test-targets.j2 @@ -26,3 +26,14 @@ test-targets [linux_containers:children] test-targets + +[issue905] +{% for c in containers[:1] %} +ssh-common-args ansible_host={{ c.hostname }} ansible_port={{ c.port }} ansible_python_interpreter={{ c.python_path }} +{% endfor %} + +[issue905:vars] +ansible_user=mitogen__has_sudo_nopw +ansible_password=has_sudo_nopw_password +ansible_ssh_common_args=-o PermitLocalCommand=yes -o LocalCommand="touch {{ '{{' }} ssh_args_canary_file {{ '}}' }}" +ssh_args_canary_file=/tmp/ssh_args_{{ '{{' }} inventory_hostname {{ '}}' }} From f76ccbf8ed0d5468837574ebc23f0f39d58ea915 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Fri, 3 May 2024 22:08:14 +0100 Subject: [PATCH 11/21] tests: Unpin versions of Ansible 2.10, 3, & 4 --- tox.ini | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tox.ini b/tox.ini index 08f4c371..870a6345 100644 --- a/tox.ini +++ b/tox.ini @@ -74,9 +74,9 @@ basepython = deps = -r{toxinidir}/tests/requirements.txt mode_ansible: -r{toxinidir}/tests/ansible/requirements.txt - ansible2.10: ansible==2.10.7 - ansible3: ansible==3.4.0 - ansible4: ansible==4.10.0 + ansible2.10: ansible~=2.10.0 + ansible3: ansible~=3.0 + ansible4: ansible~=4.0 ansible5: ansible~=5.0 ansible6: ansible~=6.0 ansible7: ansible~=7.0 From 4b4bfdc0f3a677d3523211b14e669ed539a7f423 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Fri, 6 Sep 2024 21:43:43 +0100 Subject: [PATCH 12/21] ci: Drop macOS Python 3.12 + Ansible 9 tests They were meant to be replaced by Python 3.12 + ANsible 10, not supplemented. --- .ci/azure-pipelines.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.ci/azure-pipelines.yml b/.ci/azure-pipelines.yml index a3ffa9b4..0bf4556b 100644 --- a/.ci/azure-pipelines.yml +++ b/.ci/azure-pipelines.yml @@ -28,10 +28,6 @@ jobs: matrix: Mito_312: tox.env: py312-mode_mitogen - Loc_312_9: - tox.env: py312-mode_localhost-ansible9 - Van_312_9: - tox.env: py312-mode_localhost-ansible9-strategy_linear Loc_312_10: tox.env: py312-mode_localhost-ansible10 Van_312_10: From 7d9eebdb9a548fbaff9163a6afce250f8777c2a3 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 10 Sep 2024 15:44:17 +0100 Subject: [PATCH 13/21] tests: Close file object in six_brokenpkg --- tests/data/importer/six_brokenpkg/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/data/importer/six_brokenpkg/__init__.py b/tests/data/importer/six_brokenpkg/__init__.py index e5944b83..32356972 100644 --- a/tests/data/importer/six_brokenpkg/__init__.py +++ b/tests/data/importer/six_brokenpkg/__init__.py @@ -53,4 +53,4 @@ if _system_six: else: from . import _six as six six_py_file = '{0}.py'.format(os.path.splitext(six.__file__)[0]) -exec(open(six_py_file, 'rb').read()) +with open(six_py_file, 'rb') as f: exec(f.read()) From 7c92b8ef2bfa418ef7501297826d641c4d8d2d04 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 10 Sep 2024 15:47:01 +0100 Subject: [PATCH 14/21] tests: Shutdown contexts on completion This should terminate any child processes and connections. --- tests/id_allocation_test.py | 3 +++ tests/ssh_test.py | 1 + 2 files changed, 4 insertions(+) diff --git a/tests/id_allocation_test.py b/tests/id_allocation_test.py index 850a68a5..91ff3d4b 100644 --- a/tests/id_allocation_test.py +++ b/tests/id_allocation_test.py @@ -27,3 +27,6 @@ class SlaveTest(testlib.RouterMixin, testlib.TestCase): # Subsequent master allocation does not collide c2 = self.router.local() self.assertEqual(1002, c2.context_id) + + context.shutdown() + c2.shutdown() diff --git a/tests/ssh_test.py b/tests/ssh_test.py index 3149fcbc..ce7dce96 100644 --- a/tests/ssh_test.py +++ b/tests/ssh_test.py @@ -190,6 +190,7 @@ class BannerTest(testlib.DockerMixin, testlib.TestCase): self.dockerized_ssh.port, ) self.assertEqual(name, context.name) + context.shutdown(wait=True) class StubPermissionDeniedTest(StubSshMixin, testlib.TestCase): From 72384033920d541199a882624d110d7977c43eb7 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 10 Sep 2024 15:47:55 +0100 Subject: [PATCH 15/21] tests: Add missing logging import --- tests/connection_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/connection_test.py b/tests/connection_test.py index 5c3e678d..6cfa384c 100644 --- a/tests/connection_test.py +++ b/tests/connection_test.py @@ -1,3 +1,4 @@ +import logging import os import signal import sys From 598de81143a629dffab45b1602b8dcbb06646f5b Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 10 Sep 2024 16:02:49 +0100 Subject: [PATCH 16/21] mitogen: Fix subprocess ResourceWarning Python 3.x emits `ResourceWarning`s if certains resources aren't correctly closed. Due to the way Mitogen has been terminating child processes this has been occurring. ``` test_dev_tty_open_succeeds (create_child_test.TtyCreateChildTest.test_dev_tty_open_succeeds) ... /opt/hostedtoolcache/Python/3.12.5/x64/lib/python3.12/subprocess.py:1127: ResourceWarning: subprocess 3313 is still running _warn("subprocess %s is still running" % self.pid, ResourceWarning: Enable tracemalloc to get the object allocation traceback ok ``` During garbage collection subprocess.Popen() objects emit ResourceWarning("subprocess 123 is still running") if proc.returncode hasn't been set. Typically calling proc.wait() does so, once the sub-process has exited. Calling os.waitpid(proc.pid, 0) also waits for the sub-process to exit, but it doesn't update proc.returncode, so the ResourceWarning is still emitted. This change exposes `subprocess.Popen` methods on `mitogen.parent.PopenProcess`, so that the returncode can be set. See https://gist.github.com/moreati/b8d157ff82cb15234bece4033accc5e5 --- mitogen/parent.py | 16 +++++++++++++++- tests/connection_test.py | 8 +++++--- tests/create_child_test.py | 1 + tests/reaper_test.py | 20 +++++++++----------- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/mitogen/parent.py b/mitogen/parent.py index 4b96dcf4..2a43cad2 100644 --- a/mitogen/parent.py +++ b/mitogen/parent.py @@ -2542,7 +2542,7 @@ class Reaper(object): # because it is setuid, so this is best-effort only. LOG.debug('%r: sending %s', self.proc, SIGNAL_BY_NUM[signum]) try: - os.kill(self.proc.pid, signum) + self.proc.send_signal(signum) except OSError: e = sys.exc_info()[1] if e.args[0] != errno.EPERM: @@ -2662,6 +2662,17 @@ class Process(object): """ raise NotImplementedError() + def send_signal(self, sig): + os.kill(self.pid, sig) + + def terminate(self): + "Ask the process to gracefully shutdown." + self.send_signal(signal.SIGTERM) + + def kill(self): + "Ask the operating system to forcefully destroy the process." + self.send_signal(signal.SIGKILL) + class PopenProcess(Process): """ @@ -2678,6 +2689,9 @@ class PopenProcess(Process): def poll(self): return self.proc.poll() + def send_signal(self, sig): + self.proc.send_signal(sig) + class ModuleForwarder(object): """ diff --git a/tests/connection_test.py b/tests/connection_test.py index 6cfa384c..c3146954 100644 --- a/tests/connection_test.py +++ b/tests/connection_test.py @@ -55,7 +55,9 @@ def do_detach(econtext): class DetachReapTest(testlib.RouterMixin, testlib.TestCase): def test_subprocess_preserved_on_shutdown(self): c1 = self.router.local() + c1_stream = self.router.stream_by_id(c1.context_id) pid = c1.call(os.getpid) + self.assertEqual(pid, c1_stream.conn.proc.pid) l = mitogen.core.Latch() mitogen.core.listen(c1, 'disconnect', l.put) @@ -65,8 +67,8 @@ class DetachReapTest(testlib.RouterMixin, testlib.TestCase): self.broker.shutdown() self.broker.join() - os.kill(pid, 0) # succeeds if process still alive + self.assertIsNone(os.kill(pid, 0)) # succeeds if process still alive # now clean up - os.kill(pid, signal.SIGTERM) - os.waitpid(pid, 0) + c1_stream.conn.proc.terminate() + c1_stream.conn.proc.proc.wait() diff --git a/tests/create_child_test.py b/tests/create_child_test.py index acf3ea66..57b04b3f 100644 --- a/tests/create_child_test.py +++ b/tests/create_child_test.py @@ -76,6 +76,7 @@ def close_proc(proc): proc.stdout.close() if proc.stderr: proc.stderr.close() + proc.proc.wait() def wait_read(fp, n): diff --git a/tests/reaper_test.py b/tests/reaper_test.py index 8588a1bc..560d48ff 100644 --- a/tests/reaper_test.py +++ b/tests/reaper_test.py @@ -10,8 +10,7 @@ import mitogen.parent class ReaperTest(testlib.TestCase): - @mock.patch('os.kill') - def test_calc_delay(self, kill): + def test_calc_delay(self): broker = mock.Mock() proc = mock.Mock() proc.poll.return_value = None @@ -24,8 +23,7 @@ class ReaperTest(testlib.TestCase): self.assertEqual(752, int(1000 * reaper._calc_delay(5))) self.assertEqual(1294, int(1000 * reaper._calc_delay(6))) - @mock.patch('os.kill') - def test_reap_calls(self, kill): + def test_reap_calls(self): broker = mock.Mock() proc = mock.Mock() proc.poll.return_value = None @@ -33,20 +31,20 @@ class ReaperTest(testlib.TestCase): reaper = mitogen.parent.Reaper(broker, proc, True, True) reaper.reap() - self.assertEqual(0, kill.call_count) + self.assertEqual(0, proc.send_signal.call_count) reaper.reap() - self.assertEqual(1, kill.call_count) + self.assertEqual(1, proc.send_signal.call_count) reaper.reap() reaper.reap() reaper.reap() - self.assertEqual(1, kill.call_count) + self.assertEqual(1, proc.send_signal.call_count) reaper.reap() - self.assertEqual(2, kill.call_count) + self.assertEqual(2, proc.send_signal.call_count) - self.assertEqual(kill.mock_calls, [ - mock.call(proc.pid, signal.SIGTERM), - mock.call(proc.pid, signal.SIGKILL), + self.assertEqual(proc.send_signal.mock_calls, [ + mock.call(signal.SIGTERM), + mock.call(signal.SIGKILL), ]) From a3192d2bebc0e38a3f48c13a14cdd4d052a72b3d Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 10 Sep 2024 16:05:28 +0100 Subject: [PATCH 17/21] mitogen: close mitogen.unix.Listener socket in error conditions To avoid ResourceWarning --- mitogen/unix.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/mitogen/unix.py b/mitogen/unix.py index 1af1c0ec..b241a403 100644 --- a/mitogen/unix.py +++ b/mitogen/unix.py @@ -143,19 +143,23 @@ class Listener(mitogen.core.Protocol): def on_accept_client(self, sock): sock.setblocking(True) try: - pid, = struct.unpack('>L', sock.recv(4)) + data = sock.recv(4) + pid, = struct.unpack('>L', data) except (struct.error, socket.error): - LOG.error('listener: failed to read remote identity: %s', - sys.exc_info()[1]) + LOG.error('listener: failed to read remote identity, got %d bytes: %s', + len(data), sys.exc_info()[1]) + sock.close() return context_id = self._router.id_allocator.allocate() try: + # FIXME #1109 send() returns number of bytes sent, check it sock.send(struct.pack('>LLL', context_id, mitogen.context_id, os.getpid())) except socket.error: LOG.error('listener: failed to assign identity to PID %d: %s', pid, sys.exc_info()[1]) + sock.close() return context = mitogen.parent.Context(self._router, context_id) From 315204271e3fd58a6db8981e48629986cd451f4a Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 10 Sep 2024 16:07:53 +0100 Subject: [PATCH 18/21] tests: Don't suppress output while testing unix Listener It's not noisy, and it has been hiding an error I wasn't aware of. --- tests/unix_test.py | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/tests/unix_test.py b/tests/unix_test.py index 14fc54ae..e251a7ad 100644 --- a/tests/unix_test.py +++ b/tests/unix_test.py @@ -65,17 +65,13 @@ class ListenerTest(testlib.RouterMixin, testlib.TestCase): def test_constructor_basic(self): listener = self.klass.build_stream(router=self.router) - capture = testlib.LogCapturer() - capture.start() - try: - self.assertFalse(mitogen.unix.is_path_dead(listener.protocol.path)) - os.unlink(listener.protocol.path) - # ensure we catch 0 byte read error log message - self.broker.shutdown() - self.broker.join() - self.broker_shutdown = True - finally: - capture.stop() + self.assertFalse(mitogen.unix.is_path_dead(listener.protocol.path)) + os.unlink(listener.protocol.path) + + # ensure we catch 0 byte read error log message + self.broker.shutdown() + self.broker.join() + self.broker_shutdown = True class ClientTest(testlib.TestCase): From d032c591c2f861a66430ac0a2741be1b3b118198 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 10 Sep 2024 16:12:47 +0100 Subject: [PATCH 19/21] tests: Retry container process check during teardown I'm about 75% sure the check is an unavoidable race condition, see https://github.com/mitogen-hq/mitogen/issues/694#issuecomment-2338001694. If it occurs again, then reopen the issue. Fixes #694 --- docs/changelog.rst | 2 ++ tests/testlib.py | 34 ++++++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 8cc24424..14f86e77 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -29,6 +29,8 @@ Unreleased * :gh:issue:`905` Initial support for templated ``ansible_ssh_args``, ``ansible_ssh_common_args``, and ``ansible_ssh_extra_args`` variables. NB: play or task scoped variables will probably still fail. +* :gh:issue:`694` CI: Fixed a race condition and some resource leaks causing + some of intermittent failures when running the test suite. v0.3.9 (2024-08-13) diff --git a/tests/testlib.py b/tests/testlib.py index 8c40e7ff..a52292ce 100644 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -146,6 +146,17 @@ def data_path(suffix): return path +def retry(fn, on, max_attempts, delay): + for i in range(max_attempts): + try: + return fn() + except on: + if i >= max_attempts - 1: + raise + else: + time.sleep(delay) + + def threading__thread_is_alive(thread): """Return whether the thread is alive (Python version compatibility shim). @@ -562,18 +573,24 @@ class DockerizedSshDaemon(object): wait_for_port(self.get_host(), self.port, pattern='OpenSSH') def check_processes(self): - args = ['docker', 'exec', self.container_name, 'ps', '-o', 'comm='] + # Get Accounting name (ucomm) & command line (args) of each process + # in the container. No truncation (-ww). No column headers (foo=). + ps_output = subprocess.check_output([ + 'docker', 'exec', self.container_name, + 'ps', '-w', '-w', '-o', 'ucomm=', '-o', 'args=', + ]) + ps_lines = ps_output.decode().splitlines() + processes = [tuple(line.split(None, 1)) for line in ps_lines] counts = {} - for comm in subprocess.check_output(args).decode().splitlines(): - comm = comm.strip() - counts[comm] = counts.get(comm, 0) + 1 + for ucomm, _ in processes: + counts[ucomm] = counts.get(ucomm, 0) + 1 if counts != {'ps': 1, 'sshd': 1}: assert 0, ( 'Docker container %r contained extra running processes ' 'after test completed: %r' % ( self.container_name, - counts + processes, ) ) @@ -630,7 +647,12 @@ class DockerMixin(RouterMixin): @classmethod def tearDownClass(cls): - cls.dockerized_ssh.check_processes() + retry( + cls.dockerized_ssh.check_processes, + on=AssertionError, + max_attempts=5, + delay=0.1, + ) cls.dockerized_ssh.close() super(DockerMixin, cls).tearDownClass() From 2ba1b2b3f86814498bdc48cdfb77872a55c15124 Mon Sep 17 00:00:00 2001 From: Gaige B Paulsen Date: Fri, 20 Sep 2024 15:13:33 -0400 Subject: [PATCH 20/21] Fix: termios.error: (22, 'Invalid argument') during `become` on Solaris/Illumos/SmartOS (#1089) This fixes compatibility with Solaris/Illumos/SmartOS, addressing an issue that shows up most frequently with become. The issue was mostly due to differences in how the TTY driver is handled and the pty driver not supporting echo on both sides of the pipe (as designed, from a Solaris point of view). Fixes #950 Co-authored-by: Alex Willmer --- docs/changelog.rst | 1 + mitogen/parent.py | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 14f86e77..1d8deb92 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -21,6 +21,7 @@ To avail of fixes in an unreleased version, please download a ZIP file Unreleased ---------- +* :gh:issue:`950` Fix Solaris/Illumos/SmartOS compatibility with become * :gh:issue:`1087` Fix :exc:`mitogen.core.StreamError` when Ansible template module is called with a ``dest:`` filename that has an extension * :gh:issue:`1110` Fix :exc:`mitogen.core.StreamError` when Ansible copy diff --git a/mitogen/parent.py b/mitogen/parent.py index 2a43cad2..2ed7e8ba 100644 --- a/mitogen/parent.py +++ b/mitogen/parent.py @@ -147,6 +147,8 @@ LINUX_TIOCGPTN = _ioctl_cast(2147767344) LINUX_TIOCSPTLCK = _ioctl_cast(1074025521) IS_LINUX = os.uname()[0] == 'Linux' +IS_SOLARIS = os.uname()[0] == 'SunOS' + SIGNAL_BY_NUM = dict( (getattr(signal, name), name) @@ -411,7 +413,7 @@ def _acquire_controlling_tty(): # On Linux, the controlling tty becomes the first tty opened by a # process lacking any prior tty. os.close(os.open(os.ttyname(2), os.O_RDWR)) - if hasattr(termios, 'TIOCSCTTY') and not mitogen.core.IS_WSL: + if hasattr(termios, 'TIOCSCTTY') and not mitogen.core.IS_WSL and not IS_SOLARIS: # #550: prehistoric WSL does not like TIOCSCTTY. # On BSD an explicit ioctl is required. For some inexplicable reason, # Python 2.6 on Travis also requires it. @@ -479,7 +481,8 @@ def openpty(): master_fp = os.fdopen(master_fd, 'r+b', 0) slave_fp = os.fdopen(slave_fd, 'r+b', 0) - disable_echo(master_fd) + if not IS_SOLARIS: + disable_echo(master_fd) disable_echo(slave_fd) mitogen.core.set_block(slave_fd) return master_fp, slave_fp From cea2e7b98dc9e255b87f64471ed808a8e004afc1 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Fri, 20 Sep 2024 20:24:31 +0100 Subject: [PATCH 21/21] Prepare v0.3.10 --- docs/changelog.rst | 4 ++-- mitogen/__init__.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 1d8deb92..ee4faafc 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -18,8 +18,8 @@ To avail of fixes in an unreleased version, please download a ZIP file `directly from GitHub `_. -Unreleased ----------- +v0.3.10 (2024-09-20) +-------------------- * :gh:issue:`950` Fix Solaris/Illumos/SmartOS compatibility with become * :gh:issue:`1087` Fix :exc:`mitogen.core.StreamError` when Ansible template diff --git a/mitogen/__init__.py b/mitogen/__init__.py index 1d483ead..9c1ddc6c 100644 --- a/mitogen/__init__.py +++ b/mitogen/__init__.py @@ -35,7 +35,7 @@ be expected. On the slave, it is built dynamically during startup. #: Library version as a tuple. -__version__ = (0, 3, 10, 'dev') +__version__ = (0, 3, 10) #: This is :data:`False` in slave contexts. Previously it was used to prevent