Skip to content

Commit c15d870

Browse files
sanderrinmantaci
authored andcommitted
Made various improvements to the AutostartedAgent._ensure_agents method (Issue #7612, PR #7612)
# Description Fixes bug in autostarted agent manager that occasionally causes agent's sessions to time out, dropping calls, resulting in a "stuck" orchestrator until a redeploy is triggered. ~~The bug has been there forever, but the conditions for it to trigger happen to be set up by #7278.~~ ~~The core of the bug is the following. When we need a certain agent to be up, we call `AutostartedAgentManager._ensure_agents`. This makes sure an agent process is running for these agents and waits for them to be up. However, instead of starting a process to track the autostarted agent map and to trust that it would keep up with changes to it, we instead start a process with an explicit list of agent endpoints.~~ If a new call comes in to `_ensure_agents`, and the agent is not yet up, we would kill the current process and start a new one. In killing the process, we would not expire its sessions, letting them time out on the 30s heartbeat timeout, losing any calls made to it in that window. EDIT: Scratched some of the above: I wrote a test case for the first bullet point below, which I thought to be the root cause of the reason for killing the old agent process. However, turns out the test also succeeds on master. The agent's `on_reconnect` actually updates the process' agent map. So, I think my root cause analysis may have been wrong (at least less likely), but the second and third bullet points should fix the issue anyway (doesn't matter exactly *how* we got in the situation where only part of the agent endpoints were up, as long as we handle it correctly), and even if part of the issue persists, the logging improvements should help future investigation. And the first bullet point is still a good consistency fix imo. This PR makes the following changes: - An agent process for autostarted agents (`use_autostart_agent_map` in the config file) now ignores any agent endpoints in the config file. Instead it runs purely in autostarted agent mode, starting instances for each of the endpoints in the agent map. It then tracks any changes made to the agent map. This last part was already in place, and resulted in a small inconsistency where the process would respect agent names in the config at start, and then suddenly move to consider the agent map the authority as soon as a change comes in. This inconsistency is now resolved by considering the agent map the authority for the entire lifetime of the agent process. - The autostarted agent manager now trusts in its processes to track the agent map. If a process is already running for an environment, but the agent endpoints are not yet up, it waits for them rather than to kill the process and start fresh. - When an explicit restart is requested, the autostarted agent manager now expires all sessions for the agents in the agent map, i.e. the endpoints for the killed process. - Improved robustness of wait condition for an agent to be up, specifically make sure we don't consider expired sessions to be up. - Made some log messages a bit more informative, no major changes. # Self Check: Strike through any lines that are not applicable (`~~line~~`) then check the box - [x] ~Attached issue to pull request~ - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [x] ~End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )~ - [x] ~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)~
1 parent d50f3bf commit c15d870

File tree

7 files changed

+370
-188
lines changed

7 files changed

+370
-188
lines changed
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
description: "Made various improvements to the AutostartedAgent._ensure_agents method"
2+
sections:
3+
bugfix: "Fixed a race condition where autostarted agents might become unresponsive for 30s when restarted"
4+
issue-nr: 7612
5+
change-type: patch
6+
destination-branches:
7+
- master
8+
- iso7
9+
- iso6
10+

src/inmanta/agent/agent.py

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -975,16 +975,18 @@ async def _init_agent_map(self) -> None:
975975
self.agent_map = dict(cfg.agent_map.get())
976976

977977
async def _init_endpoint_names(self) -> None:
978-
if self.hostname is not None:
979-
await self.add_end_point_name(self.hostname)
980-
else:
981-
# load agent names from the config file
982-
agent_names = cfg.agent_names.get()
983-
if agent_names is not None:
984-
for name in agent_names:
985-
if "$" in name:
986-
name = name.replace("$node-name", self.node_name)
987-
await self.add_end_point_name(name)
978+
assert self.agent_map is not None
979+
endpoints: Iterable[str] = (
980+
[self.hostname]
981+
if self.hostname is not None
982+
else (
983+
self.agent_map.keys()
984+
if cfg.use_autostart_agent_map.get()
985+
else (name if "$" not in name else name.replace("$node-name", self.node_name) for name in cfg.agent_names.get())
986+
)
987+
)
988+
for endpoint in endpoints:
989+
await self.add_end_point_name(endpoint)
988990

989991
async def stop(self) -> None:
990992
await super().stop()
@@ -1067,6 +1069,13 @@ async def update_agent_map(self, agent_map: dict[str, str]) -> None:
10671069
await self._update_agent_map(agent_map)
10681070

10691071
async def _update_agent_map(self, agent_map: dict[str, str]) -> None:
1072+
if "internal" not in agent_map:
1073+
LOGGER.warning(
1074+
"Agent received an update_agent_map() trigger without internal agent in the agent_map %s",
1075+
agent_map,
1076+
)
1077+
agent_map = {"internal": "local:", **agent_map}
1078+
10701079
async with self._instances_lock:
10711080
self.agent_map = agent_map
10721081
# Add missing agents

src/inmanta/data/__init__.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
import warnings
3030
from abc import ABC, abstractmethod
3131
from collections import abc, defaultdict
32-
from collections.abc import Awaitable, Callable, Iterable, Sequence
32+
from collections.abc import Awaitable, Callable, Iterable, Sequence, Set
3333
from configparser import RawConfigParser
3434
from contextlib import AbstractAsyncContextManager
3535
from itertools import chain
@@ -1219,7 +1219,7 @@ def get_connection(
12191219
"""
12201220
if connection is not None:
12211221
return util.nullcontext(connection)
1222-
# Make pypi happy
1222+
# Make mypy happy
12231223
assert cls._connection_pool is not None
12241224
return cls._connection_pool.acquire()
12251225

@@ -3344,10 +3344,12 @@ def get_valid_field_names(cls) -> list[str]:
33443344
return super().get_valid_field_names() + ["process_name", "status"]
33453345

33463346
@classmethod
3347-
async def get_statuses(cls, env_id: uuid.UUID, agent_names: set[str]) -> dict[str, Optional[AgentStatus]]:
3347+
async def get_statuses(
3348+
cls, env_id: uuid.UUID, agent_names: Set[str], *, connection: Optional[asyncpg.connection.Connection] = None
3349+
) -> dict[str, Optional[AgentStatus]]:
33483350
result: dict[str, Optional[AgentStatus]] = {}
33493351
for agent_name in agent_names:
3350-
agent = await cls.get_one(environment=env_id, name=agent_name)
3352+
agent = await cls.get_one(environment=env_id, name=agent_name, connection=connection)
33513353
if agent:
33523354
result[agent_name] = agent.get_status()
33533355
else:

0 commit comments

Comments
 (0)