From ee0742b02f2d178efb85ba3ea19694186feae7e6 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Tue, 9 Feb 2021 14:48:06 -0500 Subject: [PATCH 1/2] test_instance: move LXD-only test to LXD-specific test path And remove the parameterisation, as we only have a single parameter set. --- pycloudlib/lxd/tests/test_instance.py | 22 +++++++++++++++++++ pycloudlib/tests/test_instance.py | 31 --------------------------- 2 files changed, 22 insertions(+), 31 deletions(-) create mode 100644 pycloudlib/lxd/tests/test_instance.py diff --git a/pycloudlib/lxd/tests/test_instance.py b/pycloudlib/lxd/tests/test_instance.py new file mode 100644 index 00000000..f08081c2 --- /dev/null +++ b/pycloudlib/lxd/tests/test_instance.py @@ -0,0 +1,22 @@ +"""Tests for pycloudlib.lxd.instance.""" +from unittest import mock + +from pycloudlib.lxd.instance import LXDInstance + + +class TestExecute: + """Tests covering pycloudlib.lxd.instance.Instance.execute.""" + + def test_all_rcs_acceptable_when_using_exec(self): + """Test that we invoke util.subp with rcs=None for exec calls. + + rcs=None means that we will get a Result object back for all return + codes, rather than an exception for non-zero return codes. + """ + instance = LXDInstance(None) + with mock.patch("pycloudlib.lxd.instance.subp") as m_subp: + instance.execute("some_command") + assert 1 == m_subp.call_count + args, kwargs = m_subp.call_args + assert "exec" in args[0] + assert kwargs.get("rcs", mock.sentinel.not_none) is None diff --git a/pycloudlib/tests/test_instance.py b/pycloudlib/tests/test_instance.py index 8c66aac9..ab42c519 100644 --- a/pycloudlib/tests/test_instance.py +++ b/pycloudlib/tests/test_instance.py @@ -4,7 +4,6 @@ import pytest from pycloudlib.instance import BaseInstance -from pycloudlib.lxd.instance import LXDInstance from pycloudlib.result import Result # Disable this pylint check as fixture usage incorrectly triggers it: @@ -21,36 +20,6 @@ def concrete_instance_cls(): yield BaseInstance -class TestExecute: - """Tests covering pycloudlib.instance.Instance.execute. - - TODO: There are elements of `execute` which could be refactored onto the - relevant subclasses. Some of these tests should move along with that - refactor. - """ - - @pytest.mark.parametrize( - "instance_cls,cloud_name", - ( - (LXDInstance, "lxd"), - ), - ) - def test_all_rcs_acceptable(self, instance_cls, cloud_name): - """Test that we invoke util.subp with rcs=None. - - rcs=None means that we will get a Result object back for all return - codes, rather than an exception for non-zero return codes. - """ - instance = instance_cls(None) - with mock.patch( - "pycloudlib.{}.instance.subp".format(cloud_name) - ) as m_subp: - instance.execute("some_command") - assert 1 == m_subp.call_count - _args, kwargs = m_subp.call_args - assert kwargs.get("rcs", mock.sentinel.not_none) is None - - class TestWait: """Tests covering pycloudlib.instance.Instance.wait.""" From 64b032117e7703b1d783db270011734594f366f1 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Fri, 5 Feb 2021 10:04:02 -0500 Subject: [PATCH 2/2] lxd/instance: use SSH by default, allow switching to `exec` Analysis performed (and captured in #112) indicates that accessing LXD instances via SSH rather than via `lxc exec` leads to faster test execution, and also brings LXD's default behaviour in line with the default behaviour of other clouds. This commit introduces a `execute_via_ssh` parameter and instance variable, which are used by `execute()` to determine whether to connect to the instance via SSH, or use `lxc exec`. The default value of `execute_via_ssh` is `True`. (Prior to this commit, we would execute via SSH but only if we had a key manually set against the instance, leading to inconsistencies in behaviour depending on local configuration. This also addresses that inconsistency, and removes code that was required to support it.) Fixes: #112 --- .travis.yml | 4 +++ pycloudlib/lxd/cloud.py | 44 ++++++++------------------- pycloudlib/lxd/instance.py | 5 +-- pycloudlib/lxd/tests/test_instance.py | 2 +- 4 files changed, 21 insertions(+), 34 deletions(-) diff --git a/.travis.yml b/.travis.yml index ed6fb613..a5dbb8c8 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,6 +12,8 @@ matrix: include: - name: run-lxd-example install: + # Generate an SSH key to connect to test instances + - ssh-keygen -N "" -f ~/.ssh/id_rsa - pip install -r requirements.txt script: - sudo apt-get remove --yes --purge lxd lxd-client @@ -22,6 +24,8 @@ matrix: - sg lxd -c "python examples/lxd.py" - name: run-cloudinit-integration-tests install: + # Generate an SSH key to connect to test instances + - ssh-keygen -N "" -f ~/.ssh/id_rsa - pip install -r requirements.txt - pip install pytest script: diff --git a/pycloudlib/lxd/cloud.py b/pycloudlib/lxd/cloud.py index f78f4393..d7e3bb92 100644 --- a/pycloudlib/lxd/cloud.py +++ b/pycloudlib/lxd/cloud.py @@ -37,18 +37,6 @@ class _BaseLXD(BaseCloud): _daily_remote = 'ubuntu-daily' _releases_remote = 'ubuntu' - def __init__(self, tag, timestamp_suffix=True): - """Initialize LXD cloud class. - - Args: - tag: string used to name and tag resources with - timestamp_suffic: Append a timestamped suffix to the tag string. - """ - super().__init__(tag, timestamp_suffix) - - # User must manually specify the key pair to be used - self.key_pair = None - def clone(self, base, new_instance_name): """Create copy of an existing instance or snapshot. @@ -121,21 +109,7 @@ def get_instance(self, instance_id): The existing instance as a LXD instance object """ - instance = LXDInstance(instance_id) - - if self.key_pair: - local_path = "/tmp/{}-authorized-keys".format(instance_id) - - instance.pull_file( - remote_path="/home/ubuntu/.ssh/authorized_keys", - local_path=local_path - ) - - with open(local_path, "r") as f: - if self.key_pair.public_key_content in f.read(): - instance.key_pair = self.key_pair - - return instance + return LXDInstance(instance_id, key_pair=self.key_pair) def _normalize_image_id(self, image_id: str) -> str: if ':' not in image_id: @@ -228,7 +202,7 @@ def _prepare_command( def init( self, name, image_id, ephemeral=False, network=None, storage=None, inst_type=None, profile_list=None, user_data=None, - config_dict=None): + config_dict=None, execute_via_ssh=True): """Init a container. This will initialize a container, but not launch or start it. @@ -245,6 +219,8 @@ def init( profile_list: list, optional, profile(s) to use user_data: used by cloud-init to run custom scripts/configuration config_dict: dict, optional, configuration values to pass + execute_via_ssh: bool, optional, execute commands on the instance + via SSH if True (the default) Returns: The created LXD instance object @@ -272,11 +248,14 @@ def init( self._log.debug('Created %s', name) - return LXDInstance(name, self.key_pair) + return LXDInstance( + name, self.key_pair, execute_via_ssh=execute_via_ssh + ) def launch(self, image_id, instance_type=None, user_data=None, wait=True, name=None, ephemeral=False, network=None, storage=None, - profile_list=None, config_dict=None, **kwargs): + profile_list=None, config_dict=None, execute_via_ssh=True, + **kwargs): """Set up and launch a container. This will init and start a container with the provided settings. @@ -293,6 +272,8 @@ def launch(self, image_id, instance_type=None, user_data=None, wait=True, storage: string, storage name to use profile_list: list, profile(s) to use config_dict: dict, configuration values to pass + execute_via_ssh: bool, optional, execute commands on the instance + via SSH if True (the default) Returns: The created LXD instance object @@ -307,7 +288,8 @@ def launch(self, image_id, instance_type=None, user_data=None, wait=True, inst_type=instance_type, profile_list=profile_list, user_data=user_data, - config_dict=config_dict + config_dict=config_dict, + execute_via_ssh=execute_via_ssh, ) instance.start(wait) diff --git a/pycloudlib/lxd/instance.py b/pycloudlib/lxd/instance.py index d2d9a871..0f223087 100644 --- a/pycloudlib/lxd/instance.py +++ b/pycloudlib/lxd/instance.py @@ -13,7 +13,7 @@ class LXDInstance(BaseInstance): _type = 'lxd' _is_vm = None - def __init__(self, name, key_pair=None): + def __init__(self, name, key_pair=None, execute_via_ssh=True): """Set up instance. Args: @@ -23,6 +23,7 @@ def __init__(self, name, key_pair=None): super().__init__(key_pair=key_pair) self._name = name + self.execute_via_ssh = execute_via_ssh def __repr__(self): """Create string representation for class.""" @@ -30,7 +31,7 @@ def __repr__(self): def _run_command(self, command, stdin): """Run command in the instance.""" - if self.key_pair: + if self.execute_via_ssh: return super()._run_command(command, stdin) base_cmd = [ diff --git a/pycloudlib/lxd/tests/test_instance.py b/pycloudlib/lxd/tests/test_instance.py index f08081c2..da59d0cf 100644 --- a/pycloudlib/lxd/tests/test_instance.py +++ b/pycloudlib/lxd/tests/test_instance.py @@ -13,7 +13,7 @@ def test_all_rcs_acceptable_when_using_exec(self): rcs=None means that we will get a Result object back for all return codes, rather than an exception for non-zero return codes. """ - instance = LXDInstance(None) + instance = LXDInstance(None, execute_via_ssh=False) with mock.patch("pycloudlib.lxd.instance.subp") as m_subp: instance.execute("some_command") assert 1 == m_subp.call_count