-
Notifications
You must be signed in to change notification settings - Fork 258
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
B2151549 ansible runner artifacts not cleaned up #786
B2151549 ansible runner artifacts not cleaned up #786
Conversation
dangel101
commented
Jan 1, 2023
- add debug logging to AnsibleRunnerCleanUpService
- executing ansible runner artifacts clean up at fixed rate
e7faf00
to
27af0bd
Compare
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.
Generally change looks good, but please signoff you commits and put description to each commit
@@ -40,14 +40,17 @@ public void scheduleJob() { | |||
final int HOURS_TO_MINUTES = 60; | |||
long intervalInMinutes = Math.round(interval * HOURS_TO_MINUTES); | |||
|
|||
executor.schedule(this::checkExecutionTimeStamp, | |||
executor.scheduleAtFixedRate(this::checkExecutionTimeStamp, |
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.
Wouldn't it be better to use scheduleWithFixedDelay
the same as we use for other periodical services?
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ScheduledExecutorService.html
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.
the functions are pretty similar,
I thought it'd be better to trigger each execution at the same time, as FixedRate does
FixedDelay starts to count the time after the last execution was finished.
For the clean up service there shouldn't be much of a difference between them, so if you prefer that it would be similar to the other services I can change 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.
Hmm, I'd say it's safer to trigger next execution only after the 1st one finishes (we can't be sure what users will enter into the engine-config options) :-)
27af0bd
to
38ba1e1
Compare
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.
+1
Ansible-Runner artifacts are cleaned up periodically according to the properties configured in engine-config: AnsibleRunnerArtifactsLifetimeInDays: How many days artifacts will be kept before they are cleaned up AnsibleRunnerArtifactsCleanupCheckTimeInHours: The periodic time to execute the clean up Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2151549 Signed-off-by: dangel101 <dangel101@gmail.com>
38ba1e1
to
433e2dc
Compare