Skip to content

[WIP] lxc_container: replace subprocess.Popen() with run_command() #11204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
russoz wants to merge 3 commits into
base: main
Choose a base branch
from
+12 −31
Open

[WIP] lxc_container: replace subprocess.Popen() with run_command() #11204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5664cd
lxc_container: replace subprocess.Popen() with run_command()
russoz Nov 23, 2025
ab40c33
Update plugins/modules/lxc_container.py
russoz Nov 24, 2025
a813295
Update plugins/modules/lxc_container.py
russoz Nov 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 12 additions & 26 deletions plugins/modules/lxc_container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,6 @@
import os.path
import re
import shutil
import subprocess
import tempfile
import time
import shlex
Expand Down Expand Up @@ -526,44 +525,31 @@
"""


def create_script(command):
def create_script(command: str, module: AnsibleModule) -> None:
"""Write out a script onto a target.

This method should be backward compatible with Python when executing
from within the container.

:param command: command to run, this can be a script and can use spacing
with newlines as separation.
:type command: ``str``
:param module: AnsibleModule to run commands with.
"""

(fd, script_file) = tempfile.mkstemp(prefix="lxc-attach-script")
f = os.fdopen(fd, "wb")
try:
script_file = None
with tempfile.NamedTemporaryFile(prefix="lxc-attach-script", delete_on_close=False, mode="wb") as f:
f.write(to_bytes(ATTACH_TEMPLATE % {"container_command": command}, errors="surrogate_or_strict"))
script_file = f.name
f.flush()
finally:
f.close()

# Ensure the script is executable.
os.chmod(script_file, int("0700", 8))
os.chmod(script_file, int("0700", 8))

# Output log file.
stdout_file = os.fdopen(tempfile.mkstemp(prefix="lxc-attach-script-log")[0], "ab")

# Error log file.
stderr_file = os.fdopen(tempfile.mkstemp(prefix="lxc-attach-script-err")[0], "ab")

# Execute the script command.
try:
subprocess.Popen([script_file], stdout=stdout_file, stderr=stderr_file).communicate()
finally:
# Close the log files.
stderr_file.close()
stdout_file.close()

# Remove the script file upon completion of execution.
os.remove(script_file)
with tempfile.NamedTemporaryFile(prefix="lxc-attach-script-log", delete=False, mode="ab") as stdout_file:
with tempfile.NamedTemporaryFile(prefix="lxc-attach-script-err", delete=False, mode="ab") as stderr_file:
rc, out, err = module.run_command([script_file], binary_data=True, encoding=None)
stdout_file.write(out)
stderr_file.write(err)
Copy link
Collaborator

@felixfontein felixfontein Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this makes sense anymore. I think the temporary files are created so that if something goes wrong while executing the external command, that you can look at the temporary files to figure out what's happening. Like if the program doesn't terminate, takes too long, crashes - as the files arent' deleted.

Now stdout/stderr is only written after the program is done, and right afterwards the temporary files are deleted.

If we really want to use AnsibleModule.run_command here, we shouldn't create these files. But I'm not sure I understand this function good enough to be able to say whether Popen isn't better here...

@cloudnull, do you remember what's going on here?

Copy link
Collaborator Author

@russoz russoz Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_script is called using lxc.Container.attach_wait(), which basically is a fork() that also changes the "container" where the process runs. The only temporary file that is deleted is the script_file one, and I agree it might be a good candidate for removal, but I did not want to change things too much in this PR.



class LxcContainerManagement:
Expand Down Expand Up @@ -869,7 +855,7 @@ def _execute_command(self):
elif container_state == "stopped":
self._container_startup()

self.container.attach_wait(create_script, container_command)
self.container.attach_wait(create_script, (container_command, self.module))
self.state_change = True

def _container_startup(self, timeout=60):
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.17.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choic
plugins/modules/homectl.py import-3.11 # Uses deprecated stdlib library 'crypt'
plugins/modules/homectl.py import-3.12 # Uses deprecated stdlib library 'crypt'
plugins/modules/iptables_state.py validate-modules:undocumented-parameter # params _back and _timeout used by action plugin
plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen
plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
plugins/modules/rhevm.py validate-modules:parameter-state-invalid-choice
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.18.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choic
plugins/modules/homectl.py import-3.11 # Uses deprecated stdlib library 'crypt'
plugins/modules/homectl.py import-3.12 # Uses deprecated stdlib library 'crypt'
plugins/modules/iptables_state.py validate-modules:undocumented-parameter # params _back and _timeout used by action plugin
plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen
plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
plugins/modules/rhevm.py validate-modules:parameter-state-invalid-choice
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.19.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choic
plugins/modules/homectl.py import-3.11 # Uses deprecated stdlib library 'crypt'
plugins/modules/homectl.py import-3.12 # Uses deprecated stdlib library 'crypt'
plugins/modules/iptables_state.py validate-modules:undocumented-parameter # params _back and _timeout used by action plugin
plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen
plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
plugins/modules/rhevm.py validate-modules:parameter-state-invalid-choice
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.20.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choic
plugins/modules/homectl.py import-3.11 # Uses deprecated stdlib library 'crypt'
plugins/modules/homectl.py import-3.12 # Uses deprecated stdlib library 'crypt'
plugins/modules/iptables_state.py validate-modules:undocumented-parameter # params _back and _timeout used by action plugin
plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen
plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
plugins/modules/rhevm.py validate-modules:parameter-state-invalid-choice
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.21.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ plugins/modules/interfaces_file.py validate-modules:bad-return-value-key # TODO
plugins/modules/iptables_state.py validate-modules:undocumented-parameter # params _back and _timeout used by action plugin
plugins/modules/keycloak_realm_info.py validate-modules:bad-return-value-key # TODO: rename offending return values if possible, or adjust this comment in case the name is OK
plugins/modules/keycloak_realm_keys_metadata_info.py validate-modules:bad-return-value-key # TODO: rename offending return values if possible, or adjust this comment in case the name is OK
plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen
plugins/modules/nosh.py validate-modules:bad-return-value-key # TODO: rename offending return values if possible, or adjust this comment in case the name is OK
plugins/modules/omapi_host.py validate-modules:bad-return-value-key # TODO: rename offending return values if possible, or adjust this comment in case the name is OK
plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
Expand Down