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
containers: Update to support Pulp 3.0 rc2 #127
containers: Update to support Pulp 3.0 rc2 #127
Conversation
Update to support installation from git nightly Also some changes necessary for pulp-operator to work Also some badly needed fixes that were always needed Other/included fixes/improvements: README.md: Update container list List all of those that currently can be installed at container build time. Update for new Pulp 3 default ports Update commands to start services; including them matching the systemd unit more closely Set DJANGO_SETTINGS_MODULE in all container images Fix possibly applicable security vulns in images by running `yum update` Install nightly version of Pulp (currently limited to always doing so) run migrations Set pulp admin password if defined Run django-admin collectstatic at runtime (so that /var/lib/pulp can be a persistent volume. i.e., so that stopping a container does not delete all your data.) [noissue]
Codecov Report
@@ Coverage Diff @@
## master #127 +/- ##
=======================================
Coverage 67.22% 67.22%
=======================================
Files 64 64
Lines 3030 3030
=======================================
Hits 2037 2037
Misses 993 993Continue to review full report at Codecov.
|
| exec scl enable rh-python36 "rq worker --url 'redis://${REDIS_SERVICE_HOST}:${REDIS_SERVICE_PORT}' -n reserved_resource_worker_${PULP_WORKER_NUMBER}@${HOSTNAME} -w 'pulpcore.tasking.worker.PulpWorker'" | ||
| # TODO: Set ${PULP_WORKER_NUMBER} to the Pod Number | ||
| # In the meantime, the hostname provides uniqueness. | ||
| exec scl enable rh-python36 "rq worker --url 'redis://${REDIS_SERVICE_HOST}:${REDIS_SERVICE_PORT}' -n reserved-resource-worker-${PULP_WORKER_NUMBER}@${HOSTNAME} -w 'pulpcore.tasking.worker.PulpWorker' -c 'pulpcore.rqconfig'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is working, but I'm not sure if you want to mix the commandline approach w/ the -c 'pulpcore.rqconfig approach. Maybe one or the other? For k8s maybe commandline is better? The -c option would be set from an environment variable as the other option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure. I am under the impression that multiple settings get set via pulpcore.rqconfig, but the commandline is overriding a small number of them. (Reading more about redis is something on my to-do list.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are six and they get set here: https://github.com/pulp/pulpcore/blob/master/pulpcore/rqconfig.py#L4
| RUN scl enable rh-python36 'pip install pulpcore-plugin pulpcore[postgres] pulpcore[mysql]' | ||
| RUN scl enable rh-python36 'pip install git+https://github.com/pulp/pulpcore.git' | ||
| RUN scl enable rh-python36 'pip install git+https://github.com/pulp/pulpcore-plugin.git' | ||
| RUN scl enable rh-python36 'pip install git+https://github.com/pulp/pulpcore.git#egg=pulpcore[postgres]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we want both to be baked in is that the idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what we did previously I think when we installed from pip. This is similar to the discussion over how to handle pulp's content plugins with containers.
|
|
||
| export DJANGO_SETTINGS_MODULE=pulpcore.app.settings | ||
|
|
||
| exec "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last line is necessary for an entrypoint.
The entrypoint is how you are supposed to set env vars for example. I am going to look into just doing the scl sourcing (a little bit more complex) in the entrypoint as well later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in previous versions of this branch (before I squashed the commits), I did even more in the entrypoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fine we can improve it over time. Thanks for the info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend not to see the value in this entrypoint over the basic entrypoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once I move/adapt the scl command into it, I think it will pay off for reducing duplicate code.
| ansible-playbook build.yaml -e '{"plugins": ["pulp_file", "pulp_ansible"]}' | ||
| ansible-playbook build.yaml -e '{"plugins": ["pulp_file", "pulp_ansible", "pulp_cookbook", "pulp_docker", "pulp_maven", "pulp_python"]}' | ||
|
|
||
| ansible-playbook build.yaml -e '{"plugins": ["git+https://github.com/pulp/pulp_file.git", "git+https://github.com/pulp/pulp_ansible.git", "git+https://github.com/gmbnomis/pulp_cookbook.git", "git+https://github.com/pulp/pulp_docker.git", "git+https://github.com/pulp/pulp_maven.git", "git+https://github.com/pulp/pulp_python.git"]}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be cleaner to encourage users to create a vars file and use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I'll do that in future PRs.
| yum -y install epel-release centos-release-scl && \ | ||
| yum -y install wget git rh-python36-python-pip && \ | ||
| yum -y install @development mariadb-devel rh-python36-python-devel && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this, we should consider using a multi-stage build container to avoid the development bits being needed in the production container and minimizing the size of the resulting image. Where are you tracking issues for container related deployments? I can file things there (as I would also ike to file an issue to consider switching to Universal Base Image (UBI) 8.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be installed only when we install from any git/nightly source. That is an improvement I'd like to make later on; auto-detection for whether these packages are needed.
| scl enable rh-python36 'django-admin migrate --noinput' | ||
| # Generating /var/lib/pulp/static at runtime rather than at container build time | ||
| # facilitates all of /var/lib/pulp being a separate volume. | ||
| scl enable rh-python36 "django-admin collectstatic --noinput" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the comment but I am still struggling with why generate static files on container start/restart. These files seem like they should be... well static for an installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I cannot think of a better way of handling it that isn't a lot of work up front / maintenance. Multiple subdirs under /var/lib/pulp/ need to be persistent storage. This one should be static and at build time.
|
|
||
| exec scl enable rh-python36 "gunicorn -b 0.0.0.0:8000 pulpcore.app.wsgi:application" | ||
| #TODO: Determine list of installed plugins by inspecting image contents | ||
| scl enable rh-python36 "django-admin makemigrations file ansible cookbook docker maven python" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are plugins not yet shipping their migrations? Even so, this feels like a build time effort not a runtime effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. The bigger problem is that makemigrations requires access to the db. So it has to be runtime, not build time.
| @@ -0,0 +1,5 @@ | |||
| #!/bin/bash | |||
|
|
|||
| export DJANGO_SETTINGS_MODULE=pulpcore.app.settings | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this value ever get set to anything different? If not, can it be set on the images during buildtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I didn't know about the ENV syntax. I'm adding this to a list of future improvements.
| exec scl enable rh-python36 "gunicorn pulpcore.content:server \ | ||
| --bind 0.0.0.0:24816 \ | ||
| --worker-class 'aiohttp.GunicornWebWorker' \ | ||
| -w 2 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be $THREADS or something similar to allow customization at runtime depending on a users setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should merge this as-is and we should make additional improvements in followup PRs.
Update to support installation from git nightly
Also some changes necessary for pulp-operator to work
Also some badly needed fixes that were always needed
Other/included fixes/improvements:
These have been successfully used with the entire operator:
https://github.com/mikedep333/pulp-operator/tree/summit-demo
And the patched redis only from here:
https://github.com/mikedep333/carafe/tree/summit-demo
Also, to clarify, these are not supposed to be in a perfect state. Just a significantly better state. Much more work remains to be done in the operator, and a moderate amount remains here. Largely because the containers are dependent on each other, they need to be used with an operator. Theoretically you could use another solution as well.