Clear tmp dir #344

obasekiosa · 2021-08-09T17:06:11Z

Description

This creates a scheduled process to clear the /tmp dir of files older than a day every single day
fixes #200 #279

Motivation and Context

Previous to this the issue of large files(pull workflow repositories) being stuck in the /tmp dir caused issues on the server and were cleared manually or with a system cron job. This PR moves that process within the CWL viewer application.

How Has This Been Tested?

Running the application and checking that the /tmp files get cleared as scheduled

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

tetron · 2021-08-16T16:39:24Z

src/main/java/org/commonwl/view/Scheduler.java

+        // wipe tmp dir and log info
+        try {
+            // Run command
+            String[] command = {"find", "/tmp", "-ctime", "+" + TMP_DIR_AGE_LIMIT_DAYS, "-exec", "rm", "-rf", "{}", "+"};


As discussed, I suggest investigating a java-based solution instead of using a subprocess.

kinow · 2021-10-02T22:18:03Z

src/main/java/org/commonwl/view/Scheduler.java

+
+    @Scheduled(cron = "${cron.clearTmpDir}")
+    public void clearTmpDir() {
+        // TODO: find source of tmp dir creation and delete from there


👍 we can change the PR to draft until the TODOs are resolved.

kinow · 2021-10-02T22:18:25Z

src/main/java/org/commonwl/view/Scheduler.java

+        // wipe tmp dir and log info
+        try {
+            // Run command
+            String[] command = {"find", "/tmp", "-ctime", "+" + TMP_DIR_AGE_LIMIT_DAYS, "-exec", "rm", "-rf", "{}", "+"};


kinow · 2021-10-02T22:19:41Z

src/main/java/org/commonwl/view/Scheduler.java

+            if (exitCode == 0) {
+                inputGobbler.join();
+                logger.info(inputGobbler.getContent());
+                logger.info("Successfully Cleared /tmp directory");


s/Cleared/cleared for consistency with other messages? 😬

Also, once the TODO above about locating the tmp dir is solved, we should probably include the location in the logs to help in case a sysadmin needs to check the directory permissions, modes, etc.

s/Cleared/cleared for consistency with other messages? 😬

Also, once the TODO above about locating the tmp dir is solved, we should probably include the location in the logs to help in case a sysadmin needs to check the directory permissions, modes, etc.

Hi @kinow ,
The idea was to delete the file within the process that actually creates these files.
But that would be a mistake since the file may still be in use.

Another idea was to delete the file the moment it is no longer in use, but an exact time would be difficult to determine

Hence a solution that works would be to delete all files above a certain age.

so I would be removing the TODO remark.
But before I do that, do you have any comments on a much better approach?

Sounds good to me. The other comment about using a Java native solution is still valid I think.

So instead of forking a new process we could try first using Java to iterate/walk the temp dir, filter for files/dirs older than the threshold, and finally delete or mark them for deletion (I assume we will try to delete it and in case it fails, call deleteOnExit or just log it... maybe Commons IO has something that we can use... just food for thought 👍 )

Moves the changes in that refactor the scheduler to this branch.

kinow · 2021-10-15T03:54:26Z

src/main/resources/application.properties

@@ -48,6 +48,7 @@ spring.data.mongodb.port = 27017
 sparql.endpoint = http://localhost:3030/cwlviewer/


+


nit-picking, but unnecessary blank line 😬

kinow · 2021-10-15T03:57:22Z

src/main/java/org/commonwl/view/util/FileUtils.java

+
+        String[] command = {"find", directoryPath, "-ctime", "+" + days, "-exec", "rm", "-rf", "{}", "+"};
+        ProcessBuilder clearProcess = new ProcessBuilder(command);
+        Process process = clearProcess.start();


This code was in Scheduler.java before in this PR, and then was moved here, so it's not showing @tetron 's review comment. The comment is still visible in the GitHub UI, but will copy it here just in case that disappears:

As discussed, I suggest investigating a java-based solution instead of using a subprocess.

kinow · 2021-10-15T03:59:02Z

src/main/java/org/commonwl/view/util/FileUtils.java

+
+    }
+
+    private void deleteWithinDirectoryCMD(String directoryPath, int days) throws IOException, InterruptedException {


Probably CMD -> Cmd or Command? I don't have a strong preference for CMD or Cmd, but given we have classes like CwlViewerApplication and not CWLViewerApplication, I take it the project is using the convention of keeping only the first letter of an abbreviation upper-case.

Also, I searched this file in GitHub UI for where it was used... but apparently it's not used anymore?

Ah! Got it! We are now using deleteWithinDirectory. IMO we probably don't need/want this method (and the one above if not used elsewhere) now that we have deleteWithinDirectory. So probably delete deleteWithinDirectoryCMD and other private functions that are used only in deleteWithinDirectoryCMD.

Yes, will do that. Left them both for the review.

kinow · 2021-10-15T04:00:29Z

src/main/java/org/commonwl/view/util/FileUtils.java

+        if (files != null) {
+            for (File subfile : files) {
+                if (subfile.isDirectory()) {
+                deleteWithinDirectory(subfile, days);


Wrong indentation.

kinow · 2021-10-15T04:02:46Z

src/main/java/org/commonwl/view/util/FileUtils.java

+                            subfile.delete();
+                            logger.info("Deleted directory " + subfile.getPath());
+                        }
+                    }


The if/for/if`... Could possibly be rewritten in Java 8's streams and filters... but not a blocker, can be done in a follow-up.

kinow · 2021-10-15T04:05:02Z

src/main/java/org/commonwl/view/util/FileUtils.java

+
+    public FileUtils(Logger logger) {
+        this.logger = logger;
+    }


Not necessary? ☝️

Normally utility classes have private constructors and static public methods. But since this is not using static methods, I think we should delete this constructor, and create a logger here too, as it's done in other files?

I struggled a bit with this decision because

I wanted the ability to log the individual steps in a way that should show the scheduled process

I didn't want the schedule function to deal with exceptions.

but looking at your comment. I guess I could have different logs at the scheduled function level and also the file utils level. That way the util logs would be nested and surrounded by the scheduled functions logs

kinow · 2021-10-15T04:05:39Z

src/main/java/org/commonwl/view/util/FileUtils.java

+
+public class FileUtils {
+
+    private final Logger logger;


Suggested change

private final Logger logger;

private final Logger logger = LoggerFactory.getLogger(this.getClass());

kinow · 2021-10-15T04:10:43Z

src/main/java/org/commonwl/view/Scheduler.java

+    public void clearTmpDir() {
+        // wipe tmp dir and log info
+        FileUtils utils = new FileUtils(logger);
+        utils.clearDirectory("/tmp", TMP_DIR_AGE_LIMIT_DAYS);


I run cwlviewer in my laptop, directly from IntelliJ, executing the SpringBoot application. If I understood correctly, here if I were to leave the process running until midnight, then it would go over my /tmp directory removing any files older than the age limit.

I have other applications running that also use /tmp to store temporary files. That would probably cause issues for me. So in this case, I would probably store temporary data for cwlviewer elsewhere, and expect the Scheduler to be able to cope with that too.

So I think we should:

confirm cwlviewer is using /tmp (not sure if anyone uses Windows with cwlviewer, but someone could be using a different TMPDIR in linux too, normally in Java we rely on java.io.tmpdir)

if we are using /tmp, or something that resolves /tmp, then we need to make sure it doesn't affect other processes running too, so we either

delete only cwlviewer files

move the temporary directory to another configurable location

… and Commons IO to traverse and delete old files and directories.

…rse and delete old files and directories.

obasekiosa added 10 commits July 23, 2021 03:42

Create cronjob value for scheduler

62e9766

Merge from main

1ee0ccf

Update tmpdir cron job cleaning settings

9ea8783

Create cron job to wipe tmp dir

eca1c9a

Update /tmp wipe scheduled process

9b0fc86

Add logging to clearing process

8421be8

Fix scheduler logging issue

431b62d

Fix exception module import error

5469a45

Update log levels for clearing process

4f070ca

Update log level in Scheduler

b88eb0b

tetron self-requested a review August 16, 2021 16:08

tetron suggested changes Aug 16, 2021

View reviewed changes

tetron and others added 2 commits September 21, 2021 12:19

Merge branch 'main' into clear-tmp-dir

673d8c1

Refactor code changes

423a428

obasekiosa force-pushed the clear-tmp-dir branch from 423a428 to 673d8c1 Compare September 30, 2021 09:32

Add missing imports

8c38030

kinow reviewed Oct 2, 2021

View reviewed changes

mr-c and others added 5 commits October 4, 2021 11:38

Merge branch 'main' into clear-tmp-dir

d70e6a0

Fix: class not found compile error

96223fa

Merge branch 'refactor-scheduler' into clear-tmp-dir

d12c5fd

Moves the changes in that refactor the scheduler to this branch.

Update log messages

d8b3f2a

Fix: String type typo

51c6415

kinow reviewed Oct 15, 2021

View reviewed changes

kinow mentioned this pull request Jun 14, 2022

Intermittent errors with CWL Viewer #424

Closed

kinow added a commit to kinow/cwlviewer that referenced this pull request Jun 21, 2022

Using parts of common-workflow-language#344 by @obasekiosa (thanks!),…

77808cd

… and Commons IO to traverse and delete old files and directories.

kinow mentioned this pull request Jun 21, 2022

Delete git/RO directories when not needed, delete old files/dirs in cron #426

Merged

10 tasks

kinow added a commit to kinow/cwlviewer that referenced this pull request Jul 11, 2022

Using parts of common-workflow-language#344 by @obasekiosa (thanks!),…

e909de8

… and Commons IO to traverse and delete old files and directories.

kinow added a commit to kinow/cwlviewer that referenced this pull request Aug 14, 2022

Using parts of common-workflow-language#344 by @obasekiosa (thanks!),…

9790884

… and Commons IO to traverse and delete old files and directories.

mr-c pushed a commit to kinow/cwlviewer that referenced this pull request Dec 30, 2022

Using parts of common-workflow-language#344 by @obasekiosa (thanks!),…

b98bd15

… and Commons IO to traverse and delete old files and directories.

mr-c pushed a commit that referenced this pull request Dec 30, 2022

Using parts of #344 by @obasekiosa (thanks!), and Commons IO to trave…

3759a12

…rse and delete old files and directories.

mr-c closed this in #426 Dec 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear tmp dir #344

Clear tmp dir #344

obasekiosa commented Aug 9, 2021 •

edited

Loading

tetron Aug 16, 2021

kinow Oct 2, 2021

kinow Oct 2, 2021

kinow Oct 2, 2021

kinow Oct 2, 2021

obasekiosa Oct 4, 2021

kinow Oct 4, 2021

kinow Oct 15, 2021

kinow Oct 15, 2021

kinow Oct 15, 2021

kinow Oct 15, 2021

kinow Oct 15, 2021

obasekiosa Oct 26, 2021

kinow Oct 15, 2021

kinow Oct 15, 2021

kinow Oct 15, 2021

obasekiosa Oct 26, 2021

kinow Oct 15, 2021

kinow Oct 15, 2021

		@@ -48,6 +48,7 @@ spring.data.mongodb.port = 27017
		sparql.endpoint = http://localhost:3030/cwlviewer/


		}

		private void deleteWithinDirectoryCMD(String directoryPath, int days) throws IOException, InterruptedException {

	private final Logger logger;
	private final Logger logger = LoggerFactory.getLogger(this.getClass());

Clear tmp dir #344

Clear tmp dir #344

Conversation

obasekiosa commented Aug 9, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obasekiosa commented Aug 9, 2021 •

edited

Loading