Skip to content
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

Jump to bottom

skip process isolation check for transmit and process #1084

Merged
merged 2 commits into from Jun 3, 2022
Merged

skip process isolation check for transmit and process #1084

merged 2 commits into from Jun 3, 2022

Conversation

TheRealHaoLiu
Copy link
Member

@TheRealHaoLiu TheRealHaoLiu commented May 31, 2022 •
edited

@TheRealHaoLiu TheRealHaoLiu requested a review from a team as a code owner May 31, 2022 22:59
@github-actions github-actions bot added the needs_triage New item that needs to be triaged label May 31, 2022
@TheRealHaoLiu TheRealHaoLiu marked this pull request as draft May 31, 2022 23:09
@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch from e2d29e5 to 961c7f1 Compare June 1, 2022 01:14
@TheRealHaoLiu TheRealHaoLiu marked this pull request as ready for review June 1, 2022 01:27
shanemcd
shanemcd reviewed Jun 1, 2022
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
matburt
matburt reviewed Jun 1, 2022
View reviewed changes
ansible_runner/interface.py Outdated
@@ -83,7 +83,7 @@ def init_runner(**kwargs):
if logfile:
output.set_logfile(logfile)

if kwargs.get("process_isolation", False):
if kwargs.get("process_isolation", False) and kwargs.get("streamer") not in ('transmit', 'process'):
Copy link
Member

Choose a reason for hiding this comment

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

I have this in my PR also: https://github.com/ansible/ansible-runner/pull/1079/files#diff-179ccecedba1c6ccf73b8c02f0321b252c018226e41ce82887a93ace762d2fbbR55

This line causes an astonishing amount of problems when interfacing with Runner from the CLI

Copy link
Member

Choose a reason for hiding this comment

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

I think the reason it does this has to do with the rest of my PR, which is that the CLI entrypoint clashes with the module level entrypoint when managing process isolation paths.

My work on #1079 focuses on realigning those two paths.

@matburt
Copy link
Member

matburt commented Jun 1, 2022

Fix that vscode comment and then I think we should merge this asap.

@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch 2 times, most recently from 5db7742 to 0f12e05 Compare June 1, 2022 13:34
@TheRealHaoLiu
Copy link
Member Author

.gitignore change amended as per suggestion

@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch 2 times, most recently from 8c70756 to 7083535 Compare June 1, 2022 14:46
@TheRealHaoLiu
Copy link
Member Author

code change after peer programing with @jbradberry moved the location of the check

@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch from 7083535 to 612216e Compare June 1, 2022 14:57
@TheRealHaoLiu @jbradberry
skip process isolation check for transmit and process
5370620 
moving down the process isolation executable so that transmit and
process no longer checks for its existance

ansible-runner worker will still run the check since it will invoke
ansible-runner run with process-isolation-executable and it will still
be checked properly

Co-Authored-By: Jeff Bradberry <685957+jbradberry@users.noreply.github.com>
Signed-off-by: Hao Liu <haoli@redhat.com>
@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch from 612216e to 5370620 Compare June 1, 2022 15:01
@Shrews
Copy link
Contributor

Shrews commented Jun 1, 2022

Manually tested this to verify that worker will bail if the executable is not found. I'd like to have a test for this before merging it. Thanks!

@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch from 0324646 to 0e67829 Compare June 2, 2022 19:24
@TheRealHaoLiu
Copy link
Member Author

Manually tested this to verify that worker will bail if the executable is not found. I'd like to have a test for this before merging it. Thanks!

testcase added

