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

Jump to bottom

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 all commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ in development
* Fix runaway action triggers caused by state miscalculation for mistral workflow. (bug fix)
* Throw a more friendly error message if casting parameter value fails because the value contains
an invalid type or similar. (improvement)
* Use ``--always-copy`` option when creating virtualenv for packs from packs.setup_virtualenv
action. This is required when st2actionrunner is kicked off from python within a virtualenv.

1.2.0 - December 07, 2015
-------------------------
Expand Down
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
6 changes: 3 additions & 3 deletions conf/st2.conf.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ logging = conf/logging.conf

[api]
# List of origins allowed
allow_origin = ['http://localhost:3000']
allow_origin = http://localhost:3000 # comma separated list allowed here.
# location of the logging.conf file
logging = conf/logging.conf
# True to mask secrets in API responses
Expand Down Expand Up @@ -114,15 +114,15 @@ collection_interval = 600
# Controls if stderr should be redirected to the logs.
redirect_stderr = False
# Exclusion list of loggers to omit.
excludes =
excludes =
# True to mask secrets in the log files.
mask_secrets = True

[messaging]
# URL of the messaging server.
url = amqp://guest:guest@127.0.0.1:5672//
# URL of all the nodes in a messaging service cluster.
cluster_urls = []
cluster_urls = # comma separated list or URLs allowed here.

[mistral]
# URL Mistral uses to talk back to the API.If not provided it defaults to public API URL. Note: This needs to be a base URL without API version (e.g. http://127.0.0.1:9101)
Expand Down
1 change: 1 addition & 0 deletions conf/st2.package.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ logging = /etc/st2/logging.rulesengine.conf

[actionrunner]
logging = /etc/st2/logging.actionrunner.conf
virtualenv_opts = --always-copy

[resultstracker]
logging = /etc/st2/logging.resultstracker.conf
Expand Down
17 changes: 15 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,28 @@ 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))

if not os.path.isfile(virtualenv_binary):
raise Exception('Virtualenv binary "%s" doesn\'t exist.' % (virtualenv_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 as e:
raise Exception('Error executing command %s. %s.' % (' '.join(cmd),
e.message))

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