CHE-2874 Let users configure the 'Z' flag when mouting a volume #2921

snjeza · 2016-10-27T18:57:53Z

What does this PR do?

It adds the following two properties:

che.machine.docker.volumes.projects.options (Z by default)

that is used when che mounts the /projects volume

che.machine.docker.volumes.workspace.options (ro,Z by default)

used when mounting the following volumes:

/mnt/che/terminal
/mnt/che/ws-agent.tar.gz
/mnt/che/conf

Notice:
che-launcher also uses the Z option. I haven't changed it.

What issues does this PR fix or reference?

#2874

Signed-off-by: Snjezana Peco [email protected]

codenvy-ci · 2016-10-27T18:59:02Z

Can one of the admins verify this patch?

TylerJewell · 2016-10-27T18:59:49Z

@snjeza - thanks for this contribution. You may not have seen it, but there was a major merge this morning where we renamed all of the che.properties so that they all start with che. and we simplified them to make them clearer for users to understand. Let me dig up the new che.properties and the merge, and perhaps you can propose a new name and location for these properties?

#2707

l0rd · 2016-10-31T16:34:31Z

LGTM @garagatyi @benoitf can you please have a look at this PR that fixes #2874

benoitf · 2016-10-31T16:40:00Z

...-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/MachineProviderImpl.java

                                                workspaceFolderPathProvider.getPath(workspaceId),
                                                projectFolderPath);
+            if (volumeOptions != null && !volumeOptions.isEmpty()) {


you should use guava helper isNullOrEmpty / com.google.common.base.Strings.isNullOrEmpty;

benoitf · 2016-10-31T16:40:11Z

...-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/MachineProviderImpl.java

                                                workspaceFolderPathProvider.getPath(workspaceId),
                                                projectFolderPath);
