Add support for cron expressions for autostart_agent_deploy_interval …

…and autostart_agent_repair_interval environment settings. (Issue #6549, PR #6574)

closes #6549

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [ ] Changelog entry
- [ ] Type annotations are present
- [ ] Code is clear and sufficiently documented
- [ ] No (preventable) type errors (check using make mypy or make mypy-diff)
- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
- [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)

changelogs/unreleased/6549-fix-issues-with-deploy-or-repair-interval-cron-expressions.yml

@@ -0,0 +1,6 @@
+description: Add support for cron expressions for autostart_agent_deploy_interval and autostart_agent_repair_interval environment settings.
+issue-nr: 6549
+change-type: minor
+destination-branches: [master, iso6]
+sections:
+  minor-improvement: "{{description}}"

src/inmanta/agent/agent.py

@@ -29,7 +29,7 @@
 from collections import defaultdict
 from concurrent.futures.thread import ThreadPoolExecutor
 from logging import Logger
-from typing import Any, Dict, Iterable, List, Optional, Sequence, Set, Tuple, Union, cast
+from typing import Any, Awaitable, Callable, Dict, Iterable, List, Optional, Sequence, Set, Tuple, Union, cast
 
 from inmanta import const, data, env, protocol
 from inmanta.agent import config as cfg
@@ -725,36 +725,34 @@ async def repair_action() -> None:
                 )
             )
 
-        now = datetime.datetime.now().astimezone()
-        if self._deploy_interval > 0:
-            self.logger.info(
-                "Scheduling periodic deploy with interval %d and splay %d (first run at %s)",
-                self._deploy_interval,
-                self._deploy_splay_value,
-                (now + datetime.timedelta(seconds=self._deploy_splay_value)).strftime(const.TIME_LOGFMT),
-            )
-            interval_schedule_deploy: IntervalSchedule = IntervalSchedule(
-                interval=float(self._deploy_interval), initial_delay=float(self._deploy_splay_value)
-            )
-            self._enable_time_trigger(deploy_action, interval_schedule_deploy)
-        if isinstance(self._repair_interval, int):
-            if self._repair_interval <= 0:
-                return
-            self.logger.info(
-                "Scheduling repair with interval %d and splay %d (first run at %s)",
-                self._repair_interval,
-                self._repair_splay_value,
-                (now + datetime.timedelta(seconds=self._repair_splay_value)).strftime(const.TIME_LOGFMT),
-            )
-            interval_schedule_repair: IntervalSchedule = IntervalSchedule(
-                interval=float(self._repair_interval), initial_delay=float(self._repair_splay_value)
-            )
-            self._enable_time_trigger(repair_action, interval_schedule_repair)
+        def periodic_schedule(
+            kind: str,
+            action: Callable[[], Awaitable[object]],
+            interval: Union[int, str],
+            splay_value: int,
+            initial_time: datetime.datetime,
+        ) -> None:
+            if isinstance(interval, int) and interval > 0:
+                self.logger.info(
+                    "Scheduling periodic %s with interval %d and splay %d (first run at %s)",
+                    kind,
+                    interval,
+                    splay_value,
+                    (initial_time + datetime.timedelta(seconds=splay_value)).strftime(const.TIME_LOGFMT),
+                )
+                interval_schedule: IntervalSchedule = IntervalSchedule(
+                    interval=float(interval), initial_delay=float(splay_value)
+                )
+                self._enable_time_trigger(action, interval_schedule)
+
+            if isinstance(interval, str):
+                self.logger.info("Scheduling periodic %s with cron expression '%s'", kind, interval)
+                cron_schedule = CronSchedule(cron=interval)
+                self._enable_time_trigger(action, cron_schedule)
 
-        if isinstance(self._repair_interval, str):
-            self.logger.info("Scheduling repair with cron expression '%s'", self._repair_interval)
-            cron_schedule = CronSchedule(cron=self._repair_interval)
-            self._enable_time_trigger(repair_action, cron_schedule)
+        now = datetime.datetime.now().astimezone()
+        periodic_schedule("deploy", deploy_action, self._deploy_interval, self._deploy_splay_value, now)
+        periodic_schedule("repair", repair_action, self._repair_interval, self._repair_splay_value, now)
 
     def _enable_time_trigger(self, action: TaskMethod, schedule: TaskSchedule) -> None:
         self.process._sched.add_action(action, schedule)

src/inmanta/agent/config.py

@@ -95,8 +95,14 @@
     "config",
     "agent-deploy-interval",
     0,
