feat: support for search reprovisioning #392

m-alisafaee · 2024-09-10T08:23:26Z

Adds a Sanic background task that reads data from the database and sends them as events.

Search reprovisioning.

/deploy

coveralls · 2024-09-10T08:53:00Z

Pull Request Test Coverage Report for Build 11163620994

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

204 of 209 (97.61%) changed or added relevant lines in 13 files are covered.
4 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.1%) to 90.54%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
components/renku_data_services/authz/authz.py	17	18	94.44%
components/renku_data_services/namespace/db.py	6	7	85.71%
components/renku_data_services/project/db.py	7	8	87.5%
components/renku_data_services/message_queue/core.py	49	51	96.08%

Files with Coverage Reduction	New Missed Lines	%
components/renku_data_services/base_api/auth.py	1	89.61%
components/renku_data_services/crc/models.py	1	84.57%
components/renku_data_services/storage/blueprints.py	2	94.76%

Totals
Change from base Build 11140653681:	0.1%
Covered Lines:	9523
Relevant Lines:	10518

💛 - Coveralls

RenkuBot · 2024-09-19T11:11:51Z

You can access the deployment of this PR at https://renku-ci-ds-392.dev.renku.ch

olevski

Thanks this looks really good Mohammad. I have a few questions and requests for changes. For some of the questions it may be easier to have a live discussion - but you tell me what you prefer.

components/renku_data_services/message_queue/api.spec.yaml

pyproject.toml

test/bases/renku_data_services/data_api/test_message_queue.py

olevski · 2024-09-27T07:48:15Z

components/renku_data_services/message_queue/orm.py

+    This table is used to make sure that only one instance of reprovisioning is run at any given time.
+    It gets updated with the reprovisioning progress.


suggestion: I think we should add a constraint to this table that makes so that either:

it can only ever contain one row

OR it can contain a history of all reprovisionings but we add an active boolean column and add a constraint so that only 1 row can have active set to true at any time.

i.e. some way so that even if we mess things up in our code we can only have 1 reprovisioning happening at a time.

components/renku_data_services/message_queue/db.py

olevski · 2024-09-27T07:53:16Z

components/renku_data_services/message_queue/core.py

+    async with session_maker() as session, session.begin():
+        start_event = make_event(
+            message_type="reprovisioning.started", payload=v2.ReprovisioningStarted(id=str(reprovisioning.id))
+        )
+        await event_repo.store_event(session, start_event)


suggestion: I know we check if a reprovisioning is already going on in the db.py. But I think it is safer/nicer to not even send the reprovisioning.started event if another reprovisioning is underway. So the first thing we check inside this db transaction should be whether another reprovisioning is already in progress. And if it is then fail/do nothing. We should do this in addition with a constraint on the table in the DB.

We check if a reprovisioning is active, in the blueprint and before calling this function. So, the reprovisioning.started message isn't sent twice.

I'll add the DB constraint in the follow up PR.

olevski · 2024-09-27T13:52:43Z

components/renku_data_services/message_queue/blueprints.py

+    def delete(self) -> BlueprintFactoryResponse:
+        """Stop reprovisioning (if any)."""


question: Do we really want this at all? Because if you start a reprovisioning but then stop it you leave the event queue in a weird inconsistent state. So maybe once you start a reprovisioning you have to wait for it to finish or error out.

Some 2 cents: while a delete action may not be desirable, there should be a way to force a reprovisioning to be marked as errored. You cannot always ensure that a job will report back it has errored, so an admin needs to be able to mark it that way (and attempt a new one).

Yeah the DELETE endpoint to stop a reprovisioning will stay.

olevski · 2024-09-27T13:56:35Z

components/renku_data_services/message_queue/orm.py

+    id: Mapped[ULID] = mapped_column("id", ULIDType, primary_key=True, default_factory=lambda: str(ULID()), init=False)
+    start_date: Mapped[datetime] = mapped_column("start_date", DateTime(timezone=True), nullable=False)


question: How do we handle errors during reprovisioning? And we have 2 kind here:

Errors that fully crash the whole reprovisioning process

Errors that may result from trying to reprovision a specific entity only

At least for no 1 we should save something in the database and report it back to the admin. For no 2 the question is whether we want to keep going if we encounter errors with some entities or do we fully stop?

And related to this is another question. What do we do if we encounter an error? Should we try to delete the / cleanup the events that were added so that we can effectively rollback to the pre-reprovisioining request state?

components/renku_data_services/message_queue/core.py

olevski

That is great just one minor thing I noticed in the apispec.

components/renku_data_services/message_queue/api.spec.yaml

RenkuBot · 2024-10-03T14:36:54Z

Tearing down the temporary RenkuLab deplyoment for this PR.

m-alisafaee force-pushed the pitch/reprovisioning-search branch 2 times, most recently from 1bdbd6f to f0100b2 Compare September 18, 2024 22:20

m-alisafaee temporarily deployed to renku-ci-ds-392 September 19, 2024 11:11 — with GitHub Actions Inactive

m-alisafaee temporarily deployed to renku-ci-ds-392 September 19, 2024 13:42 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from d4fa5aa to 7a35a65 Compare September 25, 2024 03:26

m-alisafaee had a problem deploying to renku-ci-ds-392 September 25, 2024 03:26 — with GitHub Actions Error

m-alisafaee force-pushed the pitch/reprovisioning-search branch from 7a35a65 to 6e4aa7c Compare September 25, 2024 03:33

m-alisafaee temporarily deployed to renku-ci-ds-392 September 25, 2024 03:34 — with GitHub Actions Inactive

