Implement kill, delete and remove endpoints #64

simon-mazenoux · 2023-08-24T15:48:50Z

Implement kill, delete and remove endpoints (see #22, #63)

simon-mazenoux · 2023-09-12T09:17:26Z

In this PR, and unlike what is done in DIRAC, I used the ON DELETE CASCADE constraint. This change would therefore require an ALTER statement on some tables of the DB. Should we do this or duplicate the logic of what is currently done in DIRAC ?

chrisburr · 2023-09-12T09:25:37Z

In this PR, and unlike what is done in DIRAC, I used the ON DELETE CASCADE constraint. This change would therefore require an ALTER statement on some tables of the DB. Should we do this or duplicate the logic of what is currently done in DIRAC ?

This seems very sensible to me, would any changes be needed in DIRAC v8.1 to make it tollerant to this? We should probably discuss it in the BiLD, can you open an issue in vanilla DIRAC?

simon-mazenoux · 2023-09-12T09:35:52Z

In this PR, and unlike what is done in DIRAC, I used the ON DELETE CASCADE constraint. This change would therefore require an ALTER statement on some tables of the DB. Should we do this or duplicate the logic of what is currently done in DIRAC ?

This seems very sensible to me, would any changes be needed in DIRAC v8.1 to make it tollerant to this? We should probably discuss it in the BiLD, can you open an issue in vanilla

I think DIRAC is already tolerant: by adding the ON DELETE CASCADE constraint, we would just avoid having to delete manually every foreign key reference to the row we want to delete. Therefore, I don't really see how it could affect the codebase besides not raising an error anymore if a foreign key is not deleted before deleting the primary key.

fstagni · 2023-09-12T10:13:49Z

src/diracx/db/jobs/schema.py

@@ -198,3 +210,99 @@ class LoggingInfo(JobLoggingDBBase):
    StatusTimeOrder = Column(Numeric(precision=12, scale=3), default=0)
    StatusSource = Column(String(32), default="Unknown")
    __table_args__ = (PrimaryKeyConstraint("JobID", "SeqNum"),)
+
+
+class TaskQueues(TaskQueueDBBase):


This is just stylistic, but: you added this in the same module where JobDB (and JobLoggingDB) is. SandboxMetadataDB is instead defined in another module. We either put all "WMS-related" schemas together or we split them in different files like it was in DIRAC.

fstagni · 2023-09-12T10:15:56Z

src/diracx/db/jobs/schema.py

+    )
+
+
+class BannedSitesQueue(TaskQueueDBBase):


Would you check if this is still used anywhere?

This is still used by the JobScheduling optimizer (__sendToTQ method that create/fills the JobRequirements section of the JDL, which is then interpreted by the insertIntoTQ method of the JobState class which calls the TQDB insertJob method). This can be the case if a user specifies a list of banned sites and the job doesn't require any site in particular.

However, I would argue that having a BannedSitesQueue table is not very appealing.

One solution would be to get the list of all sites, remove from this list the banned sites and insert the remaining sites in the tq_TQToSites table.

I think we should also ask the community if banning sites is a real use case, because this feature brings quite a lot of complexity.

This feature is there because, most probably, of some (LHCb) user requirement of long time ago. This reminds me that we have often argued that it did not make much sense and that could even create issues. I would be tempted to remove it also.

As long as we use the ON DELETE CASCADE constraint in DIRAC to remove the entries from this table, then I think we can remove this table from diracx.

src/diracx/db/sandbox_metadata/db.py

src/diracx/db/jobs/db.py

src/diracx/db/jobs/status_utility.py

src/diracx/routers/job_manager/__init__.py

tests/routers/test_job_manager.py

chrisburr · 2023-09-13T03:40:27Z

src/diracx/routers/job_manager/__init__.py

+        await asyncio.gather(
+            *(
+                delete_job(
+                    job_id,
+                    config,
+                    job_db,
+                    job_logging_db,
+                    task_queue_db,
+                    background_task,
+                )
+                for job_id in job_ids
+            )
+        )


Would it be practical to make this an actual bulk operation on the DB side? i.e. something like:

DELETE FROM JOBS WHERE JOB_ID IN (123, 124, 125, 126)

rather than

DELETE FROM JOBS WHERE JOB_ID = 123 DELETE FROM JOBS WHERE JOB_ID = 124 DELETE FROM JOBS WHERE JOB_ID = 125 DELETE FROM JOBS WHERE JOB_ID = 126

(This is probably better as an issue and then a separate PR rather than adding complexity to this one.)

I ended up doing it here because I had problems using the TaskGroup+except* as you advised, and I wanted to see if making bulk methods could help me avoid theses. In the end, I still had to use them, but most of the work was already done. Sorry for the complexity.

chrisburr · 2023-09-13T03:50:05Z

src/diracx/routers/job_manager/__init__.py

+                for job_id in job_ids
+            )
+        )
+    except NoResultFound as e:


