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

Allow a pack to specify Python 3 and have corresponding virtualenvs use Python 3 #3922

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ $(VIRTUALENV_DIR)/bin/activate:
@echo
test -f $(VIRTUALENV_DIR)/bin/activate || virtualenv --python=$(PYTHON_VERSION) --no-site-packages $(VIRTUALENV_DIR)


# Setup PYTHONPATH in bash activate script...
echo '' >> $(VIRTUALENV_DIR)/bin/activate
echo '_OLD_PYTHONPATH=$$PYTHONPATH' >> $(VIRTUALENV_DIR)/bin/activate
Expand Down
7 changes: 5 additions & 2 deletions conf/st2.conf.sample
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ python_runner_log_level = DEBUG
workflows_pool_size = 40
# Virtualenv binary which should be used to create pack virtualenvs.
virtualenv_binary = /data/stanley/virtualenv/bin/virtualenv
# Python 3 binary which will be used by Python actions.
python3_binary = /data/stanley/virtualenv/bin/python3
# True to store and stream action output (stdout and stderr) in real-time.
stream_output = False
# location of the logging.conf file
logging = conf/logging.conf
# True to store and stream action output (stdout and stderr) in real-time.
Expand Down Expand Up @@ -154,7 +158,7 @@ collection_interval = 600

[keyvalue]
# Location of the symmetric encryption key for encrypting values in kvstore. This key should be in JSON and should've been generated using keyczar.
encryption_key_path =
encryption_key_path =
# Allow encryption of values in key value stored qualified as "secret".
enable_encryption = True

Expand Down Expand Up @@ -309,4 +313,3 @@ local_timezone = America/Los_Angeles
[webui]
# Base https URL to access st2 Web UI. This is used to constructhistory URLs that are sent out when chatops is used to kick off executions.
webui_base_url = https://localhost

4 changes: 2 additions & 2 deletions conf/st2.dev.conf
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ mask_secrets = True

[system_user]
user = stanley
ssh_key_file = /home/vagrant/.ssh/stanley_rsa
ssh_key_file = /home/stanley/.ssh/stanley_rsa

[messaging]
url = amqp://guest:[email protected]:5672/
Expand All @@ -110,4 +110,4 @@ v2_base_url = http://127.0.0.1:8989/v2
jitter_interval = 0

[packs]
enable_common_libs = True
enable_common_libs = True
2 changes: 2 additions & 0 deletions st2common/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ rpm:
mkdir -p rbac/roles && cp -f ../st2tests/st2tests/fixtures/rbac/roles/role_sample.yaml rbac/roles/sample.yaml
mkdir -p rbac/assignments && cp -f ../st2tests/st2tests/fixtures/rbac/assignments/user_sample.yaml rbac/assignments/sample.yaml
sed -i '/\[actionrunner\]/a python_binary = /usr/bin/python2.7' st2/st2.conf
sed -i '/\[actionrunner\]/a python3_binary = /usr/bin/python3' st2/st2.conf
tar --transform=s~^~$(COMPONENTS)-$(VER)/~ --exclude=correlation -czf $(RPM_SOURCES_DIR)/$(COMPONENTS).tar.gz bin st2 logrotate.d rbac $(COMPONENTS) ../contrib ../tools/ ../requirements.txt
cp packaging/rpm/$(COMPONENTS).spec $(RPM_SPECS_DIR)/
cd $(RPM_SPECS_DIR) && rpmbuild --clean --rmsource -ba $(COMPONENTS).spec
Expand All @@ -27,6 +28,7 @@ rhel-rpm:
mkdir -p rbac/roles && cp -f ../st2tests/st2tests/fixtures/rbac/roles/role_sample.yaml rbac/roles/sample.yaml
mkdir -p rbac/assignments && cp -f ../st2tests/st2tests/fixtures/rbac/assignments/user_sample.yaml rbac/assignments/sample.yaml
sed -i '/\[actionrunner\]/a python_binary = /usr/bin/python2.7' st2/st2.conf
sed -i '/\[actionrunner\]/a python3_binary = /usr/bin/python3' st2/st2.conf
tar --transform=s~^~$(COMPONENTS)-$(VER)/~ --exclude=correlation -czf $(RPM_SOURCES_DIR)/$(COMPONENTS).tar.gz bin st2 logrotate.d rbac $(COMPONENTS) ../contrib ../tools/ ../requirements.txt
cp packaging/rpm/$(COMPONENTS)-rhel6.spec $(RPM_SPECS_DIR)/
cd $(RPM_SPECS_DIR) && rpmbuild --clean --rmsource -ba $(COMPONENTS)-rhel6.spec
Expand Down
3 changes: 3 additions & 0 deletions st2common/st2common/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ def register_opts(ignore_errors=False):

