feat: enable remote pilot logging system #269

martynia · 2024-07-01T14:05:11Z

Enable remote logging system with open search. This PR only shows a router in its preliminary form.
The router defines following operations:

@router.post("/") - bulk insert log record sent by a pilot. The method returns a pilotID, so it can use it next time to insert messaged w/o consulting PilotAgentsDB to get PilotID from PilotStamp.
@router.get("/logs") - to retrieve logs for a given pilot_id
@router.delete("/logs") - to delete logs by:
- pilotID
- from a given minimum date till now.
- for a date range - not yet implemented

Policies allow pilots to store VO-aware logs and operators/admins to perform delete operations for all VOs. Pilots and normal users can only see (get) logs for their own VO.

martynia · 2024-07-01T14:45:28Z

@chrisburr I think we need a pilot logging router, even in its basic form. One of the tests fails because it is missing.

martynia · 2024-10-25T09:34:33Z

@chrisburr What is a reason of test failures here ? The demo runs fine locally, can be contacted with a client etc. The test fails with dirac login, waiting for diracX to be installed. Could it be that I missed a setting somewhere ? The failure is persistent.

martynia · 2024-11-01T15:54:23Z

@chrisburr @chaen
I managed to reproduce the problem locally. There is the same failure on GitHub and locally. Since the demo runs fine (the DB is there), is it possible that integration tests fail (NotImplementedError: Cannot enable system_name='pilotlogs' as it requires missing_os_dbs={<class 'diracx.db.os.pilot_logs.PilotLogsDB'>} because of charts being no discoverable by the CI workflow ?

This type of error occurs (in the demo) where charts don't list a relevant DB

Both local and CI tests reach the same point - server installation failure.

chrisburr · 2024-11-14T10:17:34Z

https://github.com/DIRACGrid/DIRAC/blob/7fe654eef5f4b0c23a7047e6e046580b1e354e97/tests/CI/docker-compose.yml#L174-L209

martynia · 2024-11-21T11:03:56Z

@chaen I fixed the database problem, now integration tests are passing, however I'm getting gubbins errors in unit tests. Is it an internal gubbins problem ?

diracx-routers/src/diracx/routers/pilot_logging/__init__.py

diracx-routers/pyproject.toml

diracx-db/src/diracx/db/os/utils.py

aldbr · 2024-12-20T15:01:21Z

diracx-routers/src/diracx/routers/pilot_logging/remote_logger.py

+    # here, users with privileged properties will see logs from all VOs. Is it what we want ?
+    search_params = [{"parameter": "PilotID", "operator": "eq", "value": pilot_id}]
+    if _non_privileged(user_info):
+        search_params.append(
+            {"parameter": "VO", "operator": "eq", "value": user_info.vo}
+        )
+    result = await db.search(
+        ["Message"],
+        search_params,
+        [{"parameter": "LineNumber", "direction": "asc"}],
+    )
+    if not result:
+        return [{"Message": f"No logs for pilot ID = {pilot_id}"}]


Similarly, this bloc of code should be a method of diracx-dbs/src/diracx/db/sql/pilots/pilot_agents.py PilotAgentsDB

aldbr · 2024-12-20T15:02:59Z

diracx-routers/src/diracx/routers/pilot_logging/remote_logger.py

+    """Delete either logs for a specific PilotID or a creation date range.
+    Non-privileged users can only delete log files within their own VO.
+    """
+    message = "no-op"


Why do you initialize message as "no-op"?

Well, this is a return message to the client. As far as I can see, the delete by query operation is silent. It does not complain when you delete something which doesn't not exist etc. Or I might have missed something in the docs.

Well, this is a return message to the client.

But IIUC, this "no-op" can't be returned to the client, right?
Then may be you can just initialize it to None instead of "no-op"?

As far as I can see, the delete by query operation is silent. It does not complain when you delete something which doesn't not exist etc. Or I might have missed something in the docs.

You mean that if you pass a wrong pilot id or a wrong date to OpenSearch, it does not inform you whether pilot ids and/or dates are all valid?

I am not an expert of opensearch, I don't have much knowledge about it, but from what I can read, https://opensearch.org/docs/latest/api-reference/document-apis/delete-by-query/ seems to provide the number of deleted items, but not the details (which ones).

One solution to identify them could consist in searching for the pilot ids first, and then return the states of each one after the delete query.

Something like (this is pseudo-code):

pilots_ids: [1,2,3,4,5] result = search(pilots_ids) # result = [1,2,3] unknown_pilot_ids = set(pilot_ids) - set(result) # unkown_pilots_ids = [4,5] delete_by_query(result) return {"success": result ,"failed": unknown_pilot_ids}

aldbr · 2024-12-20T15:15:52Z

diracx-routers/src/diracx/routers/pilot_logging/remote_logger.py

+def _non_privileged(user_info: AuthorizedUserInfo):
+    return (
+        SERVICE_ADMINISTRATOR not in user_info.properties
+        and OPERATOR not in user_info.properties
+    )


I feel like this should be moved in the access_policies.py function? May be by adding an action type? Or by adding some logic like in:

diracx/diracx-routers/src/diracx/routers/jobs/access_policies.py

Lines 79 to 82 in 320c554

job_owners = await job_db.summary(

["Owner", "VO"],

[{"parameter": "JobID", "operator": "in", "values": job_ids}],

)

I had a logical problem what to do here. I would like to search only once (and delete by query for a delete operation w/o prior searching). This is a reason why I removed the DB argument from a policy call. The problem is more visible in the delete case:

I could search in the policy for a given client VO and if pilot IDs are not from the VO throw an exception (not sure what to do if some IDs are and some are not from the VO the client has - for a bulk PilotID search)

then delete by query (which searches again)

Or could I use the AgentsDB to match the VO ? Still an extra DB query, this time and SQL one.
I opted for a restricted silent delete and and a restricted search by adding the VO, but outside the policy.

aldbr · 2024-12-20T15:23:27Z

diracx-routers/src/diracx/routers/pilot_logging/remote_logger.py

+
+@router.delete("/logs")
+async def delete(
+    pilot_id: int,


We also tend to make bulk requests to the db to improve the performances of the system. I would take a list of pilot_ids as inputs instead of a single pilot_id.

Then you can return sucess and failures as it is done for the jobs for instance:

diracx/diracx-routers/src/diracx/routers/jobs/status.py

Lines 35 to 62 in 320c554

@router.delete("/")

async def remove_bulk_jobs(

job_ids: Annotated[list[int], Query()],

config: Config,

job_db: JobDB,

job_logging_db: JobLoggingDB,

sandbox_metadata_db: SandboxMetadataDB,

task_queue_db: TaskQueueDB,

background_task: BackgroundTasks,

check_permissions: CheckWMSPolicyCallable,

):

"""Fully remove a list of jobs from the WMS databases.

WARNING: This endpoint has been implemented for the compatibility with the legacy DIRAC WMS

and the JobCleaningAgent. However, once this agent is ported to diracx, this endpoint should

be removed, and a status change to Deleted (PATCH /jobs/status) should be used instead for any other purpose.

"""

await check_permissions(action=ActionType.MANAGE, job_db=job_db, job_ids=job_ids)

return await remove_jobs(

job_ids,

config,

job_db,

job_logging_db,

sandbox_metadata_db,

task_queue_db,

background_task,

)

aldbr · 2024-12-20T15:23:56Z

diracx-routers/src/diracx/routers/pilot_logging/remote_logger.py

+
+@router.get("/logs")
+async def get_logs(
+    pilot_id: int,


Similarly,

Suggested change

pilot_id: int,

pilot_ids: List[int],

aldbr · 2025-01-10T16:21:19Z

diracx-routers/src/diracx/routers/pilots/logging.py

+        if _non_privileged(user_info):
+            search_params.append(non_privil_params)
+        await db.delete(search_params)
+        message = f"Logs for for pilots with submission data >='{data.min}' successfully deleted"


Suggested change

message = f"Logs for for pilots with submission data >='{data.min}' successfully deleted"

message = f"Logs for pilots with submission data >='{data.min}' successfully deleted"

aldbr · 2025-01-10T16:49:04Z

diracx-routers/src/diracx/routers/pilots/logging.py

+    non_privil_params = {"parameter": "VO", "operator": "eq", "value": user_info.vo}
+
+    # id pilot_id is provided we ignore data.min and data.max
+    if not pilot_ids and data.min and data.max:


May be I didn't read carefully, but don't you mean:

Suggested change

if not pilot_ids and data.min and data.max:

if not pilot_ids and not (data.min or data.max):

The logic is correct. pilot_idhas priority, if it is not given then we have to cases:

a range is given both min and max (this requires a (simple) change in DiracX query language - open search support range out of the box; also open range.)

only max is given, then delete everything olde. that max.

aldbr · 2025-01-10T16:50:15Z

diracx-routers/src/diracx/routers/pilots/logging.py

+
+
+@router.delete("/logs")
+async def delete(


By the way, I am just curious, what is the use case of DELETE /pilots/logs?

to delete old logs ?

martynia mentioned this pull request Jul 18, 2024

feat: add PilotLogsDB to osDbs DIRACGrid/diracx-charts#108

Merged

martynia force-pushed the main branch from 9f225ae to 02c24a9 Compare July 19, 2024 14:37

martynia force-pushed the main branch from e90d0ea to 8155438 Compare August 9, 2024 16:37

martynia force-pushed the main branch from a6f1eff to a83c39d Compare September 19, 2024 16:13

martynia mentioned this pull request Oct 4, 2024

feat: Implement PilotAgents schema #292

Merged

martynia force-pushed the main branch from c0c4a28 to 43bd81e Compare October 16, 2024 13:54

martynia force-pushed the main branch from 43bd81e to ccf286f Compare October 25, 2024 09:05

martynia force-pushed the main branch from b8c3a42 to 85da2cc Compare November 1, 2024 14:37

martynia force-pushed the main branch from 1c22760 to 6217509 Compare November 21, 2024 10:06

martynia mentioned this pull request Nov 21, 2024

Fix URL definition to adapt to new version of pydantic #326

Closed

martynia force-pushed the main branch from 6217509 to 4209217 Compare November 28, 2024 13:26

martynia force-pushed the main branch from bd838a7 to 921adec Compare December 9, 2024 16:35

martynia force-pushed the main branch 2 times, most recently from 3b0ea4c to 6eb1ebd Compare December 19, 2024 13:47

aldbr requested changes Dec 20, 2024

View reviewed changes

martynia added 8 commits January 6, 2025 15:34

feat: enable remote pilot logging system

8da8e2b

test: add a policy test

e2d0bb3

feat: add remote_logger router test

02622b0

feat: implement bulk insert

050dc97

fix: fix multiple inheritance.

fb3dbb9

fix: remove refs to DB in check_permissions

0c1ecde

fix: fix pyproject.toml

f02efba

fix: refactoring pilot logging code

e9ad537

martynia force-pushed the main branch from d65c3bd to e9ad537 Compare January 6, 2025 14:57

fix: refactor pilot logging code (2)

752143c

martynia added 3 commits January 6, 2025 16:28

fix: fix test after rebasing

1774fb4

fix: modify method docstrings

fff5434

feat: implement bulk delete

2c59c0c

aldbr reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable remote pilot logging system #269

feat: enable remote pilot logging system #269

martynia commented Jul 1, 2024 •

edited

Loading

martynia commented Jul 1, 2024 •

edited

Loading

martynia commented Oct 25, 2024

martynia commented Nov 1, 2024

chrisburr commented Nov 14, 2024

martynia commented Nov 21, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

martynia Dec 20, 2024

aldbr Jan 10, 2025

aldbr Dec 20, 2024

martynia Dec 20, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

aldbr Jan 10, 2025

martynia Jan 10, 2025

aldbr Jan 10, 2025

martynia Jan 10, 2025

aldbr Jan 10, 2025

martynia Jan 10, 2025

	job_owners = await job_db.summary(
	["Owner", "VO"],
	[{"parameter": "JobID", "operator": "in", "values": job_ids}],
	)

	@router.delete("/")
	async def remove_bulk_jobs(
	job_ids: Annotated[list[int], Query()],
	config: Config,
	job_db: JobDB,
	job_logging_db: JobLoggingDB,
	sandbox_metadata_db: SandboxMetadataDB,
	task_queue_db: TaskQueueDB,
	background_task: BackgroundTasks,
	check_permissions: CheckWMSPolicyCallable,
	):
	"""Fully remove a list of jobs from the WMS databases.

	WARNING: This endpoint has been implemented for the compatibility with the legacy DIRAC WMS
	and the JobCleaningAgent. However, once this agent is ported to diracx, this endpoint should
	be removed, and a status change to Deleted (PATCH /jobs/status) should be used instead for any other purpose.
	"""
	await check_permissions(action=ActionType.MANAGE, job_db=job_db, job_ids=job_ids)

	return await remove_jobs(
	job_ids,
	config,
	job_db,
	job_logging_db,
	sandbox_metadata_db,
	task_queue_db,
	background_task,
	)

	message = f"Logs for for pilots with submission data >='{data.min}' successfully deleted"
	message = f"Logs for pilots with submission data >='{data.min}' successfully deleted"

	if not pilot_ids and data.min and data.max:
	if not pilot_ids and not (data.min or data.max):

feat: enable remote pilot logging system #269

Are you sure you want to change the base?

feat: enable remote pilot logging system #269

Conversation

martynia commented Jul 1, 2024 • edited Loading

martynia commented Jul 1, 2024 • edited Loading

martynia commented Oct 25, 2024

martynia commented Nov 1, 2024

chrisburr commented Nov 14, 2024

martynia commented Nov 21, 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

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

martynia commented Jul 1, 2024 •

edited

Loading

martynia commented Jul 1, 2024 •

edited

Loading