+            if (volumeOptions != null && !volumeOptions.isEmpty()) {
+                projectFolderVolume = projectFolderVolume + ":" + volumeOptions;


use of += to add to current ?

benoitf · 2016-10-31T16:41:08Z

...in/java/org/eclipse/che/plugin/docker/machine/ext/provider/DockerExtConfBindingProvider.java

@@ -55,18 +61,22 @@ public String get() {
            return null;
        }

+        if (volumeOptions == null) {
+            volumeOptions="ro,Z";


IMHO default options should be in properties file or to be a constant

benoitf · 2016-10-31T16:42:33Z

...in/java/org/eclipse/che/plugin/docker/machine/ext/provider/DockerExtConfBindingProvider.java

            } catch (IOException e) {
                LOG.warn(e.getMessage());
                throw new RuntimeException(e);
            }
        } else {
-            return extConfDir.getAbsolutePath() + CONTAINER_TARGET;
+            return extConfDir.getAbsolutePath() + CONTAINER_TARGET + ":" + volumeOptions;


small concern : maybe to have a getTargetOptions() helper method as there are some duplicates between unix and windows

benoitf · 2016-10-31T16:43:38Z

.../src/main/java/org/eclipse/che/plugin/docker/machine/ext/provider/WsAgentVolumeProvider.java

        if (SystemInfo.isWindows()) {
+            if (volumeOptions == null) {
+                volumeOptions="ro,Z";


constants or directly as default properties ?

snjeza · 2016-10-31T17:47:54Z

I have updated the patch.

benoitf · 2016-10-31T18:13:37Z

...in/java/org/eclipse/che/plugin/docker/machine/ext/provider/DockerExtConfBindingProvider.java

-    public static final String EXT_CHE_LOCAL_CONF_DIR = "/mnt/che/conf";
+    private static final String RO_Z = "ro,Z";
+
+	public static final String EXT_CHE_LOCAL_CONF_DIR = "/mnt/che/conf";


indent is not correct

benoitf · 2016-10-31T18:14:53Z

...in/java/org/eclipse/che/plugin/docker/machine/ext/provider/DockerExtConfBindingProvider.java

@@ -55,18 +63,27 @@ public String get() {
            return null;
        }

+        if (volumeOptions == null) {


as @Inject on volumeOptions is not optional, I think that we could get rid of default value from the code as it becomes a mandatory parameter

benoitf · 2016-10-31T18:15:44Z

...src/main/java/org/eclipse/che/plugin/docker/machine/ext/provider/TerminalVolumeProvider.java

    @Override
    public String get() {
+        if (volumeOptions == null) {


same here, as it's a required parameter it needs to be provided so we don't have to handle this

benoitf · 2016-10-31T18:19:03Z

ci-build

snjeza · 2016-10-31T18:32:35Z

The patch has been updated.

benoitf

need to sync before merging this property for codenvy properties

codenvy-ci · 2016-10-31T19:47:47Z

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

TylerJewell · 2016-10-31T20:01:19Z

Nicely done @snjeza - @l0rd - will you be managing the merge process + milestone selection + QA communications?

l0rd · 2016-11-01T00:53:20Z

@TylerJewell can you be more explicit on the QA communications? I would like to help but I'm not sure to understand what is expected.
@benoitf in what repository are codenvy properties? Should @snjeza update them?

TylerJewell · 2016-11-01T04:38:22Z

Yes, sure thing. When you merge the issue that is targeted for a particular milestone, please talk to @vkuznyetsov about whether there should be any additional QA or integration tests added into the CI system that can be run regularly to test this behavior.

All of the "base" che.properties are in assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes.

Note that there is a new che.properties that has a new naming scheme. They are all designed to be user friendly. There is an alias file which maps the new names to the old names so that if you still use the old properties then they will still work for awhile.

garagatyi

Overall PR is good. Please address my comments

garagatyi · 2016-11-01T08:45:53Z

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

+che.machine.docker.volumes.projects.options=Z
+
+# Adds options when mounting the /mnt/che/terminal, /mnt/che/ws-agent.tar.gz, /mnt/che/conf volume
+che.machine.docker.volumes.workspace.options=ro,Z


According to a comment it related to agents rather to workspace. Probably property name should be aligned with the comment.

garagatyi · 2016-11-01T08:49:17Z

...plugin-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/json/HostConfig.java

@@ -41,6 +41,7 @@
    private String         pidMode;
    private boolean        readonlyRootfs;
    private Ulimit[]       ulimits;
+    private String         volumeOptions;


I don't see such field in Docker Create container API. Can you point me out where it is?

garagatyi · 2016-11-01T08:51:53Z

...-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/MachineProviderImpl.java

@@ -146,7 +147,8 @@ public MachineProviderImpl(DockerConnector docker,
                               @Named("machine.docker.machine_env") Set<String> allMachinesEnvVariables,
                               @Named("che.docker.registry_for_snapshots") boolean snapshotUseRegistry,
                               @Named("che.docker.swap") double memorySwapMultiplier,
-                               @Named("machine.docker.networks") Set<Set<String>> additionalNetworks)
+                               @Named("machine.docker.networks") Set<Set<String>> additionalNetworks,
+                               @Nullable @Named("che.machine.docker.volumes.projects.options") String volumeOptions)


We use dots to separate categories of implementation. We use underscores to separate words of single term. Please ensure that you are following that policy

garagatyi · 2016-11-01T08:53:41Z

...in/java/org/eclipse/che/plugin/docker/machine/ext/provider/DockerExtConfBindingProvider.java

    private static final Logger LOG              = LoggerFactory.getLogger(DockerExtConfBindingProvider.class);

+    @Inject
+    @Named("che.machine.docker.volumes.workspace.options")


Please ensure that you are following properties naming policy described above

snjeza · 2016-11-01T15:43:03Z

I have changed the names of the properties and added the volumeOptions field to the Docker Create container API.

garagatyi · 2016-11-02T08:10:53Z

Probably we misunderstand each other. I meant that I can't find volumeOptions in Docker API documentation. So I don't understand why you added such a field in HostConfig object which is our representation of Docker API model.

Can you elaborate?

snjeza · 2016-11-02T14:12:49Z

I have reviewed the che.docker.privilege property and implemented the che.docker.volumes_projects_options property in a similar way.

garagatyi · 2016-11-03T11:24:29Z

che.docker.privilege property represents a flag from Docker API documentation, whereas volumeOptions is not present in HostConfig representation.

snjeza · 2016-11-03T15:15:01Z

I will try to move the volumeOptions property to another class.

snjeza · 2016-11-03T18:41:20Z

I have removed the volumeOptions property from Docker API documentation.

bmicklea · 2016-11-08T15:20:38Z

M7 needs to be complete by tomorrow (Nov 8) pushing to M8 but if it's merged tomorrow it can be reassigned to M7.

l0rd · 2016-11-11T14:00:13Z

@snjeza please let me know when you have confirmed that this PR will fix the issue when running Che on CentOS 7 with RH packaged Docker.

garagatyi · 2016-11-14T10:24:02Z

Sorry for delay, @snjeza. I've refactored code around container bootstrapping for simplifying code for environment SPI implementation. Please rework your PR because of that.

Now volume for projects is bound in LocalCheInfrastructureProvisioner. Other changes in this PR looks good to me

snjeza · 2016-11-15T00:11:03Z

I have updated the patch.
@l0rd
I will test the new patch with centos 7. Currently, I can't start the workspace when using the master branch with or without my patch.

garagatyi

Good job!

Signed-off-by: Snjezana Peco <[email protected]>

snjeza · 2016-11-17T13:08:05Z

@l0rd,
I have tested the patch with centos 7. It works. I have had to use the overlay driver to start che with and without the patch.

l0rd · 2016-11-17T13:51:23Z

Thank you @snjeza 🎉 LGTM

TylerJewell · 2016-11-17T13:54:55Z

@benoitf - very important that we capture these changes and merge them as part of the CLI consolidation.

TylerJewell added the kind/enhancement A feature request - must adhere to the feature request template. label Oct 27, 2016

snjeza force-pushed the issue2874 branch 3 times, most recently from bcb6efa to 6b354f9 Compare October 27, 2016 20:11

benoitf reviewed Oct 31, 2016

View reviewed changes

snjeza force-pushed the issue2874 branch from 6b354f9 to 1608def Compare October 31, 2016 17:45

benoitf reviewed Oct 31, 2016

View reviewed changes

snjeza force-pushed the issue2874 branch from 1608def to b4744fc Compare October 31, 2016 18:31

benoitf approved these changes Oct 31, 2016

View reviewed changes

benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Oct 31, 2016

l0rd added this to the 5.0.0-M7 milestone Nov 1, 2016

garagatyi suggested changes Nov 1, 2016

View reviewed changes

snjeza force-pushed the issue2874 branch from b4744fc to d3d688f Compare November 1, 2016 15:42

snjeza force-pushed the issue2874 branch from d3d688f to 194c8fa Compare November 3, 2016 18:29

TylerJewell mentioned this pull request Nov 3, 2016

Eclipse Che CLI Refactoring + Improvements #2977

Closed

24 tasks

bmicklea assigned l0rd Nov 8, 2016

bmicklea modified the milestones: 5.0.0-M8, 5.0.0-M7 Nov 8, 2016

snjeza force-pushed the issue2874 branch from 194c8fa to 2b83425 Compare November 15, 2016 00:09

garagatyi approved these changes Nov 15, 2016

View reviewed changes

CHE-2874 Let users configure the 'Z' flag when mouting a volume

4620c17

Signed-off-by: Snjezana Peco <[email protected]>

snjeza force-pushed the issue2874 branch from 2b83425 to 4620c17 Compare November 15, 2016 14:43

l0rd merged commit b7682dd into eclipse-che:master Nov 17, 2016

l0rd removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 17, 2016

l0rd mentioned this pull request Dec 1, 2016

Let users configure the "Z" flag when mouting a volume #2874

Closed

eriklogtenberg mentioned this pull request Feb 11, 2018

Support for volume :z and :Z flags for swarm/stack deployment moby/moby#36282

Closed

CHE-2874 Let users configure the 'Z' flag when mouting a volume #2921

CHE-2874 Let users configure the 'Z' flag when mouting a volume #2921

Conversation

snjeza commented Oct 27, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

codenvy-ci commented Oct 27, 2016

TylerJewell commented Oct 27, 2016 • edited Loading

l0rd commented Oct 31, 2016

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

snjeza commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented Oct 31, 2016

snjeza commented Oct 31, 2016

benoitf left a comment

Choose a reason for hiding this comment

codenvy-ci commented Oct 31, 2016

TylerJewell commented Oct 31, 2016

l0rd commented Nov 1, 2016

TylerJewell commented Nov 1, 2016

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

snjeza commented Nov 1, 2016

garagatyi commented Nov 2, 2016

snjeza commented Nov 2, 2016

garagatyi commented Nov 3, 2016

snjeza commented Nov 3, 2016

snjeza commented Nov 3, 2016

bmicklea commented Nov 8, 2016

l0rd commented Nov 11, 2016

garagatyi commented Nov 14, 2016

snjeza commented Nov 15, 2016

garagatyi left a comment

Choose a reason for hiding this comment

snjeza commented Nov 17, 2016

l0rd commented Nov 17, 2016

TylerJewell commented Nov 17, 2016

snjeza commented Oct 27, 2016 •

edited

Loading

TylerJewell commented Oct 27, 2016 •

edited

Loading