Change worskpace project files storage path #7844

mkuznyetsov · 2017-12-12T15:20:22Z

What does this PR do?

Workspace projects are now stored in folders, named after their workspace id, rather than name,
to prevent issues with workspace renaming. Applies to Docker infrastructure only.
Add migration, that would move workspace projects to their new location. Occurs on Che startup.
Added configuration property to enable/disable this migration.
Improved Workspace DAO/Manager to use pagination for methods, that return lists of workspaces

What issues does this PR fix or reference?

should fix #3574

Changelog

Workspace projects are stored in folders, named after their workspace ID (previously, after their workspace name

Release Notes

N/A

Docs PR

N/A

skabashnyuk · 2017-12-12T19:56:05Z

ci-test

codenvy-ci · 2017-12-13T01:50:56Z

ci-test build report:
Build details
Test report
selenium tests report data
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

skabashnyuk · 2017-12-13T07:40:54Z

infrastructures/docker/infrastructure/pom.xml

@@ -42,6 +42,10 @@
            <groupId>com.google.inject.extensions</groupId>
            <artifactId>guice-multibindings</artifactId>
        </dependency>
+        <dependency>
+            <groupId>commons-io</groupId>


skabashnyuk · 2017-12-13T07:41:42Z

.../che/workspace/infrastructure/docker/local/projects/LocalProjectsFolderPathProviderTest.java

@@ -61,6 +74,11 @@ public void setUp() throws Exception {
    oldWorkspacesRoot = Paths.get(workspacesRoot, "oldWorkspacesRoot").toString();
  }

+  @AfterMethod
+  public void tearDown() throws Exception {
+    FileUtils.deleteDirectory(workspacesRootFile);


IoUtil.deleteRecursive

skabashnyuk · 2017-12-13T07:45:03Z

...ipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsFolderPathProvider.java

  }

-  public String getPathByName(String workspaceName) throws IOException {
+  public String getPathByName(String workspaceName, String workspaceNamespace) throws IOException {


remove this method

skabashnyuk · 2017-12-13T07:45:45Z

...ipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsFolderPathProvider.java

+              workspaceStoredByIdLocation,
+              StandardCopyOption.ATOMIC_MOVE);
+          LOG.info(
+              "Successfully workspace with id '{}' and name '{}'",


Make this message more clear

skabashnyuk · 2017-12-13T07:48:00Z

...n/java/org/eclipse/che/multiuser/resource/api/usage/tracker/RuntimeResourceUsageTracker.java

-        workspaceManagerProvider
-            .get()
-            .getByNamespace(account.getName(), false)
+        Lists.newArrayList(


Too complex

skabashnyuk · 2017-12-13T07:48:10Z

...java/org/eclipse/che/multiuser/resource/api/usage/tracker/WorkspaceResourceUsageTracker.java

@@ -48,7 +51,13 @@ public WorkspaceResourceUsageTracker(
      throws NotFoundException, ServerException {
    final Account account = accountManager.getById(accountId);
    final List<WorkspaceImpl> accountWorkspaces =
-        workspaceManagerProvider.get().getByNamespace(account.getName(), false);
+        Lists.newArrayList(


too complex

garagatyi · 2017-12-13T08:14:41Z

...ipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsFolderPathProvider.java

+   * Get all workspaces, and check if they are stored in workspacesMountPoint by their name, and
+   * migrate them, so they will be stored by id
+   */
+  private void performWorkspaceLocationMigration() {


It is better to keep all the logic in a separate class.

garagatyi · 2017-12-13T08:18:21Z

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

@@ -225,7 +227,12 @@ public WorkspaceDto getByKey(
    // TODO add maxItems & skipCount to manager


Looks like this todo is not needed anymore

garagatyi · 2017-12-13T08:20:19Z

...i-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/tck/WorkspaceDaoTest.java


-    assertEquals(new HashSet<>(found), new HashSet<>(asList(workspace1, workspace2)));
+    assertEquals(new HashSet<>(found.getItems()), new HashSet<>(asList(workspace1, workspace2)));


Are you going to add new tests for checking the new functionality?

sleshchenko · 2017-12-13T10:19:43Z

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

-              workspace.getId(),
-              e.getLocalizedMessage());
-        }
+    ;


Please check if this semicolon is needed here

sleshchenko · 2017-12-13T10:23:56Z

...kspace/src/test/java/org/eclipse/che/api/workspace/server/TemporaryWorkspaceRemoverTest.java

    remover.removeTemporaryWs();

-    verify(workspaceDao, times(COUNT_OF_WORKSPACES)).remove(anyString());
+    verify(workspaceDao, times(250)).remove(anyString());


Maybe it would be better to use constant here

…cture

mkuznyetsov · 2017-12-14T15:51:15Z

ci-test

codenvy-ci · 2017-12-14T21:10:09Z

ci-test build report:
Build details
Test report
selenium tests report data
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2017-12-15T07:39:51Z

...ipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsFolderPathProvider.java

@@ -209,4 +231,9 @@ private void ensureExist(String path, String prop) throws IOException {
      }
    }
  }
+
+  /**
+   * Get all workspaces, and check if they are stored in workspacesMountPoint by their name, and


Please check that this comment is located in correct place.

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi

Looks good. Few minor things left.

garagatyi · 2017-12-19T15:08:07Z

...org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectMigratorTest.java

+  }
+  //
+  //  @Test
+  //  public void shouldMigrateOnlyWorkspacesWithOldLocation() throws Exception {


Please, fix commented test

garagatyi · 2017-12-19T15:14:26Z

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

-              workspace.getId(),
-              e.getLocalizedMessage());
-        }
+    ;


@mkuznyetsov Please, take a look

sleshchenko · 2017-12-20T08:29:51Z

...va/org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsMigrator.java

+
+/**
+ * Performs migration of projects that are stored for workspace in directories, named respectively
+ * by their names, to new


is something missing after to new?

sleshchenko · 2017-12-20T08:30:29Z

...va/org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsMigrator.java

+            workspaceStoredByNameLocation,
+            workspaceStoredByIdLocation,
+            StandardCopyOption.ATOMIC_MOVE);
+        LOG.info(


I'd change info level here to debug.

no, stay with info. We need to provide this information to the user.

sleshchenko · 2017-12-20T08:32:13Z

...va/org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsMigrator.java

+        // migration is not needed for this workspace
+        continue;
+      }
+      LOG.info(


I'd change info level here to debug and add LOG.info with a message that migration stated before the loop.

sleshchenko · 2017-12-20T08:36:57Z

...va/org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsMigrator.java

+      Map<String, String> result = new HashMap<>();
+
+      for (WorkspaceImpl workspace :
+          iterate(


Iterate will eager fetch all objects from a database. I'd be nice to check how many memory it will consume when a number of workspaces is like 50 000 or the same as we have on codenvy.io.
If quite a lot then we should fetch workspace ids and names only. Something like that we've already done for another migration.

I think that's unnecessary, I don't think there will ever happen a migration of significant size of workspaces, ever.

benoitf · 2017-12-20T09:11:06Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties

@@ -292,6 +292,11 @@ che.infra.docker.bootstrapper.installer_timeout_sec=180
 # Once servers for one installer available - checks stopped.
 che.infra.docker.bootstrapper.server_check_period_sec=3

+# Enable to perform migration of workpace projects at Che startup.


should it be only for che.infra.docker ? (prefix)

Unfortunately, I was not able to protect such a naming when Tyler did choose another naming technique. I don't remember that anyone was supporting me at that point. Does it seem better now?

benoitf · 2017-12-20T09:13:45Z

dockerfiles/che/entrypoint.sh

@@ -246,6 +244,7 @@ init() {
    export CHE_DOCKER_IP_EXTERNAL=${HOSTNAME}
  fi
  ### Necessary to allow the container to write projects to the folder
+  export CHE_WORKSPACE_STORAGE__MASTER__PATH=/data/workspaces


why do we need to duplicate the property CHE_WORKSPACE_STORAGE ?
so we have
CHE_WORKSPACE_STORAGE__MASTER__PATH=/data/workspaces
and
CHE_WORKSPACE_STORAGE="${CHE_DATA_HOST}/workspaces"

also /data should be replaced by ${CHE_DATA} property

Because these are different paths?

skabashnyuk · 2017-12-20T20:08:58Z

Please change target branch of this pr to master branch.

skabashnyuk · 2017-12-22T09:26:29Z

...va/org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsMigrator.java

+            workspaceStoredByNameLocation,
+            workspaceStoredByIdLocation,
+            StandardCopyOption.ATOMIC_MOVE);
+        LOG.info(


no, stay with info. We need to provide this information to the user.

skabashnyuk · 2017-12-22T09:26:43Z

...va/org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsMigrator.java

+            entry.getKey(),
+            entry.getValue());
+      } catch (IOException e) {
+        LOG.debug(


skabashnyuk · 2017-12-22T09:30:12Z

...i-workspace/src/main/java/org/eclipse/che/api/workspace/server/model/impl/WorkspaceImpl.java

+  ),
+  @NamedQuery(
+    name = "Workspace.getByTemporaryCount",
+    query = "SELECT COUNT(w) " + "FROM Workspace w " + "WHERE w.isTemporary = :temporary "


Make sure we have separate index on isTemporary field

index_workspace_istemporary exists. Nevermind

sleshchenko · 2017-12-22T09:33:00Z

...va/org/eclipse/che/workspace/infrastructure/docker/local/projects/LocalProjectsMigrator.java

+  private static final Logger LOG = LoggerFactory.getLogger(LocalProjectsMigrator.class);
+
+  public void performMigration(String workspaceProjectsRootFolder) {
+    LOG.debug("Starting migration of workspace project files");


let's change it to info

sunix · 2018-01-02T11:17:44Z

Could we have a documentation on how to migrate ?

Add migration, that would move workspace projects to their new location. Occurs on Che startup.
Added configuration property to enable/disable this migration.

slemeur · 2018-01-02T11:18:38Z

So now when a user wants to explore content of a workspace - he has to know the workspace id ?

sunix · 2018-01-02T11:20:10Z

@slemeur no it's just the inderlying host folder name

sunix · 2018-01-02T11:20:44Z

it needs a release note for users who have existing workspaces, notify them that migration is needed.

skabashnyuk · 2018-01-09T09:15:28Z

@sunix

Could we have a documentation on how to migrate ?
it needs a release note for users who have existing workspaces, notify them that migration is >needed.

In this case, no action needed from the user, everything is going to happen automatically.

@slemeur

So now when a user wants to explore content of a workspace - he has to know the workspace id

Yes and no. If use specifies host.projects.root this folder will be mounted to each workspace he doesn't need to know anything about workspace id. But if data folder is configured by default we will put files in a tree where workspace id is used. That is important especially for multi-user use case when workspace names can be same for different users. And many users complain that this folder was overlapped in Che5.

skabashnyuk and others added 2 commits December 12, 2017 17:08

Change default workspace path

ea165e5

Add migration of workspaces storage on Docker infrastructure

4d4290a

mkuznyetsov requested review from garagatyi, skabashnyuk and vparfonov as code owners December 12, 2017 15:20

benoitf added target/che6 status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Dec 12, 2017

skabashnyuk requested a review from sleshchenko December 13, 2017 07:40

skabashnyuk reviewed Dec 13, 2017

View reviewed changes

garagatyi reviewed Dec 13, 2017

View reviewed changes

sleshchenko reviewed Dec 13, 2017

View reviewed changes

mkuznyetsov added 2 commits December 14, 2017 17:45

fixup! Add migration of workspaces storage on Docker infrastructure

2dd371c

fixup! fixup! Add migration of workspaces storage on Docker infrastru…

ccee5b0

…cture

mkuznyetsov requested a review from riuvshin as a code owner December 14, 2017 15:46

sleshchenko reviewed Dec 15, 2017

View reviewed changes

mkuznyetsov requested a review from benoitf as a code owner December 19, 2017 13:47

Allow getting workspace storage path inside of Che master container

cfc9af3

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi force-pushed the che-3574 branch from 632e28f to cfc9af3 Compare December 19, 2017 14:52

garagatyi reviewed Dec 19, 2017

View reviewed changes

Merge branch 'che6' of github.com:eclipse/che into che-3574

7f97478

mkuznyetsov changed the title ~~[WIP] Migrate workspace projects storage for Docker infrastructure~~ Migrate workspace projects storage for Docker infrastructure Dec 20, 2017

sleshchenko reviewed Dec 20, 2017

View reviewed changes

benoitf reviewed Dec 20, 2017

View reviewed changes

mkuznyetsov changed the base branch from che6 to master December 21, 2017 07:57

fixup

eb7ecf6

garagatyi approved these changes Dec 22, 2017

View reviewed changes

skabashnyuk reviewed Dec 22, 2017

View reviewed changes

skabashnyuk approved these changes Dec 22, 2017

View reviewed changes

sleshchenko reviewed Dec 22, 2017

View reviewed changes

sleshchenko approved these changes Dec 22, 2017

View reviewed changes

mkuznyetsov changed the title ~~Migrate workspace projects storage for Docker infrastructure~~ Change worskpace project files storage path Dec 22, 2017

fixup! fixup

b139a61

mkuznyetsov merged commit 93b974d into master Dec 22, 2017

mkuznyetsov deleted the che-3574 branch December 22, 2017 13:55

benoitf added this to the 6.0.0-M4 milestone Dec 22, 2017

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Dec 22, 2017

		@@ -225,7 +227,12 @@ public WorkspaceDto getByKey(
		// TODO add maxItems & skipCount to manager


		assertEquals(new HashSet<>(found), new HashSet<>(asList(workspace1, workspace2)));
		assertEquals(new HashSet<>(found.getItems()), new HashSet<>(asList(workspace1, workspace2)));

Change worskpace project files storage path #7844

Change worskpace project files storage path #7844

Conversation

mkuznyetsov commented Dec 12, 2017 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

skabashnyuk commented Dec 12, 2017

codenvy-ci commented Dec 13, 2017

skabashnyuk Dec 13, 2017 • edited by garagatyi 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

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

mkuznyetsov commented Dec 14, 2017

codenvy-ci commented Dec 14, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk commented Dec 20, 2017

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

sleshchenko Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

sunix commented Jan 2, 2018

slemeur commented Jan 2, 2018

sunix commented Jan 2, 2018

sunix commented Jan 2, 2018 • edited Loading

skabashnyuk commented Jan 9, 2018

mkuznyetsov commented Dec 12, 2017 •

edited

Loading

skabashnyuk Dec 13, 2017 •

edited by garagatyi

Loading

sleshchenko Dec 22, 2017 •

edited

Loading

sunix commented Jan 2, 2018 •

edited

Loading