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

limit the number of artifacts directories retained on disk #314

Merged

Conversation

d-honeybadger
Copy link
Collaborator

Description of your changes

Fixes #311

This prevents too much data from accumulating on disk, especially now that retries are happening since each retry creates an artifacts directory.

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

  1. Create an ansiblerun - a failing one is best cause it'll be retried without one having to change the spec
  2. exec into the provider-ansible pod, navigate to ansibleDir/$UID-of-the-run/artifacts
  3. watch that the number of directories never exceeds the limit (you might want to set a limit lower than default for speedy testing)

Signed-off-by: Dasha Komsa <komsa.darya@gmail.com>
@d-honeybadger d-honeybadger force-pushed the clean-artifacts-history branch from a078f9c to 2807ad0 Compare February 16, 2024 19:41
@fahedouch
Copy link
Collaborator

integration test is very welcome :)

Signed-off-by: Dasha Komsa <komsa.darya@gmail.com>
@d-honeybadger d-honeybadger force-pushed the clean-artifacts-history branch from 6e79322 to 7d81d24 Compare February 27, 2024 14:37
@d-honeybadger
Copy link
Collaborator Author

integration test is very welcome :)

Since there's no prior art, are you thinking something like this: https://github.com/crossplane/crossplane/blob/master/test/e2e/README.md? e.g. run the provider against a kind k8s cluster and have ansible-runner actually run playbooks
Or I could add a test without kind cluster but with ansible-runner installed and running real playbooks - this would be more like a unit test but with real ansible, so I guess it'd just test the ansiblerun controller and not the entire flow of setting a flag

@fahedouch
Copy link
Collaborator

fahedouch commented Feb 28, 2024

Thanks, overall LGTM

integration test is very welcome :)

Since there's no prior art, are you thinking something like this: https://github.com/crossplane/crossplane/blob/master/test/e2e/README.md? e.g. run the provider against a kind k8s cluster and have ansible-runner actually run playbooks Or I could add a test without kind cluster but with ansible-runner installed and running real playbooks - this would be more like a unit test but with real ansible, so I guess it'd just test the ansiblerun controller and not the entire flow of setting a flag

I think it is fine to go with unit test for the moment.

let's introduce integration test in a follow-up PR! we can be inspired from other projects such as aws provider

@fahedouch fahedouch added this to the v0.5.1 (tentative) milestone Feb 28, 2024
Signed-off-by: Dasha Komsa <komsa.darya@gmail.com>
@d-honeybadger
Copy link
Collaborator Author

I think it is fine to go with unit test for the moment.

Added a couple unit tests. I know I mentioned in the beginning using the actual ansible-runner, but when I looked closer, there's no good way to do that without running the test in a container with a custom dockerfile: ansible-runner install requires python, and that's not something we can't just curl the binary for. Adding makefile targets and/or scripts that mess with python install on the system seemed like a bad idea, and doing a custom dockerfile and make targets to build that dockerfile seemed like an overkill too, especially since there's the plan to do e2e tests with kind.

So the tests I added are your typical unit tests using dummy commands, just checking that the runner settings and command args are set correctly.

Copy link
Collaborator

@fahedouch fahedouch left a comment

Choose a reason for hiding this comment

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

Thanks

@fahedouch fahedouch merged commit 2f90b4a into crossplane-contrib:main Mar 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.

Old ansible-runner job event directories should be cleaned up
2 participants