Skip to content

Conversation

@russoz
Copy link
Collaborator

@russoz russoz commented Nov 24, 2025

SUMMARY

The module was using subprocess.Popen() to run an external command, whilst the recommended way is to use AnsibleModule.run_command().

ISSUE TYPE
  • Refactoring Pull Request
COMPONENT NAME

lxc_container

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added module module plugins plugin (any type) tests tests labels Nov 24, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-12 Automatically create a backport for the stable-12 branch labels Nov 24, 2025
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

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

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.

russoz and others added 2 commits November 25, 2025 08:47
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
@russoz
Copy link
Collaborator Author

russoz commented Nov 24, 2025

Another thing I am not sure of: it is not clear how lxc.Container.attach_wait() handles the parameters for the called function. In its spec I saw *args, suggesting we can pass as many params we want. OTOH, every single example I saw calls attach_wait(func, [param1, param2]), suggesting that we should pass only one list/tuple with the params in it.

This part of the current code was from AI, and even though I did some spelunking in lxc code, at the end of the day that package is mostly bindings for C code. I am not confident of how the rules work in that case.

@russoz russoz changed the title lxc_container: replace subprocess.Popen() with run_command() [WIP] lxc_container: replace subprocess.Popen() with run_command() Nov 24, 2025
@ansibullbot ansibullbot added the WIP Work in progress label Nov 24, 2025
@russoz
Copy link
Collaborator Author

russoz commented Nov 24, 2025

I will write the changelog fragment after the code change is settled.

@russoz
Copy link
Collaborator Author

russoz commented Dec 1, 2025

friendly ping @cloudnull sorry to bother, any chance you could help with this? TIA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-12 Automatically create a backport for the stable-12 branch check-before-release PR will be looked at again shortly before release and merged if possible. has_issue module module plugins plugin (any type) tests tests WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants