Move root-required bits of drama-free-django build into Dockerfile by hkeeler · Pull Request #5145 · cfpb/consumerfinance.gov

hkeeler · 2019-07-25T22:35:08Z

The current drama-free-django Docker-based build tool (see docker/drama-free-django) works well in most cases, but we've had issues in CI/CD environments where the files written back to the host environment cannot be cleaned up since they're owned by root. This change moves the the root-required bits into a new Dockerfile where all the tools can be installed, and allows the DFD-related processes to run in the container whichever user you'd like.

Notes

When running the container as a user that exists on the host, but not in the container, you may notice a warning similar to:
```
/usr/bin/id: cannot find name for user ID 502
```
This is not anything to worry about. It simply means the uid/gid don't match any users/groups setup in the container.

Checklist

This change was made to make it easier to run the build and test processes as alternate users, which is sometimes necessary to make the volumes permissions line up with the Docker host. Additionally, changes paths using `/`, which was causing permissions issues when running as non-root.

…rfile

chosak

Generally this looks good, and works for me locally. I added a bunch of questions, and a few other things:

Is there somewhere sensible to put your note above about the ID warning? Maybe in the DFD README? Or are people not even going to notice that?
There are a bunch of warnings during the Yarn build:
```
warning Skipping preferred cache folder "/.cache/yarn" because it is not writable.
warning Selected the next writable cache folder in the list, will be "/tmp/.yarn-cache-501".
```
I don't know enough about Yarn to know what this means, but these don't appear during our normal frontend builds. Is this something to be concerned about?

We get similar warnings during the DFD build about pip:

WARNING: The directory '/.cache/pip/http' or its parent directory is not owned by the current user and the cache has been disabled. Please check the permissions and owner of that directory. If executing pip with sudo, you may want sudo's -H flag.

chosak · 2019-07-26T14:03:14Z

 # This is used by DFD to set Django's settings.STATIC_ROOT.
-echo '{"static_out": "../../../static"}' > ./dfd_paths.json
+# Q: Why do we need to override the default?
+# echo '{"static_out": "../static"}' > ./dfd_paths.json


This is needed because the default drama-free-django behavior is to set its static_out variable to 'static'. This is used to set Django STATIC_ROOT, which specifies where collectstatic puts its files. The default behavior would have this command collect staticfiles to a static relative path within the DFD deploy.

We instead want these files to go to '../../../static/', in practice going from, say, /srv/cfgov/versions/20190712095508/current to /srv/cfgov/static.

@rosskarchner's open PR #5133 currently includes a change that would set the default static root to /srv/cfgov/static as desired, but that'll only work once cfpb/drama-free-django#25 or something like it removes the DFD behavior that overwrites the static root at runtime.

Makes sense. I'll change it back.

chosak · 2019-07-26T14:04:33Z

 # Extract the artifact in /tmp.
-cp "$artifact_volume/$artifact_filename" /tmp
-cd /tmp
+mkdir -p $dfd_test_dir


Not a big deal, but I'm curious why you made a new directory for this. I figured that working in /tmp was fine since the container is ephemeral.

The issue was that by having it in /tmp, it resulted in directories being created in the / root directory, and the non-root user didn't have permission to create those directories. An alternative would be to open up permissions on /, but this seems like a better option.

chosak · 2019-07-26T14:06:59Z

+RUN yum install -y centos-release-scl && \
+    curl -sL https://rpm.nodesource.com/setup_10.x | bash - && \
+    curl -sL https://dl.yarnpkg.com/rpm/yarn.repo | tee /etc/yum.repos.d/yarn.repo && \
+    yum install -y ${SCL_PYTHON_VERSION} gcc git nodejs which yarn && \


Is the inclusion of which here leftover from debugging?

Yes, I can remove that. I find it strange that CentOS doesn't have which in its base distribution, but yeah.

chosak · 2019-07-26T14:07:28Z

+
+COPY _build.sh _test.sh docker-entrypoint.sh ./
+
+ENTRYPOINT [ "./docker-entrypoint.sh"]


Suggested change

ENTRYPOINT [ "./docker-entrypoint.sh"]

ENTRYPOINT ["./docker-entrypoint.sh"]

chosak · 2019-07-26T14:10:09Z

+docker run \
+  --rm \
+  -u $(id -u):$(id -g) \
+  -v $(pwd):/cfgov:cached \


Why is :cached used here in the test script but not in the build script? Is this Mac-specific, as documented here -- is it a no-op when run on Linux systems?

Unfortunately, it fails currently on Linux. I'll remove it.

chosak · 2019-07-26T14:13:05Z

+  --rm \
+  -u $(id -u):$(id -g) \
+  -v $(pwd):/cfgov:cached \
+  cfgov-dfd-builder ./_test.sh


This shouldn't hold up this PR, but, if we are going to maintain this DFD testing capability, I wonder if it's worth entertaining the idea of a distinct Dockerfile just for that purpose. We don't really need everything in the "cfgov-dfd-builder" image just to run the DFD image, and it would be nice to actually determine what is needed. But probably thinking more about that should wait until/if we want to think about migrating Ansible code here.

Yeah, it does feel a little awkward to have the two scripts that both build the same image, but my assumption was that generally when you'd run test.sh, you would have run build.sh just before it, and so the docker build part would be fully cached and be pretty instant, but I didn't want to make that a requirement to running test.sh, so it's duplicated in both scripts.

As for a separate image, yeah we could. It just seemed like it'd be better to only maintain one Dockerfile that could do both. But if there's a scenario where we'd be running just test.sh, maybe that starts to make more sense.