m-alisafaee temporarily deployed to renku-ci-ds-392 September 25, 2024 07:39 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from d69d7b8 to 66a3610 Compare September 25, 2024 07:56

m-alisafaee temporarily deployed to renku-ci-ds-392 September 25, 2024 07:57 — with GitHub Actions Inactive

m-alisafaee added 4 commits September 25, 2024 20:41

feat: support for search reprovisioning

2e5d8e7

send events in fg

c321440

swagger

ccc1870

refactor

a028799

m-alisafaee force-pushed the pitch/reprovisioning-search branch from 66a3610 to a028799 Compare September 25, 2024 18:41

m-alisafaee temporarily deployed to renku-ci-ds-392 September 25, 2024 18:42 — with GitHub Actions Inactive

m-alisafaee marked this pull request as ready for review September 25, 2024 19:55

m-alisafaee requested a review from a team as a code owner September 25, 2024 19:55

m-alisafaee temporarily deployed to renku-ci-ds-392 September 25, 2024 19:55 — with GitHub Actions Inactive

olevski requested changes Sep 27, 2024

View reviewed changes

eikek reviewed Oct 1, 2024

View reviewed changes

components/renku_data_services/message_queue/core.py Outdated Show resolved Hide resolved

m-alisafaee marked this pull request as draft October 3, 2024 01:09

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 01:09 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from a8d9d6f to eedd5b5 Compare October 3, 2024 08:29

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 08:30 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from 95583bc to 2868fe3 Compare October 3, 2024 09:28

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 09:28 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from 2868fe3 to 9b5ec4c Compare October 3, 2024 10:00

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 10:00 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from 9b5ec4c to 4a2c768 Compare October 3, 2024 11:41

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 11:41 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from 4a2c768 to 21316e2 Compare October 3, 2024 11:54

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 11:55 — with GitHub Actions Inactive

m-alisafaee force-pushed the pitch/reprovisioning-search branch from 21316e2 to eb521da Compare October 3, 2024 12:09

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 12:10 — with GitHub Actions Inactive

address review comments

d545020

m-alisafaee force-pushed the pitch/reprovisioning-search branch from eb521da to d545020 Compare October 3, 2024 12:30

m-alisafaee had a problem deploying to renku-ci-ds-392 October 3, 2024 12:31 — with GitHub Actions Error

Merge branch 'main' into pitch/reprovisioning-search

a25d687

m-alisafaee temporarily deployed to renku-ci-ds-392 October 3, 2024 12:33 — with GitHub Actions Inactive

m-alisafaee marked this pull request as ready for review October 3, 2024 12:45

m-alisafaee requested a review from olevski October 3, 2024 12:45

olevski requested changes Oct 3, 2024

View reviewed changes

components/renku_data_services/message_queue/api.spec.yaml Outdated Show resolved Hide resolved

fix

13d8bb0

m-alisafaee deployed to renku-ci-ds-392 October 3, 2024 14:09 — with GitHub Actions View deployment

m-alisafaee enabled auto-merge (squash) October 3, 2024 14:10

olevski self-requested a review October 3, 2024 14:12

olevski approved these changes Oct 3, 2024

View reviewed changes

m-alisafaee merged commit d475878 into main Oct 3, 2024
12 of 13 checks passed

m-alisafaee deleted the pitch/reprovisioning-search branch October 3, 2024 14:36

leafty added a commit that referenced this pull request Oct 7, 2024

fix after #392

bc9d8a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for search reprovisioning #392

feat: support for search reprovisioning #392

m-alisafaee commented Sep 10, 2024 •

edited

Loading

coveralls commented Sep 10, 2024 •

edited

Loading

RenkuBot commented Sep 19, 2024

olevski left a comment

olevski Sep 27, 2024

olevski Sep 27, 2024

m-alisafaee Oct 3, 2024

olevski Sep 27, 2024

leafty Oct 3, 2024

olevski Oct 3, 2024

olevski Sep 27, 2024

olevski left a comment

RenkuBot commented Oct 3, 2024

		This table is used to make sure that only one instance of reprovisioning is run at any given time.
		It gets updated with the reprovisioning progress.

		def delete(self) -> BlueprintFactoryResponse:
		"""Stop reprovisioning (if any)."""

		id: Mapped[ULID] = mapped_column("id", ULIDType, primary_key=True, default_factory=lambda: str(ULID()), init=False)
		start_date: Mapped[datetime] = mapped_column("start_date", DateTime(timezone=True), nullable=False)

feat: support for search reprovisioning #392

feat: support for search reprovisioning #392

Conversation

m-alisafaee commented Sep 10, 2024 • edited Loading

coveralls commented Sep 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11163620994

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

RenkuBot commented Sep 19, 2024

olevski left a comment

Choose a reason for hiding this comment

olevski Sep 27, 2024

Choose a reason for hiding this comment

olevski Sep 27, 2024

Choose a reason for hiding this comment

m-alisafaee Oct 3, 2024

Choose a reason for hiding this comment

olevski Sep 27, 2024

Choose a reason for hiding this comment

leafty Oct 3, 2024

Choose a reason for hiding this comment

olevski Oct 3, 2024

Choose a reason for hiding this comment

olevski Sep 27, 2024

Choose a reason for hiding this comment

olevski left a comment

Choose a reason for hiding this comment

RenkuBot commented Oct 3, 2024

m-alisafaee commented Sep 10, 2024 •

edited

Loading

coveralls commented Sep 10, 2024 •

edited

Loading