From 4e066ddca5e65a650a998bd09c2ec19d7b766e44 Mon Sep 17 00:00:00 2001 From: Gabriel Cocenza Date: Tue, 3 Sep 2024 09:37:08 -0300 Subject: [PATCH] Check to see if it's not using the upstream snap (#93) * block units if upstream snap is installed via resource * not run config-changed when upstream resource is used * added func test showing that units go into blocked state if upstream resource is added. * used snap cli to query snaps installed on functional tests Closes: #94 --- src/charm.py | 18 ++++- src/service.py | 5 +- .../tests/charm_tests/openstack_exporter.py | 71 +++++++++++++------ tests/unit/test_charm.py | 16 ++++- tests/unit/test_service.py | 2 +- 5 files changed, 85 insertions(+), 27 deletions(-) diff --git a/src/charm.py b/src/charm.py index befad59..c396b8e 100755 --- a/src/charm.py +++ b/src/charm.py @@ -23,7 +23,7 @@ WaitingStatus, ) -from service import SNAP_NAME, get_installed_snap_service, snap_install_or_refresh +from service import SNAP_NAME, UPSTREAM_SNAP, get_installed_snap_service, snap_install_or_refresh logger = logging.getLogger(__name__) @@ -168,8 +168,11 @@ def _configure(self, _: ops.HookEvent) -> None: """ self.install() - snap_service = get_installed_snap_service(SNAP_NAME) + upstream_snap = get_installed_snap_service(UPSTREAM_SNAP) + if upstream_snap.present: + return + snap_service = get_installed_snap_service(SNAP_NAME) # if cos is not related then we should block and not run anything if not self.model.relations.get("cos-agent"): snap_service.stop() @@ -214,6 +217,17 @@ def _on_collect_unit_status(self, event: ops.CollectStatusEvent) -> None: if not self.model.relations.get("cos-agent"): event.add_status(BlockedStatus("Grafana Agent is not related")) + upstream_snap = get_installed_snap_service(UPSTREAM_SNAP) + + # this is necessary when doing a charm upgrade coming from revision 27 + if upstream_snap.present: + event.add_status( + BlockedStatus( + "golang-openstack-exporter detected. Please see: " + "https://charmhub.io/openstack-exporter#known-issues" + ) + ) + snap_service = get_installed_snap_service(SNAP_NAME) if not snap_service.present: diff --git a/src/service.py b/src/service.py index 1ade2d3..c13fa77 100644 --- a/src/service.py +++ b/src/service.py @@ -9,6 +9,7 @@ logger = getLogger(__name__) SNAP_NAME = "charmed-openstack-exporter" +UPSTREAM_SNAP = "golang-openstack-exporter" class SnapService: @@ -93,9 +94,9 @@ def remove_upstream_snap() -> None: Raises an exception on error. """ try: - snap.remove(["golang-openstack-exporter"]) + snap.remove([UPSTREAM_SNAP]) except snap.SnapError as e: - logger.error("failed to remove golang-openstack-exporter snap: %s", str(e)) + logger.error("failed to remove %s snap: %s", UPSTREAM_SNAP, str(e)) raise e diff --git a/tests/integration/tests/charm_tests/openstack_exporter.py b/tests/integration/tests/charm_tests/openstack_exporter.py index 64d310b..a0020eb 100644 --- a/tests/integration/tests/charm_tests/openstack_exporter.py +++ b/tests/integration/tests/charm_tests/openstack_exporter.py @@ -3,6 +3,7 @@ # # See LICENSE file for licensing details. +import json import logging import subprocess import tempfile @@ -12,7 +13,7 @@ import yaml from zaza import model -from charm import CLOUD_NAME, OS_CLIENT_CONFIG, SNAP_NAME +from charm import CLOUD_NAME, OS_CLIENT_CONFIG, SNAP_NAME, UPSTREAM_SNAP logger = logging.getLogger(__name__) @@ -85,7 +86,7 @@ def test_configure_port(self): def test_configure_snap_channel(self): """Test changing the snap channel.""" - snap_info_before = get_snap_exporter_info() + snap_info_before = get_snap_info(SNAP_NAME) self.assertEqual(snap_info_before["channel"], "latest/stable") # Change snap channel @@ -93,7 +94,7 @@ def test_configure_snap_channel(self): model.set_application_config(APP_NAME, {"snap_channel": new_channel}) model.block_until_all_units_idle() - snap_info_after = get_snap_exporter_info() + snap_info_after = get_snap_info(SNAP_NAME) self.assertEqual(snap_info_after["channel"], new_channel) model.reset_application_config(APP_NAME, ["snap_channel"]) @@ -105,7 +106,7 @@ def test_snap_as_resource(self): by the user changing the snap channel charm configuration. """ # snap from snapstore won't have "x" in the revision - snap_info_before = get_snap_exporter_info() + snap_info_before = get_snap_info(SNAP_NAME) self.assertNotIn("x", snap_info_before["revision"]) # juju cannot access resources at /tmp, so we create a tmp folder at the current directory @@ -119,7 +120,7 @@ def test_snap_as_resource(self): model.block_until_all_units_idle() # local installed snaps will have "x" in the revision - snap_info_resource = get_snap_exporter_info() + snap_info_resource = get_snap_info(SNAP_NAME) self.assertIn("x", snap_info_resource["revision"]) # changing channel won't affect the snap installed as resource @@ -127,7 +128,7 @@ def test_snap_as_resource(self): new_channel = "latest/beta" model.set_application_config(APP_NAME, {"snap_channel": new_channel}) model.block_until_all_units_idle() - channel_after = get_snap_exporter_info()["channel"] + channel_after = get_snap_info(SNAP_NAME)["channel"] self.assertNotEqual(channel_after, new_channel) self.assertEqual(channel_after, snap_info_resource["channel"]) @@ -138,7 +139,7 @@ def test_snap_as_resource(self): model.attach_resource(APP_NAME, "openstack-exporter", str(temp_file_path)) model.block_until_all_units_idle() - snap_info_after = get_snap_exporter_info() + snap_info_after = get_snap_info(SNAP_NAME) self.assertNotIn("x", snap_info_after["revision"]) self.assertEqual(snap_info_after["channel"], new_channel) @@ -256,18 +257,46 @@ def test_openstack_exporter_snap_down(self): ) self.assertEqual(self.leader_unit.workload_status_message, "") + def test_upstream_exporter_as_resource(self): + """Test using the upstream golang as resource will block the unit.""" + # juju cannot access resources at /tmp, so we create a tmp folder at the current directory + with tempfile.TemporaryDirectory(dir="./") as tmpdir: + tmp_path = Path(tmpdir) + download_cmd = ( + f"wget -q -P {tmpdir} https://github.com/canonical/openstack-exporter-operator/" + "releases/download/rev2/golang-openstack-exporter_amd64.snap" + ) + subprocess.run(download_cmd.split(), check=False) + + resource_file = tmp_path / "golang-openstack-exporter_amd64.snap" + model.attach_resource(APP_NAME, "openstack-exporter", str(resource_file)) + model.block_until_unit_wl_status( + self.leader_unit_entity_id, "blocked", timeout=STATUS_TIMEOUT + ) + snaps_installed = [snap["name"] for snap in get_snaps_installed()] + self.assertIn(UPSTREAM_SNAP, snaps_installed) + expected_msg = ( + "golang-openstack-exporter detected. Please see: " + "https://charmhub.io/openstack-exporter#known-issues" + ) + self.assertEqual(self.leader_unit.workload_status_message, expected_msg) + + # passing an empty file will unblock the unit + temp_file_path = Path(tmpdir) / f"{SNAP_NAME}.snap" + temp_file_path.touch() + model.attach_resource(APP_NAME, "openstack-exporter", str(temp_file_path)) + model.block_until_unit_wl_status( + self.leader_unit_entity_id, "active", timeout=STATUS_TIMEOUT + ) + + +def get_snaps_installed() -> dict[str, str]: + """Get the snaps installed in the leader unit.""" + cmd = "curl -sS --unix-socket /run/snapd.socket http://localhost/v2/snaps" + return json.loads(model.run_on_leader(APP_NAME, cmd).get("Stdout", ""))["result"] + -def get_snap_exporter_info() -> dict[str, str]: - """Get the openstack exporter snap information. - - The snap list expected format as input is: - Name Version Rev Tracking Publisher Notes - charmed-openstack-exporter 1.7.0-26-g3be9ddb 4 latest/stable canonical✓ - - """ - results = model.run_on_leader(APP_NAME, f"snap list {SNAP_NAME}").get("Stdout", "").strip() - snap_info = results.splitlines()[1].split() - return { - "version": snap_info[VERSION_COLUMN], - "revision": snap_info[REVISION_COLUMN], - "channel": snap_info[CHANNEL_COLUMN], - } +def get_snap_info(snap_name: str) -> list[dict[str, str]]: + """Get the snap information.""" + snaps = get_snaps_installed() + return [snap for snap in snaps if snap["name"] == snap_name][0] diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 9d6ca1c..a59aa2f 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -37,7 +37,11 @@ def teardown_method(self, _): def test_config_changed(self, config, mocker): mock_get_installed_snap_service = mocker.patch("charm.get_installed_snap_service") mock_snap_service = mocker.Mock(spec_set=SnapService) - mock_get_installed_snap_service.return_value = mock_snap_service + mock_upstream_service = mocker.Mock(spec_set=SnapService) + mock_upstream_service.present = False + mock_get_installed_snap_service.side_effect = ( + lambda snap: mock_snap_service if snap == SNAP_NAME else mock_upstream_service + ) mocker.patch("charm.OpenstackExporterOperatorCharm.install") @@ -81,6 +85,16 @@ def test_config_changed_snap_channel(self, mock_install, _): self.harness.update_config({"snap_channel": "latest/edge"}) mock_install.assert_called_once() + @mock.patch("charm.get_installed_snap_service") + @mock.patch("charm.OpenstackExporterOperatorCharm.install") + def test_config_changed_upstream_resource(self, mock_install, mocked_installed_snap): + """Test config change when using the upstream resource.""" + mocked_upstream = mock.MagicMock() + mocked_upstream.present = True + mocked_installed_snap.return_value = mocked_upstream + self.harness.update_config({"snap_channel": "latest/edge"}) + mock_install.assert_not_called() + @mock.patch("charm.OpenstackExporterOperatorCharm.get_resource", return_value="") @mock.patch("charm.snap_install_or_refresh") def test_install_snap(self, mock_install, _): diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py index f0de50f..be578d5 100644 --- a/tests/unit/test_service.py +++ b/tests/unit/test_service.py @@ -53,7 +53,7 @@ def test_snap_install_or_refresh_exception_raises(mock_snap, mock_workaround): def test_remove_upstream_snap(mock_snap): """Test remove upstream snap function.""" service.remove_upstream_snap() - mock_snap.remove.assert_called_with(["golang-openstack-exporter"]) + mock_snap.remove.assert_called_with([service.UPSTREAM_SNAP]) @mock.patch("service.snap.remove")