resource scheduler: implement first stage #8001

sanderr · 2024-08-23T16:21:00Z

Description

First stage of the resource scheduler (#8007, ZenHub epic). Focus is mainly on the data structures for model state and scheduled work, and the algorithms to update that state when a new version comes in or as tasks are executed.

There is still some polishing to do. I propose to delegate that to #8008. That means that the focus of this review is on the stability of the server and agent interfaces, so that work on those will not conflict with further refinements in #8008.

Of course, feel free to already leave feedback for the other parts, I'll make sure to include it in #8008. I've already marked a lot of notes to self in the code as FIXME[#8008].

Please make sure to also consider the module structure. Do you agree that inmanta.deploy makes sense as a namespace?

closes #7548

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)~~ -> resource scheduler: polish scheduling algorithm #8008
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)~~

sanderr · 2024-08-23T16:24:04Z

src/inmanta/deploy/scheduler.py

I'd suggest to start your review here and to follow the flow from there.

wouterdb

Cool stuff

src/inmanta/deploy/scheduler.py

src/inmanta/util/collections.py

src/inmanta/deploy/work.py

wouterdb · 2024-08-23T17:35:45Z

src/inmanta/deploy/work.py

+
+        :param ensure_scheduled: Set of resources that should be deployed. Adds a deploy task to the scheduled work for each
+            of these, unless it is already scheduled.
+        :param running_deploys: Set of resources for which a deploy is currently running, for the latest desired state, i.e.


this I don't understand

It was a bit of a last-minute addition, may need more refinement. I'll see what I can do in #8008 and if nothing else I'll clarify it there.

…duler

sanderr · 2024-08-28T08:52:40Z

@wouterdb I updated the description with some more context.

src/inmanta/deploy/scheduler.py

src/inmanta/util/collections.py

arnaudsjs · 2024-08-28T09:21:11Z

src/inmanta/util/collections.py

+P = TypeVar("P", bound=Hashable)
+S = TypeVar("S", bound=Hashable)
+# type vars not bound to the class
+K = TypeVar("K", bound=Hashable)
+V = TypeVar("V", bound=Hashable)


This can use some comments. Single character type names are very to interpret.

I found it hard to come up with meaningful names here. I changed it to the conventional S and T now. I'm not sure comments would be helpful, since the names really don't have much meaning.

