Issue/7694 executor cleanup #7765

Hugo-Inmanta · 2024-06-24T14:43:07Z

Description

Closes #7694

Add 2 config options:

executor_retention: executors idle for longer than this get cleaned up
executor_cap_per_agent: if this limit is already reached when attempting to create a new executor for this agent, the oldest executor is first stopped.

Self Check:

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 for more info)~~

src/inmanta/agent/forking_executor.py

src/inmanta/agent/agent.py

src/inmanta/agent/forking_executor.py

src/inmanta/agent/config.py

src/inmanta/data/__init__.py

wouterdb · 2024-07-03T07:35:46Z

src/inmanta/agent/config.py

@@ -188,7 +189,7 @@ def is_executor_mode(value: str | AgentExecutorMode) -> AgentExecutorMode:
    3,
    "Maximum number of concurrent executors to keep per environment, per agent. If this limit is already reached "
    "when creating a new executor, the oldest one will be stopped first.",
-    is_int,
+    functools.partial(is_lower_bounded_int, 1),


We use the docstring of the function to generate documentation, so this won't look so pretty in the docs

wouterdb · 2024-07-03T07:37:04Z

src/inmanta/agent/forking_executor.py

+
+        self.stopping: bool = False
+        # We keep a reference to the periodic cleanup task to prevent it
+        # from disappearing mid-execution https://docs.python.org/3.11/library/asyncio-task.html#creating-tasks


wouterdb · 2024-07-03T07:38:13Z

src/inmanta/agent/forking_executor.py

        return p, parent_conn

+    async def start(self) -> None:
+        self.cleanup_job = asyncio.create_task(self.cleanup_inactive_executors())


in the join method, you could await self.cleanup_job to make sure it is down.

src/inmanta/config.py

wouterdb · 2024-07-03T07:49:59Z

src/inmanta/agent/forking_executor.py

+        self.executor_retention_time = inmanta.agent.config.agent_executor_retention_time.get()
+        self.max_executors_per_agent = inmanta.agent.config.agent_executor_cap.get()
+
+        self.stopping: bool = False


I've already given more comment than I'm comfortable, so feel free to ignore this one: when used like this, I usually call this 'running' (and invert it) to make sure it doesn't only indicate 'stopping' but also 'stopped'

Co-authored-by: Wouter De Borger <[email protected]>

Hugo-Inmanta · 2024-07-03T08:59:53Z

tests/forking_agent/test_executor.py

+    # Clean up executors after 3s of inactivity
+    inmanta.agent.config.agent_executor_retention_time.set("3")


Increased this a bit because it was causing intempestive cleanup in the test case: the old executor is supposed to get cleaned up when the agent cap is reached, but with this setting too low, it would automatically get cleaned up before then.

sanderr · 2024-07-03T11:15:03Z

src/inmanta/agent/forking_executor.py

@@ -397,7 +399,21 @@ async def serve() -> None:
    exit(0)


-class MPExecutor(executor.Executor):
+class PoolMember(abc.ABC):


Nice! Could use a docstring though.

sanderr · 2024-07-03T11:16:11Z

src/inmanta/agent/forking_executor.py

@@ -518,7 +537,24 @@ async def get_facts(self, resource: "inmanta.agent.executor.ResourceDetails") ->
        return await self.connection.call(FactsCommand(resource))


-class MPManager(executor.ExecutorManager[MPExecutor]):
+class PoolManager:
+    def __init__(self, max_time: int, max_pool_size: int, min_pool_size: int):


Suggested change

def __init__(self, max_time: int, max_pool_size: int, min_pool_size: int):

def __init__(self, *, max_time: int, max_pool_size: int, min_pool_size: int):

sanderr · 2024-07-03T11:19:41Z

src/inmanta/agent/forking_executor.py

@@ -518,7 +537,24 @@ async def get_facts(self, resource: "inmanta.agent.executor.ResourceDetails") ->
        return await self.connection.call(FactsCommand(resource))


-class MPManager(executor.ExecutorManager[MPExecutor]):
+class PoolManager:


Suggested change

class PoolManager:

class PoolManager(abc.ABC):

sanderr · 2024-07-03T11:21:21Z

src/inmanta/agent/forking_executor.py

@@ -518,7 +537,24 @@ async def get_facts(self, resource: "inmanta.agent.executor.ResourceDetails") ->
        return await self.connection.call(FactsCommand(resource))


-class MPManager(executor.ExecutorManager[MPExecutor]):
+class PoolManager:


To be honest, I don't see the benefit of this class, or even the PoolMember class, the way they're implemented now. Sure, they present a common interface, but it doesn't seem like we use the interface in any meaningful way?

the get_pool_members method is not used

neither class offers any funcionality, meaning the cleanup methods are still custom

Am I missing something?

Agreed, this is not useful right now, I'll remove it

That's not entirely what I'm trying to say. I think the concept is very good, if it allows us to share behavior between both use cases. So when @wouterdb proposed it, I'm assuming that's what he had in mind? By removing it, we're back to square one, aren't we?

I'll leave them out for now, see discussion on slack. I think it was intended for @hlloreda to factor out if necessary all along

sanderr

My review is blocked on Wouter's response to this discussion.

inmantaci · 2024-07-03T12:26:07Z

Processing this pull request

inmantaci · 2024-07-03T12:26:10Z

Merged into branches master in 04983c4

