From 0c89117fe2a532b713d65346b8eb7a3b380f1734 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Fri, 7 Jan 2022 10:27:43 +0000 Subject: [PATCH 1/7] Switch unit.public_address to unit.get_public_address() Due to the bug [1] on OpenStack providers, unit.public_address doesn't actually work reliably. The fix [2] is only for the async function unit.get_public_address(). Sadly, zaza relied on unit.public_address and so it needs this patch for juju 2.9 support on OpenStack providers. [1]: https://github.com/juju/python-libjuju/issues/551 [2]: https://github.com/juju/python-libjuju/pull/600 --- unit_tests/test_zaza_model.py | 16 +++++++-- .../utilities/test_zaza_utilities_juju.py | 19 +++++++++-- zaza/model.py | 33 ++++++++++++++++--- zaza/utilities/juju.py | 2 +- 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/unit_tests/test_zaza_model.py b/unit_tests/test_zaza_model.py index 0e3ce0dd6..3dd330d07 100644 --- a/unit_tests/test_zaza_model.py +++ b/unit_tests/test_zaza_model.py @@ -159,12 +159,24 @@ async def _inner_is_leader(): self.machine3 = mock.MagicMock(status='active') self.machine7 = mock.MagicMock(status='active') self.unit1 = mock.MagicMock() - self.unit1.public_address = 'ip1' + + def make_get_public_address(ip): + async def _get_public_address(): + return ip + + return _get_public_address + + def fail_on_use(): + raise RuntimeError("Don't use this property.") + + self.unit1.public_address = property(fail_on_use) + self.unit1.get_public_address = make_get_public_address('ip1') self.unit1.name = 'app/2' self.unit1.entity_id = 'app/2' self.unit1.machine = self.machine3 self.unit2 = mock.MagicMock() - self.unit2.public_address = 'ip2' + self.unit2.public_address = property(fail_on_use) + self.unit2.get_public_address = make_get_public_address('ip2') self.unit2.name = 'app/4' self.unit2.entity_id = 'app/4' self.unit2.machine = self.machine7 diff --git a/unit_tests/utilities/test_zaza_utilities_juju.py b/unit_tests/utilities/test_zaza_utilities_juju.py index f99f72fea..7f89f9d4c 100644 --- a/unit_tests/utilities/test_zaza_utilities_juju.py +++ b/unit_tests/utilities/test_zaza_utilities_juju.py @@ -43,19 +43,33 @@ def __init__(self, display_name=''): self.machine2_mock = MachineMock() self.machine2_mock[self.key] = self.key_data + + def make_get_public_address(ip): + async def _get_public_address(): + return ip + + return _get_public_address + + def fail_on_use(): + raise RuntimeError("Don't use this property.") + self.unit0 = "app/0" self.unit0_data = {"machine": self.machine0} self.unit0_mock = mock.MagicMock() self.unit0_mock.entity_id = self.unit0 self.unit0_mock.data = {'machine-id': self.machine0} - self.unit0_mock.public_address = '10.0.0.11' + self.unit0_mock.public_address = property(fail_on_use) + self.unit0_mock.get_public_address = make_get_public_address( + '10.0.0.11') self.unit1 = "app/1" self.unit1_data = {"machine": self.machine1} self.unit1_mock = mock.MagicMock() self.unit1_mock.entity_id = self.unit1 self.unit1_mock.data = {'machine-id': self.machine1} - self.unit1_mock.public_address = '10.0.0.1' + self.unit1_mock.public_address = property(fail_on_use) + self.unit1_mock.get_public_address = make_get_public_address( + '10.0.0.1') self.unit2 = "app/2" self.unit2_data = {"machine": self.machine2} @@ -406,6 +420,7 @@ def test_get_application_ip(self): juju_utils.get_application_ip('app'), '10.0.0.10') self.model.get_application_config.return_value = {} + self.model.get_unit_public_address.return_value = '10.0.0.1' self.assertEqual( juju_utils.get_application_ip('app'), '10.0.0.1') diff --git a/zaza/model.py b/zaza/model.py index bc481b375..507b57228 100644 --- a/zaza/model.py +++ b/zaza/model.py @@ -664,7 +664,29 @@ async def async_get_lead_unit_name(application_name, model_name=None): get_lead_unit_name = sync_wrapper(async_get_lead_unit_name) -def get_app_ips(application_name, model_name=None): +def get_unit_public_address(unit): + """Get the public address of a the unit. + + The libjuju library, in theory, supports a unit.public_address attribute + that provides the publick address of the unit. However, when the unit is + an OpenStack VM, there is a race and it's possible it will be None. + Therefore, there is a 'get_public_address()' funtion on unit that does + provide the function. See [1]. + + 1. https://github.com/juju/python-libjuju/issues/551 + + :param unit: The libjuju unit object to get the public address for. + :type unit: juju.Unit + :returns: the IP address of the unit. + :rtype: str + """ + async def _get(unit_): + return await unit_.get_public_address() + + return sync_wrapper(_get)(unit) + + +async def async_get_app_ips(application_name, model_name=None): """Return public address of all units of an application. :param model_name: Name of model to query. @@ -674,10 +696,13 @@ def get_app_ips(application_name, model_name=None): :returns: List of ip addresses :rtype: [str, str,...] """ - return [u.public_address + return [await u.get_public_address() for u in get_units(application_name, model_name=model_name)] +get_app_ips = sync_wrapper(async_get_app_ips) + + async def async_get_lead_unit_ip(application_name, model_name=None): """Return the IP address of the lead unit of a given application. @@ -689,8 +714,8 @@ async def async_get_lead_unit_ip(application_name, model_name=None): :rtype: str :raises: zaza.utilities.exceptions.JujuError """ - return (await async_get_lead_unit( - application_name, model_name)).public_address + return await (await async_get_lead_unit( + application_name, model_name)).get_public_address() get_lead_unit_ip = sync_wrapper(async_get_lead_unit_ip) diff --git a/zaza/utilities/juju.py b/zaza/utilities/juju.py index 31e280b7a..41642bedb 100644 --- a/zaza/utilities/juju.py +++ b/zaza/utilities/juju.py @@ -511,5 +511,5 @@ def get_application_ip(application, model_name=None): unit = model.get_units( application, model_name=model_name)[0] - ip = unit.public_address + ip = model.get_unit_public_address(unit) return ip From c0a699c3e497a8c24d221cf508137925e8a82f03 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Fri, 7 Jan 2022 12:33:16 +0000 Subject: [PATCH 2/7] Fix pep8 issue --- unit_tests/utilities/test_zaza_utilities_juju.py | 1 - 1 file changed, 1 deletion(-) diff --git a/unit_tests/utilities/test_zaza_utilities_juju.py b/unit_tests/utilities/test_zaza_utilities_juju.py index 7f89f9d4c..f15b3e4f5 100644 --- a/unit_tests/utilities/test_zaza_utilities_juju.py +++ b/unit_tests/utilities/test_zaza_utilities_juju.py @@ -43,7 +43,6 @@ def __init__(self, display_name=''): self.machine2_mock = MachineMock() self.machine2_mock[self.key] = self.key_data - def make_get_public_address(ip): async def _get_public_address(): return ip From 1e926f5fea09ebc82fb1d6207f7a4534cadbb51b Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 10 Jan 2022 15:21:29 +0000 Subject: [PATCH 3/7] Increase test coverage by providing test --- unit_tests/test_zaza_model.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/unit_tests/test_zaza_model.py b/unit_tests/test_zaza_model.py index 3dd330d07..266efd76c 100644 --- a/unit_tests/test_zaza_model.py +++ b/unit_tests/test_zaza_model.py @@ -535,6 +535,9 @@ def test_get_lead_unit_name(self): model.get_lead_unit_name('app', 'model'), 'app/4') + def test_get_unit_public_address(self): + self.assertEqual(model.get_unit_public_address(self.unit1), 'ip1') + def test_get_lead_unit_ip(self): self.patch_object(model, 'get_juju_model', return_value='mname') self.patch_object(model, 'get_units') From c510eb48288288422b1d71ab574262aaaff17300 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Tue, 11 Jan 2022 12:27:44 +0000 Subject: [PATCH 4/7] Resolve 'double' async loop issue with get_app_ips() Bug [1] indicated a fault in the logic for get_app_ips() where the code went sync -> async -> sync -> async and thus ended up trying to reuse the same event loop. --- tests-extended/tests.yaml | 1 - tests/bundles/first.yaml | 2 +- tests/bundles/second.yaml | 2 +- tests/tests.yaml | 1 + zaza/charm_tests/libjuju/__init__.py | 16 +++++++++++ zaza/charm_tests/libjuju/tests.py | 42 ++++++++++++++++++++++++++++ zaza/model.py | 3 +- 7 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 zaza/charm_tests/libjuju/__init__.py create mode 100644 zaza/charm_tests/libjuju/tests.py diff --git a/tests-extended/tests.yaml b/tests-extended/tests.yaml index 652bb708b..11491337f 100644 --- a/tests-extended/tests.yaml +++ b/tests-extended/tests.yaml @@ -28,7 +28,6 @@ tests_options: log-to-stdout: false log-to-python-logging: true python-logging-level: info - logger-name: DEFAULT raise-exceptions: true upload: - type: InfluxDB diff --git a/tests/bundles/first.yaml b/tests/bundles/first.yaml index 4673fcb63..9ebba1609 100644 --- a/tests/bundles/first.yaml +++ b/tests/bundles/first.yaml @@ -9,4 +9,4 @@ applications: num_units: 2 ubuntu: charm: cs:ubuntu - num_units: 1 + num_units: 3 diff --git a/tests/bundles/second.yaml b/tests/bundles/second.yaml index d3581d5dd..fec4cac25 100644 --- a/tests/bundles/second.yaml +++ b/tests/bundles/second.yaml @@ -9,4 +9,4 @@ applications: num_units: 2 ubuntu: charm: cs:ubuntu - num_units: 1 + num_units: 3 diff --git a/tests/tests.yaml b/tests/tests.yaml index 8aaf9c904..2856bbc88 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -24,6 +24,7 @@ configure: - zaza.charm_tests.noop.setup.basic_setup tests: - zaza.charm_tests.noop.tests.NoopTest +- zaza.charm_tests.libjuju.tests.RegressionTest tests_options: force_deploy: first diff --git a/zaza/charm_tests/libjuju/__init__.py b/zaza/charm_tests/libjuju/__init__.py new file mode 100644 index 000000000..480703596 --- /dev/null +++ b/zaza/charm_tests/libjuju/__init__.py @@ -0,0 +1,16 @@ +# Copyright 2022 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Collection of regression tests checking zaza/libjuju integration.""" + diff --git a/zaza/charm_tests/libjuju/tests.py b/zaza/charm_tests/libjuju/tests.py new file mode 100644 index 000000000..3e1e84b04 --- /dev/null +++ b/zaza/charm_tests/libjuju/tests.py @@ -0,0 +1,42 @@ +#!/usr/bin/env python3 + +# Copyright 2018 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Test regression libjuju / zaza integration.""" + +import logging +import unittest + +import zaza.model + + +class RegressionTest(unittest.TestCase): + """Regression Tests.""" + + def test_get_unit_public_address(self): + """Verify get_unit_public_address().""" + logging.info('Verify that get_unit_public_address() function works.') + units = zaza.model.get_units('ubuntu') + ips = [zaza.model.get_unit_public_address(unit) for unit in units] + for ip in ips: + self.assertIsNotNone(ip) + + def test_get_app_ips(self): + """Verify that get_app_ips() doesn't invoke to async loops.""" + logging.info('Verify that get_app_ips() works.') + ips = zaza.model.get_app_ips('ubuntu') + for ip in ips: + self.assertIsNotNone(ip) + diff --git a/zaza/model.py b/zaza/model.py index 507b57228..6076f623a 100644 --- a/zaza/model.py +++ b/zaza/model.py @@ -697,7 +697,8 @@ async def async_get_app_ips(application_name, model_name=None): :rtype: [str, str,...] """ return [await u.get_public_address() - for u in get_units(application_name, model_name=model_name)] + for u in await async_get_units( + application_name, model_name=model_name)] get_app_ips = sync_wrapper(async_get_app_ips) From 5457dc3505ff555207ac92782a12f56c020f8e76 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Tue, 11 Jan 2022 14:47:28 +0000 Subject: [PATCH 5/7] Fix code for py3.5 and add a regression test * Added a regression test for the get_app_ips() function for associated error [1]. * Added third.yaml bundle with just ubuntu apps for public IP tests * Add pins for py35 support (oslo.config and kubernetes) [1]: https://github.com/openstack-charmers/zaza/issues/470 --- requirements.txt | 2 ++ tests/bundles/third.yaml | 5 +++++ tests/tests.yaml | 1 + unit_tests/test_zaza_model.py | 8 ++++++-- zaza/charm_tests/libjuju/__init__.py | 1 - zaza/charm_tests/libjuju/tests.py | 1 - zaza/model.py | 7 ++++--- 7 files changed, 18 insertions(+), 7 deletions(-) create mode 100644 tests/bundles/third.yaml diff --git a/requirements.txt b/requirements.txt index edc52ec5e..80ec7a12a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,7 @@ pyparsing<3.0.0 # pin for aodhclient which is held for py35 aiounittest async_generator +kubernetes<18.0.0; python_version < '3.6' # pined, as juju uses kubernetes juju juju_wait PyYAML>=3.0 @@ -19,6 +20,7 @@ Jinja2>=2.6 # BSD License (3 clause) six>=1.9.0 dnspython>=1.12.0 psutil>=1.1.1,<2.0.0 +oslo.config<6.9.0;python_version < '3.6' # pin for py3.5 support oslo.context<3.0.0;python_version < '3.6' # pin for py3.5 support osprofiler<3.0.0;python_version < '3.6' # pin for py3.5 support python-openstackclient>=3.14.0 diff --git a/tests/bundles/third.yaml b/tests/bundles/third.yaml new file mode 100644 index 000000000..06a08d670 --- /dev/null +++ b/tests/bundles/third.yaml @@ -0,0 +1,5 @@ +applications: + ubuntu: + charm: cs:ubuntu + num_units: 10 + diff --git a/tests/tests.yaml b/tests/tests.yaml index 2856bbc88..eee96d9af 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -29,3 +29,4 @@ tests_options: force_deploy: first second + third diff --git a/unit_tests/test_zaza_model.py b/unit_tests/test_zaza_model.py index 266efd76c..015f85aa9 100644 --- a/unit_tests/test_zaza_model.py +++ b/unit_tests/test_zaza_model.py @@ -578,8 +578,12 @@ def test_get_unit_from_name(self): def test_get_app_ips(self): self.patch_object(model, 'get_juju_model', return_value='mname') - self.patch_object(model, 'get_units') - self.get_units.return_value = self.units + self.patch_object(model, 'async_get_units') + + async def mock_async_aget_units(*args, **kwargs): + return self.units + + self.async_get_units.side_effect = mock_async_aget_units self.assertEqual(model.get_app_ips('model', 'app'), ['ip1', 'ip2']) def test_run_on_unit(self): diff --git a/zaza/charm_tests/libjuju/__init__.py b/zaza/charm_tests/libjuju/__init__.py index 480703596..3f727d603 100644 --- a/zaza/charm_tests/libjuju/__init__.py +++ b/zaza/charm_tests/libjuju/__init__.py @@ -13,4 +13,3 @@ # limitations under the License. """Collection of regression tests checking zaza/libjuju integration.""" - diff --git a/zaza/charm_tests/libjuju/tests.py b/zaza/charm_tests/libjuju/tests.py index 3e1e84b04..844e03413 100644 --- a/zaza/charm_tests/libjuju/tests.py +++ b/zaza/charm_tests/libjuju/tests.py @@ -39,4 +39,3 @@ def test_get_app_ips(self): ips = zaza.model.get_app_ips('ubuntu') for ip in ips: self.assertIsNotNone(ip) - diff --git a/zaza/model.py b/zaza/model.py index 6076f623a..4e6fe65cc 100644 --- a/zaza/model.py +++ b/zaza/model.py @@ -696,9 +696,10 @@ async def async_get_app_ips(application_name, model_name=None): :returns: List of ip addresses :rtype: [str, str,...] """ - return [await u.get_public_address() - for u in await async_get_units( - application_name, model_name=model_name)] + addresses = [] + for u in await async_get_units(application_name, model_name=model_name): + addresses.append(await u.get_public_address()) + return addresses get_app_ips = sync_wrapper(async_get_app_ips) From 0d7212a444d0a9369b8acc927fb22717391ff6df Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 13 Jan 2022 16:36:48 +0000 Subject: [PATCH 6/7] Add fallback for unit.get_public_address() Due to bug [1], it seems that there is a issue with libjuju communicating with the Juju controller which causes a permanent model disconnect that libjuju doesn't resolve. Thus, for zaza, this patch wraps unit.get_public.address() with a wrapper that can choose, based on the environment variable `ZAZA_FEATURE_BUG472` to use a subprocess shell call to the `juju status` command to get the public address. This should always succeed. The feature environment variable `ZAZA_FEATURE_BUG472` was added so that the library can switch between the native libjuju function and the fallback wrapper to enable testing of the issue as libjuju continues to evolve. By default, the wrapper function is used, to enable zaza to interoperate with libjuju with Juju 2.9 on OpenStack providers. The implementation is slightly complicated because an async version of the wrapper `get_unit_public_address()` is needed as it is called from async code. [1]: https://github.com/juju/python-libjuju/issues/615 --- scripts/README.md | 5 + scripts/bash_tester.sh | 15 ++ scripts/fetch1.py | 98 +++++++++++++ scripts/tester.py | 47 +++++++ tests/tests.yaml | 1 + unit_tests/test_zaza_model.py | 131 ++++++++++++++++-- .../utilities/test_zaza_utilities_generic.py | 58 ++++++++ zaza/charm_tests/libjuju/tests.py | 29 ++-- zaza/model.py | 75 ++++++++-- zaza/utilities/generic.py | 20 ++- 10 files changed, 444 insertions(+), 35 deletions(-) create mode 100644 scripts/README.md create mode 100755 scripts/bash_tester.sh create mode 100755 scripts/fetch1.py create mode 100755 scripts/tester.py diff --git a/scripts/README.md b/scripts/README.md new file mode 100644 index 000000000..751f5767d --- /dev/null +++ b/scripts/README.md @@ -0,0 +1,5 @@ +# Scripts sub-directory + +The scripts in this directory are for regression/manual testing of the +libjuju unit.get_public_address() function. The tox target 'third' can +also be used for regression testing. diff --git a/scripts/bash_tester.sh b/scripts/bash_tester.sh new file mode 100755 index 000000000..e4d705bf6 --- /dev/null +++ b/scripts/bash_tester.sh @@ -0,0 +1,15 @@ +#!/bin/bash + +_dir="$( cd "$(dirname "${BASH_SOURCE[0]}" )" && pwd)" +runner="${_dir}/fetch1.py" + +# loop 10 times and fetch an instance address +i=0 +while [ $i -ne 10 ]; +do + printf "\n\n\n!!!!!!" + printf "\n\n\nDoing number $i" + printf "\n\n" + $runner $i + i=$(($i+1)) +done diff --git a/scripts/fetch1.py b/scripts/fetch1.py new file mode 100755 index 000000000..4c93a8ca0 --- /dev/null +++ b/scripts/fetch1.py @@ -0,0 +1,98 @@ +#!/usr/bin/env python3 + +from juju.model import Model +import asyncio +import sys + +# set to use something other than the current model +MODEL=None + + +async def get_units(): + model = Model() + if MODEL is None: + await model.connect() + else: + await model.connect_model(MODEL) + units = sorted(model.applications['ubuntu'].units, key=lambda u: u.name) + await model.disconnect() + return units + + +async def get_address(unit): + model = Model() + await model.connect_model(MODEL) + print("{} Address: .public_address {}".format(unit.name, unit.public_address)) + while True: + try: + print("{} Address: get_public_address() {}".format(unit.name, await unit.get_public_address())) + break + except Exception as e: + print("Exception was: %s", e) + await asyncio.sleep(.25) + await ensure_model_connected(model) + print("{} Address: .public_address {}".format(unit.name, unit.public_address)) + print("\n") + await model.disconnect() + + +def run_it(step): + loop = asyncio.get_event_loop() + task = loop.create_task(step) + loop.run_until_complete(asyncio.wait([task], loop=loop)) + result = task.result() + return result + + +async def get_unit(n): + units = await get_units() + print("units", units) + await get_address(units[n]) + + +def is_model_disconnected(model): + """Return True if the model is disconnected. + + :param model: the model to check + :type model: :class:'juju.Model' + :returns: True if disconnected + :rtype: bool + """ + print("is_model_disconnected?: %s, %s", model.is_connected(), model.connection().is_open) + return not (model.is_connected() and model.connection().is_open) + + +async def ensure_model_connected(model): + """Ensure that the model is connected. + + If model is disconnected then reconnect it. + + :param model: the model to check + :type model: :class:'juju.Model' + """ + if is_model_disconnected(model): + model_name = model.info.name + print( + "model: %s has disconnected, forcing full disconnection " + "and then reconnecting ...", model_name) + try: + await model.disconnect() + except Exception: + # We don't care if disconnect fails; we're much more + # interested in re-connecting, and this is just to clean up + # anything that might be left over (i.e. + # model.is_connected() might be true, but + # model.connection().is_open may be false + pass + print("Attempting to reconnect model %s", model_name) + await model.connect_model(model_name) + + +if __name__ == '__main__': + unit_num = 0 + if len(sys.argv) > 1: + unit_num = int(sys.argv[1]) + + run_it(get_unit(unit_num)) + asyncio.get_event_loop().close() + diff --git a/scripts/tester.py b/scripts/tester.py new file mode 100755 index 000000000..98bb9c719 --- /dev/null +++ b/scripts/tester.py @@ -0,0 +1,47 @@ +#!/usr/bin/env python3 + +from juju.model import Model +import asyncio + +# set to use something other than the current model +MODEL=None + + +async def get_units(): + model = Model() + if MODEL is None: + await model.connect() + else: + await model.connect_model(MODEL) + units = sorted(model.applications['ubuntu'].units, key=lambda u: u.name) + await model.disconnect() + return units + + +async def get_address(unit): + model = Model() + await model.connect_model(MODEL) + print("{} Address: .public_address {}".format(unit.name, unit.public_address)) + print("{} Address: get_public_address() {}".format(unit.name, await unit.get_public_address())) + print("{} Address: .public_address {}".format(unit.name, unit.public_address)) + print("\n") + await model.disconnect() + + +def run_it(step): + loop = asyncio.get_event_loop() + task = loop.create_task(step) + loop.run_until_complete(asyncio.wait([task], loop=loop)) + result = task.result() + return result + + +def get_all_units(): + units = run_it(get_units()) + print("units", units) + for unit in units: + run_it(get_address(unit)) + + +get_all_units() +asyncio.get_event_loop().close() diff --git a/tests/tests.yaml b/tests/tests.yaml index eee96d9af..55447409f 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -5,6 +5,7 @@ gate_bundles: # destruction - first - second +- third target_deploy_status: magpie-xenial: workload-status: active diff --git a/unit_tests/test_zaza_model.py b/unit_tests/test_zaza_model.py index 015f85aa9..4f8a5bd11 100644 --- a/unit_tests/test_zaza_model.py +++ b/unit_tests/test_zaza_model.py @@ -31,6 +31,7 @@ import copy import concurrent import mock +import yaml import unit_tests.utils as ut_utils from juju import loop @@ -536,14 +537,105 @@ def test_get_lead_unit_name(self): 'app/4') def test_get_unit_public_address(self): - self.assertEqual(model.get_unit_public_address(self.unit1), 'ip1') + async def mock_fallback(*args, **kwargs): + return 'fip' + + async def mock_libjuju(*args, **kwargs): + return 'ljip' + + self.patch_object(model, + 'async_get_unit_public_address__fallback', + name='mock_fallback') + self.patch_object(model, + 'async_get_unit_public_address__libjuju', + name='mock_libjuju') + self.mock_fallback.side_effect = mock_fallback + self.mock_libjuju.side_effect = mock_libjuju + + _env = {"ZAZA_FEATURE_BUG472": ""} + with mock.patch.dict(model.os.environ, _env): + self.assertEqual(model.get_unit_public_address( + self.unit1, model_name='a-model'), 'fip') + self.mock_fallback.assert_called_once_with( + self.unit1, model_name='a-model') + self.mock_libjuju.assert_not_called() + _env = {"ZAZA_FEATURE_BUG472": "1"} + self.mock_fallback.reset_mock() + with mock.patch.dict(model.os.environ, _env): + self.assertEqual(model.get_unit_public_address(self.unit1), 'ljip') + self.mock_libjuju.assert_called_once_with( + self.unit1, model_name=None) + self.mock_fallback.assert_not_called() + + def test_get_unit_public_address__libjuju(self): + sync_get_unit_public_address__libjuju = model.sync_wrapper( + model.async_get_unit_public_address__libjuju) + self.assertEqual( + sync_get_unit_public_address__libjuju( + self.unit1, model_name='a-model'), 'ip1') + + def test_get_unit_public_address__fallback(self): + self.patch_object(model, 'logging', name='mock_logging') + sync_get_unit_public_address__fallback = model.sync_wrapper( + model.async_get_unit_public_address__fallback) + + async def mock_async_get_juju_model(): + return 'a-model' + + self.patch_object(model, 'async_get_juju_model') + self.async_get_juju_model.side_effect = mock_async_get_juju_model + + status = yaml.safe_dump({ + 'applications': { + 'an-app': { + 'units': { + 'an-app/0': { + 'public-address': '2.3.4.5' + } + } + } + } + }) + + async def mock_check_output(*args, **kwargs): + return {'Stdout': status} + + self.patch_object( + model.generic_utils, 'check_output', name='async_check_output') + self.async_check_output.side_effect = mock_check_output + + mock_unit = mock.Mock() + mock_p_name = mock.PropertyMock(return_value='an-app/0') + type(mock_unit).name = mock_p_name + self.assertEqual( + sync_get_unit_public_address__fallback( + mock_unit, model_name='b-model'), '2.3.4.5') + self.async_check_output.assert_called_once_with( + "juju status --format=yaml -m b-model".split(), log_stderr=False, + log_stdout=False) + mock_p_name = mock.PropertyMock(return_value='an-app/1') + type(mock_unit).name = mock_p_name + + self.async_check_output.reset_mock() + self.assertEqual( + sync_get_unit_public_address__fallback(mock_unit), None) + self.async_check_output.assert_called_once_with( + "juju status --format=yaml -m a-model".split(), log_stderr=False, + log_stdout=False) def test_get_lead_unit_ip(self): - self.patch_object(model, 'get_juju_model', return_value='mname') - self.patch_object(model, 'get_units') - self.get_units.return_value = self.units - self.patch_object(model, 'Model') - self.Model.return_value = self.Model_mock + async def mock_async_get_lead_unit(*args, **kwargs): + return [self.unit2] + + async def mock_async_get_unit_public_address(*args): + return 'ip2' + + self.patch_object( + model, 'async_get_lead_unit', side_effect=mock_async_get_lead_unit) + self.patch_object( + model, + 'async_get_unit_public_address', + side_effect=mock_async_get_unit_public_address) self.assertEqual( model.get_lead_unit_ip('app', 'model'), 'ip2') @@ -577,14 +669,31 @@ def test_get_unit_from_name(self): model.get_unit_from_name('bad_name', model_name='mname') def test_get_app_ips(self): - self.patch_object(model, 'get_juju_model', return_value='mname') - self.patch_object(model, 'async_get_units') + # self.patch_object(model, 'get_juju_model', return_value='mname') - async def mock_async_aget_units(*args, **kwargs): + async def mock_async_get_units(*args, **kwargs): return self.units - self.async_get_units.side_effect = mock_async_aget_units - self.assertEqual(model.get_app_ips('model', 'app'), ['ip1', 'ip2']) + async def mock_async_get_unit_public_address(unit, **kwargs): + if unit is self.unit1: + return 'ip1' + elif unit is self.unit2: + return 'ip2' + return None + + self.patch_object(model, 'async_get_units', + side_effect=mock_async_get_units) + self.patch_object( + model, + 'async_get_unit_public_address', + side_effect=mock_async_get_unit_public_address) + + self.assertEqual(model.get_app_ips('app', 'model'), ['ip1', 'ip2']) + self.async_get_units.assert_called_once_with( + 'app', model_name='model') + self.async_get_unit_public_address.assert_has_calls([ + mock.call(self.unit1, model_name='model'), + mock.call(self.unit2, model_name='model')]) def test_run_on_unit(self): self.patch_object(model, 'get_juju_model', return_value='mname') diff --git a/unit_tests/utilities/test_zaza_utilities_generic.py b/unit_tests/utilities/test_zaza_utilities_generic.py index ef33e6616..bcad02dc4 100644 --- a/unit_tests/utilities/test_zaza_utilities_generic.py +++ b/unit_tests/utilities/test_zaza_utilities_generic.py @@ -13,7 +13,10 @@ # limitations under the License. import mock +import subprocess + import unit_tests.utils as ut_utils +import zaza from zaza.utilities import generic as generic_utils import zaza.utilities.exceptions as zaza_exceptions @@ -573,3 +576,58 @@ def test_validate_unit_process_ids(self): } ret = generic_utils.validate_unit_process_ids(expected, actual) self.assertTrue(ret) + + def test_check_call(self): + + async def async_check_output(*args, **kwargs): + return "hello" + + self.patch_object( + generic_utils, 'check_output', side_effect=async_check_output) + check_call = zaza.sync_wrapper(generic_utils.check_call) + check_call("a command") + self.check_output.assert_called_once_with( + 'a command', log_stdout=True, log_stderr=True) + self.check_output.reset_mock() + check_call("b command", log_stdout=False) + self.check_output.assert_called_once_with( + 'b command', log_stdout=False, log_stderr=True) + self.check_output.reset_mock() + check_call("c command", log_stderr=False) + self.check_output.assert_called_once_with( + 'c command', log_stdout=True, log_stderr=False) + + def test_check_output(self): + self.patch_object(generic_utils, 'logging', name='mock_logging') + + async def mock_communicate(): + return (b"output log", b"error log") + + mock_proc = mock.Mock( + communicate=mock_communicate, + returncode=0) + + async def mock_create_subprocess_exec(*args, **kwargs): + return mock_proc + + self.patch_object(generic_utils.asyncio, 'create_subprocess_exec', + side_effect=mock_create_subprocess_exec) + check_call = zaza.sync_wrapper(generic_utils.check_output) + expected = { + 'Code': str(mock_proc.returncode), + 'Stderr': "error log", + 'Stdout': "output log", + } + self.assertEqual(check_call(['a', 'command']), expected) + # check for raising an error + mock_proc.returncode = 5 + expected = { + 'Code': str(mock_proc.returncode), + 'Stderr': "error log", + 'Stdout': "output log", + } + self.subprocess.CalledProcessError = subprocess.CalledProcessError + try: + check_call(['a', 'command']) + except subprocess.CalledProcessError: + pass diff --git a/zaza/charm_tests/libjuju/tests.py b/zaza/charm_tests/libjuju/tests.py index 844e03413..ca266a127 100644 --- a/zaza/charm_tests/libjuju/tests.py +++ b/zaza/charm_tests/libjuju/tests.py @@ -25,17 +25,28 @@ class RegressionTest(unittest.TestCase): """Regression Tests.""" - def test_get_unit_public_address(self): - """Verify get_unit_public_address().""" - logging.info('Verify that get_unit_public_address() function works.') - units = zaza.model.get_units('ubuntu') - ips = [zaza.model.get_unit_public_address(unit) for unit in units] + @classmethod + def setUpClass(cls): + """Run class setup.""" + super().setUpClass() + cls._model = zaza.model.get_juju_model() + logging.info("model is %s", cls._model) + + def test_01_get_app_ips(self): + """Verify that get_app_ips() doesn't invoke to async loops.""" + logging.info('Verify that get_app_ips() works.') + ips = zaza.model.get_app_ips('ubuntu', model_name=self._model) for ip in ips: + logging.info("Ip found %s", ip) self.assertIsNotNone(ip) - def test_get_app_ips(self): - """Verify that get_app_ips() doesn't invoke to async loops.""" - logging.info('Verify that get_app_ips() works.') - ips = zaza.model.get_app_ips('ubuntu') + def test_02_get_unit_public_address(self): + """Verify get_unit_public_address().""" + logging.info('Verify that get_unit_public_address() function works.') + units = zaza.model.get_units('ubuntu') + logging.info('units found: %s', units) + ips = [zaza.model.get_unit_public_address(unit, model_name=self._model) + for unit in units] for ip in ips: + logging.info("Ip found %s", ip) self.assertIsNotNone(ip) diff --git a/zaza/model.py b/zaza/model.py index 4e6fe65cc..5784aaffd 100644 --- a/zaza/model.py +++ b/zaza/model.py @@ -664,8 +664,34 @@ async def async_get_lead_unit_name(application_name, model_name=None): get_lead_unit_name = sync_wrapper(async_get_lead_unit_name) -def get_unit_public_address(unit): - """Get the public address of a the unit. +async def async_get_unit_public_address(unit, model_name=None): + """Get the public address of a unit. + + Based on a feature flag "ZAZA_FEATURE_BUG472" existing, the function will + call `get_unit_public_address__libjuju()`. Otherwise, it will fall back to + using `get_unit_public_address__fallback()` so that the public address can + be extracted. + + Bug: https://github.com/openstack-charmers/zaza/issues/472 + + :param unit: The libjuju unit object to get the public address for. + :type unit: juju.Unit + :returns: the IP address of the unit, or None + :rtype: Optional(str) + """ + if os.environ.get('ZAZA_FEATURE_BUG472', None): + return await async_get_unit_public_address__libjuju( + unit, model_name=model_name) + else: + return await async_get_unit_public_address__fallback( + unit, model_name=model_name) + + +get_unit_public_address = sync_wrapper(async_get_unit_public_address) + + +async def async_get_unit_public_address__libjuju(unit, model_name=None): + """Get the public address of a unit. The libjuju library, in theory, supports a unit.public_address attribute that provides the publick address of the unit. However, when the unit is @@ -673,17 +699,47 @@ def get_unit_public_address(unit): Therefore, there is a 'get_public_address()' funtion on unit that does provide the function. See [1]. + Note, if the underlying provider hasn't provided an address (yet) then this + will return None. + 1. https://github.com/juju/python-libjuju/issues/551 :param unit: The libjuju unit object to get the public address for. :type unit: juju.Unit :returns: the IP address of the unit. - :rtype: str + :rtype: Optional(str) """ - async def _get(unit_): - return await unit_.get_public_address() + return await unit.get_public_address() + + +async def async_get_unit_public_address__fallback(unit, model_name=None): + """Get the public address of a unit via juju status shell command. - return sync_wrapper(_get)(unit) + Due to bug [1], this function calls juju status and extracts the public + address as provided by the juju go client, as libjuju is unreliable. + This is a stop-gap solution to work around the bug. If the IP address + can't be found, then None is returned. + + [1]: https://github.com/juju/python-libjuju/issues/615 + + :param unit: The libjuju unit object to get the public address for. + :type unit: juju.Unit + :returns: the IP address of the unit. + :rtype: Optional[str] + """ + if model_name is None: + model_name = await async_get_juju_model() + cmd = "juju status --format=yaml -m {}".format(model_name) + result = await generic_utils.check_output( + cmd.split(), log_stderr=False, log_stdout=False) + status = yaml.safe_load(result['Stdout']) + try: + app = unit.name.split('/')[0] + return ( + status['applications'][app]['units'][unit.name]['public-address']) + except KeyError: + logging.warn("Public address not found for %s", unit.name) + return None async def async_get_app_ips(application_name, model_name=None): @@ -698,7 +754,8 @@ async def async_get_app_ips(application_name, model_name=None): """ addresses = [] for u in await async_get_units(application_name, model_name=model_name): - addresses.append(await u.get_public_address()) + addresses.append( + await async_get_unit_public_address(u, model_name=model_name)) return addresses @@ -716,8 +773,8 @@ async def async_get_lead_unit_ip(application_name, model_name=None): :rtype: str :raises: zaza.utilities.exceptions.JujuError """ - return await (await async_get_lead_unit( - application_name, model_name)).get_public_address() + return await async_get_unit_public_address(await async_get_lead_unit( + application_name, model_name)) get_lead_unit_ip = sync_wrapper(async_get_lead_unit_ip) diff --git a/zaza/utilities/generic.py b/zaza/utilities/generic.py index fde017da0..b3281d9f4 100644 --- a/zaza/utilities/generic.py +++ b/zaza/utilities/generic.py @@ -657,7 +657,7 @@ def validate_unit_process_ids(expected, actual): return True -async def check_call(cmd): +async def check_call(cmd, log_stdout=True, log_stderr=True): """Asynchronous function to check a subprocess call. :param cmd: Command to execute @@ -666,10 +666,10 @@ async def check_call(cmd): :rtype: None :raises: subprocess.CalledProcessError if returncode !=0 """ - await check_output(cmd) + await check_output(cmd, log_stdout=log_stdout, log_stderr=log_stderr) -async def check_output(cmd): +async def check_output(cmd, log_stdout=True, log_stderr=True): """Asynchronous function to run a subprocess and get the output. Note, as the code raises an Exception on returncode != 0, 'Code' in the @@ -677,6 +677,10 @@ async def check_output(cmd): :param cmd: Command to execute :type cmd: List[str] + :param log_stdout: Whether to log stdout on success, defaults to True + :type log_stdout: bool + :param log_stderr: Whether to log stderr on success, defaults to True + :type log_stderr: bool :returns: {'Code': '', 'Stderr': '', 'Stdout': ''} :rtype: dict :raises: subprocess.CalledProcessError if returncode !=0 @@ -691,11 +695,15 @@ async def check_output(cmd): if proc.returncode != 0: logging.warn("STDOUT: {}".format(stdout)) logging.warn("STDERR: {}".format(stderr)) - raise subprocess.CalledProcessError(proc.returncode, cmd) + raise subprocess.CalledProcessError( + returncode=proc.returncode, + cmd=cmd, + output=stdout, + stderr=stderr) else: - if stderr: + if stderr and log_stderr: logging.info("STDERR: {} ({})".format(stderr, ' '.join(cmd))) - if stdout: + if stdout and log_stdout: logging.info("STDOUT: {} ({})".format(stdout, ' '.join(cmd))) return { 'Code': str(proc.returncode), From 41b736add544feb919a5ed4e8d98ebd8df557210 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 13 Jan 2022 18:03:53 +0000 Subject: [PATCH 7/7] Add scripts and zaza/charm_tests/ to codecov ignore These are tests and thus shouldn't be analysed by code coverage tools. --- codecov.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/codecov.yml b/codecov.yml index a033cde07..d4dab4543 100644 --- a/codecov.yml +++ b/codecov.yml @@ -2,3 +2,5 @@ ignore: - "unit_tests/*" - "unit_tests/utilities/*" - "unit_tests/**/*" + - "scripts/*" + - "zaza/charm_tests/**/*"