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

let ansible-runner shut down gracefully on timeout #299

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

d-honeybadger
Copy link
Collaborator

@d-honeybadger d-honeybadger commented Jan 24, 2024

Description of your changes

Fixes #297. Please see issue description and comments for details.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Use the same steps as #297. Exec into the provider-ansible pod and run top. Watch ansible-playbook processes shut down at the timeout time (cause ansible-runner shuts down the main ansible-playbook process it spun up).

Signed-off-by: Dasha Komsa <komsa.darya@gmail.com>
}
// if it doesn't respond to the SIGINT within 10s,
// it's going to be forcefully shut down with SIGKILL
dc.WaitDelay = 10 * time.Second
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious if it makes sense to allow the value of WaitDelay configurable.

Copy link
Collaborator Author

@d-honeybadger d-honeybadger Jan 29, 2024

Choose a reason for hiding this comment

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

I'd leave it non-configurable until someone comes up with a use-case or asks for it, just to keep the configuration from bloating. But that said, can totally add the flag - you're the boss :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense to me. Let's deliver it first, then see the feedback from community.

Copy link
Collaborator

@morningspace morningspace left a comment

Choose a reason for hiding this comment

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

LGTM

@morningspace morningspace merged commit 153108a into crossplane-contrib:main Feb 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ansible processes are orphaned and remain running when reconciliation timeout occurs
2 participants