(If you get notified about these comments, no need to look at them now, I'll make sure to link here from the new PR.)

arnaudsjs · 2024-08-28T09:34:53Z

src/inmanta/util/collections.py

+                self.set_primary(key, values)
+
+    @staticmethod
+    def _set(primary: dict[K, set[V]], reverse: dict[V, set[K]], key: K, values: Set[V]) -> None:


This method can use documentation.

arnaudsjs · 2024-08-28T09:35:31Z

src/inmanta/util/collections.py

+
+
+# TODO: better name?
+class BidirectionalManyToManyMapping(MutableMapping[P, Set[S]], Generic[P, S]):


This class can use a docstring.

arnaudsjs · 2024-08-28T11:11:57Z

src/inmanta/deploy/work.py

+        # can not drop tasks from queue without breaking the heap invariant, or potentially breaking asyncio.Queue invariants
+        # => take approach suggested in heapq docs: simply mark as deleted.
+        # => Keep view on all active tasks for a given resource, which also doubles as lookup for client operations
+        # Only tasks in this collection are considered queued, as far as this class' client interface is concerned.


This part I don't understand. Where does the problem with the deletion come from? As far as I can see the documentation of the PriorityQueue class doesn't say anything about this.

Indeed it does not, but the heapq documentation does: https://docs.python.org/3/library/heapq.html#priority-queue-implementation-notes

And indeed, asyncio.PriorityQueue doesn't offer a remove method or similar.

src/inmanta/deploy/scheduler.py

arnaudsjs · 2024-08-29T09:53:36Z

src/inmanta/deploy/scheduler.py

+        #   short, synchronous operations (and therefore we wouldn't gain anything by allowing multiple readers).
+        self._scheduler_lock: asyncio.Lock = asyncio.Lock()
+        # - lock to serialize scheduler state updates (i.e. process new version)
+        self._update_lock: asyncio.Lock = asyncio.Lock()


Suggested change

self._update_lock: asyncio.Lock = asyncio.Lock()

self._desired_state_lock: asyncio.Lock = asyncio.Lock()

Co-authored-by: arnaudsjs <[email protected]>

sanderr · 2024-08-30T12:08:44Z

@arnaudsjs thanks for the review. I committed the bugfix suggestion you commented, but I'm leaving the rest of your comments for #8008 to unstuck @hlloreda for #8009.

inmantaci · 2024-08-30T12:53:06Z

Processing this pull request

inmantaci · 2024-08-30T12:53:08Z

Pull request rejected by merge tool. This pull request has not been approved yet.

Merging ASAP to unblock next tasks. #8008 will process final review comments.

inmantaci · 2024-08-30T14:30:07Z

Processing this pull request

inmantaci · 2024-08-30T14:30:10Z

Merged into branches master in d09c832

# Description First stage of the resource scheduler (#8007, [ZenHub epic](https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/inmanta-core/8007)). Focus is mainly on the data structures for model state and scheduled work, and the algorithms to update that state when a new version comes in or as tasks are executed. There is still some polishing to do. I propose to delegate that to #8008. That means that the focus of this review is on the stability of the server and agent interfaces, so that work on those will not conflict with further refinements in #8008. Of course, feel free to already leave feedback for the other parts, I'll make sure to include it in #8008. I've already marked a lot of notes to self in the code as `FIXME[#8008]`. Please make sure to also consider the module structure. Do you agree that `inmanta.deploy` makes sense as a namespace? closes #7548 # 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)~~ -> #8008 - [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)~~

# Description Polished the resource scheduler and addressed todos and review comments from #8001. I attached some comments to the diff with more details / motivation. Please also have another look at the comments you made in #8001 to see if they have been resolved to your satisfaction. closes #8008 # 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)~~

sanderr added 14 commits July 4, 2024 15:12

resource scheduler: requires-provides mapping

43081d9

stuff

05528eb

scale testing

1735028

Merge branch 'master' into issue/resource-scheduler

a953bc0

implemented part of new_version

43ba03c

WIP

ac311de

refinements

c081b80

improvements

33d7e52

removed outdated methods

a46a5c3

refinement

0eabac0

moved file

ebb12e2

split off state.py

be0b43b

split off work.py

70d0787

polishing

fbbc5fe

sanderr commented Aug 23, 2024

View reviewed changes

src/inmanta/deploy/scheduler.py

Copy link

Contributor Author

sanderr Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to start your review here and to follow the flow from there.

sanderr requested a review from wouterdb August 23, 2024 16:28

wouterdb approved these changes Aug 23, 2024

View reviewed changes

sanderr added 3 commits August 27, 2024 17:53

todos

32c7c94

mypy

9745cae

final refinements

34b021a

sanderr changed the title ~~Issue/resource scheduler~~ resource scheduler: implement first stage Aug 28, 2024

sanderr mentioned this pull request Aug 28, 2024

agent rework: deploy sequencing first steps #7548

Closed

sanderr added 3 commits August 28, 2024 10:38

change entry

1c679ff

Merge remote-tracking branch 'origin/master' into issue/resource-sche…

595ad77

…duler

pep8

32f5e57

sanderr requested a review from wouterdb August 28, 2024 08:52

sanderr marked this pull request as ready for review August 28, 2024 08:52

sanderr requested a review from arnaudsjs August 28, 2024 08:52

wouterdb approved these changes Aug 28, 2024

View reviewed changes

src/inmanta/deploy/scheduler.py Show resolved Hide resolved

src/inmanta/util/collections.py Outdated Show resolved Hide resolved

arnaudsjs previously requested changes Aug 29, 2024

View reviewed changes

Update src/inmanta/deploy/scheduler.py

871474c

Co-authored-by: arnaudsjs <[email protected]>

sanderr requested review from arnaudsjs and removed request for arnaudsjs August 30, 2024 12:09

sanderr added the merge-tool-ready This ticket is ready to be merged in label Aug 30, 2024

Merge branch 'master' into issue/resource-scheduler

c999c9b

inmantaci removed the merge-tool-ready This ticket is ready to be merged in label Aug 30, 2024

inmantaci assigned sanderr Aug 30, 2024

moved old inmanta.deploy

be2d5f5

sanderr added the merge-tool-ready This ticket is ready to be merged in label Aug 30, 2024

inmantaci closed this Aug 30, 2024

inmantaci deleted the issue/resource-scheduler branch August 30, 2024 14:30

sanderr mentioned this pull request Sep 20, 2024

Issue/8008 resource scheduler polishing #8116

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource scheduler: implement first stage #8001

resource scheduler: implement first stage #8001

sanderr commented Aug 23, 2024 •

edited

Loading

sanderr Aug 23, 2024

wouterdb left a comment

wouterdb Aug 23, 2024

sanderr Aug 28, 2024

sanderr commented Aug 28, 2024

arnaudsjs Aug 28, 2024

sanderr Sep 20, 2024

sanderr Sep 20, 2024

arnaudsjs Aug 28, 2024

arnaudsjs Aug 28, 2024

arnaudsjs Aug 28, 2024

sanderr Aug 30, 2024

arnaudsjs Aug 29, 2024

sanderr commented Aug 30, 2024

inmantaci commented Aug 30, 2024

inmantaci commented Aug 30, 2024

inmantaci commented Aug 30, 2024

inmantaci commented Aug 30, 2024



		# TODO: better name?
		class BidirectionalManyToManyMapping(MutableMapping[P, Set[S]], Generic[P, S]):

	self._update_lock: asyncio.Lock = asyncio.Lock()
	self._desired_state_lock: asyncio.Lock = asyncio.Lock()

resource scheduler: implement first stage #8001

resource scheduler: implement first stage #8001

Conversation

sanderr commented Aug 23, 2024 • edited Loading

Description

Self Check:

Choose a reason for hiding this comment

wouterdb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr commented Aug 28, 2024

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 commented Aug 30, 2024

inmantaci commented Aug 30, 2024

inmantaci commented Aug 30, 2024

inmantaci commented Aug 30, 2024

inmantaci commented Aug 30, 2024

sanderr commented Aug 23, 2024 •

edited

Loading