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 ?

aldbr · 2024-12-20T14:15:01Z

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

I would rename pilot_logging into pilots, and we will have everything related to the pilots in this directory. We tend to go in that direction (see the jobs directory).

OK, could do this.

aldbr · 2024-12-20T14:20:18Z

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

+
+import logging
+
+logger = logging.getLogger(__name__)


Don't you need to add the remote_logger router here?
What about renaming remote_logger.py as logging.py?

Suggested change

logger = logging.getLogger(__name__)

from .logging import router as logging_router

logger = logging.getLogger(__name__)

router = DiracxRouter()

router.include_router(logging_router)

aldbr · 2024-12-20T14:21:03Z

diracx-routers/pyproject.toml

@@ -48,6 +48,7 @@ types = [
 ]

 [project.entry-points."diracx.services"]
+pilotlogs = "diracx.routers.pilot_logging.remote_logger:router"


Following my previous comments:

Suggested change

pilotlogs = "diracx.routers.pilot_logging.remote_logger:router"

pilots = "diracx.routers.pilots.router"

aldbr · 2024-12-20T14:41:08Z

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

@@ -190,6 +191,13 @@ async def upsert(self, doc_id, document) -> None:
        )
        print(f"{response=}")

+    async def bulk_insert(self, index_name: str, docs: list[dict[str, Any]]) -> None:
+        # bulk inserting to database


Suggested change

# bulk inserting to database

"""bulk inserting to database."""

aldbr · 2024-12-20T14:41:37Z

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

+
+        # Delete multiple documents by query.
+


Suggested change

# Delete multiple documents by query.

"""Delete multiple documents by query."""

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.

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],

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 from 25ab41a to 3b0ea4c Compare December 19, 2024 13:23

martynia added 6 commits December 19, 2024 14:47

feat: enable remote pilot logging system

f4fe168

test: add a policy test

de43741

feat: add remote_logger router test

e0ae2a5

feat: implement bulk insert

667446f

fix: fix multiple inheritance.

60a8d17

fix: remove refs to DB in check_permissions

6eb1ebd

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

fix: fix pyproject.toml

d65c3bd

aldbr requested changes Dec 20, 2024

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

martynia Dec 20, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

martynia Dec 20, 2024

aldbr Dec 20, 2024

martynia Dec 20, 2024

aldbr Dec 20, 2024

aldbr Dec 20, 2024

-logger = logging.getLogger(__name__)
+from .logging import router as logging_router
+logger = logging.getLogger(__name__)
+router = DiracxRouter()
+router.include_router(logging_router)

	pilotlogs = "diracx.routers.pilot_logging.remote_logger:router"
	pilots = "diracx.routers.pilots.router"

	# bulk inserting to database
	"""bulk inserting to database."""


	# Delete multiple documents by query.
	"""Delete multiple documents by query."""

	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,
	)

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

martynia commented Jul 1, 2024 •

edited

Loading

martynia commented Jul 1, 2024 •

edited

Loading