-    "The number of seconds between two (incremental) deployment runs of the agent. Set this to 0 to disable the scheduled deploy runs.",
-    is_time,
+    "Either the number of seconds between two (incremental) deployment runs of the agent or a cron-like expression."
+    " If a cron-like expression is specified, a deploy will be run following a cron-like time-to-run specification,"
+    " interpreted in UTC. The expected format is `[sec] min hour dom month dow [year]` ( If only 6 values are provided, they"
+    " are interpreted as `min hour dom month dow year`)."
+    " A deploy will be requested at the scheduled time. Note that if a cron"
+    " expression is used the 'agent_deploy_splay_time' setting will be ignored."
+    " Set this to 0 to disable the scheduled deploy runs.",
+    is_time_or_cron,
     predecessor_option=agent_interval,
 )
 agent_deploy_splay_time = Option(
@@ -118,7 +124,9 @@
     600,
     "Either the number of seconds between two repair runs (full deploy) of the agent or a cron-like expression."
     " If a cron-like expression is specified, a repair will be run following a cron-like time-to-run specification,"
-    " interpreted in UTC (e.g. `min hour dom month dow`). A repair will be requested at the scheduled time. Note that if a cron"
+    " interpreted in UTC. The expected format is `[sec] min hour dom month dow [year]` ( If only 6 values are provided, they"
+    " are interpreted as `min hour dom month dow year`)."
+    " A repair will be requested at the scheduled time. Note that if a cron"
     " expression is used the 'agent_repair_splay_time' setting will be ignored."
     " Setting this to 0 to disable the scheduled repair runs.",
     is_time_or_cron,

src/inmanta/config.py

@@ -208,15 +208,16 @@ def is_time(value: str) -> int:
     return int(value)
 
 
-def is_time_or_cron(value: Union[int, str]) -> Union[int, str]:
+def is_time_or_cron(value: str) -> Union[int, str]:
     """Time, the number of seconds represented as an integer value or a cron-like expression"""
-    if isinstance(value, int):
-        return value
     try:
-        CronTab(value)
-    except ValueError as e:
-        return int(value)
-    return value
+        return is_time(value)
+    except ValueError:
+        try:
+            CronTab(value)
+        except ValueError as e:
+            raise ValueError("Not an int or cron expression: %s" % value)
+        return value
 
 
 def is_bool(value: Union[bool, str]) -> bool:
@@ -277,7 +278,7 @@ class Option(Generic[T]):
     """
     Defines an option and exposes it for use
 
-    All config option should be define prior to use
+    All config option should be defined prior to use
     For the document generator to work properly, they should be defined at the module level.
 
     :param section: section in the config file

src/inmanta/data/__init__.py

@@ -2377,9 +2377,22 @@ def convert_agent_trigger_method(value: object) -> str:
     return value
 
 
-def validate_cron(value: str) -> str:
+def validate_cron_or_int(value: Union[int, str]) -> str:
+    try:
+        return str(int(value))
+    except ValueError:
+        try:
+            assert isinstance(value, str)  # Make mypy happy
+            return validate_cron(value, allow_empty=False)
+        except ValueError as e:
+            raise ValueError("'%s' is not a valid cron expression or int: %s" % (value, e))
+
+
+def validate_cron(value: str, allow_empty: bool = True) -> str:
     if not value:
-        return ""
+        if allow_empty:
+            return ""
+        raise ValueError("The given cron expression is an empty string")
     try:
         CronTab(value)
     except ValueError as e:
@@ -2588,11 +2601,12 @@ def to_dto(self) -> m.Environment:
         ),
         AUTOSTART_AGENT_DEPLOY_INTERVAL: Setting(
             name=AUTOSTART_AGENT_DEPLOY_INTERVAL,
-            typ="int",
-            default=600,
-            doc="The deployment interval of the autostarted agents."
+            typ="str",
+            default="600",
+            doc="The deployment interval of the autostarted agents. Can be specified as a number of seconds"
+            " or as a cron-like expression."
             " See also: :inmanta.config:option:`config.agent-deploy-interval`",
-            validator=convert_int,
+            validator=validate_cron_or_int,
             agent_restart=True,
         ),
         AUTOSTART_AGENT_DEPLOY_SPLAY_TIME: Setting(
@@ -2606,11 +2620,14 @@ def to_dto(self) -> m.Environment:
         ),
         AUTOSTART_AGENT_REPAIR_INTERVAL: Setting(
             name=AUTOSTART_AGENT_REPAIR_INTERVAL,
-            typ="int",
-            default=86400,
-            doc="The repair interval of the autostarted agents."
-            " See also: :inmanta.config:option:`config.agent-repair-interval`",
-            validator=convert_int,
+            typ="str",
+            default="86400",
+            doc=(
+                "The repair interval of the autostarted agents. Can be specified as a number of seconds"
+                " or as a cron-like expression."
+                " See also: :inmanta.config:option:`config.agent-repair-interval`"
+            ),
+            validator=validate_cron_or_int,
             agent_restart=True,
         ),
         AUTOSTART_AGENT_REPAIR_SPLAY_TIME: Setting(
@@ -2659,8 +2676,9 @@ def to_dto(self) -> m.Environment:
             typ="str",
             validator=validate_cron,
             doc=(
-                "Periodically run a full compile following a cron-like time-to-run specification, interpreted in UTC"
-                " (e.g. `min hour dom month dow`). A compile will be requested at the scheduled time. The actual"
+                "Periodically run a full compile following a cron-like time-to-run specification interpreted in UTC with format"
+                " `[sec] min hour dom month dow [year]` (If only 6 values are provided, they are interpreted as"
+                " `min hour dom month dow year`). A compile will be requested at the scheduled time. The actual"
                 " compilation may have to wait in the compile queue for some time, depending on the size of the queue and the"
                 " RECOMPILE_BACKOFF environment setting. This setting has no effect when server_compile is disabled."
             ),

src/inmanta/server/agentmanager.py

@@ -1174,10 +1174,10 @@ async def _make_agent_config(
         privatestatedir: str = os.path.join(Config.get("config", "state-dir", "/var/lib/inmanta"), environment_id)
 
         agent_deploy_splay: int = cast(int, await env.get(data.AUTOSTART_AGENT_DEPLOY_SPLAY_TIME, connection=connection))
-        agent_deploy_interval: int = cast(int, await env.get(data.AUTOSTART_AGENT_DEPLOY_INTERVAL, connection=connection))
+        agent_deploy_interval: str = cast(str, await env.get(data.AUTOSTART_AGENT_DEPLOY_INTERVAL, connection=connection))
 
         agent_repair_splay: int = cast(int, await env.get(data.AUTOSTART_AGENT_REPAIR_SPLAY_TIME, connection=connection))
-        agent_repair_interval: int = cast(int, await env.get(data.AUTOSTART_AGENT_REPAIR_INTERVAL, connection=connection))
+        agent_repair_interval: str = cast(str, await env.get(data.AUTOSTART_AGENT_REPAIR_INTERVAL, connection=connection))
 
         # The internal agent always needs to have a session. Otherwise the agentmap update trigger doesn't work
         if "internal" not in agent_names:
@@ -1192,9 +1192,9 @@ async def _make_agent_config(
 environment=%(env_id)s
 
 agent-deploy-splay-time=%(agent_deploy_splay)d
-agent-deploy-interval=%(agent_deploy_interval)d
+agent-deploy-interval=%(agent_deploy_interval)s
 agent-repair-splay-time=%(agent_repair_splay)d
-agent-repair-interval=%(agent_repair_interval)d
+agent-repair-interval=%(agent_repair_interval)s
 
 agent-get-resource-backoff=%(agent_get_resource_backoff)f
 

tests/agent_server/test_server_agent.py

@@ -24,6 +24,7 @@
 from itertools import groupby
 from logging import DEBUG
 from typing import Any, Dict, List, Optional, Tuple
+from uuid import UUID
 
 import psutil
 import pytest
@@ -36,7 +37,13 @@
 from inmanta.ast import CompilerException
 from inmanta.config import Config
 from inmanta.const import AgentAction, AgentStatus, ParameterSource, ResourceState
-from inmanta.data import ENVIRONMENT_AGENT_TRIGGER_METHOD, Setting, convert_boolean
+from inmanta.data import (
+    AUTOSTART_AGENT_DEPLOY_INTERVAL,
+    AUTOSTART_AGENT_REPAIR_INTERVAL,
+    ENVIRONMENT_AGENT_TRIGGER_METHOD,
+    Setting,
+    convert_boolean,
+)
 from inmanta.server import (
     SLICE_AGENT_MANAGER,
     SLICE_AUTOSTARTED_AGENT_MANAGER,
@@ -303,8 +310,20 @@ async def test_server_restart(
     assert not resource_container.Provider.isset("agent1", "key3")
 
 
+@pytest.mark.parametrize(
+    "agent_deploy_interval",
+    ["2", "*/2 * * * * * *"],
+)
 async def test_spontaneous_deploy(
-    resource_container, server, client, environment, clienthelper, no_agent_backoff, async_finalizer, caplog
+    resource_container,
+    server,
+    client,
+    environment,
+    clienthelper,
+    no_agent_backoff,
+    async_finalizer,
+    caplog,
+    agent_deploy_interval,
 ):
     """
     dryrun and deploy a configuration model
@@ -313,9 +332,9 @@ async def test_spontaneous_deploy(
     with caplog.at_level(DEBUG):
         resource_container.Provider.reset()
 
-        env_id = environment
+        env_id = UUID(environment)
 
-        Config.set("config", "agent-deploy-interval", "2")
+        Config.set("config", "agent-deploy-interval", agent_deploy_interval)
         Config.set("config", "agent-deploy-splay-time", "2")
         Config.set("config", "agent-repair-interval", "0")
 
@@ -387,20 +406,19 @@ async def test_spontaneous_deploy(
 
 
 @pytest.mark.parametrize(
-    "cron",
-    [False, True],
+    "agent_repair_interval",
+    [
+        "2",
+        "*/2 * * * * * *",
+    ],
 )
 async def test_spontaneous_repair(
-    resource_container, environment, client, clienthelper, no_agent_backoff, async_finalizer, server, cron
+    resource_container, environment, client, clienthelper, no_agent_backoff, async_finalizer, server, agent_repair_interval
 ):
     """
     Test that a repair run is executed every 2 seconds as specified in the agent_repair_interval (using a cron or not)
     """
     resource_container.Provider.reset()
-    agent_repair_interval = "2"
-    if cron:
-        agent_repair_interval = "*/2 * * * * * *"
-
     env_id = environment
 
     Config.set("config", "agent-repair-interval", agent_repair_interval)
@@ -485,6 +503,36 @@ async def verify_deployment_result():
     await resource_action_consistency_check()
 
 
+@pytest.mark.parametrize(
+    "interval_code",
+    [(2, 200), ("2", 200), ("*/2 * * * * * *", 200), ("", 400)],
+)
+async def test_env_setting_wiring_to_autostarted_agent(
+    resource_container, environment, client, clienthelper, no_agent_backoff, async_finalizer, server, interval_code
+):
+    """
+    Test that the AUTOSTART_AGENT_DEPLOY_INTERVAL and AUTOSTART_AGENT_REPAIR_INTERVAL
+    env settings are properly wired through to auto-started agents.
+    """
+    env_id = UUID(environment)
+    interval, expected_code = interval_code
+    result = await client.set_setting(environment, AUTOSTART_AGENT_DEPLOY_INTERVAL, interval)
+    assert result.code == expected_code
+    result = await client.set_setting(environment, AUTOSTART_AGENT_REPAIR_INTERVAL, interval)
+    assert result.code == expected_code
+
+    if expected_code == 200:
+        env = await data.Environment.get_by_id(env_id)
+        autostarted_agent_manager = server.get_slice(SLICE_AUTOSTARTED_AGENT_MANAGER)
+
+        config = await autostarted_agent_manager._make_agent_config(
+            env, agent_names=[], agent_map={"internal": ""}, connection=None
+        )
+
+        assert f"agent-deploy-interval={interval}" in config
+        assert f"agent-repair-interval={interval}" in config
+
+
 async def test_failing_deploy_no_handler(
     resource_container, agent, environment, client, clienthelper, async_finalizer, no_agent_backoff
 ):

tests/test_data.py

@@ -429,23 +429,23 @@ async def test_environment_deprecated_setting(init_dataclasses_and_load_schema,
     env = data.Environment(name="dev", project=project.id, repo_url="", repo_branch="")
     await env.insert()
 
-    for deprecated_option, new_option in [
-        (data.AUTOSTART_AGENT_INTERVAL, data.AUTOSTART_AGENT_DEPLOY_INTERVAL),
-        (data.AUTOSTART_SPLAY, data.AUTOSTART_AGENT_DEPLOY_SPLAY_TIME),
+    for deprecated_option, new_option, old_value, new_value in [
+        (data.AUTOSTART_AGENT_INTERVAL, data.AUTOSTART_AGENT_DEPLOY_INTERVAL, 22, "23"),
+        (data.AUTOSTART_SPLAY, data.AUTOSTART_AGENT_DEPLOY_SPLAY_TIME, 22, 23),
     ]:
-        await env.set(deprecated_option, 22)
+        await env.set(deprecated_option, old_value)
         caplog.clear()
-        assert (await env.get(new_option)) == 22
+        assert (await env.get(new_option)) == old_value
         assert "Config option %s is deprecated. Use %s instead." % (deprecated_option, new_option) in caplog.text
 
-        await env.set(new_option, 23)
+        await env.set(new_option, new_value)
         caplog.clear()
-        assert (await env.get(new_option)) == 23
+        assert (await env.get(new_option)) == new_value
         assert "Config option %s is deprecated. Use %s instead." % (deprecated_option, new_option) not in caplog.text
 
         await env.unset(deprecated_option)
         caplog.clear()
-        assert (await env.get(new_option)) == 23
+        assert (await env.get(new_option)) == new_value
         assert "Config option %s is deprecated. Use %s instead." % (deprecated_option, new_option) not in caplog.text