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.

Sign up for GitHub

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

RFR: Add --always-copy option to create_virtualenv method for pack virtual… #2372

Merged
merged 6 commits into from
Jan 13, 2016
Merged

RFR: Add --always-copy option to create_virtualenv method for pack virtual… #2372

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1ea7c45
Add --always-copy option to create_virtualenv method for pack virtual…
Jan 12, 2016
b938ff0
Better error messages for OSError
Jan 13, 2016
20fe3cb
Update CHANGELOG
Jan 13, 2016
30ccc43
Merge branch 'master' into virtualenv_fix
Jan 13, 2016
ae80051
Set virtualenv_opts to --always-copy in st2.conf for new packages
Jan 13, 2016
8f421df
Comma separated list is the only one supported for ListOpt in oslo
Jan 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ requirements: virtualenv .sdist-requirements

# Make sure we use latest version of pip
$(VIRTUALENV_DIR)/bin/pip install --upgrade pip
$(VIRTUALENV_DIR)/bin/pip install virtualenv # Required for packs.install in dev envs.

# Generate all requirements to support current CI pipeline.
$(VIRTUALENV_DIR)/bin/python scripts/fixate-requirements.py --skip=virtualenv -s st2*/in-requirements.txt -f fixed-requirements.txt -o requirements.txt
Expand Down
13 changes: 11 additions & 2 deletions contrib/packs/actions/pack_mgmt/setup_virtualenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,24 @@ def _setup_pack_virtualenv(self, pack_name, update=False):

def _create_virtualenv(self, virtualenv_path):
python_binary = cfg.CONF.actionrunner.python_binary
virtualenv_binary = cfg.CONF.actionrunner.virtualenv_binary
virtualenv_opts = cfg.CONF.actionrunner.virtualenv_opts

if not os.path.isfile(python_binary):
raise Exception('Python binary "%s" doesn\'t exist' % (python_binary))

self.logger.debug('Creating virtualenv in "%s" using Python binary "%s"' %
(virtualenv_path, python_binary))

cmd = ['virtualenv', '-p', python_binary, '--system-site-packages', virtualenv_path]
exit_code, _, stderr = run_command(cmd=cmd)
cmd = [virtualenv_binary, '-p', python_binary]
cmd.extend(virtualenv_opts)
cmd.extend([virtualenv_path])
self.logger.debug('Running command "%s" to create virtualenv.', ' '.join(cmd))

try:
exit_code, _, stderr = run_command(cmd=cmd)
except OSError:
raise Exception('Virtualenv binary "%s" doesn\'t exist.' % virtualenv_binary)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would also include original error message at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original error message was "No such file or directory" without naming the file. This was confusing. Thus this new message.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

But OSError doesn't necessary only mean that the file doesn't exist, it could also represent a permissions error, etc. That's why I think it's also good to include the original message somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me log the original exception and raise this new one. Does that sound good to you?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to include it in the exception message, but whatever makes you happy :P


if exit_code != 0:
raise Exception('Failed to create virtualenv in "%s": %s' %
Expand Down
13 changes: 11 additions & 2 deletions st2actions/st2actions/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
Configuration options registration and useful routines.
"""

import os
import sys

from oslo_config import cfg
Expand All @@ -41,11 +42,19 @@ def _register_common_opts():


def _register_action_runner_opts():
default_python_bin_path = sys.executable
base_dir = os.path.dirname(os.path.realpath(default_python_bin_path))
default_virtualenv_bin_path = os.path.join(base_dir, 'virtualenv')
logging_opts = [
cfg.StrOpt('logging', default='conf/logging.conf',
help='location of the logging.conf file'),
cfg.StrOpt('python_binary', default=sys.executable,
help='Python binary which will be used by Python actions.')
cfg.StrOpt('python_binary', default=default_python_bin_path,
help='Python binary which will be used by Python actions.'),
cfg.StrOpt('virtualenv_binary', default=default_virtualenv_bin_path,
help='Virtualenv binary which should be used to create pack virtualenvs.'),
cfg.ListOpt('virtualenv_opts', default=['--always-copy', '--system-site-packages'],
Copy link
Member

Choose a reason for hiding this comment

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

Did we verify how much of a difference in terms of virtualenv disk usage --always-copy makes?

Copy link
Member

Choose a reason for hiding this comment

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

I also googled around a bit and it looks like --always-copy didn't always work correctly in some older versions of virtualenv.

Because of that we need to make sure the latest confirmed working version of virtualenv and installed and used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see StackStorm/st2-packages#53 with regards to disk space requirements. I already pasted the disk spaces there.

Because of that we need to make sure the latest confirmed working version of virtualenv and installed and used.

Yep. I saw the issue and tested with virtualenv 12.1.1 which was five versions earlier and --always-copy seems to work. dh-virtualenv uses latest virtualenv as far as I can tell so this shouldn't be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding --system-site-packages

If you build with virtualenv --system-site-packages ENV, your virtual environment will inherit packages from /usr/lib/python2.7/site-packages (or wherever your global site-packages directory is).

It should copy global packages, but since virtualenv is not really relocatable, that just works in some weird way.

with --system-site-packages

root@3cad709e2da4:/opt/stackstorm/virtualenvs/github# bin/pip list
beautifulsoup4 (4.3.2)
bzr (2.6.0.dev2)
chardet (2.0.1)
configobj (4.7.2)
mercurial (2.2.2)
pip (7.1.2)
PyGithub (1.26.0)
python-apt (0.8.8.2)
python-debian (0.1.21)
requests (2.5.3)
setuptools (18.2)
six (1.10.0)

As we can see this is the list just of a few modules which could be relocated, but this is far from being complete, no anything of that what our /usr/share/python/st2 venv has.

without

root@3cad709e2da4:~# /opt/stackstorm/virtualenvs/github/bin/pip list
beautifulsoup4 (4.3.2)
pip (7.1.2)
PyGithub (1.26.0)
requests (2.5.3)
setuptools (18.2)
six (1.10.0)
wheel (0.24.0)

So what is unique only for origin venv is, and it's not required by github pack!

bzr (2.6.0.dev2)
chardet (2.0.1)
configobj (4.7.2)
mercurial (2.2.2)
python-apt (0.8.8.2)
python-debian (0.1.21)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, as noted on Slack, we added this a while back as a quick workaround to get things working.

I would much prefer to not use this option if possible.

For sensors and actions to work though, they also need access to st2common package and all the inherited st2common dependencies (mongoengine, kombu, etc.).

If we can get it to work without --system-site-packages, that would be great. I think it might even be possible now with new packages since st2actions and st2reactor already depend on st2common package.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to explaations of @lakshmi-kannan , PYTHONPATH is injected with an origin virtualenv, that's why --system-site-packages should be removed!?

Copy link
Member

Choose a reason for hiding this comment

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

To clarify - the best way to test it is to remove this option, create virtualenv and make sure actions and sensors still work.

help='List of virtualenv options to be passsed to "virtualenv" command that ' +
'creates pack virtualenv.')
]
CONF.register_opts(logging_opts, group='actionrunner')

Expand Down