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.

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

Add more ansible runner specs #23091

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jul 16, 2024

@agrare Please review.

I'm not sure about the wait method, so would like your thoughts there on how better to represent that or perhaps a better method signature.

The first commit is partially copied from #23063 cc @jaisejose1123

@miq-bot
Copy link
Member

miq-bot commented Jul 16, 2024

Checked commits Fryguy/manageiq@39b67ef~...b6d0c4e with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 2 offenses detected

lib/ansible/runner/response_async.rb

spec/lib/ansible/runner/data/hello_world_vault_encrypted.yml

  • ⚠️ - Line 38, Col 5 - indentation - wrong indentation: expected 2 but found 4

@agrare
Copy link
Member

agrare commented Jul 16, 2024

Probably scope creep, but I wonder if we could use inotify like we do for Ansible::Runner#wait_for to tell when the playbook starts running by waiting for the pid file. That would eliminate the sleep/poll loop

@Fryguy
Copy link
Member Author

Fryguy commented Jul 17, 2024

Oh yes, Inotify is a much better option - let me try that

end
# If the process is still running, then stop it
if result.nil?
stop
Copy link
Member

Choose a reason for hiding this comment

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

I could see wanting to wait X seconds for this to complete but not want to stop it, could we pull this out and return nil if it timed out or something? That way it is up to the caller if they want to kill the run on timeout or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I want to leave this as is, because I the interface for wait is to return a response object (i.e. turn a response_async object into a response object.

@agrare
Copy link
Member

agrare commented Jul 17, 2024

@Fryguy actually if we just want to wait for the process to finish I think it is even easier than using inotify. All ansible-runner is-alive does is a kill signal check to see if the process exists. We could use waitpid to wait for the process to finish, and since we wait for the pid file to be created before returning we don't have to worry about a race if we do File.read(pid) here

@Fryguy
Copy link
Member Author

Fryguy commented Jul 17, 2024

Oh neat - I didn't realize is-alive was so simple.

@agrare
Copy link
Member

agrare commented Jul 17, 2024

    if vargs.get('command') == 'is-alive':
        try:
            os.kill(pid, signal.SIG_DFL)
            return 0
        except OSError:
            return 1

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

I'll help with the non-polling version in a follow-up

@agrare agrare merged commit 80ff67c into ManageIQ:master Jul 29, 2024
8 checks passed
@Fryguy Fryguy deleted the add_more_ansible_runner_specs branch July 29, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants