WebSafe 3.7web.archive.org
|
|
🏠
The Wayback Machine - https://web.archive.org/web/20200207161943/https://github.com/internetarchive/openlibrary/pull/1012
Skip to content

/ openlibrary

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker Branch, can co-exist with current Vagrant workflow #1012

Merged
merged 19 commits into from Aug 22, 2018
Merged

Docker Branch, can co-exist with current Vagrant workflow #1012

merged 19 commits into from Aug 22, 2018

Conversation

@hornc
Copy link
Collaborator

hornc commented Jun 29, 2018 •
edited

I believe this is ready for merging. Ideally all the services will NOT be in the one container, but this is a starting point to begin with something that runs as well as the Vagrant dev env as a working baseline.

TODO

  • split out services into separate images Not in this PR
  • remove all vagrant / upstart / bootstrap.sh code Not yet, we will try to make Vagrant and Docker work in parallel before making a full switch
  • enable login (currently login fails trying to connect to port 8080)
  • fix vendor/js errors
  • test vagrant dev environment still works

Current Issues

  • What is going on with nginx service not being required? I thought it was critical in vagrant, doesn't appear to be necessary here?

  • Investigate why tomcat7 service reports error on start, but appears to function perfectly

  • sporadic test failure from openlibrary/catalog/add_book/test_add_book.py , test_duplicate_ia_book (looks like a problem with the test in general, not containers)

  • Perhaps we should move the initial solr indexing into the image as it triggers every time the container is run and increases startup time.

RESOLVED

  • postrgres start up seems very slow, site 500s and reports other errors if db not yet up, see #1050 , was because postgres was not shut down cleanly when creating the image.

  • js errors, Search dropdown not populated. most of vendor/js not populated (except wmd, which occurs via git submodule update)
@hornc

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

hornc commented Jul 15, 2018

This might explain why tomcat7 is reporting failure on service start, even though it appears to be running:
https://stackoverflow.com/questions/24451047/service-tomcat7-start-fails-but-the-process-exists-and-tomcat-is-running