We should probably use a TaskGroup+except* instead of gather so we get the better exception handling.

Ideally I think we should return a more explicit error like:

detail={ "message": "Failed to remove 5 jobs out of 167", "failed_ids": [1, 78, 32, 25, 90] }

or maybe even:

detail={ "message": "Failed to remove 5 jobs out of 167", "failed_ids": { "1": str(e1), "78": str(e2), "32": str(e3), "25": str(e4), "90": str(e5) } }

Do you think we should cancel any action if one of them fail ? (for example here should we remove the other 162 jobs or not ?)

If that’s practical I think that would be nice. What do you think @fstagni @chaen?

If that’s practical I think that would be nice.

I concur.

@chaen Agrees as well

Linked discussion: #97

simon-mazenoux · 2023-09-25T07:39:16Z

@chaen I think the pytest-integration tests are failing because the TaskQueueDB and SandboxMetadataDB are not in diracx-charts. Is this normal ? If so, do you want me to add them in a pull request there, or should I let you do it?

simon-mazenoux marked this pull request as draft August 24, 2023 15:49

simon-mazenoux changed the title ~~Implement taskqueuedb~~ Implement Taskqueuedb Aug 24, 2023

simon-mazenoux changed the title ~~Implement Taskqueuedb~~ Implement TaskQueueDB Aug 24, 2023

simon-mazenoux force-pushed the feat-implement-taskqueuedb branch 3 times, most recently from f61699c to 6e86eb1 Compare August 30, 2023 16:38

simon-mazenoux force-pushed the feat-implement-taskqueuedb branch 2 times, most recently from d64eb2a to bd52dc7 Compare September 11, 2023 08:39

simon-mazenoux changed the title ~~Implement TaskQueueDB~~ Implement kill and delete endpoints Sep 11, 2023

simon-mazenoux force-pushed the feat-implement-taskqueuedb branch from bd52dc7 to 92ce100 Compare September 12, 2023 08:57

simon-mazenoux changed the title ~~Implement kill and delete endpoints~~ Implement kill, delete and remove endpoints Sep 12, 2023

simon-mazenoux force-pushed the feat-implement-taskqueuedb branch from 92ce100 to 9d9c21f Compare September 12, 2023 09:12

simon-mazenoux marked this pull request as ready for review September 12, 2023 09:14

fstagni reviewed Sep 12, 2023

View reviewed changes

simon-mazenoux force-pushed the feat-implement-taskqueuedb branch 2 times, most recently from 9b4990c to 993d2d4 Compare September 12, 2023 14:22

chrisburr reviewed Sep 13, 2023

View reviewed changes

simon-mazenoux force-pushed the feat-implement-taskqueuedb branch 7 times, most recently from f17af3a to f503216 Compare September 21, 2023 16:20

chaen mentioned this pull request Sep 26, 2023

Add TaskQueueDB DIRACGrid/diracx-charts#42

Merged

simon-mazenoux force-pushed the feat-implement-taskqueuedb branch from f503216 to 30e2adf Compare September 28, 2023 07:45

chrisburr force-pushed the feat-implement-taskqueuedb branch from 30e2adf to 1721185 Compare October 26, 2023 09:00

simon-mazenoux and others added 2 commits October 26, 2023 11:38

feat: implement kill, delete and remove job endpoints

7107065

Regenerate client

b333ae4

chrisburr force-pushed the feat-implement-taskqueuedb branch from 1721185 to b333ae4 Compare October 26, 2023 09:38

chrisburr merged commit ba6966b into DIRACGrid:main Oct 26, 2023
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement kill, delete and remove endpoints #64

Implement kill, delete and remove endpoints #64

simon-mazenoux commented Aug 24, 2023 •

edited

Loading

simon-mazenoux commented Sep 12, 2023

chrisburr commented Sep 12, 2023

simon-mazenoux commented Sep 12, 2023

fstagni Sep 12, 2023

fstagni Sep 12, 2023

simon-mazenoux Sep 12, 2023

fstagni Sep 12, 2023

simon-mazenoux Sep 12, 2023

chrisburr Sep 13, 2023

simon-mazenoux Sep 18, 2023

chrisburr Sep 13, 2023

simon-mazenoux Sep 13, 2023

chrisburr Sep 13, 2023

fstagni Sep 15, 2023

chrisburr Sep 15, 2023

simon-mazenoux Sep 21, 2023

simon-mazenoux commented Sep 25, 2023

Implement kill, delete and remove endpoints #64

Implement kill, delete and remove endpoints #64

Conversation

simon-mazenoux commented Aug 24, 2023 • edited Loading

simon-mazenoux commented Sep 12, 2023

chrisburr commented Sep 12, 2023

simon-mazenoux commented Sep 12, 2023

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

Choose a reason for hiding this comment

simon-mazenoux commented Sep 25, 2023

simon-mazenoux commented Aug 24, 2023 •

edited

Loading