From c38a696d168531e3c067451044262ef4d78ef11f Mon Sep 17 00:00:00 2001 From: David Garcia Date: Wed, 16 Sep 2020 13:31:33 +0200 Subject: [PATCH] Remove EntityType from juju watcher and workaround juju bug for retrieving the status - The juju watcher was doing an unnecessary translation with the entity types. The entity already provides an attribute .entity_type - Juju has a bug in version 2.8.2 that returns a wrong status. Therefore, charms were getting stuck in waiting for ever. Change-Id: I44709190acc41601e8a67f4c52074fda00c3d495 Signed-off-by: David Garcia --- n2vc/juju_watcher.py | 74 ++++++++++++++++---------- n2vc/libjuju.py | 17 +++--- n2vc/n2vc_conn.py | 4 +- n2vc/tests/unit/test_juju_watcher.py | 79 +++++++++++++++++++++++----- n2vc/tests/unit/test_utils.py | 8 +-- n2vc/tests/unit/utils.py | 60 ++++++++++----------- n2vc/utils.py | 20 ++----- 7 files changed, 159 insertions(+), 103 deletions(-) diff --git a/n2vc/juju_watcher.py b/n2vc/juju_watcher.py index 8da62ff..842e990 100644 --- a/n2vc/juju_watcher.py +++ b/n2vc/juju_watcher.py @@ -15,17 +15,38 @@ import asyncio import time from juju.client import client -from n2vc.utils import FinalStatus, EntityType from n2vc.exceptions import EntityInvalidException from n2vc.n2vc_conn import N2VCConnector from juju.model import ModelEntity, Model from juju.client.overrides import Delta +from juju.status import derive_status +from juju.application import Application from websockets.exceptions import ConnectionClosed import logging logger = logging.getLogger("__main__") +def status(application: Application) -> str: + unit_status = [] + for unit in application.units: + unit_status.append(unit.workload_status) + return derive_status(unit_status) + + +def entity_ready(entity: ModelEntity) -> bool: + entity_type = entity.entity_type + if entity_type == "machine": + return entity.agent_status in ["started"] + elif entity_type == "action": + return entity.status in ["completed", "failed", "cancelled"] + elif entity_type == "application": + # Workaround for bug: https://github.com/juju/python-libjuju/issues/441 + return status(entity) in ["active", "blocked"] + else: + raise EntityInvalidException("Unknown entity type: {}".format(entity_type)) + + class JujuModelWatcher: @staticmethod async def wait_for( @@ -54,21 +75,14 @@ class JujuModelWatcher: if total_timeout is None: total_timeout = 3600.0 - entity_type = EntityType.get_entity(type(entity)) - if entity_type not in FinalStatus: - raise EntityInvalidException("Entity type not found") - - # Get final states - final_states = FinalStatus[entity_type].status - field_to_check = FinalStatus[entity_type].field + entity_type = entity.entity_type + if entity_type not in ["application", "action", "machine"]: + raise EntityInvalidException("Unknown entity type: {}".format(entity_type)) # Coroutine to wait until the entity reaches the final state wait_for_entity = asyncio.ensure_future( asyncio.wait_for( - model.block_until( - lambda: entity.__getattribute__(field_to_check) in final_states - ), - timeout=total_timeout, + model.block_until(lambda: entity_ready(entity)), timeout=total_timeout, ) ) @@ -100,7 +114,7 @@ class JujuModelWatcher: async def model_watcher( model: Model, entity_id: str, - entity_type: EntityType, + entity_type: str, timeout: float, db_dict: dict = None, n2vc: N2VCConnector = None, @@ -110,7 +124,7 @@ class JujuModelWatcher: :param: model: Model to observe :param: entity_id: ID of the entity to be observed - :param: entity_type: EntityType (p.e. .APPLICATION, .MACHINE, and .ACTION) + :param: entity_type: Entity Type (p.e. "application", "machine, and "action") :param: timeout: Maximum time between two updates in the model :param: db_dict: Dictionary with data of the DB to write the updates :param: n2vc: N2VC Connector objector @@ -122,8 +136,8 @@ class JujuModelWatcher: # Genenerate array with entity types to listen entity_types = ( - [entity_type, EntityType.UNIT] - if entity_type == EntityType.APPLICATION # TODO: Add .ACTION too + [entity_type, "unit"] + if entity_type == "application" # TODO: Add "action" too else [entity_type] ) @@ -138,14 +152,14 @@ class JujuModelWatcher: delta_entity = None # Get delta EntityType - delta_entity = EntityType.get_entity_from_delta(delta.entity) + delta_entity = delta.entity if delta_entity in entity_types: # Get entity id - if entity_type == EntityType.APPLICATION: + if entity_type == "application": id = ( delta.data["application"] - if delta_entity == EntityType.UNIT + if delta_entity == "unit" else delta.data["name"] ) else: @@ -156,9 +170,11 @@ class JujuModelWatcher: # Update timeout timeout_end = time.time() + timeout - (status, status_message, vca_status) = JujuModelWatcher.get_status( - delta, entity_type=delta_entity - ) + ( + status, + status_message, + vca_status, + ) = JujuModelWatcher.get_status(delta) if write and n2vc is not None and db_dict: # Write status to DB @@ -168,7 +184,7 @@ class JujuModelWatcher: status=status, detailed_status=status_message, vca_status=vca_status, - entity_type=delta_entity.value.__name__.lower(), + entity_type=delta_entity, ) # Check if timeout if time.time() > timeout_end: @@ -180,34 +196,34 @@ class JujuModelWatcher: # the model connection is closed afterwards @staticmethod - def get_status(delta: Delta, entity_type: EntityType) -> (str, str, str): + def get_status(delta: Delta) -> (str, str, str): """ Get status from delta :param: delta: Delta generated by the allwatcher - :param: entity_type: EntityType (p.e. .APPLICATION, .MACHINE, and .ACTION) + :param: entity_type: Entity Type (p.e. "application", "machine, and "action") :return (status, message, vca_status) """ - if entity_type == EntityType.MACHINE: + if delta.entity == "machine": return ( delta.data["agent-status"]["current"], delta.data["instance-status"]["message"], delta.data["instance-status"]["current"], ) - elif entity_type == EntityType.ACTION: + elif delta.entity == "action": return ( delta.data["status"], delta.data["status"], delta.data["status"], ) - elif entity_type == EntityType.APPLICATION: + elif delta.entity == "application": return ( delta.data["status"]["current"], delta.data["status"]["message"], delta.data["status"]["current"], ) - elif entity_type == EntityType.UNIT: + elif delta.entity == "unit": return ( delta.data["workload-status"]["current"], delta.data["workload-status"]["message"], diff --git a/n2vc/libjuju.py b/n2vc/libjuju.py index c62b11c..e73e552 100644 --- a/n2vc/libjuju.py +++ b/n2vc/libjuju.py @@ -242,7 +242,9 @@ class Libjuju: """ if not model_names: raise Exception( - "model_names must be a non-empty array. Given value: {}".format(model_names) + "model_names must be a non-empty array. Given value: {}".format( + model_names + ) ) non_existing_models = [] models = await self.list_models() @@ -624,7 +626,9 @@ class Libjuju: if await u.is_leader_from_status(): unit = u if unit is None: - raise JujuLeaderUnitNotFound("Cannot execute action: leader unit not found") + raise JujuLeaderUnitNotFound( + "Cannot execute action: leader unit not found" + ) actions = await application.get_actions() @@ -703,10 +707,7 @@ class Libjuju: await self.disconnect_controller(controller) async def add_relation( - self, - model_name: str, - endpoint_1: str, - endpoint_2: str, + self, model_name: str, endpoint_1: str, endpoint_2: str, ): """Add relation @@ -863,9 +864,7 @@ class Libjuju: # wait for machine removal machines = await model.get_machines() while machine_id in machines and time.time() < end: - self.log.debug( - "Waiting for machine {} is destroyed".format(machine_id) - ) + self.log.debug("Waiting for machine {} is destroyed".format(machine_id)) await asyncio.sleep(0.5) machines = await model.get_machines() self.log.debug("Machine destroyed: {}".format(machine_id)) diff --git a/n2vc/n2vc_conn.py b/n2vc/n2vc_conn.py index b9c3002..eada20b 100644 --- a/n2vc/n2vc_conn.py +++ b/n2vc/n2vc_conn.py @@ -34,7 +34,7 @@ from osm_common.dbmongo import DbException import yaml from n2vc.loggable import Loggable -from n2vc.utils import EntityType, JujuStatusToOSM, N2VCDeploymentStatus +from n2vc.utils import JujuStatusToOSM, N2VCDeploymentStatus class N2VCConnector(abc.ABC, Loggable): @@ -492,7 +492,7 @@ class N2VCConnector(abc.ABC, Loggable): else: self.log.info("Exception writing status to database: {}".format(e)) - def osm_status(self, entity_type: EntityType, status: str) -> N2VCDeploymentStatus: + def osm_status(self, entity_type: str, status: str) -> N2VCDeploymentStatus: if status not in JujuStatusToOSM[entity_type]: self.log.warning("Status {} not found in JujuStatusToOSM.".format(status)) return N2VCDeploymentStatus.UNKNOWN diff --git a/n2vc/tests/unit/test_juju_watcher.py b/n2vc/tests/unit/test_juju_watcher.py index 56b4bbd..593ff0d 100644 --- a/n2vc/tests/unit/test_juju_watcher.py +++ b/n2vc/tests/unit/test_juju_watcher.py @@ -15,12 +15,16 @@ import asynctest import asyncio -from unittest import mock +from unittest import mock, TestCase from unittest.mock import Mock -from n2vc.juju_watcher import JujuModelWatcher -from n2vc.utils import EntityType +from n2vc.juju_watcher import JujuModelWatcher, entity_ready, status from n2vc.exceptions import EntityInvalidException from .utils import FakeN2VC, AsyncMock, Deltas, FakeWatcher +from juju.application import Application +from juju.model import Model +from juju.annotation import Annotation +from juju.machine import Machine +from juju.action import Action class JujuWatcherTest(asynctest.TestCase): @@ -32,9 +36,7 @@ class JujuWatcherTest(asynctest.TestCase): def test_get_status(self): tests = Deltas for test in tests: - (status, message, vca_status) = JujuModelWatcher.get_status( - test.delta, test.entity.type - ) + (status, message, vca_status) = JujuModelWatcher.get_status(test.delta) self.assertEqual(status, test.entity_status.status) self.assertEqual(message, test.entity_status.message) self.assertEqual(vca_status, test.entity_status.vca_status) @@ -61,22 +63,18 @@ class JujuWatcherTest(asynctest.TestCase): self.n2vc.last_written_values = None @mock.patch("n2vc.juju_watcher.asyncio.wait") - @mock.patch("n2vc.juju_watcher.EntityType.get_entity") - def test_wait_for(self, get_entity, wait): + def test_wait_for(self, wait): wait.return_value = asyncio.Future() wait.return_value.set_result(None) - get_entity.return_value = EntityType.MACHINE machine = AsyncMock() self.loop.run_until_complete(JujuModelWatcher.wait_for(self.model, machine)) @mock.patch("n2vc.juju_watcher.asyncio.wait") - @mock.patch("n2vc.juju_watcher.EntityType.get_entity") - def test_wait_for_exception(self, get_entity, wait): + def test_wait_for_exception(self, wait): wait.return_value = asyncio.Future() wait.return_value.set_result(None) wait.side_effect = Exception("error") - get_entity.return_value = EntityType.MACHINE machine = AsyncMock() with self.assertRaises(Exception): @@ -85,5 +83,60 @@ class JujuWatcherTest(asynctest.TestCase): def test_wait_for_invalid_entity_exception(self): with self.assertRaises(EntityInvalidException): self.loop.run_until_complete( - JujuModelWatcher.wait_for(self.model, AsyncMock(), total_timeout=0) + JujuModelWatcher.wait_for( + self.model, + Annotation(0, self.model), + total_timeout=None, + progress_timeout=None, + ) ) + + +class EntityReadyTest(TestCase): + @mock.patch("juju.application.Application.units") + def setUp(self, mock_units): + self.model = Model() + self.model._connector = mock.MagicMock() + + def test_invalid_entity(self): + with self.assertRaises(EntityInvalidException): + entity_ready(Annotation(0, self.model)) + + @mock.patch("juju.machine.Machine.agent_status") + def test_machine_entity(self, mock_machine_agent_status): + entity = Machine(0, self.model) + self.assertEqual(entity.entity_type, "machine") + self.assertTrue(isinstance(entity_ready(entity), bool)) + + @mock.patch("juju.action.Action.status") + def test_action_entity(self, mock_action_status): + entity = Action(0, self.model) + self.assertEqual(entity.entity_type, "action") + self.assertTrue(isinstance(entity_ready(entity), bool)) + + @mock.patch("juju.application.Application.status") + def test_application_entity(self, mock_application_status): + entity = Application(0, self.model) + self.assertEqual(entity.entity_type, "application") + self.assertTrue(isinstance(entity_ready(entity), bool)) + + +class StatusTest(TestCase): + def setUp(self): + self.model = Model() + self.model._connector = mock.MagicMock() + + @mock.patch("n2vc.juju_watcher.derive_status") + def test_invalid_entity(self, mock_derive_status): + application = mock.MagicMock() + mock_derive_status.return_value = "active" + + class FakeUnit: + @property + def workload_status(self): + return "active" + + application.units = [FakeUnit()] + value = status(application) + mock_derive_status.assert_called_once() + self.assertTrue(isinstance(value, str)) diff --git a/n2vc/tests/unit/test_utils.py b/n2vc/tests/unit/test_utils.py index 3bab705..c5ab84f 100644 --- a/n2vc/tests/unit/test_utils.py +++ b/n2vc/tests/unit/test_utils.py @@ -40,14 +40,14 @@ class UtilsTest(TestCase): def test_juju_status_to_osm(self): tests = [ { - "entity_type": EntityType.MACHINE, + "entity_type": "machine", "status": [ {"juju": "pending", "osm": N2VCDeploymentStatus.PENDING}, {"juju": "started", "osm": N2VCDeploymentStatus.COMPLETED}, ], }, { - "entity_type": EntityType.APPLICATION, + "entity_type": "application", "status": [ {"juju": "waiting", "osm": N2VCDeploymentStatus.RUNNING}, {"juju": "maintenance", "osm": N2VCDeploymentStatus.RUNNING}, @@ -57,7 +57,7 @@ class UtilsTest(TestCase): ], }, { - "entity_type": EntityType.UNIT, + "entity_type": "unit", "status": [ {"juju": "waiting", "osm": N2VCDeploymentStatus.RUNNING}, {"juju": "maintenance", "osm": N2VCDeploymentStatus.RUNNING}, @@ -67,7 +67,7 @@ class UtilsTest(TestCase): ], }, { - "entity_type": EntityType.ACTION, + "entity_type": "action", "status": [ {"juju": "running", "osm": N2VCDeploymentStatus.RUNNING}, {"juju": "completed", "osm": N2VCDeploymentStatus.COMPLETED}, diff --git a/n2vc/tests/unit/utils.py b/n2vc/tests/unit/utils.py index fe7362e..ee4dd96 100644 --- a/n2vc/tests/unit/utils.py +++ b/n2vc/tests/unit/utils.py @@ -14,7 +14,7 @@ import asyncio -from n2vc.utils import Dict, EntityType, N2VCDeploymentStatus +from n2vc.utils import Dict, N2VCDeploymentStatus from n2vc.n2vc_conn import N2VCConnector from unittest.mock import MagicMock @@ -55,7 +55,7 @@ class FakeMachine(MagicMock): entity_id = "2" dns_name = "FAKE ENDPOINT" model_name = "FAKE MODEL" - entity_type = EntityType.MACHINE + entity_type = "machine" async def destroy(self, force): pass @@ -201,8 +201,8 @@ FAKE_DELTA_ACTION_COMPLETED = Dict( Deltas = [ Dict( { - "entity": Dict({"id": "2", "type": EntityType.MACHINE}), - "filter": Dict({"entity_id": "2", "entity_type": EntityType.MACHINE}), + "entity": Dict({"id": "2", "type": "machine"}), + "filter": Dict({"entity_id": "2", "entity_type": "machine"}), "delta": FAKE_DELTA_MACHINE_PENDING, "entity_status": Dict( {"status": "pending", "message": "Running", "vca_status": "running"} @@ -224,8 +224,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "2", "type": EntityType.MACHINE}), - "filter": Dict({"entity_id": "1", "entity_type": EntityType.MACHINE}), + "entity": Dict({"id": "2", "type": "machine"}), + "filter": Dict({"entity_id": "1", "entity_type": "machine"}), "delta": FAKE_DELTA_MACHINE_PENDING, "entity_status": Dict( {"status": "pending", "message": "Running", "vca_status": "running"} @@ -235,8 +235,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "2", "type": EntityType.MACHINE}), - "filter": Dict({"entity_id": "2", "entity_type": EntityType.MACHINE}), + "entity": Dict({"id": "2", "type": "machine"}), + "filter": Dict({"entity_id": "2", "entity_type": "machine"}), "delta": FAKE_DELTA_MACHINE_STARTED, "entity_status": Dict( {"status": "started", "message": "Running", "vca_status": "running"} @@ -258,8 +258,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "2", "type": EntityType.MACHINE}), - "filter": Dict({"entity_id": "1", "entity_type": EntityType.MACHINE}), + "entity": Dict({"id": "2", "type": "machine"}), + "filter": Dict({"entity_id": "1", "entity_type": "machine"}), "delta": FAKE_DELTA_MACHINE_STARTED, "entity_status": Dict( {"status": "started", "message": "Running", "vca_status": "running"} @@ -269,8 +269,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git/0", "type": EntityType.UNIT}), - "filter": Dict({"entity_id": "git", "entity_type": EntityType.APPLICATION}), + "entity": Dict({"id": "git/0", "type": "unit"}), + "filter": Dict({"entity_id": "git", "entity_type": "application"}), "delta": FAKE_DELTA_UNIT_PENDING, "entity_status": Dict( {"status": "waiting", "message": "", "vca_status": "waiting"} @@ -292,8 +292,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git/0", "type": EntityType.UNIT}), - "filter": Dict({"entity_id": "2", "entity_type": EntityType.MACHINE}), + "entity": Dict({"id": "git/0", "type": "unit"}), + "filter": Dict({"entity_id": "2", "entity_type": "machine"}), "delta": FAKE_DELTA_UNIT_PENDING, "entity_status": Dict( {"status": "waiting", "message": "", "vca_status": "waiting"} @@ -303,8 +303,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git/0", "type": EntityType.UNIT}), - "filter": Dict({"entity_id": "git", "entity_type": EntityType.APPLICATION}), + "entity": Dict({"id": "git/0", "type": "unit"}), + "filter": Dict({"entity_id": "git", "entity_type": "application"}), "delta": FAKE_DELTA_UNIT_STARTED, "entity_status": Dict( {"status": "active", "message": "", "vca_status": "active"} @@ -326,8 +326,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git/0", "type": EntityType.UNIT}), - "filter": Dict({"entity_id": "1", "entity_type": EntityType.ACTION}), + "entity": Dict({"id": "git/0", "type": "unit"}), + "filter": Dict({"entity_id": "1", "entity_type": "action"}), "delta": FAKE_DELTA_UNIT_STARTED, "entity_status": Dict( {"status": "active", "message": "", "vca_status": "active"} @@ -337,8 +337,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git", "type": EntityType.APPLICATION}), - "filter": Dict({"entity_id": "git", "entity_type": EntityType.APPLICATION}), + "entity": Dict({"id": "git", "type": "application"}), + "filter": Dict({"entity_id": "git", "entity_type": "application"}), "delta": FAKE_DELTA_APPLICATION_MAINTENANCE, "entity_status": Dict( { @@ -364,8 +364,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git", "type": EntityType.APPLICATION}), - "filter": Dict({"entity_id": "2", "entity_type": EntityType.MACHINE}), + "entity": Dict({"id": "git", "type": "application"}), + "filter": Dict({"entity_id": "2", "entity_type": "machine"}), "delta": FAKE_DELTA_APPLICATION_MAINTENANCE, "entity_status": Dict( { @@ -379,8 +379,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git", "type": EntityType.APPLICATION}), - "filter": Dict({"entity_id": "git", "entity_type": EntityType.APPLICATION}), + "entity": Dict({"id": "git", "type": "application"}), + "filter": Dict({"entity_id": "git", "entity_type": "application"}), "delta": FAKE_DELTA_APPLICATION_ACTIVE, "entity_status": Dict( {"status": "active", "message": "Ready!", "vca_status": "active"} @@ -402,8 +402,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git", "type": EntityType.APPLICATION}), - "filter": Dict({"entity_id": "1", "entity_type": EntityType.ACTION}), + "entity": Dict({"id": "git", "type": "application"}), + "filter": Dict({"entity_id": "1", "entity_type": "action"}), "delta": FAKE_DELTA_APPLICATION_ACTIVE, "entity_status": Dict( {"status": "active", "message": "Ready!", "vca_status": "active"} @@ -413,8 +413,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "1", "type": EntityType.ACTION}), - "filter": Dict({"entity_id": "1", "entity_type": EntityType.ACTION}), + "entity": Dict({"id": "1", "type": "action"}), + "filter": Dict({"entity_id": "1", "entity_type": "action"}), "delta": FAKE_DELTA_ACTION_COMPLETED, "entity_status": Dict( { @@ -440,8 +440,8 @@ Deltas = [ ), Dict( { - "entity": Dict({"id": "git", "type": EntityType.ACTION}), - "filter": Dict({"entity_id": "1", "entity_type": EntityType.MACHINE}), + "entity": Dict({"id": "git", "type": "action"}), + "filter": Dict({"entity_id": "1", "entity_type": "machine"}), "delta": FAKE_DELTA_ACTION_COMPLETED, "entity_status": Dict( { diff --git a/n2vc/utils.py b/n2vc/utils.py index 62892eb..e8cf64d 100644 --- a/n2vc/utils.py +++ b/n2vc/utils.py @@ -67,36 +67,24 @@ class EntityType(Enum): return cls.get_entity(v) -FinalStatus = Dict( - { - EntityType.MACHINE: Dict({"field": "agent_status", "status": ["started"]}), - EntityType.APPLICATION: Dict( - {"field": "status", "status": ["active", "blocked"]} - ), - EntityType.ACTION: Dict( - {"field": "status", "status": ["completed", "failed", "cancelled"]} - ), - } -) - JujuStatusToOSM = { - EntityType.MACHINE: { + "machine": { "pending": N2VCDeploymentStatus.PENDING, "started": N2VCDeploymentStatus.COMPLETED, }, - EntityType.APPLICATION: { + "application": { "waiting": N2VCDeploymentStatus.RUNNING, "maintenance": N2VCDeploymentStatus.RUNNING, "blocked": N2VCDeploymentStatus.RUNNING, "error": N2VCDeploymentStatus.FAILED, "active": N2VCDeploymentStatus.COMPLETED, }, - EntityType.ACTION: { + "action": { "pending": N2VCDeploymentStatus.PENDING, "running": N2VCDeploymentStatus.RUNNING, "completed": N2VCDeploymentStatus.COMPLETED, }, - EntityType.UNIT: { + "unit": { "waiting": N2VCDeploymentStatus.RUNNING, "maintenance": N2VCDeploymentStatus.RUNNING, "blocked": N2VCDeploymentStatus.RUNNING, -- 2.17.1