to investigate.
@hornc hornc changed the title WIP: Docker Branch, DONT MERGE, for review only! Docker Branch, can co-exist with current Vagrant workflow Jul 17, 2018
@cdrini
cdrini requested changes Jul 19, 2018
View changes
.dockerignore
@@ -0,0 +1,7 @@
.vagrant
.git/objects/*

This comment has been minimized.

Sign in to view
Copy link
@cdrini

cdrini Jul 19, 2018

Collaborator

Can this be just .git?

This comment has been minimized.

Sign in to view
Copy link
@hornc

hornc Jul 19, 2018

Author Collaborator

This was done to enable the git sub-modules fetching of 3rd party stuff, including js, but to reduce the size of the context send to docker. I'll have to check all of this in light of your 404ing js below, because those items should have been fetched, so it sounds like there is a problem somewhere else.

This comment has been minimized.

Sign in to view
Copy link
@hornc

hornc Aug 14, 2018

Author Collaborator

@cdrini I rechecked this by setting it to .git and the make command fails on

openlibrary/Makefile

Line 32 in 495c389

git:
which runs as part of git all, so for now .git/objects/* excludes as much of the git cruft from the Docker build context as I could manage. There may be a way to remove this, but it will involve digging into the git task in the Makefile
.dockerignore
errors
infogami
config
build

This comment has been minimized.

Sign in to view
Copy link
@cdrini

cdrini Jul 19, 2018

Collaborator

errors, infogami, and build don't appear on my system; what are these files relative to?

This comment has been minimized.

Sign in to view
Copy link
@hornc

hornc Jul 19, 2018

Author Collaborator

They all appear in the root dir (at least on my dev machine :) ), I think they are all artefacts from the vagrant provisioning -- I'm guessing you started with a clean checkout? config is a symlink that is created to conf, which is odd... I thought that was done by bootstrap.sh, but now I can't find where the symlink was created.

I wanted to at least for now exclude stuff created by Vagrant provisioning from getting copied over to the image before we remove it completely. I think I need to confirm that Vagrant is still creating all of these. errors is only written to to log errors from the vagrant vm . It'll be a bit of extra work to support Vagrant in parallel, and should all be removed once Vagrant is completely gone.
docker/Dockerfile.olbase Outdated
# required for postgres
ENV LC_ALL POSIX

RUN apt-get -qq update

This comment has been minimized.

Sign in to view
Copy link
@cdrini

cdrini Jul 19, 2018

Collaborator

Can this be removed? It's done a few lines later.

This comment has been minimized.

Sign in to view
Copy link
@hornc

hornc Jul 19, 2018

Author Collaborator

good catch, this wasn't even temporarily useful :)
scripts/ol-docker-start.sh Outdated
@@ -0,0 +1,36 @@
#!/bin/bash

This comment has been minimized.

Sign in to view
Copy link
@cdrini

cdrini Jul 19, 2018

Collaborator

Can we put this file docker/? It feels a little more organized, cause the scripts/ directory already has way too much stuff in it :P

This comment has been minimized.

Sign in to view
Copy link
@hornc

hornc Jul 19, 2018

Author Collaborator

Good thinking!
docker/Dockerfile.oldev Outdated
ENV LANG en_US.UTF-8

# required for postgres
ENV LC_ALL POSIX

This comment has been minimized.

Sign in to view
Copy link
@cdrini

cdrini Jul 19, 2018

Collaborator

It seems like ENV variables are inherited ( moby/moby#33242 ), so these 2 can probably be removed from oldev.
docker/Dockerfile.oldev Outdated
# Using vim to change and test things, may want to remove?
RUN apt-get install -y vim

#COPY . /openlibrary # already copied from olbase

This comment has been minimized.

Sign in to view
Copy link
@cdrini

cdrini Jul 19, 2018

Collaborator

Can probably remove this.

This comment has been minimized.

Sign in to view
Copy link
@hornc

hornc Jul 19, 2018

Author Collaborator

A lot of these dupes over the two Docker files is because I was rebuilding ol-dev frequently and wanted local file changes copied into the image. I guess this won't be such an issue if the containers are generated at the same time, and we volume mount local files anyway.

Now is the time to DRY them up :)
docker/Dockerfile.oldev Outdated
FROM olbase

# Expose Open Library, Infogami, and SOLR
EXPOSE 80 7000 8983

This comment has been minimized.

Sign in to view
Copy link
@cdrini

cdrini Jul 19, 2018

Collaborator

I believe EXPOSE is inherited, so this can be removed.

This comment has been minimized.

Sign in to view
Copy link
@hornc

hornc Aug 14, 2018

Author Collaborator

I wanted to keep it there to document which ports that particular container expected, so each container could technically stand alone.
@cdrini

This comment has been minimized.

Sign in to view
Copy link
Collaborator

cdrini commented Jul 19, 2018 •
edited

After running docker run -it --rm -p8080:80 -p7000:7000 -p8983:8983 oldev I got a bunch of "connection rejected" errors for postgres for ~5 minutes. Specifically

/var/run/postgresql:5432 - rejecting connections

Then localhost:8080 wasn't working, but then I remembered it's because docker doesn't use localhost with Docker Toolbox :P Can you add this to the readme:

If you are using Docker Toolbox on Windows, use the Docker Machine IP instead of localhost. For example, 192.168.99.100:8080. To find the IP address, use the command docker-machine ip.

I could then access openlibrary locally running docker \o/ Although I got 404s for some static files, and the UI looked a little weird.

[19/Jul/2018 10:18:33] "HTTP/1.1 GET /static/js/jquery-1.11.0.min.js" - 404 File not found         
[19/Jul/2018 10:18:33] "HTTP/1.1 GET /static/js/jquery-migrate-1.2.1.min.js" - 404 File not found  
[19/Jul/2018 10:18:33] "HTTP/1.1 GET /static/js/slick-1.6.0.min.js" - 404 File not found

Otherwise:

  • Basic editing worked
  • I could access the solr admin endpoint
  • I could access the infobase endpoint

So overall woot! It's a bit slow to launch now because it has to setup the postgres stuff each time, but I think we can figure that out later.
@hornc

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

hornc commented Jul 19, 2018

@cdrini I thought I'd fixed the js issues and UI weirdness, I'll take another look tomorrow and see if I can figure out what has been missed.

If you have any thoughts on how to speed up the postgres start, that'd be super helpful too! Based on something you said, I was putting that down to a Xenial issue, rather than containers, but I haven't been able to find any obvious cause.
@hornc hornc force-pushed the docker branch from 63f7416 to 4430157 Aug 14, 2018
@hornc

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

hornc commented Aug 14, 2018

@cdrini, I've made changes per your comments. I tried running everything from a clean checkout and could not reproduce the js 404s. I did rebase, so if it was an unrelated code change that has since been fixed, it may be resolved? Could you get this updated branch and see if you are still getting those errors? I found that the local OL can get confused with exisiting cookies which can break the nav bar and logged in info. Using an incognito window is a quick way to check if that's the problem with UI display.

Any other comments?
@hornc hornc mentioned this pull request Aug 15, 2018
Closed
@cdrini

This comment has been minimized.

Sign in to view
Copy link
Collaborator

cdrini commented Aug 16, 2018 •
edited

I got it running, but I'm still having the same issue with static files :/ Interestingly enough, I can access file in /static/build/, but not /static/js/jquery-1.3.2.min.js, for example. I wonder if this might be related to the fact that openlibrary isn't on localhost on my computer? I'll look into it a bit longer, but it's probably better if we just move forward and resolve the issue later.
@cdrini

This comment has been minimized.

Sign in to view
Copy link
Collaborator

cdrini commented Aug 16, 2018

OHHHH, I think it's the symlinks. JQuery/Slick (the files that aren't loading for me) are symlinks. That might be related.
@cdrini
cdrini requested changes Aug 16, 2018
View changes
Copy link
Collaborator

cdrini left a comment •
edited

Ok, it looks like that static issues are because of symlinks on Windows. I think it's related to this: docker/for-win#109 . In the interest of progress, let's move forward though. I'll create an issue for further investigating this.

Final todos:

  • Add a link to the issue about symlinks: #1051
  • Could you fixup some of the commits that don't really make sense anymore? There' no need to keep some of those "fix typo"/"revert X"/etc commits hanging around.
@mekarpeles

This comment has been minimized.

Sign in to view
Copy link
Member

mekarpeles commented Aug 16, 2018

Great catch about symbolic links for static files and also I'm shutting down the database gracefully
hornc added 17 commits Jun 23, 2018
@hornc
add dockerignore
4f30ba7
@hornc
add initial dockerfiles
46f0849
@hornc
update base image olbase
6cac7fe
@hornc
make updater script executable
c8c1a7b
@hornc
get all services running inside one container for starters
ed35a63
@hornc
README for docker
fc03a0a
@hornc
absolute path to make symlink work
149f5f5
@hornc
vendor is requried for js components
1a0c81f
@hornc
modify dev db dump in place to support dual vagrant/docker use
7816a3e
@hornc
update docker readme based on feedback
08f74e9
@hornc
fix login for docker dev
0df1299
@hornc
docker test instructions
faf2f9e
@hornc
remove duplicate update command
da30ce3
@hornc
remove further duplicates pointed out in review
c866401
@hornc
add note for Windows Docker Toolbox
d229196
@hornc
move docker container startup script
da50f82
@hornc
use gunicorn to be more like prod
d8a69e5
@hornc
cleanly shutdown postgres, fixes #1050
Loading status checks…
feca04d
@hornc hornc force-pushed the docker branch from c9db3a8 to feca04d Aug 21, 2018
@hornc
add todo for fixing Windows symlink issue
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
2b933b4
@cdrini

This comment has been minimized.

Sign in to view
Copy link
Collaborator

cdrini commented Aug 21, 2018

Awesome! I'll run it on my machine to triple-check everything is ok, and then it should be g2g!
@cdrini
cdrini approved these changes Aug 21, 2018
View changes
Copy link
Collaborator

cdrini left a comment

Works like a charm! The postgres changes seem to have done the trick!
@mekarpeles mekarpeles merged commit 919f79a into master Aug 22, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Docker
3 participants
@hornc @cdrini @mekarpeles
You can’t perform that action at this time.