Skip to content
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

18.09 update #464

Merged
merged 38 commits into from
Dec 21, 2018
Merged

18.09 update #464

merged 38 commits into from
Dec 21, 2018

Conversation

abretaud
Copy link
Collaborator

Various fixes for the 18.09 compose images
It's still failing currently until galaxyproject/ansible-galaxy-extras#220 is merged

@@ -27,7 +27,7 @@ RUN echo "force-unsafe-io" > /etc/dpkg/dpkg.cfg.d/02apt-speedup && \
# apt-get update -qq && apt-get upgrade -y && \
# apt-get update -qq && \
apt-get install --no-install-recommends -y \
nginx-extras nginx-common supervisor autofs libslurm32 libslurmdb32 && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although beyond this PR, it would be really great if things like slurm dependencies could be installable via ansible and then that could be enabled or disabled via injection of arguments on build time.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcm32 are you using the pre-build images or are you building the images on your own again? Or what is your preferred use-case? We could also define those libs via an ENV and create different web containers during build-time - not sure this is a use-case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ourselves building different flavours might be too much trouble to maintain, but I would at least leave the flexibility for the advanced user who knows what is doing. I like the model of the labels that we have, where to add the support for certain things, you need to add that label (ie. k8s,cvmfs, etc). So I would extend that, as man power allows, to other sections (like telegraf, slumr, condor, etc) that are optional requirements depending on how you are using this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgruening I'm probably still building my own because I need to point to a version of ansible galaxy extras that includes this: galaxyproject/ansible-galaxy-extras#213
On my setup, it fails because uswgi process I think points to that galaxy.yml but that file doesn't exists at that point... the test not passed seems to be unrelated to the logic added on that PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgruening in your compose setup works without my PR ansible-galaxy-extras due to https://github.com/bgruening/docker-galaxy-stable/blob/dev/compose/docker-compose.yml#L327 but the current PR (464) is actually removing that line, so the compose setup will need the solution proposed in https://github.com/bgruening/docker-galaxy-stable/blob/dev/compose/docker-compose.yml#L327 I think....

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the mount of galaxy.yml because when you launch docker-compose up, the file doesn't exist yet, so Docker decides to create an empty directory named "galaxy.yml". This makes it impossible later to copy the good galaxy.yml file during startup. So galaxy fails to start because it can't read galaxy.yml.
Mounting a non existing file will never work as it will be considered a directory by docker

@@ -8237,7 +8126,7 @@ GalaxyTools lib/tool_shed/galaxy_install/migrate 1
--

COPY migrate_version (repository_id, repository_path, version) FROM stdin;
Galaxy lib/galaxy/model/migrate 144
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I must have been testing a few things on 19.01 when I pushed this... sorry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been thinking for a while on incorporating

https://github.com/bgruening/docker-galaxy-stable/blob/dev/compose/dumpsql.sh#L9-L36

inside the build-orchestration-images script, right after the init container creation (using that init container created). Then this whole file would be generated on the fly when building the containers, based on the version that it was used of Galaxy for those containers (so fairly failure-proof). Would you consider adding that to your PR @abretaud ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that would be cool. I'm not sure how to do it properly though: do you mean that at the end of build-orchestration-images we should:

  1. run a docker-compose with an empty postgresql database
  2. run the install_db.sh in galaxy-web
  3. run the dumpsql.sh script
  4. (re)build the galaxy-postgres image

Not sure I'll have time to write this now, maybe it could be another PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, don't worry, I'll PR it soon.

@abretaud
Copy link
Collaborator Author

There are a few errors left, I'm looking into them.
For https://travis-ci.org/bgruening/docker-galaxy-stable/jobs/467400898, the problem is that nginx is not listening on port 443 as it fails to load the config when reading the "upload_store" instruction in /etc/nginx/nginx.conf.
So the problem is that the nginx-upload-module is not installed, and probably not in any ubuntu 18.04 package. I've found https://launchpad.net/~galaxyproject/+archive/ubuntu/nginx, but it doesn't seem to be up-to-date.
Any help on this?

@bgruening
Copy link
Owner

Let's remove the upload module, it is not needed in recent Galaxy versions afaik.

…ted later by galaxy-init). It was mounted as an empty dir, so galaxy refused to run. The /usr/bin/startup script will copy the good file from $GALAXY_ROOT a bit later during galaxy-web startup, after galaxy-init has created it (https://github.com/galaxyproject/ansible-galaxy-extras/blob/18.09/templates/startup.sh.j2#L42).
@abretaud
Copy link
Collaborator Author

Still a few errors, I'll try to continue next week
Is it intended that the client is not built in compose? I guess it should now in 18.09?

@pcm32
Copy link
Contributor

pcm32 commented Dec 14, 2018

mmm... weird, I remember adding some commits around a month ago or so to have the client built in these containers. But it needs to be built, it was only when I had it built that these images worked for me (I was a bit ahead of 18.09 though, but the same should apply).

@pcm32
Copy link
Contributor

pcm32 commented Dec 14, 2018

Yes, look, we are building the client in the init (and that is why we kept the --skip-build-client on web):

5915886

and then this gets copied to the web container on runtime. Due to this I would also say that you don't need to install nodejs/yarn either on web (and the whole setup works for me without installing those on web AFAIK).

@abretaud
Copy link
Collaborator Author

@pcm32 for some reason, the static/scripts dir is missing in the galaxy-web container when I run it locally, I'll investigate.
For nodejs, we still need it galaxy-web to run the GIE proxy (got a supervisor error without it when it's not there)

@abretaud abretaud changed the title [WIP] 18.09 update 18.09 update Dec 19, 2018
@abretaud
Copy link
Collaborator Author

I think it's ok now, the last failing test was probably a temporary error from the toolshed. Rerunning it should solve it (I don't have the perm to trigger it)
The k8s test is still failing, but I guess it's more or less expected

Copy link
Owner

@bgruening bgruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super cool! I owe you a couple of beers :)

Just two small comments inline.

@@ -1,6 +1,6 @@
#!/bin/bash

TAG=18.09
TAG=v18.09
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should that be 18.09?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested, but no, it failed (=empty dump) without the v, because all images are tagged with this v

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mh, ok I might remove this at some point. We should standardize it ...

@@ -18,3 +18,8 @@ RUN wget https://dl.influxdata.com/telegraf/releases/telegraf-1.5.0_linux_amd64.
rm telegraf-1.5.0_linux_amd64.tar.gz

ADD telegraf.conf /etc/telegraf/telegraf.conf


RUN touch /var/log/condor/StartLog /var/log/condor/StarterLog /var/log/condor/CollectorLog /var/log/condor/NegotiatorLog && \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed now I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested it: I think it's still needed as this image is not based on galaxy-base. I get errors at startup without this line

@bgruening bgruening merged commit 3eabd62 into bgruening:dev Dec 21, 2018
@bgruening
Copy link
Owner

Thanks a lot!!!!

AndreasSko pushed a commit to AndreasSko/docker-galaxy-stable that referenced this pull request May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants