From 254c7beeb14892ae138618ae898409df0684609a Mon Sep 17 00:00:00 2001 From: Manuel Giffels Date: Wed, 11 May 2022 12:44:01 +0200 Subject: [PATCH 1/3] Change drone state initialisation and notification of plugins --- docs/source/changelog.rst | 4 ++-- tardis/plugins/sqliteregistry.py | 2 +- tardis/resources/drone.py | 8 +++++++- tardis/resources/poolfactory.py | 3 +-- tests/plugins_t/test_sqliteregistry.py | 14 +++++++------- tests/resources_t/test_drone.py | 4 ++-- tests/resources_t/test_poolfactory.py | 14 ++++++-------- 7 files changed, 26 insertions(+), 23 deletions(-) diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 44a08a8d..eb0f09f9 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -1,4 +1,4 @@ -.. Created by changelog.py at 2022-05-06, command +.. Created by changelog.py at 2022-05-11, command '/Users/giffler/.cache/pre-commit/repor6pnmwlm/py_env-default/bin/changelog docs/source/changes compile --output=docs/source/changelog.rst' based on the format of 'https://keepachangelog.com/' @@ -6,7 +6,7 @@ CHANGELOG ######### -[Unreleased] - 2022-05-06 +[Unreleased] - 2022-05-11 ========================= Added diff --git a/tardis/plugins/sqliteregistry.py b/tardis/plugins/sqliteregistry.py index b088f1e1..7e8d2a37 100644 --- a/tardis/plugins/sqliteregistry.py +++ b/tardis/plugins/sqliteregistry.py @@ -27,7 +27,7 @@ def __init__(self): self._db_file = configuration.Plugins.SqliteRegistry.db_file self._deploy_db_schema() self._dispatch_on_state = dict( - BootingState=self.insert_resource, DownState=self.delete_resource + RequestState=self.insert_resource, DownState=self.delete_resource ) for site in configuration.Sites: diff --git a/tardis/resources/drone.py b/tardis/resources/drone.py index 95cd783c..1bdac186 100644 --- a/tardis/resources/drone.py +++ b/tardis/resources/drone.py @@ -28,7 +28,7 @@ def __init__( plugins: Optional[List[Plugin]] = None, remote_resource_uuid=None, drone_uuid=None, - state: RequestState = RequestState(), + state: Optional[State] = None, created: float = None, updated: float = None, ): @@ -93,6 +93,12 @@ def site_agent(self) -> SiteAgent: return self._site_agent async def run(self): + if not self.state: + # The state of a newly created Drone is None, since the plugins need + # to be notified on the first state change. As calling the + # ``set_state`` coroutine is not possible in the constructor, we + # initiate the first state change here + await self.set_state(RequestState()) while True: current_state = self.state await current_state.run(self) diff --git a/tardis/resources/poolfactory.py b/tardis/resources/poolfactory.py index 06946a87..d275dda8 100644 --- a/tardis/resources/poolfactory.py +++ b/tardis/resources/poolfactory.py @@ -6,7 +6,6 @@ from ..agents.siteagent import SiteAgent from ..configuration.configuration import Configuration from ..resources.drone import Drone -from ..resources.dronestates import RequestState from cobald.composite.weighted import WeightedComposite from cobald.composite.factory import FactoryPool @@ -100,7 +99,7 @@ def create_drone( plugins: Optional[Iterable[Plugin]] = None, remote_resource_uuid=None, drone_uuid=None, - state: State = RequestState(), + state: Optional[State] = None, created: float = None, updated: float = None, ): diff --git a/tests/plugins_t/test_sqliteregistry.py b/tests/plugins_t/test_sqliteregistry.py index 0a0d4d47..9649e701 100644 --- a/tests/plugins_t/test_sqliteregistry.py +++ b/tests/plugins_t/test_sqliteregistry.py @@ -2,7 +2,7 @@ from tardis.resources.dronestates import BootingState from tardis.resources.dronestates import IntegrateState -from tardis.resources.dronestates import DownState +from tardis.resources.dronestates import RequestState, DownState from tardis.interfaces.state import State from tardis.plugins.sqliteregistry import SqliteRegistry from tardis.utilities.attributedict import AttributeDict @@ -48,7 +48,7 @@ def setUpClass(cls): "remote_resource_uuid" ], "drone_uuid": cls.test_resource_attributes["drone_uuid"], - "state": str(BootingState()), + "state": str(RequestState()), "created": cls.test_resource_attributes["created"], "updated": cls.test_resource_attributes["updated"], } @@ -56,7 +56,7 @@ def setUpClass(cls): cls.test_notify_result = ( cls.test_resource_attributes["remote_resource_uuid"], cls.test_resource_attributes["drone_uuid"], - str(BootingState()), + str(RequestState()), cls.test_resource_attributes["site_name"], cls.test_resource_attributes["machine_type"], str(cls.test_resource_attributes["created"]), @@ -182,7 +182,7 @@ def test_double_schema_deployment(self): def test_get_resources(self): self.registry.add_site(self.test_site_name) self.registry.add_machine_types(self.test_site_name, self.test_machine_type) - run_async(self.registry.notify, BootingState(), self.test_resource_attributes) + run_async(self.registry.notify, RequestState(), self.test_resource_attributes) self.assertListEqual( self.registry.get_resources( @@ -208,13 +208,13 @@ def fetch_all(): self.registry.add_site(self.test_site_name) self.registry.add_machine_types(self.test_site_name, self.test_machine_type) - run_async(self.registry.notify, BootingState(), self.test_resource_attributes) + run_async(self.registry.notify, RequestState(), self.test_resource_attributes) self.assertEqual([self.test_notify_result], fetch_all()) with self.assertRaises(sqlite3.IntegrityError) as ie: run_async( - self.registry.notify, BootingState(), self.test_resource_attributes + self.registry.notify, RequestState(), self.test_resource_attributes ) self.assertTrue("unique" in str(ie.exception).lower()) @@ -250,7 +250,7 @@ def fetch_all(): self.registry.add_site(site_name) self.registry.add_machine_types(site_name, self.test_machine_type) - bind_parameters = {"state": "BootingState"} + bind_parameters = {"state": "RequestState"} bind_parameters.update(self.test_resource_attributes) run_async(self.registry.insert_resource, bind_parameters) diff --git a/tests/resources_t/test_drone.py b/tests/resources_t/test_drone.py index f92bbe7e..66a66367 100644 --- a/tests/resources_t/test_drone.py +++ b/tests/resources_t/test_drone.py @@ -3,7 +3,7 @@ from tardis.interfaces.plugin import Plugin from tardis.interfaces.state import State from tardis.resources.drone import Drone -from tardis.resources.dronestates import RequestState, DownState +from tardis.resources.dronestates import DownState from tardis.utilities.attributedict import AttributeDict from logging import DEBUG @@ -140,7 +140,7 @@ def test_set_state(self): def test_state(self): self.assertEqual(self.drone.state, self.drone._state) - self.assertIsInstance(self.drone.state, RequestState) + self.assertIsNone(self.drone.state) def test_notify_plugins(self): self.drone.register_plugins(self.mock_plugin) diff --git a/tests/resources_t/test_poolfactory.py b/tests/resources_t/test_poolfactory.py index f9cd10ea..3b5d64be 100644 --- a/tests/resources_t/test_poolfactory.py +++ b/tests/resources_t/test_poolfactory.py @@ -126,14 +126,12 @@ def test_create_composite( @patch("tardis.resources.poolfactory.BatchSystemAgent") @patch("tardis.resources.poolfactory.SiteAgent") def test_create_drone(self, mock_site_agent, mock_batch_system_agent, mock_drone): - self.assertEqual( - create_drone( - site_agent=mock_site_agent, batch_system_agent=mock_batch_system_agent - ), - mock_drone(), + create_drone( + site_agent=mock_site_agent, batch_system_agent=mock_batch_system_agent ) - mock_drone.has_call( + self.assertListEqual( + mock_drone.mock_calls, [ call( site_agent=mock_site_agent, @@ -141,11 +139,11 @@ def test_create_drone(self, mock_site_agent, mock_batch_system_agent, mock_drone plugins=None, remote_resource_uuid=None, drone_uuid=None, - state=RequestState(), + state=None, created=None, updated=None, ) - ] + ], ) def test_load_plugins(self): From b0960c518045153eccdf6ddd87eb36cbb9aed0b0 Mon Sep 17 00:00:00 2001 From: Manuel Giffels Date: Wed, 11 May 2022 14:58:25 +0200 Subject: [PATCH 2/3] Added changelog --- docs/source/changelog.rst | 1 + .../changes/247.change_drone_state_initialisation.yaml | 10 ++++++++++ 2 files changed, 11 insertions(+) create mode 100644 docs/source/changes/247.change_drone_state_initialisation.yaml diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index eb0f09f9..695e517f 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -19,6 +19,7 @@ Changed ------- * SSHExecutor respects the remote MaxSessions via queueing +* Change drone state initialisation and notification of plugins Fixed ----- diff --git a/docs/source/changes/247.change_drone_state_initialisation.yaml b/docs/source/changes/247.change_drone_state_initialisation.yaml new file mode 100644 index 00000000..e604dc52 --- /dev/null +++ b/docs/source/changes/247.change_drone_state_initialisation.yaml @@ -0,0 +1,10 @@ +category: changed +summary: "Change drone state initialisation and notification of plugins" +description: | + The initialisation procedure and the notification of the plugins is changed to fix a bug occurring on restarts of + Drones. A newly created Drone is now initialised with ``state = None`` and all plugins are notified first state + change ``None`` -> ``RequestState``. The Drone is now inserted in the `SqliteRegistry` when it state changes to + ``RequestState`` and all subsequent changes are DB updates. So, failing duplicated inserts due to the unique + requirement of the ``drone_uuid`` are prevented in case a Drone changes back to ``BootingState`` again. +pull_requests: + - 247 From 92937130d5f24147dcedf7753ff9755ba90361d7 Mon Sep 17 00:00:00 2001 From: Manuel Giffels Date: Fri, 13 May 2022 17:26:48 +0200 Subject: [PATCH 3/3] Explicitly check for state None Co-authored-by: Max Fischer --- tardis/resources/drone.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tardis/resources/drone.py b/tardis/resources/drone.py index 1bdac186..9db204b1 100644 --- a/tardis/resources/drone.py +++ b/tardis/resources/drone.py @@ -93,7 +93,7 @@ def site_agent(self) -> SiteAgent: return self._site_agent async def run(self): - if not self.state: + if self.state is None: # The state of a newly created Drone is None, since the plugins need # to be notified on the first state change. As calling the # ``set_state`` coroutine is not possible in the constructor, we