Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Integration] Modify dirac-admin-get-pilot-output to get remote pilot log #7105

Conversation

martynia
Copy link
Contributor

@martynia martynia commented Jul 10, 2023

BEGINRELEASENOTES

*WMS
NEW: Enhance dirac-admin-get-pilot-output to download remote pilot logs. Currently only from a SE.

ENDRELEASENOTES

Changes are:

  1. dirac_admin_get_pilot_output.py - add a -r switch to get remote pilot logs rather than classic ones.
  2. DiracAdmin.py to call a service method for remote logs
  3. PilotManagerHandler.py to load a plugin for pilot logs retrieval.
  4. FileCacheDownloadPlugin.py a plugin to download a log file and return it to a client

@martynia martynia force-pushed the integration_janusz_pilotlogsWrapper_get_dev branch 2 times, most recently from eb6b989 to 7ef7644 Compare July 11, 2023 10:07
@martynia martynia force-pushed the integration_janusz_pilotlogsWrapper_get_dev branch 4 times, most recently from 968bc9a to 5079df4 Compare July 25, 2023 09:41
src/DIRAC/Interfaces/API/DiracAdmin.py Outdated Show resolved Hide resolved
@@ -431,7 +431,7 @@ def resetJob(self, jobID):
return result

#############################################################################
def getJobPilotOutput(self, jobID, directory=""):
def getJobPilotOutput(self, jobID, directory="", remote=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow it does not feel right that user would have to decide about this (seems too low level to be exposed at API level)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be an other option? If it is used in an Dirac-admin command then it is an expert route. The intention was to have a classic log retrieval mechanism a default one, and the external log retriever only used in special cases, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then things can be tried in this order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try to get logs in a specific order: classic then remote. The specific order could even be configurable on the server. The point is, that if someone ever wants to test the remote log retrieval and the classic logs come first, then there is no way to get the remote logs by using a dirac-admin command line if we drop the -r flag.

So my proposal is:

  • remove the -r flag
  • set the default order to classic/remote
  • add a config flag to the CS (remoteLogsPriority=True) or similar. Default False.

@fstagni
Copy link
Contributor

fstagni commented Jul 26, 2023

You would need to rebase and fix the conflicts.

@martynia martynia force-pushed the integration_janusz_pilotlogsWrapper_get_dev branch from e4de37d to 7c8eed5 Compare July 26, 2023 14:26
@martynia
Copy link
Contributor Author

I rebased, but there are some strange errors I don't recognise...

Comment on lines +40 to +41
lfn = os.path.join(uploadPath, pilotStamp + ".log")
sLog.info("LFN to download: ", lfn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really an LFN? Looks like just a path to a file to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. It is passed to getFile later

Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid we have been using this pattern in other places, but os.path is not the most precise way to manipulate/create LFNs, as this is OS-dependent. We might argue that we only run on Linux so it does not really matter, but more precise would be to at least to use posixpath instead (@chaen and @atsareg might comment)

@martynia martynia force-pushed the integration_janusz_pilotlogsWrapper_get_dev branch from c026d53 to 705f6b5 Compare August 15, 2023 13:41
@martynia martynia force-pushed the integration_janusz_pilotlogsWrapper_get_dev branch 2 times, most recently from 45f05a1 to 6611f60 Compare September 12, 2023 10:25
@martynia martynia force-pushed the integration_janusz_pilotlogsWrapper_get_dev branch from 6611f60 to 21f3471 Compare September 12, 2023 12:43
@fstagni fstagni marked this pull request as ready for review September 14, 2023 08:50
@fstagni fstagni merged commit efa94db into DIRACGrid:integration Sep 19, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants