fix docker volume permissions by FrankChen021 · Pull Request #11167 · apache/druid

FrankChen021 · 2021-04-27T10:53:20Z

Fixes #11166 , See description in that issue for more info.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

xvrl · 2021-04-28T17:23:38Z

- && mkdir -p /opt/druid/var \
- && chown -R druid:druid /opt \
- && chmod 775 /opt/druid/var
+ && mkdir -p /opt


I don't think creating /opt is necessary, the COPY will take care of it

xvrl · 2021-04-28T17:28:26Z


 COPY --chown=druid:druid --from=builder /opt /opt
 COPY distribution/docker/druid.sh /druid.sh
+RUN mkdir -p /opt/druid/var \


nit, I would remove the -p to ensure /opt/druid exists and symlinks to a valid directory. When I did some testing, I realized that Docker for Mac gets confused when COPYing from the builder stage if /opt/druid is already created beforehand as a directory, causing it to fail to copy the symlink.

Not sure this is the appropriate place for this question since this has been like it since the dockerfile was added afaict, but what is the reason for using the symlink for /opt/druid? Based on some comments around this issue/PR, and also looking at #11278, it seems to cause issues in some situations, so is it actually necessary for some reason or have some benefit?

I checked the history of this file, the first commit used symlink there. I have no idea why symlink is used.

I guess:
The name of raw directory contains a version variable whose value is retrieve by executing maven command against the source directory, while the value of this version variable could not be accessed in the following stage.

xvrl · 2021-04-28T17:29:17Z

 COPY --chown=druid:druid --from=builder /opt /opt
 COPY distribution/docker/druid.sh /druid.sh
+RUN mkdir -p /opt/druid/var \
+ && chown -R druid:druid /opt/druid/var \


no need for -R here

Yes, there's no sub-dirs under /opt/druid/var, -R is unnecessary. Fixed

xvrl

LGTM, assuming the docker build has been tested.

xvrl

@FrankChen021 I just realized we also need to update Dockerfile.java11, can you please make the same changes there? Thanks!

this backports apache#11167 to our 0.21.0 branch

Signed-off-by: frank chen <frank.chen021@outlook.com>

FrankChen021 · 2021-04-30T02:17:39Z

Thanks for reminding, I forgot that. I've updated the Dockerfile for java11 in the new commit.
And the change of Dockerfile for java8 have been verified on my linux server.

xvrl · 2021-04-30T15:46:41Z

FYI @FrankChen021 I'm going to merge this. I manually tested the docker build and verified it runs, since we don't have an integration test for this.

jihoonson · 2021-04-30T15:50:46Z

LGTM. Thanks @FrankChen021 @xvrl.

@FrankChen021 Travis CI runs for both branches and PRs on this repo. Please create a branch on your fork next time, so that we don't waste our CI resources.

FrankChen021 · 2021-05-01T08:06:15Z

@jihoonson Thanks for reminding, I would follow your suggestion. I thought the branch was created on my fork when I edit the file directly on github, but it didn't.

frankiengkw · 2021-05-01T20:02:36Z

@jihoonson @FrankChen021 I faced this permission issue in my newly setup Druid. Where can I download the latest fix/patch? Thanks.

FrankChen021 · 2021-05-01T23:33:36Z

@frankiengkw there is a workaround you could try

Docker volume directory was accidentally removed due to reordering of statements. This causes ownership and permissions on the volume directory to be reset, preventing startup. fixes #11166 Signed-off-by: frank chen <frank.chen021@outlook.com>

pchang388 · 2021-05-12T21:53:17Z

We ran into this issue when trying to set things up.

Sorry to bother, but could someone help me understand why "COPY --chown" did not work as expected?

total 228
drwxr-xr-x    1 druid    druid         4096 Apr 26 17:13 licenses
-rw-r--r--    1 druid    druid        71187 Apr 26 17:28 NOTICE
-rw-r--r--    1 druid    druid        70924 Apr 26 17:28 LICENSE
-rw-r--r--    1 druid    druid         8228 Apr 26 17:28 README
drwxr-xr-x    1 druid    druid         4096 Apr 26 17:32 hadoop-dependencies
drwxr-xr-x    1 druid    druid         4096 Apr 26 17:32 extensions
drwxr-xr-x    1 druid    druid         4096 Apr 26 17:32 quickstart
drwxr-xr-x    1 druid    druid         4096 Apr 26 17:32 conf
drwxr-xr-x    1 druid    druid         4096 Apr 26 17:32 bin
drwxr-xr-x    1 druid    druid        16384 Apr 26 17:32 lib
drwxr-xr-x    2 root     root          4096 May 12 20:55 var

From what I've read it should have applied the user/group permissions recursively as it copied into "/opt". Just trying to understand the issue better.

FrankChen021 · 2021-05-13T07:37:22Z

Hi @pchang388 the /opt/druid under source directory is actually a symlink, and the COPY command replaces the target /opt/druid directory by this symbolic link. Since there's no var directory in source directory, COPY command could not copy var to the target.

pchang388 · 2021-05-13T15:18:02Z

Hi @pchang388 the /opt/druid under source directory is actually a symlink, and the COPY command replaces the target /opt/druid directory by this symbolic link. Since there's no var directory in source directory, COPY command could not copy var to the target.

Hi @FrankChen021 - thanks I took a look back at your explanation here: #11166

Essentially:

There is no var/ directory in the /opt/druid from builder
Even though we created earlier in the "mkdir -p " step - we are overwriting the entire /opt directory from builder
This means var/ disappears since we did a full overwrite of the /opt directory
Then uses "VOLUME /opt/druid/var" - which prepares the fs for mounting to external volume
Since there is no longer a var/, Docker creates it using user id of the person running the build command in this case appears to be root

Makes sense and appreciate you taking the time to help me understand.

Fix permission problem when creating dirs

c7ec1b0

FrankChen021 added Bug Docker https://hub.docker.com/r/apache/druid labels Apr 27, 2021

Fix dir in chown instruction

ee1baa6

xvrl reviewed Apr 28, 2021

View reviewed changes

Remove unnecessary instruction and arugment

642349f

xvrl approved these changes Apr 29, 2021

View reviewed changes

xvrl reviewed Apr 29, 2021

View reviewed changes

xvrl mentioned this pull request Apr 29, 2021

fix docker volume permissions confluentinc/druid#34

Merged

xvrl changed the title ~~Fix permission problem when creating dirs~~ fix docker volume permission Apr 29, 2021

xvrl added a commit to confluentinc/druid that referenced this pull request Apr 29, 2021

backport apache#11167: fix docker volume permissions (#34)

3cf13ce

this backports apache#11167 to our 0.21.0 branch

Fix Dockerfile for java11

1db8e7d

Signed-off-by: frank chen <frank.chen021@outlook.com>

xvrl mentioned this pull request Apr 30, 2021

fix Dockerfile steps order #11148

Closed

8 tasks

xvrl approved these changes Apr 30, 2021

View reviewed changes

xvrl changed the title ~~fix docker volume permission~~ fix docker volume permissions Apr 30, 2021

jihoonson approved these changes Apr 30, 2021

View reviewed changes

xvrl merged commit aec6524 into master Apr 30, 2021

xvrl deleted the FrankChen021-docker branch April 30, 2021 15:51

clintropolis added this to the 0.21.1 milestone May 4, 2021

rmorris708 mentioned this pull request May 12, 2021

Dockerfile 0.21.0 fixes - build and push to DockerHub #11244

Closed

clintropolis mentioned this pull request May 13, 2021

[Draft] 0.21.1 Release Notes #11249

Closed

FrankChen021 mentioned this pull request Aug 9, 2022

Druid Docker-Image not compatible with OpenShift default SecurityContextConstraint (SCC) #10777

Closed

Conversation

FrankChen021 commented Apr 27, 2021

Uh oh!

xvrl Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

xvrl Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

clintropolis May 21, 2021

Choose a reason for hiding this comment

Uh oh!

FrankChen021 May 21, 2021

Choose a reason for hiding this comment

Uh oh!

xvrl Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

xvrl left a comment

Choose a reason for hiding this comment

Uh oh!

xvrl left a comment

Choose a reason for hiding this comment

Uh oh!

FrankChen021 commented Apr 30, 2021

Uh oh!

xvrl commented Apr 30, 2021

Uh oh!

jihoonson commented Apr 30, 2021

Uh oh!

FrankChen021 commented May 1, 2021

Uh oh!

frankiengkw commented May 1, 2021

Uh oh!

FrankChen021 commented May 1, 2021

Uh oh!

pchang388 commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrankChen021 commented May 13, 2021

Uh oh!

pchang388 commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xvrl Apr 28, 2021 •

edited

Loading

pchang388 commented May 12, 2021 •

edited

Loading

pchang388 commented May 13, 2021 •

edited

Loading