TheRealHaoLiu added a commit to TheRealHaoLiu/ansible-runner that referenced this pull request Jun 3, 2022
@TheRealHaoLiu
add hacked personal build to pull in changes
0407be9 
pull change from
- ansible#1084
- ansible#1089
Shrews
Shrews reviewed Jun 3, 2022
View reviewed changes
test/integration/test_transmit_worker_process.py Outdated
"""
def mock_check_isolation_executable_installed(executable):
if executable == "exist":
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return True
return True
return False

@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch from 0e67829 to a5e5aba Compare June 3, 2022 15:12
@TheRealHaoLiu
Copy link
Member Author

testcase commit amended base on @Shrews's feedback

@TheRealHaoLiu TheRealHaoLiu requested a review from Shrews June 3, 2022 15:13
@TheRealHaoLiu
add test_process_isolation_executable_not_exist
696f7a6 
test_process_isolation_executable_not_exist will validate that
- transmit will run when process isolation executable missing and properly pass on the related kwargs to the output
- worker will fail with sys.exit(1) when process isolation executable missiing

Signed-off-by: Hao Liu <haoli@redhat.com>
@TheRealHaoLiu TheRealHaoLiu force-pushed the skip-process-isolation-check-for-transmit-and-receive branch from a5e5aba to 696f7a6 Compare June 3, 2022 15:53
Shrews
Shrews approved these changes Jun 3, 2022
View reviewed changes
Copy link
Contributor

@Shrews Shrews left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! If you want this in any of the previously released 2.x versions, you'll need to supply a backport to the appropriate release branches. We follow the same basic backport process as Ansible (https://docs.ansible.com/ansible/latest/community/development_process.html#backport-process).

@Shrews Shrews merged commit e723a54 into ansible:devel Jun 3, 2022
@TheRealHaoLiu TheRealHaoLiu deleted the skip-process-isolation-check-for-transmit-and-receive branch June 3, 2022 20:52
@TheRealHaoLiu
Copy link
Member Author

Thanks for the PR! If you want this in any of the previously released 2.x versions, you'll need to supply a backport to the appropriate release branches. We follow the same basic backport process as Ansible (https://docs.ansible.com/ansible/latest/community/development_process.html#backport-process).

no need for backport thanks

TheRealHaoLiu added a commit to TheRealHaoLiu/ansible-runner that referenced this pull request Jun 6, 2022
@TheRealHaoLiu @jbradberry
skip process isolation check for transmit and process (ansible#1084)
dede318 
* skip process isolation check for transmit and process

moving down the process isolation executable so that transmit and
process no longer checks for its existance

ansible-runner worker will still run the check since it will invoke
ansible-runner run with process-isolation-executable and it will still
be checked properly

Co-Authored-By: Jeff Bradberry <685957+jbradberry@users.noreply.github.com>
Signed-off-by: Hao Liu <haoli@redhat.com>
@TheRealHaoLiu TheRealHaoLiu mentioned this pull request Jun 6, 2022
Merged
@TheRealHaoLiu
Copy link
Member Author

backport to 2.2 #1094

@Shrews Shrews linked an issue Jun 7, 2022 that may be closed by this pull request
while running via the transmit-worker-process, ansible-runner should only check process isolation exec for worker #1085
Closed
Shrews pushed a commit that referenced this pull request Jun 7, 2022
@TheRealHaoLiu @jbradberry
skip process isolation check for transmit and process (#1084) (#1094)
8dff126 
* skip process isolation check for transmit and process

moving down the process isolation executable so that transmit and
process no longer checks for its existance

ansible-runner worker will still run the check since it will invoke
ansible-runner run with process-isolation-executable and it will still
be checked properly

Co-Authored-By: Jeff Bradberry <685957+jbradberry@users.noreply.github.com>
Signed-off-by: Hao Liu <haoli@redhat.com>

Co-authored-by: Jeff Bradberry <685957+jbradberry@users.noreply.github.com>
@sivel sivel removed the needs_triage New item that needs to be triaged label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbradberry jbradberry jbradberry approved these changes

@Shrews Shrews Shrews approved these changes

@matburt matburt Awaiting requested review from matburt

@shanemcd shanemcd Awaiting requested review from shanemcd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@TheRealHaoLiu @matburt @Shrews @jbradberry @shanemcd @sivel