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

Conversation

Projects
None yet
3 participants
@hornc
Collaborator

hornc commented Jun 29, 2018

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.

Show comment
Hide comment
@hornc

hornc Jul 15, 2018

Collaborator

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.

Collaborator

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 from WIP: Docker Branch, DONT MERGE, for review only! to Docker Branch, can co-exist with current Vagrant workflow Jul 17, 2018

@@ -0,0 +1,7 @@
.vagrant
.git/objects/*

This comment has been minimized.

@cdrini

cdrini Jul 19, 2018

Collaborator

Can this be just .git?

@cdrini

cdrini Jul 19, 2018

Collaborator

Can this be just .git?

This comment has been minimized.

@hornc

hornc Jul 19, 2018

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.

@hornc

hornc Jul 19, 2018

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.

@hornc

hornc Aug 14, 2018

Collaborator

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

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

@hornc

hornc Aug 14, 2018

Collaborator

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

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

errors
infogami
config
build

This comment has been minimized.

@cdrini

cdrini Jul 19, 2018

Collaborator

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

@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.

@hornc

hornc Jul 19, 2018

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.

@hornc

hornc Jul 19, 2018

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.

Show outdated Hide outdated docker/Dockerfile.olbase
Show outdated Hide outdated scripts/ol-docker-start.sh
Show outdated Hide outdated docker/Dockerfile.oldev
Show outdated Hide outdated docker/Dockerfile.oldev
Show outdated Hide outdated docker/Dockerfile.oldev
@cdrini

This comment has been minimized.

Show comment
Hide comment
@cdrini

cdrini Jul 19, 2018

Collaborator

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.

Collaborator

cdrini commented Jul 19, 2018

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.

Show comment
Hide comment
@hornc

hornc Jul 19, 2018

Collaborator

@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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@hornc

hornc Aug 14, 2018

Collaborator

@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?

Collaborator

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?

@cdrini

This comment has been minimized.

Show comment
Hide comment
@cdrini

cdrini Aug 16, 2018

Collaborator

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.

Collaborator

cdrini commented Aug 16, 2018

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.

Show comment
Hide comment
@cdrini

cdrini Aug 16, 2018

Collaborator

OHHHH, I think it's the symlinks. JQuery/Slick (the files that aren't loading for me) are symlinks. That might be related.

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 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.

Show comment
Hide comment
@mekarpeles

mekarpeles Aug 16, 2018

Member

Great catch about symbolic links for static files and also I'm shutting down the database gracefully

Member

mekarpeles commented Aug 16, 2018

Great catch about symbolic links for static files and also I'm shutting down the database gracefully

@hornc hornc added the docker label Aug 20, 2018

@cdrini

This comment has been minimized.

Show comment
Hide comment
@cdrini

cdrini Aug 21, 2018

Collaborator

Awesome! I'll run it on my machine to triple-check everything is ok, and then it should be g2g!

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

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

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