Pass environment variables through task execution #69
Conversation
Previously, the environment for a task would have been the same environment as the first task run. This first task was responsible for spawning the server which inherited that task's environment. This is a problem in cases where the environment might change in a latter task, for example, by setting the environment keyword on a task. In these cases, the environment for the new task is lost. This change wraps execution of a module in a temporary environment, passing through whatever envvars might have been set by the controller.
|
recheck |
1 similar comment
|
recheck |
|
recheck |
|
I'm working on the CI issue. |
|
recheck |
1 similar comment
|
recheck |
plugins/module_utils/turbo/server.py
Outdated
| os.environ.update(env) | ||
| yield | ||
| os.environ.clear() | ||
| os.environ.update(original) |
There was a problem hiding this comment.
seems like this may cause some issues, let's see the following use case:
- name: task 1
environment:
test_var: v1
- name: task 2
environment:
test_var: v2
- name: task 3
task 3 will see the env variable test_var with value v1 even if it is not defined anymore.
also note that for the lookup plugin the os.environ is not updated, env variables are part of variables attribute of the run method
| os.environ.update(original) |
There was a problem hiding this comment.
Yeah, good catch. I feel like completely clearing the environment for the rest of execution seems dangerous, though. I've changed it to reset the environment right before the module is run.
This prevents envvars from the original module execution from leaking into latter module executions.
plugins/module_utils/turbo/server.py
Outdated
| @@ -272,6 +273,8 @@ def _terminate(result): | |||
|
|
|||
| await embedded_module.load() | |||
| try: | |||
| os.environ.clear() | |||
| os.environ.update(env) | |||
There was a problem hiding this comment.
os.environ is a global variable. We can get two concurrent async execution of embedded_module. In this case, the second runs would overwrite the environment for the first calll. We should use a lock like in load() to protect to block.
There was a problem hiding this comment.
e.g:
#!/usr/bin/env python3
import asyncio
import os
async def my_async_loop(expectation):
for _ in range(100):
await asyncio.sleep(0.1)
assert os.environ["baba"] == expectation
async def main():
os.environ["baba"] = "loop1"
l1 = my_async_loop("loop1")
os.environ["blabla"] = "loop2"
l2 = my_async_loop("loop2")
await l2
await l1
asyncio.run(main())There was a problem hiding this comment.
I added a lock. I'll also mention two things. The EmbeddedModule.run() method needed a lock before my changes since it's modifying sys.stdin and then (possibly) awaiting the module. Also, unless I'm missing something, I don't think the lock in the load() or unload() methods is doing anything. It's not hurting anything to be there, but it's also not necessary. There's nowhere in either of those functions that a task switch can occur.
|
recheck |
2 similar comments
|
recheck |
|
recheck |
|
recheck |
|
recheck |
Major Changes ------------- - turbo - enable turbo mode for lookup plugins Bugfixes -------- - add exception handler to main async loop (ansible-collections#67). - pass current task's environment through to execution (ansible-collections#69). - turbo - AnsibleTurboModule was missing some _ansible_facts variable like _diff, _ansible_tmpdir. (ansible-collections#65) - turbo - honor the ``remote_tmp`` configuration key.
prepare 2.0.4 release Major Changes turbo - enable turbo mode for lookup plugins Bugfixes add exception handler to main async loop (#67). pass current task's environment through to execution (#69). turbo - AnsibleTurboModule was missing some _ansible_facts variable like _diff, _ansible_tmpdir. (#65) turbo - honor the remote_tmp configuration key.
Re-enable support for turbo mode SUMMARY This re-enables the ability to add turbo mode. It also adds a few more tests to cover some cases that had been broken in turbo mode previously. Testing with turbo mode is not currently enabled, and would fail until ansible-collections/cloud.common#69 can be merged and a new cloud.common release is done. This also does not add cloud.common to the collection dependencies until a decision has been made about how enabling/disabling turbo mode will work when cloud.common is already installed. ISSUE TYPE Bugfix Pull Request COMPONENT NAME ADDITIONAL INFORMATION Reviewed-by: Abhijeet Kasurde <None> Reviewed-by: Mike Graves <[email protected]> Reviewed-by: Gonéri Le Bouder <[email protected]> Reviewed-by: None <None>
Depends-On: ansible-collections/vmware.vmware_rest#235
SUMMARY
Previously, the environment for a task would have been the same
environment as the first task run. This first task was responsible for
spawning the server which inherited that task's environment. This is a
problem in cases where the environment might change in a latter task,
for example, by setting the environment keyword on a task. In these
cases, the environment for the new task is lost.
This change wraps execution of a module in a temporary environment,
passing through whatever envvars might have been set by the controller.
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION