CHE-2435: Add component for clean up workspace folder after remove workspace. #2544

AndrienkoAleksandr · 2016-09-22T12:52:56Z

What does this PR do?

Add component for clean up workspace folder after remove workspace.

What issues does this PR fix or reference?

#2435

PR type

Major change = changes existing features or docs

Minor change checklist

API updated
Tests provided / updated
Tests passed

Major change checklist

API updated
Documentation provided (include here or link to docs)
Tests provided / updated
Tests passed

@garagatyi review please.

Signed-off-by: Aleksandr Andrienko [email protected]

mshaposhnik · 2016-09-22T13:38:45Z

...space/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceProjectStorageCleaner.java

+ *
+ * @author Alexander Andrienko
+ */
+public interface WorkspaceProjectStorageCleaner {


WorkspaceFileStorageCleaner or WorkspaceFSStorageCleaner looks somehow better for me. WDYT?

Ok, it's make sense. I'll renamed it soon. Thank you.

codenvy-ci · 2016-09-22T15:50:35Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/476/

garagatyi · 2016-09-27T07:23:21Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+     * @throws TimeoutException
+     *         if process gets more time then defined by {@code timeout}
+     */
+    public static Process execute(String[] commandLine, int timeout, TimeUnit timeUnit, LineConsumer outputConsumer)


If process is ended/killed by the end of execution of this method what for it returns process object?

throws InterruptedException or IOExeption.

I see that it is needed to process return code

garagatyi

Overall OK, but needs some fixes

garagatyi · 2016-09-27T07:26:11Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+        ProcessBuilder pb = new ProcessBuilder(commandLine).redirectErrorStream(true);
+
+        // process will be stopped after timeout
+        Watchdog watcher = new Watchdog(timeout, timeUnit);


Please use name for watchdog thread - it will help understand thread dump in case of troubles with the server.

This class is utility.

garagatyi · 2016-09-27T07:29:40Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+            // consume logs until process ends
+            process(process, outputConsumer);
+
+            process.waitFor();


Please use timeout here

garagatyi · 2016-09-27T07:30:52Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+            process.waitFor();
+
+            if (isTimeoutExceeded.get()) {
+                throw new TimeoutException();


This exception would be clearer if it had a message on what has happened.

This class is utility.

garagatyi · 2016-09-27T07:32:42Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java

@@ -326,6 +329,9 @@ public void removeWorkspace(String workspaceId) throws ConflictException, Server
        if (runtimes.hasRuntime(workspaceId)) {
            throw new ConflictException("The workspace '" + workspaceId + "' is currently running and cannot be removed.");
        }
+
+        workspaceProjectStorageCleaner.remove(workspaceId);


If it is clearer then method name clear looks more natural for me than remove

garagatyi · 2016-09-27T07:49:12Z

...main/java/org/eclipse/che/plugin/docker/machine/LocalWorkspaceProjectStorageCleanerImpl.java

+    @Override
+    public void remove(String workspaceId) {
+        try {
+            File workspaceStorage = new File(workspaceFolderPathProvider.getPath(workspaceId));


Have you covered the case when workspace are sharing single folder as a storage of projects?

No, I''ll look at this case.

garagatyi · 2016-09-27T07:58:11Z

@evoevodin Please take a look

garagatyi · 2016-09-27T08:19:27Z

QA is needed

codenvy-ci · 2016-09-27T16:11:10Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/533/

codenvy-ci · 2016-09-27T19:45:17Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/538/

voievodin · 2016-09-29T13:59:30Z

...-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceFSStorageCleaner.java

+public interface WorkspaceFSStorageCleaner {
+
+    /**
+     * Removes workspace project folder with all data by {@code workspaceId}. Note: all user's projects will be deleted.


From the definition it's not clear for me what is workspace project folder? Does the workspace have the only one project?

Docs changed

voievodin · 2016-09-29T14:02:31Z

...-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceFSStorageCleaner.java

+package org.eclipse.che.api.workspace.server;
+
+/**
+ * This component removes workspace folder from workspace storage. It's used for "delete workspace" operation


It's used for "delete workspace" operation

If this interface exists for certain needs please document this needs better, as it's not clear what the phrase above means. As far as i understand we need this interface for cleaning file system resources after workspace is removed?

This interface need to realize delete workspace folder with projects after remove workspace, I'll fix doc.

voievodin · 2016-09-29T14:10:02Z

.../src/main/java/org/eclipse/che/plugin/docker/machine/LocalWorkspaceFSStorageCleanerImpl.java

+            String workspacePath = workspaceFolderPathProvider.getPath(workspaceId);
+            File workspaceStorage = new File(workspacePath);
+            if (!workspacePath.equals(hostProjectsFolder) && workspaceStorage.exists()) {
+                FileCleaner.addFile(workspaceStorage);


Are you sure that it's expected behaviour, should it be asynchronous? If yes, do you think it's better to document it?

I think remove workspace folder can be potentionally long time operation(if workspace contains a lot of files and server computer is busy to done this task quickly), so yes I think it should be asynchronous operation. Thank you, I will document it.

Documented in the interface WorkspaceFSStorageCleaner

voievodin · 2016-09-29T14:12:50Z

.../src/main/java/org/eclipse/che/plugin/docker/machine/LocalWorkspaceFSStorageCleanerImpl.java

+            if (!workspacePath.equals(hostProjectsFolder) && workspaceStorage.exists()) {
+                FileCleaner.addFile(workspaceStorage);
+            }
+        } catch (IOException e) {


I think it's better throw the exception from the method, document it, and catch it on callers side. The motivation is that if we need to add another cleaner, that cleaner will have to implement a batch of the same logic, like exception handling and it's processing.

I don't think that failed clean up workspace folder is reason to cancel delete workspace operation. That why I handle exception here.

But it is still better to handle batch of delete operations on higher level and decide there completion of which operation is optional and which not. +1 to what Eugene said

voievodin

Please comment on my comments/questions

voievodin · 2016-09-29T14:35:33Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+            // consume logs until process ends
+            process(process, outputConsumer);
+
+            process.waitFor(timeout, timeUnit);


What if instead of using Watchdog we consume all the logs in separate thread and wait for pocess to end or reach timeout, i mean we can use the value returned from the waitFor method to achive this goal,
e.g.

if (!process.waitFor(timeout, timeUnit)) { ProcessUtil.kill(process); }

@garagatyi, @AndrienkoAleksandr what do you think?

Thank you @evoevodin, I think we can do it, but this only another way of implementation, it has not any advantages. And current version of code is already tested, so I don't think that we should spend time to another implementation without good reason.

@evoevodin I think it can be really clean solution in term of code.
WDYT about usage of ComplletableFuture.runAsync in such cases? Without that addition of executor and everything like that would not make it simpler.

codenvy-ci · 2016-09-30T08:36:26Z

Build # 574 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/574/ to view the results.

garagatyi · 2016-09-30T10:56:24Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java

@@ -326,6 +329,9 @@ public void removeWorkspace(String workspaceId) throws ConflictException, Server
        if (runtimes.hasRuntime(workspaceId)) {
            throw new ConflictException("The workspace '" + workspaceId + "' is currently running and cannot be removed.");
        }
+
+        workspaceFSStorageCleaner.clear(workspaceId);


Please cover in tests

garagatyi · 2016-09-30T10:56:54Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java

-        final boolean createSnapshot = autoSnapshotAttr == null ? defaultAutoSnapshot : parseBoolean(autoSnapshotAttr);
+        boolean createSnapshot;
+        if (workspace.isTemporary()) {
+            createSnapshot = false;


It is very nice that you added that fix but , please, cover it in tests

codenvy-ci · 2016-10-03T19:11:12Z

Build # 592 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/592/ to view the results.

codenvy-ci · 2016-10-04T13:20:22Z

Build # 602 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/602/ to view the results.

codenvy-ci · 2016-10-04T15:42:36Z

Build # 609 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/609/ to view the results.

AndrienkoAleksandr · 2016-10-06T07:20:50Z

.../src/main/java/org/eclipse/che/plugin/docker/machine/cleaner/LocalWorkspaceFilesCleaner.java

+        String workspacePath = workspaceFolderPathProvider.getPathByName(workspace.getConfig().getName());
+        File workspaceStorage = new File(workspacePath);
+        if (!workspacePath.equals(hostProjectsFolder) && workspaceStorage.exists()) {
+            FileCleaner.addFile(workspaceStorage);


Maybe it would be better to use not bad alternative: IoUtil.deleteRecursive(workspaceStorage); What do you think @garagatyi @evoevodin ?

For now class LocalWorkspaceFilesCleaner.java works synchronously so I used IoUtil.deleteRecursive

codenvy-ci · 2016-10-06T11:59:21Z

Build # 643 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/643/ to view the results.

voievodin

Good job, please fix some small remarks

voievodin · 2016-10-07T14:28:36Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+     * @throws TimeoutException
+     *         if process gets more time then defined by {@code timeout}
+     */
+    public static Process execute(String[] commandLine, int timeout, TimeUnit timeUnit, LineConsumer outputConsumer)


i'd probably rename it to executeAndWait as the behaviour is different from the existing execute method

voievodin · 2016-10-07T14:32:58Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+                // consume logs until process ends
+                process(process, outputConsumer);
+            } catch (IOException e) {
+                LOG.error(format("Failed to complete process '%s'.", Joiner.on(" ").join(commandLine)), e);


let's change the exception method to something like Failed to complete reading of the process '%s' output due to occurred error

voievodin · 2016-10-07T14:37:20Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/ProcessUtil.java

+        });
+
+        if (!process.waitFor(timeout, timeUnit)) {
+            ProcessUtil.kill(process);


let's wrap process kill to ensure that user gets TimeoutException
e.g.

try { ProcessUtil.kill(process); } catch (RuntimeException x) { LOG.error("An error occurred while killing process '{}'", Joiner.on(" ").join(commandLine)); }

voievodin · 2016-10-07T15:03:06Z

...ine/src/main/java/org/eclipse/che/plugin/docker/machine/cleaner/RemoveWorkspaceListener.java

+    @Inject
+    public RemoveWorkspaceListener(EventService eventService, WorkspaceFilesCleaner workspaceFilesCleaner) {
+        this.workspaceFilesCleaner = workspaceFilesCleaner;
+        eventService.subscribe(this);


subscribe on events in @PostConstruct

voievodin · 2016-10-07T15:04:41Z

...ine/src/main/java/org/eclipse/che/plugin/docker/machine/cleaner/RemoveWorkspaceListener.java

+            try {
+                workspaceFilesCleaner.clear(workspace);
+            } catch (IOException | ServerException e) {
+                LOG.error("Failed to delete folder for workspace with id: '{}'. Cause: '{}'", workspace.getId(), e.getMessage());


consider a different message, less concrete, let's say it's Failed to remove workspace files

codenvy-ci · 2016-10-09T21:05:44Z

Build # 665 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/665/ to view the results.

codenvy-ci · 2016-10-12T13:57:13Z

Build # 695 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/695/ to view the results.

garagatyi

Looks good!

Signed-off-by: Aleksandr Andrienko <[email protected]>

codenvy-ci · 2016-10-18T20:43:04Z

Build # 734 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/734/ to view the results.

…he#2544) Signed-off-by: Aleksandr Andrienko <[email protected]>

mshaposhnik reviewed Sep 22, 2016

View reviewed changes

garagatyi reviewed Sep 27, 2016

View reviewed changes

garagatyi suggested changes Sep 27, 2016

View reviewed changes

AndrienkoAleksandr force-pushed the CHE-2435 branch 4 times, most recently from 512b665 to 6860d92 Compare September 27, 2016 13:45

AndrienkoAleksandr force-pushed the CHE-2435 branch from 6860d92 to 31b0078 Compare September 27, 2016 18:54

AndrienkoAleksandr added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Sep 28, 2016

voievodin reviewed Sep 29, 2016

View reviewed changes

voievodin suggested changes Sep 29, 2016

View reviewed changes

garagatyi suggested changes Sep 30, 2016

View reviewed changes

AndrienkoAleksandr commented Oct 6, 2016

View reviewed changes

voievodin approved these changes Oct 7, 2016

View reviewed changes

AndrienkoAleksandr changed the title ~~[WIP]CHE-2435: Add component for clean up workspace folder after remove workspace.~~ CHE-2435: Add component for clean up workspace folder after remove workspace. Oct 12, 2016

AndrienkoAleksandr force-pushed the CHE-2435 branch from 3ce3b40 to 3a184b4 Compare October 12, 2016 11:52

garagatyi approved these changes Oct 18, 2016

View reviewed changes

CHE-2435: Clean up workspace files after remove workspace.

26dcfe5

Signed-off-by: Aleksandr Andrienko <[email protected]>

AndrienkoAleksandr force-pushed the CHE-2435 branch from 3a184b4 to 26dcfe5 Compare October 18, 2016 19:55

AndrienkoAleksandr merged commit 291dbb8 into master Oct 18, 2016

AndrienkoAleksandr deleted the CHE-2435 branch October 18, 2016 20:05

bmicklea added this to the 5.0.0-M6 milestone Oct 19, 2016

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

CHE-2435: Clean up workspace files after remove workspace. (eclipse-c…

093eb7b

…he#2544) Signed-off-by: Aleksandr Andrienko <[email protected]>

CHE-2435: Add component for clean up workspace folder after remove workspace. #2544

CHE-2435: Add component for clean up workspace folder after remove workspace. #2544

Conversation

AndrienkoAleksandr commented Sep 22, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

PR type

Minor change checklist

Major change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Sep 22, 2016

Choose a reason for hiding this comment

AndrienkoAleksandr Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi left a 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

garagatyi Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

AndrienkoAleksandr Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi commented Sep 27, 2016

garagatyi commented Sep 27, 2016

codenvy-ci commented Sep 27, 2016

codenvy-ci commented Sep 27, 2016

voievodin Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrienkoAleksandr Sep 30, 2016 • edited Loading

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

voievodin Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voievodin left a comment

Choose a reason for hiding this comment

voievodin Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrienkoAleksandr Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

codenvy-ci commented Oct 3, 2016

codenvy-ci commented Oct 4, 2016

codenvy-ci commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Oct 6, 2016

voievodin left a 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

codenvy-ci commented Oct 9, 2016

codenvy-ci commented Oct 12, 2016

garagatyi left a comment

Choose a reason for hiding this comment

codenvy-ci commented Oct 18, 2016

AndrienkoAleksandr commented Sep 22, 2016 •

edited

Loading

AndrienkoAleksandr Sep 27, 2016 •

edited

Loading

garagatyi Sep 27, 2016 •

edited

Loading

AndrienkoAleksandr Sep 27, 2016 •

edited

Loading

voievodin Sep 29, 2016 •

edited

Loading

AndrienkoAleksandr Sep 30, 2016 •

edited

Loading

voievodin Sep 29, 2016 •

edited

Loading

voievodin Sep 29, 2016 •

edited

Loading

AndrienkoAleksandr Oct 3, 2016 •

edited

Loading