-
Notifications
You must be signed in to change notification settings - Fork 355
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.
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
Refactoring interface common routines #1079
base: devel
Are you sure you want to change the base?
Conversation
This should also fix up direct role execution when run from the command line using an execution environment
@@ -52,10 +52,10 @@ def init_runner(**kwargs): | |||
''' | |||
# If running via the transmit-worker-process method, we must only extract things as read-only | |||
# inside of one of these commands. That could be either transmit or worker. | |||
if kwargs.get('streamer') not in ('worker', 'process'): | |||
if kwargs.get('process_isolation') or kwargs.get('streamer') not in ('worker', 'process'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are what I was really looking for in here. Trying to use direct role execution from the CLI using an execution environment didn't work because of the path assignment behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa, wait. Just on the surface of it, it seems like making this change would break the transmit phase of streaming runner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... or I misread the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reconsidering, though, I'm not sure how it helps to do the dump_artifacts
writing to disk in the worker and processor cases when we have process isolation? The worker should already have everything by unzipping everything that came over the wire from the transmit phase, and I don't think that the processor phase needs any of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, artifacts shouldn't be getting dumped because of suppress_env_files
@@ -824,52 +721,82 @@ def main(sys_args=None): | |||
if vargs.get('command') in ('transmit', 'worker', 'process'): | |||
streamer = vargs.get('command') | |||
|
|||
# TODO: Remove / Refactor unused role context manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it was removed?
I tested in conjunction with AWX / AAP controller, and normal jobs error with this from the worker:
In this case, it should be running a normal playbook, but it seems to somehow interpret that as direct role execution. |
This should also fix up direct role execution when run from the command line using an execution environment.
Still need to sort out some tests and make sure the cli entrypoint is doing the right thing in all circumstances.