-
Notifications
You must be signed in to change notification settings - Fork 261
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
artifacts cleanup service #238
Conversation
} | ||
|
||
private void checkExecutionTimeStamp() { | ||
List<File> uuids = Stream.of(new File(AnsibleConstants.ANSIBLE_RUNNER_PATH.toString()).listFiles()).collect(Collectors.toList()); |
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.
Please reformat to make it more readable:
List<File> uuids = Stream.of(new File(AnsibleConstants.ANSIBLE_RUNNER_PATH.toString()).listFiles())
.collect(Collectors.toList());
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.
done
.map(Path::toFile) | ||
.forEach(File::delete); | ||
} catch(Exception e) { | ||
e.printStackTrace(); |
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.
Shouldn't we log the exception properly (exception message to INFO, stacktrace to DEBUG)?
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.
fixed
String timeStamp = Files.readString(executionDatePath); | ||
Date executionDate = dateFormatter.parse(timeStamp); | ||
cal.setTime(executionDate); | ||
cal.add(Calendar.DAY_OF_MONTH, 14); |
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.
Please make those 2 weeks also configuable, maybe AnsibleRunnerArtifactsLifetimeInDays
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.
done
@@ -205,6 +205,9 @@ select fn_db_add_config_value('HostPackagesUpdateTimeInHours','24','general'); | |||
-- Refresh rate (in hours) for available certification check | |||
select fn_db_add_config_value('CertificationValidityCheckTimeInHours','24','general'); | |||
|
|||
-- Refresh rate (in hours) for outdated artifacts check | |||
select fn_db_add_config_value('ArtifactsOutdatedCheckTimeInHours','24','general'); |
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.
Please rename to AnsibleRunnerArtifactsCleanupCheckTimeInHours
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.
done
.sorted(Comparator.reverseOrder()) | ||
.map(Path::toFile) | ||
.forEach(File::delete); | ||
} catch(Exception e) { |
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.
Please don't use the general Exception
but when working with files use IOException
.
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.
done
if (cal.getTime().before(today)) { | ||
deleteDir(uuid); | ||
} | ||
} catch (Exception e) { |
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.
Please don't use the general Exception
but when working with files use IOException
.
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.
done
@Singleton | ||
public class AnsibleRunnerCleanUpService implements BackendService { | ||
|
||
private final String executionDate = "execution_date.txt"; |
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.
Please instead of the file using some file created that contains the timestamp use the standard BasicFileAttributes
Example:
BasicFileAttributes attr = Files.readAttributes(file, BasicFileAttributes.class);
System.out.println("creationTime: " + attr.creationTime());
System.out.println("lastAccessTime: " + attr.lastAccessTime());
System.out.println("lastModifiedTime: " + attr.lastModifiedTime());
or if you don't wanna use the BasicFileAttributes
you can use something like:
File file = new File(fileName);
System.out.println("Before Format : " + file.lastModified());
SimpleDateFormat sdf = new SimpleDateFormat("MM/dd/yyyy HH:mm:ss");
System.out.println("After Format : " + sdf.format(file.lastModified()));
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.
I'm not sure if relying on filesystem attributes is a good option, those can be changed accidentally. Using our file with timestamp written in it is much more protected against accidental change
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.
If we would put users' interaction into perspective, they can also edit/remove the timestamp which can cause failure, or engine issues can cause not creating the timestamp.
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.
I still think that the file timestamp is the best solution, if the user would edit some files they would be just cleaned after 2 weeks.
Plus the file is no longer created after the code cleanup. https://github.com/oVirt/ovirt-engine/pull/231/files#diff-c14903676ceca0238e0ca76b26dfbb0e8c8f8786ad5ef17ced6d56f98e01c2fdL310
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.
OK, so let's use for now the creation date of /var/lib/ansible-runner/<UUID>
to determine if the directory should be cleaned up
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.
done
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.
LGTM
...dules/bll/src/main/java/org/ovirt/engine/core/bll/validator/AnsibleRunnerCleanUpService.java
Show resolved
Hide resolved
89323e3
to
522f98c
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
522f98c
to
359361f
Compare
No description provided.