Repository: MatterMiners/tardis

Issue:
Add configurable drone lifetime
To allow for a more flexible response to demands in the local batch system, it would be nice to add a configurable drone lifetime. The idea is that after the drone lifetime has been reached, the drone is drained such that the resources are released more quickly and in a smooth way. For this to work properly requires a fix for https://github.com/MatterMiners/tardis/issues/85.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/MatterMiners/tardis/pull/116?src=pr&el=h1) Report
> Merging [#116](https://codecov.io/gh/MatterMiners/tardis/pull/116?src=pr&el=desc) into [master](https://codecov.io/gh/MatterMiners/tardis/commit/7d1752e34487c3ee9274301f12c3778cfd512807?src=pr&el=desc) will **increase** coverage by `0.22%`.
> The diff coverage is `94.64%`.

[![Impacted file tree graph](https://codecov.io/gh/MatterMiners/tardis/pull/116/graphs/tree.svg?width=650&token=BKeuDMxoaj&height=150&src=pr)](https://codecov.io/gh/MatterMiners/tardis/pull/116?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   94.84%   95.06%   +0.22%     
==========================================
  Files          39       39              
  Lines        1454     1419      -35     
==========================================
- Hits         1379     1349      -30     
+ Misses         75       70       -5
```


| [Impacted Files](https://codecov.io/gh/MatterMiners/tardis/pull/116?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tardis/adapters/sites/slurm.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2FkYXB0ZXJzL3NpdGVzL3NsdXJtLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [tardis/adapters/sites/fakesite.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2FkYXB0ZXJzL3NpdGVzL2Zha2VzaXRlLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [tardis/adapters/sites/cloudstack.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2FkYXB0ZXJzL3NpdGVzL2Nsb3Vkc3RhY2sucHk=) | `100% <100%> (ø)` | :arrow_up: |
| [tardis/adapters/sites/moab.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2FkYXB0ZXJzL3NpdGVzL21vYWIucHk=) | `100% <100%> (ø)` | :arrow_up: |
| [tardis/adapters/sites/openstack.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2FkYXB0ZXJzL3NpdGVzL29wZW5zdGFjay5weQ==) | `100% <100%> (ø)` | :arrow_up: |
| [tardis/interfaces/siteadapter.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2ludGVyZmFjZXMvc2l0ZWFkYXB0ZXIucHk=) | `100% <100%> (+17.39%)` | :arrow_up: |
| [tardis/adapters/sites/htcondor.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2FkYXB0ZXJzL3NpdGVzL2h0Y29uZG9yLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [tardis/resources/drone.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL3Jlc291cmNlcy9kcm9uZS5weQ==) | `54.54% <66.66%> (+0.49%)` | :arrow_up: |
| [tardis/agents/siteagent.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL2FnZW50cy9zaXRlYWdlbnQucHk=) | `97.29% <66.66%> (-2.71%)` | :arrow_down: |
| [tardis/resources/dronestates.py](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree#diff-dGFyZGlzL3Jlc291cmNlcy9kcm9uZXN0YXRlcy5weQ==) | `98.64% <83.33%> (-0.66%)` | :arrow_down: |
| ... and [8 more](https://codecov.io/gh/MatterMiners/tardis/pull/116/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/MatterMiners/tardis/pull/116?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/MatterMiners/tardis/pull/116?src=pr&el=footer). Last update [7d1752e...9c7fde1](https://codecov.io/gh/MatterMiners/tardis/pull/116?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/docs/source/adapters/site.rst b/docs/source/adapters/site.rst
index 2cda19b..c1a12c5 100644
--- a/docs/source/adapters/site.rst
+++ b/docs/source/adapters/site.rst
@@ -10,8 +10,15 @@ Site Adapter
     of resources and a dynamic orchestration of pre-built VM images and containers.
 
     Sites are generally configured in the `Sites` configuration block. One has to specify a site name, the adapter to use
-    and a site quota in units of cores. Negative values for the site quota are interpreted as infinity. Multiple sites are
-    supported by using SequenceNodes.
+    and a site quota in units of cores. Negative values for the site quota are interpreted as infinity. Optionally a
+    minimum lifetime in seconds of the :py:class:`~tardis.resources.drone.Drone` can be specified. This is defined as
+    the time the :py:class:`~tardis.resources.drone.Drone` remains in
+    :py:class:`~tardis.resources.dronestates.AvailableState` before draining it. If no value is given, infinite lifetime
+    is assumed. Multiple sites are supported by using SequenceNodes.
+
+.. note::
+    Even a minimum lifetime is set, it is not guaranteed that the :py:class:`~tardis.resources.drone.Drone` is not
+    drained due to a dropping demand for it before its minimum lifetime is exceeded.
 
 
 Generic Site Adapter Configuration
@@ -24,15 +31,17 @@ Available configuration options
 
 .. container:: left-col
 
-    +---------+----------------------------------------------------------------------------------+-----------------+
-    | Option  | Short Description                                                                | Requirement     |
-    +=========+==================================================================================+=================+
-    | name    | Name of the site                                                                 |  **Required**   |
-    +---------+----------------------------------------------------------------------------------+-----------------+
-    | adapter | Site adapter to use. Adapter will be auto-imported (class name without Adapter)  |  **Required**   |
-    +---------+----------------------------------------------------------------------------------+-----------------+
-    | quota   | Core quota to be used for this site. Negative values are interpreted as infinity |  **Required**   |
-    +---------+----------------------------------------------------------------------------------+-----------------+
+    +------------------------+-----------------------------------------------------------------------------------------------------------------------+---------------+
+    | Option                 | Short Description                                                                                                     |  Requirement  |
+    +========================+=======================================================================================================================+===============+
+    | name                   | Name of the site                                                                                                      |  **Required** |
+    +------------------------+-----------------------------------------------------------------------------------------------------------------------+---------------+
+    | adapter                | Site adapter to use. Adapter will be auto-imported (class name without Adapter)                                       |  **Required** |
+    +------------------------+-----------------------------------------------------------------------------------------------------------------------+---------------+
+    | quota                  | Core quota to be used for this site. Negative values are interpreted as infinity                                      |  **Required** |
+    +------------------------+-----------------------------------------------------------------------------------------------------------------------+---------------+
+    | drone_minimum_lifetime | Time in seconds the drone will remain in :py:class:`~tardis.resources.dronestates.AvailableState` before draining it. |  **Optional** |
+    +------------------------+-----------------------------------------------------------------------------------------------------------------------+---------------+
 
     For each site in the `Sites` configuration block. A site specific configuration block carrying the site name
     has to be added to the configuration as well.
@@ -58,6 +67,7 @@ Available configuration options
           - name: MySiteName_1
             adapter: MyAdapter2Use
             quota: 123
+            drone_minimum_lifetime: 3600
           - name: MySiteName_2
             adapter: OtherAdapter2Use
             quota: 987
@@ -421,4 +431,4 @@ Available adapter configuration options
 
     Your favorite site is currently not supported?
     Please, have a look at
-    :ref:`how to contribute.<ref_contribute_site_adapter>`
\ No newline at end of file
+    :ref:`how to contribute.<ref_contribute_site_adapter>`
diff --git a/tardis/agents/siteagent.py b/tardis/agents/siteagent.py
index be69cd8..ced8978 100644
--- a/tardis/agents/siteagent.py
+++ b/tardis/agents/siteagent.py
@@ -31,6 +31,10 @@ class SiteAgent(SiteAdapter):
     ):
         return NotImplemented
 
+    @property
+    def drone_minimum_lifetime(self) -> int:
+        return self._site_adapter.drone_minimum_lifetime
+
     @property
     def machine_meta_data(self) -> AttributeDict:
         return self._site_adapter.machine_meta_data
diff --git a/tardis/interfaces/siteadapter.py b/tardis/interfaces/siteadapter.py
index a8151bd..ff1031b 100644
--- a/tardis/interfaces/siteadapter.py
+++ b/tardis/interfaces/siteadapter.py
@@ -113,6 +113,13 @@ class SiteAdapter(metaclass=ABCMeta):
 
         return translated_response
 
+    @property
+    def drone_minimum_lifetime(self) -> [int, None]:
+        try:
+            return self.configuration.drone_minimum_lifetime
+        except AttributeError:
+            return None
+
     @property
     def machine_meta_data(self) -> AttributeDict:
         """
diff --git a/tardis/resources/drone.py b/tardis/resources/drone.py
index 15aaef5..8fa452d 100644
--- a/tardis/resources/drone.py
+++ b/tardis/resources/drone.py
@@ -65,6 +65,10 @@ class Drone(Pool):
     def demand(self, value: float):
         self._demand = value
 
+    @property
+    def drone_minimum_lifetime(self) -> [int, None]:
+        return self.site_agent.drone_minimum_lifetime
+
     @property
     def maximum_demand(self) -> float:
         return self.site_agent.machine_meta_data["Cores"]
@@ -83,14 +87,15 @@ class Drone(Pool):
 
     async def run(self):
         while True:
-            await self.state.run(self)
-            await asyncio.sleep(60)
-            if isinstance(self.state, DownState):
+            current_state = self.state
+            await current_state.run(self)
+            if isinstance(current_state, DownState):
                 logging.debug(
                     f"Garbage Collect Drone: {self.resource_attributes.drone_uuid}"
                 )
                 self._demand = 0
                 return
+            await asyncio.sleep(60)
 
     def register_plugins(self, observer: Union[List[Plugin], Plugin]) -> None:
         self._plugins.append(observer)
diff --git a/tardis/resources/dronestates.py b/tardis/resources/dronestates.py
index b752335..9a39d56 100644
--- a/tardis/resources/dronestates.py
+++ b/tardis/resources/dronestates.py
@@ -1,4 +1,5 @@
 from collections import defaultdict
+from datetime import datetime
 import asyncio
 import logging
 
@@ -38,6 +39,18 @@ async def check_demand(state_transition, drone: "Drone", current_state: Type[Sta
     return state_transition
 
 
+async def check_minimum_lifetime(
+    state_transition, drone: "Drone", current_state: Type[State]
+):
+    if (
+        drone.drone_minimum_lifetime
+        and (datetime.now() - drone.resource_attributes.updated).total_seconds()
+        > drone.drone_minimum_lifetime
+    ):
+        raise StopProcessing(last_result=DrainState())
+    return state_transition
+
+
 async def resource_status(state_transition, drone: "Drone", current_state: Type[State]):
     try:
         drone.resource_attributes.update(
@@ -138,7 +151,12 @@ class AvailableState(State):
         ResourceStatus.Error: lambda: defaultdict(lambda: CleanupState),
     }
 
-    processing_pipeline = [check_demand, resource_status, batchsystem_machine_status]
+    processing_pipeline = [
+        check_demand,
+        check_minimum_lifetime,
+        resource_status,
+        batchsystem_machine_status,
+    ]
 
     @classmethod
     async def run(cls, drone: "Drone"):


Test patch:
diff --git a/tests/agents_t/test_siteagent.py b/tests/agents_t/test_siteagent.py
index 0dcbe4a..4640b3f 100644
--- a/tests/agents_t/test_siteagent.py
+++ b/tests/agents_t/test_siteagent.py
@@ -23,6 +23,11 @@ class TestSiteAgent(TestCase):
         self.site_agent.drone_uuid(uuid="test")
         self.site_adapter.drone_uuid.assert_called_with(uuid="test")
 
+    def test_drone_minimum_lifetime(self):
+        self.site_adapter.drone_minimum_lifetime.return_value = None
+        self.assertIsNone(self.site_agent.drone_minimum_lifetime())
+        self.site_adapter.drone_minimum_lifetime.assert_called_with()
+
     def test_handle_exceptions(self):
         self.site_adapter.handle_exceptions.return_value = None
         self.site_agent.handle_exceptions()
diff --git a/tests/interfaces_t/test_siteadapter.py b/tests/interfaces_t/test_siteadapter.py
index 663342c..a9c64ea 100644
--- a/tests/interfaces_t/test_siteadapter.py
+++ b/tests/interfaces_t/test_siteadapter.py
@@ -78,6 +78,12 @@ class TestSiteAdapter(TestCase):
         self.site_adapter._machine_type = "TestFlavour"
         self.assertEqual(self.site_adapter.machine_meta_data, "Test")
 
+    def test_drone_minimum_lifetime(self):
+        self.assertEqual(self.site_adapter.drone_minimum_lifetime, None)
+
+        self.site_adapter._configuration = AttributeDict(drone_minimum_lifetime=10)
+        self.assertEqual(self.site_adapter.drone_minimum_lifetime, 10)
+
     def test_machine_type(self):
         with self.assertRaises(AttributeError):
             self.site_adapter.machine_type
diff --git a/tests/resources_t/test_drone.py b/tests/resources_t/test_drone.py
index 82fcfb1..632fb59 100644
--- a/tests/resources_t/test_drone.py
+++ b/tests/resources_t/test_drone.py
@@ -1,7 +1,139 @@
+from ..utilities.utilities import async_return, run_async
+
+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.utilities.attributedict import AttributeDict
 
+from logging import DEBUG
 from unittest import TestCase
+from unittest.mock import MagicMock, patch
 
 
 class TestDrone(TestCase):
-    pass
+    mock_batch_system_agent_patcher = None
+    mock_site_agent_patcher = None
+
+    @classmethod
+    def setUpClass(cls) -> None:
+        cls.mock_site_agent_patcher = patch("tardis.agents.siteagent.SiteAgent")
+        cls.mock_site_agent = cls.mock_site_agent_patcher.start()
+
+        cls.mock_batch_system_agent_patcher = patch(
+            "tardis.agents.batchsystemagent.BatchSystemAgent"
+        )
+        cls.mock_batch_system_agent = cls.mock_batch_system_agent_patcher.start()
+
+    @classmethod
+    def tearDownClass(cls) -> None:
+        cls.mock_batch_system_agent_patcher.stop()
+        cls.mock_site_agent_patcher.stop()
+
+    def setUp(self) -> None:
+        self.mock_site_agent.machine_meta_data = AttributeDict(Cores=8)
+        self.mock_site_agent.drone_minimum_lifetime = None
+        self.mock_plugin = MagicMock(spec=Plugin)()
+        self.mock_plugin.notify.return_value = async_return()
+        self.drone = Drone(
+            site_agent=self.mock_site_agent,
+            batch_system_agent=self.mock_batch_system_agent,
+        )
+
+    def test_allocation(self):
+        self.assertEqual(self.drone.allocation, 0.0)
+        self.drone._allocation = 1.0
+        self.assertEqual(self.drone.allocation, 1.0)
+
+    def test_batch_system_agent(self):
+        self.assertEqual(self.drone.batch_system_agent, self.mock_batch_system_agent)
+
+    def test_demand(self):
+        self.assertEqual(self.drone.demand, 8)
+        self.drone.demand = 0
+        self.assertEqual(self.drone.demand, 0)
+
+    def test_life_time(self):
+        self.assertIsNone(self.drone.drone_minimum_lifetime, None)
+        self.mock_site_agent.drone_minimum_lifetime = 3600
+        self.assertEqual(self.drone.drone_minimum_lifetime, 3600)
+
+    def test_maximum_demand(self):
+        self.assertEqual(self.drone.maximum_demand, 8)
+        self.mock_site_agent.machine_meta_data = AttributeDict(Cores=4)
+        self.assertEqual(self.drone.maximum_demand, 4)
+
+    def test_supply(self):
+        self.assertEqual(self.drone.supply, 0.0)
+        self.drone._supply = 8.0
+        self.assertEqual(self.drone.supply, 8.0)
+
+    def test_utilisation(self):
+        self.assertEqual(self.drone.utilisation, 0.0)
+        self.drone._utilisation = 8.0
+        self.assertEqual(self.drone.utilisation, 8.0)
+
+    def test_site_agent(self):
+        self.assertEqual(self.drone.site_agent, self.mock_site_agent)
+
+    @patch("tardis.resources.drone.asyncio.sleep", async_return)
+    def test_run(self):
+        mocked_down_state = MagicMock(spec=DownState)
+        mocked_down_state.run.return_value = async_return()
+
+        async def mocked_run(drone):
+            await drone.set_state(mocked_down_state)
+
+        mocked_state = MagicMock(spec=State)
+        mocked_state.run.side_effect = mocked_run
+
+        run_async(self.drone.set_state, mocked_state)
+        self.drone.demand = 8
+        with self.assertLogs(level=DEBUG):
+            run_async(self.drone.run)
+
+        self.assertIsInstance(self.drone.state, DownState)
+        self.assertEqual(self.drone.demand, 0)
+
+        mocked_state.run.assert_called_once()
+        mocked_down_state.run.assert_called_once()
+
+    def test_register_plugins(self):
+        self.assertEqual(self.drone._plugins, [])
+        self.drone.register_plugins(self.mock_plugin)
+        self.assertEqual(self.drone._plugins, [self.mock_plugin])
+
+    def test_removal_plugins(self):
+        self.drone.register_plugins(self.mock_plugin)
+        self.assertEqual(self.drone._plugins, [self.mock_plugin])
+        self.drone.remove_plugins(self.mock_plugin)
+        self.assertEqual(self.drone._plugins, [])
+
+    def test_set_state(self):
+        self.drone.register_plugins(self.mock_plugin)
+        old_update_time_stamp = self.drone.resource_attributes.updated
+        run_async(self.drone.set_state, self.drone._state)
+        self.mock_plugin.notify.assert_not_called()
+        self.assertEqual(self.drone.resource_attributes.updated, old_update_time_stamp)
+
+        new_state = DownState()
+        run_async(self.drone.set_state, new_state)
+        self.mock_plugin.notify.assert_called_with(
+            new_state, self.drone.resource_attributes
+        )
+        self.assertNotEqual(
+            self.drone.resource_attributes.updated, old_update_time_stamp
+        )
+
+    def test_state(self):
+        self.assertEqual(self.drone.state, self.drone._state)
+        self.assertIsInstance(self.drone.state, RequestState)
+
+    def test_notify_plugins(self):
+        self.drone.register_plugins(self.mock_plugin)
+        self.assertEqual(self.drone._plugins, [self.mock_plugin])
+
+        run_async(self.drone.notify_plugins)
+        self.mock_plugin.notify.assert_called_with(
+            self.drone.state, self.drone.resource_attributes
+        )
diff --git a/tests/resources_t/test_dronestates.py b/tests/resources_t/test_dronestates.py
index 2afb777..13506bd 100644
--- a/tests/resources_t/test_dronestates.py
+++ b/tests/resources_t/test_dronestates.py
@@ -22,6 +22,7 @@ from ..utilities.utilities import async_return
 from ..utilities.utilities import run_async
 
 from functools import partial
+from datetime import datetime, timedelta
 from unittest import TestCase
 from unittest.mock import patch
 
@@ -48,10 +49,13 @@ class TestDroneStates(TestCase):
 
         self.drone = self.mock_drone.return_value
         self.drone.resource_attributes = AttributeDict(
-            drone_uuid="test-923ABF", remote_resource_uuid="0815"
+            drone_uuid="test-923ABF",
+            remote_resource_uuid="0815",
+            updated=datetime.now() - timedelta(minutes=10),
         )
         self.drone.demand = 8.0
         self.drone._supply = 8.0
+        self.drone.drone_minimum_lifetime = 3600
         self.drone.set_state.side_effect = partial(mock_set_state, self.drone)
         self.drone.site_agent.deploy_resource.return_value = async_return(
             return_value=AttributeDict()
@@ -212,6 +216,12 @@ class TestDroneStates(TestCase):
         self.assertIsInstance(self.drone.state, DrainState)
         self.assertEqual(self.drone._supply, 0.0)
 
+        # Test draining procedure due to exceeding life time of the drone
+        self.drone.drone_minimum_lifetime = 1
+        self.drone.state.return_value = AvailableState()
+        run_async(self.drone.state.return_value.run, self.drone)
+        self.assertIsInstance(self.drone.state, DrainState)
+
     def test_drain_state(self):
         self.drone.state.return_value = DrainState
         run_async(self.drone.state.return_value.run, self.drone)