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

Add support for Python virtual environment and optional pip upgrade. #159

Merged
merged 2 commits into from
Jan 24, 2017
Merged

Add support for Python virtual environment and optional pip upgrade. #159

merged 2 commits into from
Jan 24, 2017

Conversation

GrahamDumpleton
Copy link
Contributor

This set of changes addresses #90 by adding in use of a Python virtual environment. It means #58, #117 and #158 can be closed out as well since change provides way for user to indicate they want the latest pip version installed, overriding whatever version comes with SCL or which the image was explicitly installing to match RHEL.

I have checked this for CentOS images for each Python version by using OpenShift to do a Docker build of the respective directives and then deploying a test application etc. I have not checked this for RHEL variants of the images or used the top level makefile build system.

A number of points to look at when evaluating this change.

  • Execution of:
fix-permissions /opt/app-root

at end of s2i/bin/assemble generates lot of warnings.

chgrp: changing group of '/opt/app-root/include/python2.7': Operation not permitted
chgrp: changing group of '/opt/app-root/lib/python2.7/UserDict.py': Operation not permitted
chgrp: changing group of '/opt/app-root/lib/python2.7/_abcoll.py': Operation not permitted
...
chmod: changing permissions of '/opt/app-root/include/python2.7': Operation not permitted
chmod: changing permissions of '/opt/app-root/lib/python2.7/UserDict.py': Operation not permitted
chmod: changing permissions of '/opt/app-root/lib/python2.7/_abcoll.py': Operation not permitted
chmod: changing permissions of '/opt/app-root/lib/python2.7/_weakrefset.py': Operation not permitted

This is because the Python virtual environment creates symbolic links back to the main Python installation.

(app-root)sh-4.2$ pwd
/opt/app-root
(app-root)sh-4.2$ ls -las include
total 0
0 drwxrwxr-x. 2 default root 22 Nov 28 06:02 .
0 drwxrwxr-x. 7 default root 97 Nov 28 06:04 ..
0 lrwxrwxrwx. 1 default root 43 Nov 28 06:02 python2.7 -> /opt/rh/python27/root/usr/include/python2.7

The fault here is not the use of a Python virtual environment or where it is placed. The problem here is the fix-permissions script. That script has:

#!/bin/sh
# Fix permissions on the given directory to allow group read/write of
# regular files and execute of directories.
find $1 -exec chgrp 0 {} \;
find $1 -exec chmod g+rw {} \;
find $1 -type d -exec chmod g+x {} +

If any file created under the directory for which permissions are being changed is actually a symbolic link, then it will attempt to change the target of the symbolic link. If the symbolic link points to a file elsewhere in the file system which the user has no privileges to modify, you will get these warnings. As chmod on Linux has no way to say change the permissions on the link rather than the target, the way to get rid of all the warnings would be to use the -f option to chgrp and chmod. This at least suppresses the warnings. The other alternative is to use a fancy condition with find so that it doesn't exec the command when a file is a symbolic link.

Either way, this is an issue for the command base image that fix-permissions is added in.

  • The pip cache directory must be disabled.

There is no real reason to have the cache anyway as it just takes up extra space in the image and is not needed by the running application.

Leaving the pip cache directory in place causes other problems because the directory permissions get changed. As a consequence, pip will try and set them back to what it wants if pip were run in a subsequent layer. Changing permissions of a directory in a higher layer can cause the AUFS permissions bug and the directory then becomes inaccessible and could then cause pip to fail later.

To ensure the pip cache is always disabled, including for when pip is run explicitly in a derived image Dockerfile, the Dockerfile for these images should set:

PIP_NO_CACHE_DIR=off

It has to have a value of off because of the strange parsing that pip does on argument values.

  • Included optional upgrading of pip to latest version via environment variable. This is to address pip version is too old and can't install Linux wheels #158 where pip which is a part of SCL is too old to be able to install Linux wheels, plus other reported issues. Some packages are now only supplying binary wheels for Linux and no source packages. Upgrading enabled by setting UPGRADE_PIP_TO_LATEST to non empty value:
UPGRADE_PIP_TO_LATEST=on

as build time environment variable. Could also be supplied in the .s2i/environment file.

Making this change in same PR as adding Python virtual environment as it requires use of Python virtual environment. It is not possible to install a newer pip version into a per user site-packages directory as it can't first uninstall the pip version installed in system wide Python installation.

  • No longer need $HOME/.local/bin in $PATH so is removed. This should not cause any problems as was only used by Python when per user site-packages was being used. The --user option to pip has also been removed though as that is incompatible with Python virtual environments. That is, cannot use pip install --user when using a Python virtual environment.

There is a very low risk that someone had created a derived Docker image, or a custom assemble script which ran pip install --user for some reason, but that is not likely. So not expecting this change would really cause any real world impacts on existing users. If a user was affected, they would need to remove the --user option, and if running it from a Dockerfile and also activate the Python virtual environment first, in addition to enabling SCL packages as they would already have had to be doing.

  • As the Python virtual environment needs to be manually activated, if someone uses oc exec pod command or oc rsh pod command, then the Python virtual environment will not be activated. This is not seen as a problem because they also would have needed to enable the SCL Python package themselves as well. The latter would have meant they had to wrap the command in a shell script for it to work and rely on the BASH_ENV hack. So long as that is done then the Python virtual environment should be activated fine, as activation was added to the contrib/etc/scl_enable script invoked as a side effect of BASH_ENV.

  • In CentOS image for Python 3.3, still install pip 1.5.6 to match what was provided in RHEL image, but now only installed within the Python virtual environment and not in the base SCL Python installation. Also install it with pip rather than easy_install to ensure that it replaces the pip version in the Python virtual environment correctly.

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@rhscl-bot
Copy link

Can one of the admins verify this patch?

@GrahamDumpleton
Copy link
Contributor Author

May want to tweak the messages output when running pip due to:

and that can now indicate you want newer pip installed.

To be consistent all images should likely use:

if [[ -f requirements.txt ]]; then
  if [ -n "$PIP_INDEX_URL" ]; then
    echo "---> Installing dependencies via $PIP_INDEX_URL ..."
  else
    echo "---> Installing dependencies ..."
  fi
  pip install -r requirements.txt
fi

There will be duplicate information in logging when new enough pip version is available, but that is less of a problem than the fact that the assemble scripts in 2.7 and 3.5 differ to the others. For the purposes of making maintenance easy, they should all be the same and not be concerned if the progress log message from the assemble script is showing the index URL as well as pip. In the case where the URL is invalid and causes pip to fail on starting, could be said that more useful to always display in progress log message anyway in case the error message from pip is cryptic in the way it displays an invalid index URL.

@hhorak
Copy link
Member

hhorak commented Nov 29, 2016

[test]

@GrahamDumpleton
Copy link
Contributor Author

Note that of all the PRs, this one should be regarded as highest priority.

@hhorak
Copy link
Member

hhorak commented Jan 23, 2017

[test]

@pkubatrh
Copy link
Member

There should also be some test for this change present. Something along the lines of setting the env var to upgrade pip to latest and then trying to install a package that is available only as a wheel, but that can be added later. I have verified the new functionality manually for the time being.
LGTM, trying out new merge powers

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.

7 participants