From 87ee40bf1c3728ffa42d7d49d04990d95e6c8c7d Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 14 Jan 2021 14:09:45 -0600 Subject: [PATCH] Remove raise_on_failure from wait calls The 'raise_on_failure' aspect of waiting for cloud-init to start is causing a fair amount of additional code and workarounds. It was introduced as an optional parameter as to not break the existing API, but given the amount of heartburn it's causing, it seems best to remove it entirely. If one needs the original behavior, checking cloud-init status via an execute call should suffice. --- pycloudlib/instance.py | 27 ++++-------------- pycloudlib/lxd/instance.py | 39 ------------------------- pycloudlib/lxd/tests/test_instance.py | 41 --------------------------- pycloudlib/tests/test_instance.py | 32 ++++----------------- 4 files changed, 10 insertions(+), 129 deletions(-) delete mode 100644 pycloudlib/lxd/tests/test_instance.py diff --git a/pycloudlib/instance.py b/pycloudlib/instance.py index b13ceaa8..ed8f4b2b 100644 --- a/pycloudlib/instance.py +++ b/pycloudlib/instance.py @@ -91,16 +91,11 @@ def _wait_for_instance_start(self): detecting when an instance has started through their API. """ - def wait(self, *, raise_on_cloudinit_failure: bool = True): - """Wait for instance to be up and cloud-init to be complete. - - :param raise_on_cloudinit_failure: - If True (the default), raise an `OSError` if cloud-init does not - complete successfully. - """ + def wait(self): + """Wait for instance to be up and cloud-init to be complete.""" self._wait_for_instance_start() self._wait_for_execute() - self._wait_for_cloudinit(raise_on_failure=raise_on_cloudinit_failure) + self._wait_for_cloudinit() @abstractmethod def wait_for_delete(self): @@ -396,13 +391,8 @@ def _wait_for_execute(self): ) ) - def _wait_for_cloudinit(self, *, raise_on_failure: bool): - """Wait until cloud-init has finished. - - :param raise_on_failure: - When `True`, if the process waiting for cloud-init exits non-zero - then this method will raise an `OSError`. - """ + def _wait_for_cloudinit(self): + """Wait until cloud-init has finished.""" self._log.debug('_wait_for_cloudinit to complete') result = self.execute("cloud-init status --help") @@ -424,10 +414,3 @@ def _wait_for_cloudinit(self, *, raise_on_failure: bool): ) ) result = self.execute(cmd, description='waiting for start') - - if raise_on_failure and result.failed: - raise OSError( - 'cloud-init failed to start: out: %s error: %s' % ( - result.stdout, result.stderr - ) - ) diff --git a/pycloudlib/lxd/instance.py b/pycloudlib/lxd/instance.py index 69ab45e4..d2d9a871 100644 --- a/pycloudlib/lxd/instance.py +++ b/pycloudlib/lxd/instance.py @@ -332,42 +332,3 @@ def wait_for_stop(self): return time.sleep(1) raise TimeoutError - - def _wait_for_cloudinit(self, *, raise_on_failure: bool): - """Wait until cloud-init has finished. - - If the instance is a virtual machine, we need to wait for the - lxd-agent to be ready before getting the cloud-init status. - To do that, we retry on vm instances if raise_on_failure - is specified. - - :param raise_on_failure: - When `True`, if the process waiting for cloud-init exits non-zero - then this method will raise an `OSError`. - """ - if self.is_vm: - retries = [30, 45, 60, 75, 90, 105] - - for sleep_time in retries: - try: - super()._wait_for_cloudinit( - raise_on_failure=True) - break - except OSError as e: - # We are getting those types of messages when booting - # lxd vms. However, if we wait a moment before trying - # again, the error fades away. That's why we are treating - # it here. - lxd_errors = [ - "Failed to connect to lxd-agent", - "cloud-init failed to start: out: .......", - "Error: Instance is not running", - ] - if any([x in str(e) for x in lxd_errors]): - time.sleep(sleep_time) - continue - if raise_on_failure: - raise e - else: - super()._wait_for_cloudinit( - raise_on_failure=raise_on_failure) diff --git a/pycloudlib/lxd/tests/test_instance.py b/pycloudlib/lxd/tests/test_instance.py deleted file mode 100644 index 8c6802c9..00000000 --- a/pycloudlib/lxd/tests/test_instance.py +++ /dev/null @@ -1,41 +0,0 @@ -"""Tests related to pycloudlib.lxd.instance module.""" -from unittest import mock -import pytest - -from pycloudlib.instance import BaseInstance -from pycloudlib.lxd.instance import LXDInstance - - -class TestWaitForCloudinit: - """Tests covering pycloudlib.lxd.instance._wait_for_cloudinit method.""" - - @mock.patch("pycloudlib.lxd.instance.time.sleep") - @mock.patch.object(BaseInstance, "_wait_for_cloudinit") - @mock.patch.object(LXDInstance, "is_vm", new_callable=mock.PropertyMock) - def test_wait_for_vm_with_raise_parameter( - self, m_is_vm, m_wait_for_cloudinit, m_sleep - ): # pylint: disable=W0212 - """Test covering _wait_for_cloudinit on LXD vms.""" - m_is_vm.return_value = True - instance = LXDInstance(name=None) - - m_wait_for_cloudinit.side_effect = [ - OSError("Failed to connect to lxd-agent"), - OSError("Failed to connect to lxd-agent"), - OSError( - "cloud-init failed to start: out: ................... error"), - OSError("cloud-init error") - ] - - with pytest.raises(OSError) as excinfo: - instance._wait_for_cloudinit(raise_on_failure=True) - - assert m_wait_for_cloudinit.call_args_list == [ - mock.call(raise_on_failure=True), - mock.call(raise_on_failure=True), - mock.call(raise_on_failure=True), - mock.call(raise_on_failure=True) - ] - assert m_wait_for_cloudinit.call_count == 4 - assert m_sleep.call_count == 3 - assert "cloud-init error" == str(excinfo.value) diff --git a/pycloudlib/tests/test_instance.py b/pycloudlib/tests/test_instance.py index 3eab0901..8c66aac9 100644 --- a/pycloudlib/tests/test_instance.py +++ b/pycloudlib/tests/test_instance.py @@ -1,5 +1,4 @@ """Tests related to pycloudlib.instance module.""" -from contextlib import suppress as noop from unittest import mock import pytest @@ -55,8 +54,7 @@ def test_all_rcs_acceptable(self, instance_cls, cloud_name): class TestWait: """Tests covering pycloudlib.instance.Instance.wait.""" - @pytest.mark.parametrize("raise_on_cloudinit_failure", [True, False, None]) - def test_wait(self, raise_on_cloudinit_failure, concrete_instance_cls): + def test_wait(self, concrete_instance_cls): """Test wait calls the two methods it should with correct passthrough. (`None` is used to test the default.) @@ -68,21 +66,11 @@ def test_wait(self, raise_on_cloudinit_failure, concrete_instance_cls): _wait_for_execute=mock.DEFAULT, _wait_for_cloudinit=mock.DEFAULT, ) as mocks: - if raise_on_cloudinit_failure is not None: - instance.wait( - raise_on_cloudinit_failure=raise_on_cloudinit_failure - ) - else: - instance.wait() + instance.wait() assert 1 == mocks["_wait_for_instance_start"].call_count assert 1 == mocks["_wait_for_execute"].call_count assert 1 == mocks["_wait_for_cloudinit"].call_count - kwargs = mocks["_wait_for_cloudinit"].call_args[1] - if raise_on_cloudinit_failure is None: - # We expect True by default - raise_on_cloudinit_failure = True - assert kwargs["raise_on_failure"] == raise_on_cloudinit_failure @mock.patch.object(BaseInstance, "execute") @mock.patch("pycloudlib.instance.time.sleep") @@ -123,7 +111,7 @@ def side_effect(cmd, *_args, **_kwargs): instance = concrete_instance_cls(key_pair=None) with mock.patch.object(instance, "execute") as m_execute: m_execute.side_effect = side_effect - instance._wait_for_cloudinit(raise_on_failure=True) + instance._wait_for_cloudinit() assert 2 == m_execute.call_count assert ( @@ -153,7 +141,7 @@ def side_effect(cmd, *_args, **_kwargs): instance = concrete_instance_cls(key_pair=None) with mock.patch.object(instance, "execute") as m_execute: m_execute.side_effect = side_effect - instance._wait_for_cloudinit(raise_on_failure=True) + instance._wait_for_cloudinit() assert 2 == m_execute.call_count assert ( @@ -166,16 +154,10 @@ def side_effect(cmd, *_args, **_kwargs): assert "runlevel" in first_arg assert "result.json" in first_arg - @pytest.mark.parametrize( - "raise_on_failure,expectation", - [(True, pytest.raises(OSError)), (False, noop())], - ) @pytest.mark.parametrize("has_wait", [True, False]) def test_failure_path( self, has_wait, - raise_on_failure, - expectation, concrete_instance_cls, ): """Test failure for both has_wait and !has_wait cases.""" @@ -204,8 +186,4 @@ def side_effect(cmd, *_args, **_kwargs): instance = concrete_instance_cls(key_pair=None) with mock.patch.object(instance, "execute") as m_execute: m_execute.side_effect = side_effect - with expectation as excinfo: - instance._wait_for_cloudinit(raise_on_failure=raise_on_failure) - - if raise_on_failure: - assert "out: fail_out error: fail_err" in str(excinfo.value) + instance._wait_for_cloudinit()