chosak · 2019-07-26T14:21:59Z

@@ -0,0 +1,23 @@
+FROM centos:7


The current versions of the build and test scripts use centos:6(also documented here). I did this deliberately to try to match the current setup. Is there a particular reason to change that version here?

That was not intentional. Will fix.

The version of pip that comes with SCL python27 has a bug that fails to process PIP_NO_CACHE_DIR correctly. Adding --no-cache-dir overrides the envvar, preventing the error.

…o dfd-dockerfile

hkeeler · 2019-07-27T05:41:37Z

@chosak, I think I've covered most of your comments.

Is there somewhere sensible to put your note above about the ID warning? Maybe in the DFD README? Or are people not even going to notice that?

Seems like a good place. I added a Notes section with those details.

There are a bunch of warnings during the Yarn build:
warning Skipping preferred cache folder "/.cache/yarn" because it is not writable.
warning Selected the next writable cache folder in the list, will be "/tmp/.yarn-cache-501".
I don't know enough about Yarn to know what this means, but these don't appear during our normal frontend builds. Is this something to be concerned about?

This was occurring because Docker defaults the home directory to / for users it doesn't know about, and then yarn didn't have permissions to create it's cache in it's preferred location (/.cache/yarn). My preferred solution would be to just not have a cache, since it doesn't make sense in a Docker build context, but yarn doesn't support that because reasons. So, instead I just added a HOME envvar right into the Dockerfile, pointed to a directory it could write to, and that made it happy.

We get similar warnings during the DFD build about pip:

WARNING: The directory '/.cache/pip/http' or its parent directory is not owned by the current user and the cache has been disabled. Please check the permissions and owner of that directory. If executing pip with sudo, you may want sudo's -H flag.

Now pip does support disabling cache with either the --no-cache-dir arg or the PIP_NO_CACHE_DIR envvar. I went the PIP_NO_CACHE_DIR route so it'd apply to all the pip installs...and after some wrangling, it works! No more warnings package manager warnings!

Fun fact: PIP_NO_CACHE_DIR originally disabled caching when set to a 0/false/off/no, but that was confusing, so they fixed it by also supporting 1/true/on/yes. That's right, this envvar disables the cache with either truthy or falsy value!

chosak

Works great for me locally. I'm looking forward to seeing this run on Jenkins.

That's right, this envvar disables the cache with either truthy or falsy value!

That is a fun fact! Makes me feel better about some of our envvar inconsistencies. 😄

Co-Authored-By: Andy Chosak <andy@chosak.org>

chosak · 2019-08-13T15:41:40Z

@hkeeler follow-up question about this PR. One concern I had that motivated the initial implementation as a single script was worrying about caching out-of-date versions of dependencies. Specifically with this line, if and when new commits are merged to drama-free-django master, that line of the Dockerfile won't be re-run, will it? Locally I've encountered "Using cache" during the build step. How does Docker determine which lines to re-run?

Would it be a good idea to use docker build --no-cache in the build and test scripts?

chosak · 2019-08-14T18:23:23Z

I've opened #5173 to add --no-cache.

hkeeler added 3 commits July 23, 2019 13:14

Merge branch 'master' of github.com:cfpb/cfgov-refresh into dfd-docke…

f2269c6

…rfile

Converts dfd scripts to use docker build, then run

8478002

hkeeler requested review from chosak, rosskarchner and wpears July 25, 2019 22:35

chosak reviewed Jul 26, 2019

View reviewed changes

hkeeler and others added 11 commits July 26, 2019 12:47

Revert back to centos:6 Docker image

270e1a6

Revert to original static_out path

a9a63bb

Override PIP_NO_CACHE_DIR on pip upgrade

a6e09f5

The version of pip that comes with SCL python27 has a bug that fails to process PIP_NO_CACHE_DIR correctly. Adding --no-cache-dir overrides the envvar, preventing the error.

Removes Mac-specific cached volume attribute

7431fba

Merge branch 'master' into dfd-dockerfile

490fe09

Removes unneeded which package

3cd92f0

Merge branch 'dfd-dockerfile' of github.com:hkeeler/cfgov-refresh int…

139f8d8

…o dfd-dockerfile

Removes unneeded question comment

e936ae7

Add headings and "Notes" section to DFD README.md

4433633

Add yarn cache warning to DFD Docker README

44fef31

Fix yarn warnings by setting $HOME in Dockerfile

5b963b4

chosak approved these changes Jul 29, 2019

View reviewed changes

Comment thread docker/drama-free-django/Dockerfile Outdated

hkeeler and others added 2 commits July 29, 2019 11:38

Fix typo in drama-free-django/Dockerfile

8bf713f

Co-Authored-By: Andy Chosak <andy@chosak.org>

Merge branch 'master' into dfd-dockerfile

72e7382

hkeeler merged commit ffc547a into cfpb:master Jul 29, 2019

chosak mentioned this pull request Aug 21, 2019

Add ability to create deployable zipfiles #5179

Merged

11 tasks


		COPY _build.sh _test.sh docker-entrypoint.sh ./

		ENTRYPOINT [ "./docker-entrypoint.sh"] No newline at end of file

	ENTRYPOINT [ "./docker-entrypoint.sh"]
	ENTRYPOINT ["./docker-entrypoint.sh"]

Conversation

hkeeler commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Checklist

Uh oh!

chosak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkeeler commented Jul 27, 2019

Uh oh!

chosak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chosak commented Aug 13, 2019

Uh oh!

chosak commented Aug 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hkeeler commented Jul 25, 2019 •

edited

Loading