From 17b428f003fe37783c8c897d925dca1b9dccb44e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mert=20K=C4=B1rp=C4=B1c=C4=B1?= Date: Tue, 9 Aug 2022 10:55:59 +0000 Subject: [PATCH] tests: make unit and func tests pass again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This changes are mostly working on the test codes to make sure they are passing for the upcoming jammy support. In addition to that, it removes some old workarounds that are not needed anymore. There's still one open issue about the functional tests. See below. Since this is a big change, here is a summary of changes. Makefile: - remove workaround for the closed issue[0] where symlinks breaking charm push - introduce standard rename.sh tests/unit: - mock a bunch of stuff that were trying to modify system files/dirs tests/functional: - remove workaround for the closed issue[1] where zaza was not respecting local_overlay_enabled key - fix the issue where zaza waits for ubuntu charms indefinitely due to empty message[2] - add missing required config key in iscsi test - charmstore -> charmhub src: - get rid of commented code in production - add missing quotes to a config.yaml default key --- Currently open issues: tests/functional: - focal-fc bundle is not running, probably needs special hardware judging by the constraint given in the bundle file. However it is not listed in gate or smoke bundles, so it is not blocking the release. [0] https://github.com/canonical/charmcraft/issues/109 [1] https://github.com/openstack-charmers/zaza/issues/379 [2] https://github.com/openstack-charmers/zaza/issues/436 Signed-off-by: Mert Kırpıcı --- Makefile | 21 ++- charmcraft.yaml | 19 +++ config.yaml | 2 +- metadata.yaml | 3 - rename.sh | 13 ++ src/charm.py | 13 +- tests/functional/requirements.txt | 1 + .../storage-connector-func-tests/tests.py | 20 ++- tests/functional/tests/bundles/bionic.yaml | 7 +- tests/functional/tests/bundles/focal-fc.yaml | 4 +- tests/functional/tests/bundles/focal.yaml | 7 +- .../tests/bundles/overlays/bionic.yaml.j2 | 4 - .../tests/bundles/overlays/focal-fc.yaml.j2 | 4 - .../tests/bundles/overlays/focal.yaml.j2 | 4 - .../overlays/local-charm-overlay.yaml.j2 | 3 + tests/functional/tests/tests.yaml | 8 +- tests/unit/__init__.py | 4 +- tests/unit/test_charm.py | 127 ++++++++++++------ tox.ini | 4 +- 19 files changed, 169 insertions(+), 99 deletions(-) create mode 100644 charmcraft.yaml create mode 100755 rename.sh delete mode 100644 tests/functional/tests/bundles/overlays/bionic.yaml.j2 delete mode 100644 tests/functional/tests/bundles/overlays/focal-fc.yaml.j2 delete mode 100644 tests/functional/tests/bundles/overlays/focal.yaml.j2 create mode 100644 tests/functional/tests/bundles/overlays/local-charm-overlay.yaml.j2 diff --git a/Makefile b/Makefile index b806b6f..4955fad 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ PYTHON := /usr/bin/python3 PROJECTPATH=$(dir $(realpath $(MAKEFILE_LIST))) ifndef CHARM_BUILD_DIR - CHARM_BUILD_DIR=${PROJECTPATH}.build + CHARM_BUILD_DIR=${PROJECTPATH} endif METADATA_FILE="metadata.yaml" CHARM_NAME=$(shell cat ${PROJECTPATH}/${METADATA_FILE} | grep -E '^name:' | awk '{print $$2}') @@ -16,8 +16,8 @@ help: @echo " make release - run clean and build targets" @echo " make lint - run flake8 and black" @echo " make proof - run charm proof" - @echo " make unit - run the tests defined in the unittest subdirectory" - @echo " make func - run the tests defined in the functional subdirectory" + @echo " make unittests - run the tests defined in the unittest subdirectory" + @echo " make functional - run the tests defined in the functional subdirectory" @echo " make test - run lint, proof, unittests and functional targets" @echo "" @@ -25,19 +25,14 @@ clean: @echo "Cleaning files" @git clean -fXd @echo "Cleaning existing build" - @rm -rf ${CHARM_BUILD_DIR}/${CHARM_NAME} + @rm -rf ${CHARM_BUILD_DIR}/${CHARM_NAME}.charm build: - @charmcraft build + @charmcraft pack --verbose + @bash -c ./rename.sh -# bypassing bug https://github.com/canonical/charmcraft/issues/109 -unpack: build - @mkdir -p ${CHARM_BUILD_DIR}/${CHARM_NAME} - @echo "Unpacking built .charm into ${CHARM_BUILD_DIR}/${CHARM_NAME}" - @cd ${CHARM_BUILD_DIR}/${CHARM_NAME} && unzip -q ${CHARM_BUILD_DIR}/${CHARM_NAME}.charm - -release: clean build unpack - @echo "Charm is built at ${CHARM_BUILD_DIR}/${CHARM_NAME}" +release: clean build + @echo "Charm is built at ${CHARM_BUILD_DIR}/${CHARM_NAME}.charm" lint: @echo "Running lint checks" diff --git a/charmcraft.yaml b/charmcraft.yaml new file mode 100644 index 0000000..89df11b --- /dev/null +++ b/charmcraft.yaml @@ -0,0 +1,19 @@ +type: charm +parts: + charm: + charm-python-packages: [setuptools] +bases: + - build-on: + - name: ubuntu + channel: "20.04" + architectures: ["amd64"] + run-on: + - name: ubuntu + channel: "20.04" + architectures: + - amd64 + - name: ubuntu + channel: "18.04" + architectures: + - amd64 + diff --git a/config.yaml b/config.yaml index b0a4840..b77ca5a 100644 --- a/config.yaml +++ b/config.yaml @@ -38,7 +38,7 @@ options: "hostname2": "iqn.yyyy-mm.naming-authority:uniquename2}' multipath-defaults: type: string - default: "{user_friendly_names: yes}" + default: '{"user_friendly_names": "yes"}' description: | In multipath config, sets the defaults configuration. String should be of JSON dictionary format. Double quotes are essential to the correct format of this JSON string. diff --git a/metadata.yaml b/metadata.yaml index 6da3ae4..36e07f4 100644 --- a/metadata.yaml +++ b/metadata.yaml @@ -13,9 +13,6 @@ tags: - fibrechannel - fiberchannel subordinate: true -series: - - bionic - - focal requires: host: interface: juju-info diff --git a/rename.sh b/rename.sh new file mode 100755 index 0000000..956a76b --- /dev/null +++ b/rename.sh @@ -0,0 +1,13 @@ +#!/bin/bash +charm=$(grep -E "^name:" metadata.yaml | awk '{print $2}') +echo "renaming ${charm}_*.charm to ${charm}.charm" +echo -n "pwd: " +pwd +ls -al +echo "Removing previous charm if it exists" +if [[ -e "${charm}.charm" ]]; +then + rm "${charm}.charm" +fi +echo "Renaming charm here." +mv ${charm}_*.charm ${charm}.charm diff --git a/src/charm.py b/src/charm.py index 3d3351e..d6d281a 100755 --- a/src/charm.py +++ b/src/charm.py @@ -40,17 +40,6 @@ class StorageConnectorCharm(CharmBase): ISCSI_SERVICES = ['iscsid', 'open-iscsi'] MULTIPATHD_SERVICE = 'multipathd' - # ISCSI_MANDATORY_CONFIG = [ - # 'storage-type', - # 'iscsi-target', - # 'iscsi-port', - # 'multipath-devices' - # ] - # FC_MANDATORY_CONFIG = [ - # 'storage-type', - # 'fc-lun-alias', - # 'multipath-devices' - # ] VALID_STORAGE_TYPES = ["fc", "iscsi"] MANDATORY_CONFIG = { "iscsi": [ @@ -320,7 +309,7 @@ def _multipath_configuration(self, tenv): config = charm_config.get('multipath-' + section) if config: logging.info("Gather information for the multipaths section " + section) - logging.debug("multipath-" + section + " data: " + config) + logging.debug("multipath-" + section + " data: " + str(config)) try: ctxt[section] = json.loads(config) except Exception as e: diff --git a/tests/functional/requirements.txt b/tests/functional/requirements.txt index b7c9112..6f7831a 100644 --- a/tests/functional/requirements.txt +++ b/tests/functional/requirements.txt @@ -1 +1,2 @@ git+https://github.com/openstack-charmers/zaza.git#egg=zaza +git+https://github.com/openstack-charmers/zaza-openstack-tests.git#egg=zaza.openstack diff --git a/tests/functional/storage-connector-func-tests/tests.py b/tests/functional/storage-connector-func-tests/tests.py index c42545e..bc6c7e9 100644 --- a/tests/functional/storage-connector-func-tests/tests.py +++ b/tests/functional/storage-connector-func-tests/tests.py @@ -45,8 +45,9 @@ def configure_iscsi_connector(self): 'iscsi-node-session-auth-password': 'password123', 'iscsi-node-session-auth-username-in': 'iscsi-target', 'iscsi-node-session-auth-password-in': 'secretpass', + 'multipath-devices': '{}' } - zaza.model.set_application_config('iscsi-connector', conf) + zaza.model.set_application_config('storage-connector', conf) def get_unit_full_hostname(self, unit_name): """Retrieve the full hostname of a unit.""" @@ -59,7 +60,22 @@ def test_iscsi_connector(self): """Test iscsi configuration and wait for idle status.""" self.configure_iscsi_connector() logging.info('Wait for idle/ready status...') - zaza.model.wait_for_application_states() + zaza.model.wait_for_application_states( + states={ + "storage-connector": { + "workload-status": "active", + "workload-status-message-prefix": "Unit is ready" + }, + "ubuntu": { + "workload-status": "active", + "workload-status-message-regex": "^$" + }, + "ubuntu-target": { + "workload-status": "active", + "workload-status-message-regex": "^$" + } + } + ) def test_validate_iscsi_session(self): """Validate that the iscsi session is active.""" diff --git a/tests/functional/tests/bundles/bionic.yaml b/tests/functional/tests/bundles/bionic.yaml index 1eadd1f..501be16 100644 --- a/tests/functional/tests/bundles/bionic.yaml +++ b/tests/functional/tests/bundles/bionic.yaml @@ -1,12 +1,11 @@ +local_overlay_enabled: True series: bionic applications: ubuntu-target: - charm: cs:ubuntu + charm: ch:ubuntu num_units: 1 - storage-connector: - charm: #path is defined in overlay because of bug https://github.com/openstack-charmers/zaza/issues/379 ubuntu: - charm: cs:ubuntu + charm: ch:ubuntu num_units: 1 relations: - - 'ubuntu:juju-info' diff --git a/tests/functional/tests/bundles/focal-fc.yaml b/tests/functional/tests/bundles/focal-fc.yaml index d6f3ac9..9411c8c 100644 --- a/tests/functional/tests/bundles/focal-fc.yaml +++ b/tests/functional/tests/bundles/focal-fc.yaml @@ -1,14 +1,14 @@ +local_overlay_enabled: True series: focal applications: storage-connector: - charm: #path is defined in overlay because of bug https://github.com/openstack-charmers/zaza/issues/379 options: storage-type: 'fc' fc-lun-alias: 'data1' multipath-defaults: '{"user_friendly_names":"yes", "find_multipaths":"yes", "polling_interval":"10"}' multipath-devices: '{"vendor":"PURE", "product":"FlashArray", "fast_io_fail_tmo":"10", "path_selector":"queue-length 0", "path_grouping_policy":"group_by_prio", "rr_min_io":"1", "path_checker":"tur", "fast_io_fail_tmo":"1", "dev_loss_tmo":"infinity", "no_path_retry":"5", "failback":"immediate", "prio":"alua", "hardware_handler":"1 alua", "max_sectors_kb":"4096"}' ubuntu: - charm: cs:ubuntu + charm: ch:ubuntu num_units: 1 to: - 0 diff --git a/tests/functional/tests/bundles/focal.yaml b/tests/functional/tests/bundles/focal.yaml index aad2b90..6d8edee 100644 --- a/tests/functional/tests/bundles/focal.yaml +++ b/tests/functional/tests/bundles/focal.yaml @@ -1,12 +1,11 @@ +local_overlay_enabled: True series: focal applications: ubuntu-target: - charm: cs:ubuntu + charm: ch:ubuntu num_units: 1 - storage-connector: - charm: #path is defined in overlay because of bug https://github.com/openstack-charmers/zaza/issues/379 ubuntu: - charm: cs:ubuntu + charm: ch:ubuntu num_units: 1 relations: - - 'ubuntu:juju-info' diff --git a/tests/functional/tests/bundles/overlays/bionic.yaml.j2 b/tests/functional/tests/bundles/overlays/bionic.yaml.j2 deleted file mode 100644 index 552fefb..0000000 --- a/tests/functional/tests/bundles/overlays/bionic.yaml.j2 +++ /dev/null @@ -1,4 +0,0 @@ -applications: - storage-connector: - charm: {{ CHARM_BUILD_DIR }}/{{ CHARM_NAME }}.charm - diff --git a/tests/functional/tests/bundles/overlays/focal-fc.yaml.j2 b/tests/functional/tests/bundles/overlays/focal-fc.yaml.j2 deleted file mode 100644 index 552fefb..0000000 --- a/tests/functional/tests/bundles/overlays/focal-fc.yaml.j2 +++ /dev/null @@ -1,4 +0,0 @@ -applications: - storage-connector: - charm: {{ CHARM_BUILD_DIR }}/{{ CHARM_NAME }}.charm - diff --git a/tests/functional/tests/bundles/overlays/focal.yaml.j2 b/tests/functional/tests/bundles/overlays/focal.yaml.j2 deleted file mode 100644 index 552fefb..0000000 --- a/tests/functional/tests/bundles/overlays/focal.yaml.j2 +++ /dev/null @@ -1,4 +0,0 @@ -applications: - storage-connector: - charm: {{ CHARM_BUILD_DIR }}/{{ CHARM_NAME }}.charm - diff --git a/tests/functional/tests/bundles/overlays/local-charm-overlay.yaml.j2 b/tests/functional/tests/bundles/overlays/local-charm-overlay.yaml.j2 new file mode 100644 index 0000000..d19c84a --- /dev/null +++ b/tests/functional/tests/bundles/overlays/local-charm-overlay.yaml.j2 @@ -0,0 +1,3 @@ +applications: + {{ charm_name }}: + charm: "{{ CHARM_BUILD_DIR }}/{{ charm_name }}.charm" diff --git a/tests/functional/tests/tests.yaml b/tests/functional/tests/tests.yaml index cbc79df..dfdad6f 100644 --- a/tests/functional/tests/tests.yaml +++ b/tests/functional/tests/tests.yaml @@ -17,4 +17,10 @@ tests: target_deploy_status: storage-connector: workload-status: blocked - workload-status-message: "Missing" + workload-status-message-prefix: "Missing" + ubuntu: + workload-status: active + workload-status-message-regex: "^$" + ubuntu-target: + workload-status: active + workload-status-message-regex: "^$" diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py index a2f9161..f4a529f 100644 --- a/tests/unit/__init__.py +++ b/tests/unit/__init__.py @@ -2,9 +2,11 @@ """Init mocking for unit tests.""" import sys +from unittest import mock -import mock +from ops import testing +testing.SIMULATE_CAN_CONNECT = True sys.path.append('src') diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 86eeeec..6a33054 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -3,63 +3,70 @@ import os import shutil -import subprocess import tempfile import unittest from pathlib import Path -from unittest.mock import Mock, create_autospec, patch +from unittest.mock import PropertyMock, call, mock_open, patch -from charm import StorageConnectorCharm +import charm from ops.framework import EventBase from ops.testing import Harness -from src import charm - class TestCharm(unittest.TestCase): """Charm Unit Tests.""" - subprocess_mock = create_autospec(subprocess.check_call, return_value='True') - subprocess.check_call = subprocess_mock - def setUp(self): """Test setup.""" - self.tempdir = tempfile.mkdtemp() - self.harness = Harness(StorageConnectorCharm) + self.tmp_iscsi_conf_path = Path(tempfile.mkdtemp()) + self.tmp_multipath_conf_dir = Path(tempfile.mkdtemp()) + self.harness = Harness(charm.StorageConnectorCharm) self.harness.set_leader(is_leader=True) self.harness.begin() def tearDown(self): """Remove testing artifacts.""" - shutil.rmtree(self.tempdir) + shutil.rmtree(self.tmp_iscsi_conf_path) + shutil.rmtree(self.tmp_multipath_conf_dir) - def test__init__works_without_a_hitch(self): - """Test init.""" - - def test_abort_if_host_is_container(self): + @patch("charm.utils.is_container") + def test_abort_if_host_is_container(self, m_is_container): """Test if charm stops when deployed on a container.""" - charm.utils.is_container = Mock(return_value=True) + m_is_container.return_value = True + self.assertFalse(self.harness.charm._stored.installed) + self.harness.charm.on.install.emit() self.assertFalse(self.harness.charm._stored.installed) - @patch("charm.StorageConnectorCharm._iscsi_initiator") + @patch("charm.subprocess.check_call") + @patch("charm.subprocess.getoutput") @patch("charm.utils.is_container") - @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_PATH") - @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_DIR") - @patch("charm.StorageConnectorCharm.ISCSI_INITIATOR_NAME") - @patch("charm.StorageConnectorCharm.ISCSI_CONF") - @patch("charm.StorageConnectorCharm.ISCSI_CONF_PATH") - def test_on_iscsi_install(self, iscsi_conf_path, iscsi_conf, iscsi_initiator_name, - multipath_conf_dir, multipath_conf_path, - is_container, iscsi_initiator): + @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_PATH", new_callable=PropertyMock) + @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_DIR", new_callable=PropertyMock) + @patch( + "charm.StorageConnectorCharm.ISCSI_INITIATOR_NAME", new_callable=PropertyMock + ) + @patch("charm.StorageConnectorCharm.ISCSI_CONF", new_callable=PropertyMock) + @patch("charm.StorageConnectorCharm.ISCSI_CONF_PATH", new_callable=PropertyMock) + def test_on_iscsi_install(self, + m_iscsi_conf_path, + m_iscsi_conf, + m_iscsi_initiator_name, + m_multipath_conf_dir, + m_multipath_conf_path, + m_is_container, + m_getoutput, + m_check_call): """Test installation.""" - is_container.return_value = False - iscsi_conf_path.return_value = Path(tempfile.mkdtemp()) - iscsi_conf.return_value = iscsi_conf_path / 'iscsid.conf' - iscsi_initiator_name.return_value = iscsi_conf / 'initiatorname.iscsi' - multipath_conf_dir.return_value = Path(tempfile.mkdtemp()) - multipath_conf_path.return_value = multipath_conf_dir / 'conf.d' - iscsi_initiator.return_value = None + m_is_container.return_value = False + m_iscsi_conf_path.return_value = self.tmp_iscsi_conf_path + m_iscsi_conf.return_value = self.tmp_iscsi_conf_path / 'iscsid.conf' + m_iscsi_initiator_name.return_value = \ + self.tmp_iscsi_conf_path / 'initiatorname.iscsi' + m_multipath_conf_dir.return_value = self.tmp_multipath_conf_dir + m_multipath_conf_path.return_value = self.tmp_multipath_conf_dir / 'conf.d' + m_getoutput.return_value = "iqn.2020-07.canonical.com:lun1" + self.harness.update_config({ "storage-type": "iscsi", "iscsi-target": "abc", @@ -72,17 +79,40 @@ def test_on_iscsi_install(self, iscsi_conf_path, iscsi_conf, iscsi_initiator_nam self.assertTrue(os.path.exists(self.harness.charm.ISCSI_CONF)) self.assertTrue(os.path.exists(self.harness.charm.MULTIPATH_CONF_PATH)) + m_getoutput.assert_called_with("/sbin/iscsi-iname") + m_check_call.assert_has_calls( + [ + call("systemctl restart iscsid".split()), + call("systemctl restart open-iscsi".split()), + call("iscsiadm -m discovery -t sendtargets -p abc:443".split()), + call("iscsiadm -m node --login".split()) + ], + any_order=False + ) self.assertTrue(self.harness.charm._stored.installed) + @patch("charm.subprocess.getoutput") + @patch("builtins.open", new_callable=mock_open) @patch("charm.utils.is_container") - @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_PATH") - @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_DIR") - def test_on_fiberchannel_install(self, multipath_conf_dir, - multipath_conf_path, is_container): + @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_PATH", new_callable=PropertyMock) + @patch("charm.StorageConnectorCharm.MULTIPATH_CONF_DIR", new_callable=PropertyMock) + @patch("charm.StorageConnectorCharm.ISCSI_CONF", new_callable=PropertyMock) + @patch("charm.StorageConnectorCharm.ISCSI_CONF_PATH", new_callable=PropertyMock) + def test_on_fiberchannel_install(self, + m_iscsi_conf_path, + m_iscsi_conf, + m_multipath_conf_dir, + m_multipath_conf_path, + m_is_container, + m_open, + m_getoutput): """Test installation.""" - is_container.return_value = False - multipath_conf_dir.return_value = Path(tempfile.mkdtemp()) - multipath_conf_path.return_value = multipath_conf_dir / 'conf.d' + m_is_container.return_value = False + m_iscsi_conf_path.return_value = self.tmp_iscsi_conf_path + m_iscsi_conf.return_value = self.tmp_iscsi_conf_path / "fc-iscsid.conf" + m_multipath_conf_dir.return_value = self.tmp_multipath_conf_dir + m_multipath_conf_path.return_value = self.tmp_multipath_conf_dir / "conf.d" + m_getoutput.return_value = "host0" self.harness.update_config({ "storage-type": "fc", "fc-lun-alias": "data1", @@ -94,6 +124,10 @@ def test_on_fiberchannel_install(self, multipath_conf_dir, self.assertFalse(os.path.exists(self.harness.charm.ISCSI_CONF)) self.assertTrue(os.path.exists(self.harness.charm.MULTIPATH_CONF_PATH)) self.assertTrue(self.harness.charm._stored.installed) + m_open.assert_called_once_with("/sys/class/scsi_host/host0/scan", "w") + self.assertTrue( + call(self.harness.charm.ISCSI_CONF, "w") not in m_open.mock_calls + ) def test_on_start(self): """Test on start hook.""" @@ -106,16 +140,27 @@ def test_on_start(self): self.harness.charm.on.start.emit() self.assertTrue(self.harness.charm._stored.started) - def test_on_restart_iscsi_services_action(self): + @patch("charm.subprocess.check_call") + def test_on_restart_iscsi_services_action(self, m_check_call): """Test on restart action.""" action_event = FakeActionEvent() + m_check_call.return_value = True self.harness.charm.on_restart_iscsi_services_action(action_event) + m_check_call.assert_has_calls( + [ + call(["systemctl", "restart", "iscsid"]), + call(["systemctl", "restart", "open-iscsi"]) + ], + any_order=False + ) self.assertEqual(action_event.results['success'], 'True') - def test_on_reload_multipathd_service_action(self): + @patch("charm.subprocess.check_call") + def test_on_reload_multipathd_service_action(self, m_check_call): """Test on reload action.""" action_event = FakeActionEvent() self.harness.charm.on_reload_multipathd_service_action(action_event) + m_check_call.assert_called_once_with(["systemctl", "reload", "multipathd"]) self.assertEqual(action_event.results['success'], 'True') diff --git a/tox.ini b/tox.ini index 75cba87..442c9ff 100644 --- a/tox.ini +++ b/tox.ini @@ -8,12 +8,10 @@ skip_missing_interpreters = False basepython = python3 setenv = PYTHONPATH = {toxinidir}/lib/:{toxinidir} - CHARM_BUILD_DIR = {toxinidir}/.build - CHARM_NAME = storage-connector passenv = HOME CHARM_BUILD_DIR - CHARM_NAME + OS_* whitelist_externals = charmcraft [testenv:unit]