# Description Closes #7694 Add 2 config options: - `executor_retention`: executors idle for longer than this get cleaned up - `executor_cap_per_agent`: if this limit is already reached when attempting to create a new executor for this agent, the oldest executor is first stopped. # 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 ~~- [ ] 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)~~

Hugo-Inmanta added 9 commits June 21, 2024 08:22

[WIP]

0d980fd

[WIP]

ed30a8c

Merge branch 'master' into issue/7694-executor-cleanup

ca300eb

[WIP]

2578db9

[WIP]

996c9a5

[WIP]

0531ecb

[WIP]

368a2d3

[WIP]

77b38ba

[WIP]

9ccb204

Hugo-Inmanta commented Jun 25, 2024

View reviewed changes

src/inmanta/agent/forking_executor.py Outdated Show resolved Hide resolved

Hugo-Inmanta requested a review from wouterdb June 25, 2024 13:02

wouterdb reviewed Jun 25, 2024

View reviewed changes

src/inmanta/agent/agent.py Outdated Show resolved Hide resolved

src/inmanta/agent/agent.py Outdated Show resolved Hide resolved

Hugo-Inmanta added 7 commits June 26, 2024 08:12

[WIP]

4d633a8

[WIP]

f216ec4

[WIP]

b120b1a

Merge branch 'master' into issue/7694-executor-cleanup

750f0a3

[WIP]

400f1d7

[WIP]

dd1cea8

[WIP]

78063a9

Hugo-Inmanta requested review from wouterdb and sanderr June 26, 2024 08:39

wouterdb requested changes Jun 26, 2024

View reviewed changes

src/inmanta/agent/forking_executor.py Outdated Show resolved Hide resolved

src/inmanta/agent/forking_executor.py Show resolved Hide resolved

src/inmanta/agent/config.py Outdated Show resolved Hide resolved

src/inmanta/data/__init__.py Outdated Show resolved Hide resolved

Hugo-Inmanta added 7 commits June 26, 2024 12:17

[WIP]

7181c79

[WIP]

9470de1

[WIP]

44c2622

[WIP]

ef2851f

[WIP]

fcd7eb4

[WIP]

8170cc7

[WIP]

42f2455

Hugo-Inmanta requested a review from wouterdb June 26, 2024 12:54

[WIP]

9d4f7f1

Hugo-Inmanta requested a review from wouterdb July 3, 2024 06:48

wouterdb requested changes Jul 3, 2024

View reviewed changes

wouterdb reviewed Jul 3, 2024

View reviewed changes

Hugo-Inmanta and others added 8 commits July 3, 2024 10:29

Update src/inmanta/config.py

8d5963a

Co-authored-by: Wouter De Borger <[email protected]>

[WIP] increase retention time slightly to avoid unwanted cleanups

558cdde

[WIP]

b536897

[WIP]

92f4b90

[WIP]

142cfc0

[WIP]

08ee9e9

[WIP]

025a81b

[WIP]

060a005

Hugo-Inmanta commented Jul 3, 2024

View reviewed changes

[WIP]

e815992

Hugo-Inmanta requested a review from wouterdb July 3, 2024 09:01

wouterdb approved these changes Jul 3, 2024

View reviewed changes

sanderr requested changes Jul 3, 2024

View reviewed changes

Hugo-Inmanta added 3 commits July 3, 2024 13:41

[WIP]

2bedb4a

[WIP]

9b7c7b9

Merge branch 'master' into issue/7694-executor-cleanup

7cba0da

Hugo-Inmanta requested a review from sanderr July 3, 2024 11:53

sanderr reviewed Jul 3, 2024

View reviewed changes

Hugo-Inmanta requested a review from sanderr July 3, 2024 12:19

sanderr approved these changes Jul 3, 2024

View reviewed changes

Hugo-Inmanta added the merge-tool-ready This ticket is ready to be merged in label Jul 3, 2024

inmantaci closed this Jul 3, 2024

inmantaci deleted the issue/7694-executor-cleanup branch July 3, 2024 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/7694 executor cleanup #7765

Issue/7694 executor cleanup #7765

Hugo-Inmanta commented Jun 24, 2024 •

edited

Loading

wouterdb Jul 3, 2024

wouterdb Jul 3, 2024

wouterdb Jul 3, 2024

wouterdb Jul 3, 2024

Hugo-Inmanta Jul 3, 2024

sanderr Jul 3, 2024

sanderr Jul 3, 2024

sanderr Jul 3, 2024

sanderr Jul 3, 2024

Hugo-Inmanta Jul 3, 2024

sanderr Jul 3, 2024

Hugo-Inmanta Jul 3, 2024

sanderr left a comment

inmantaci commented Jul 3, 2024

inmantaci commented Jul 3, 2024

		# Clean up executors after 3s of inactivity
		inmanta.agent.config.agent_executor_retention_time.set("3")

	def __init__(self, max_time: int, max_pool_size: int, min_pool_size: int):
	def __init__(self, *, max_time: int, max_pool_size: int, min_pool_size: int):

Issue/7694 executor cleanup #7765

Issue/7694 executor cleanup #7765

Conversation

Hugo-Inmanta commented Jun 24, 2024 • edited Loading

Description

Self Check:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr left a comment

Choose a reason for hiding this comment

inmantaci commented Jul 3, 2024

inmantaci commented Jul 3, 2024

Hugo-Inmanta commented Jun 24, 2024 •

edited

Loading