# Runner options
default_python_bin_path = sys.executable
default_python3_bin_path = '/usr/bin/python3'
base_dir = os.path.dirname(os.path.realpath(default_python_bin_path))
default_virtualenv_bin_path = os.path.join(base_dir, 'virtualenv')
action_runner_opts = [
Expand All @@ -231,6 +232,8 @@ def register_opts(ignore_errors=False):
# Python runner options
cfg.StrOpt('python_binary', default=default_python_bin_path,
help='Python binary which will be used by Python actions.'),
cfg.StrOpt('python3_binary', default=default_python3_bin_path,
help='Python 3 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.StrOpt('python_runner_log_level',
Expand Down
29 changes: 25 additions & 4 deletions st2common/st2common/util/virtualenvs.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from st2common.constants.pack import BASE_PACK_REQUIREMENTS
from st2common.util.shell import run_command
from st2common.util.shell import quote_unix
from st2common.util.pack import get_pack_metadata
from st2common.util.compat import to_ascii
from st2common.content.utils import get_packs_base_paths
from st2common.content.utils import get_pack_directory
Expand Down Expand Up @@ -56,7 +57,7 @@ def setup_pack_virtualenv(pack_name, update=False, logger=None, include_pip=True
level logger.
"""
logger = logger or LOG

three = False # TODO: Change default Python version to a global setting
if not re.match(PACK_REF_WHITELIST_REGEX, pack_name):
raise ValueError('Invalid pack name "%s"' % (pack_name))

Expand All @@ -74,6 +75,19 @@ def setup_pack_virtualenv(pack_name, update=False, logger=None, include_pip=True
msg = 'Pack "%s" is not installed. Looked in: %s' % (pack_name, search_paths)
raise Exception(msg)

try:
pack_meta = get_pack_metadata(pack_path)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, for dependency injection and testing reasons, this function should take use_python3 as an argument instead of reading the metadata file itself.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud - I first thought this might slow down things quite a bit when running st2ctl register --register-setup-virtualenvs flag because we now also need to read and parse metadata file for each pack.

But after some more thought - we already need to do that, so another read and parse shouldn't add too much overhead since file should already be in filesystem cache. Future optimization would perhaps be to refactor the code a bit so we don't need to read and parse metadata file multiple times, but probably not needed right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that the pack would still install if the meta-file wasn't in place, so I didn't want to alter the original behaviour

Copy link
Member

Choose a reason for hiding this comment

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

Metadata is mandatory so it would fail at some other step, but that's not really important here and I'm fine with the current approach.

I was mostly just "thinking out loud" about the potential performance overhead from reading and parsing pack metadata multiple times during install phase :)

has_pack_meta = True
except ValueError:
# Pack is missing meta file
has_pack_meta = False

if has_pack_meta:
logger.debug('Checking pack specific Python version.')
if 'system' in pack_meta.keys() and 'python3' in pack_meta['system'].keys():
three = bool(pack_meta['system']['python3'])
logger.debug('Using Python %s in virtualenv' % (3 if three else 2))

# 1. Create virtualenv if it doesn't exist
if not update or not os.path.exists(virtualenv_path):
# 0. Delete virtual environment if it exists
Expand All @@ -82,7 +96,8 @@ def setup_pack_virtualenv(pack_name, update=False, logger=None, include_pip=True
# 1. Create virtual environment
logger.debug('Creating virtualenv for pack "%s" in "%s"' % (pack_name, virtualenv_path))
create_virtualenv(virtualenv_path=virtualenv_path, logger=logger, include_pip=include_pip,
include_setuptools=include_setuptools, include_wheel=include_wheel)
include_setuptools=include_setuptools, include_wheel=include_wheel,
three=three)

# 2. Install base requirements which are common to all the packs
logger.debug('Installing base requirements')
Expand Down Expand Up @@ -110,7 +125,7 @@ def setup_pack_virtualenv(pack_name, update=False, logger=None, include_pip=True


def create_virtualenv(virtualenv_path, logger=None, include_pip=True, include_setuptools=True,
include_wheel=True):
include_wheel=True, three=False):
"""
:param include_pip: Include pip binary and package in the newely created virtual environment.
:type include_pip: ``bool``
Expand All @@ -121,11 +136,17 @@ def create_virtualenv(virtualenv_path, logger=None, include_pip=True, include_se

:param include_wheel: Include wheel in the newely created virtual environment.
:type include_wheel : ``bool``

:param three: Use Python 3 binary
:type three: ``bool``
"""

logger = logger or LOG

python_binary = cfg.CONF.actionrunner.python_binary
if three:
python_binary = cfg.CONF.actionrunner.python3_binary
else:
python_binary = cfg.CONF.actionrunner.python_binary
virtualenv_binary = cfg.CONF.actionrunner.virtualenv_binary
virtualenv_opts = cfg.CONF.actionrunner.virtualenv_opts

Expand Down
37 changes: 37 additions & 0 deletions st2common/tests/unit/test_virtualenvs.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
'VirtualenvUtilsTestCase'
]

PACK_WITH_PY3 = {'system': {'python3': True}}


# Note: We set base requirements to an empty list to speed up the tests
@mock.patch('st2common.util.virtualenvs.BASE_PACK_REQUIREMENTS', [])
Expand Down Expand Up @@ -65,6 +67,41 @@ def test_setup_pack_virtualenv_doesnt_exist_yet(self):
# Verify that virtualenv has been created
self.assertVirtulenvExists(pack_virtualenv_dir)

@mock.patch.object(virtualenvs, 'run_command', mock.MagicMock(return_value=(0, '', '')))
@mock.patch.object(virtualenvs, 'get_pack_metadata', mock.MagicMock(return_value=PACK_WITH_PY3))
def test_setup_pack_virtualenv_python3_doesnt_exist_yet(self):
# Test a fresh virtualenv creation
pack_name = 'dummy_pack_1'
pack_virtualenv_dir = os.path.join(self.virtualenvs_path, pack_name)

# Verify virtualenv directory doesn't exist
self.assertFalse(os.path.exists(pack_virtualenv_dir))

# Create virtualenv
setup_pack_virtualenv(pack_name=pack_name, update=False,
include_setuptools=False, include_wheel=False)

virtualenvs.run_command.assert_called_once()
_, kwargs = virtualenvs.run_command.call_args
self.assertTrue('/usr/bin/python3' in kwargs['cmd'])

@mock.patch.object(virtualenvs, 'run_command', mock.MagicMock(return_value=(0, '', '')))
@mock.patch.object(virtualenvs, 'get_pack_metadata', mock.MagicMock(return_value={}))
def test_setup_pack_virtualenv_python2_doesnt_exist_yet(self):
# Test a fresh virtualenv creation
pack_name = 'dummy_pack_1'
pack_virtualenv_dir = os.path.join(self.virtualenvs_path, pack_name)

# Verify virtualenv directory doesn't exist
self.assertFalse(os.path.exists(pack_virtualenv_dir))

# Create virtualenv
setup_pack_virtualenv(pack_name=pack_name, update=False,
include_setuptools=False, include_wheel=False)

_, kwargs = virtualenvs.run_command.call_args
self.assertTrue('/usr/bin/python3' not in kwargs['cmd'])

def test_setup_pack_virtualenv_already_exists(self):
# Test a scenario where virtualenv already exists
pack_name = 'dummy_pack_1'
Expand Down
3 changes: 2 additions & 1 deletion tools/config_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@
STATIC_OPTION_VALUES = {
'actionrunner': {
'virtualenv_binary': '/data/stanley/virtualenv/bin/virtualenv',
'python_binary': '/data/stanley/virtualenv/bin/python'
'python_binary': '/data/stanley/virtualenv/bin/python',
'python3_binary': '/data/stanley/virtualenv/bin/python3'
},
'webui': {
'webui_base_url': 'https://localhost'
Expand Down