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

[v6r20] MultiVO TransformationSystem #3723

Merged
merged 13 commits into from
Jul 6, 2018

Conversation

andresailer
Copy link
Contributor

@andresailer andresailer commented Jun 14, 2018

These changes make the WorkflowTaskAgent and RequestTask agent work without a shifterProxy by setting the owner as the delegatedDN/group for all the submission clients to the WMS and RMS.
This should be completely optional. If shifterProxy is set things should work as before.

  • TaskManagerAgentBase: add option ShifterCredentials to set the credentials to use for all submissions, this is single VO only
  • TaskManagerAgentBase: WorkflowTasks/RequestTasks: pass ownerDN and ownerGroup parameter to all the submission clients if using shifterProxy ownerDN and ownerGroup are None thus reproducing the original behaviour
  • TaskManagerAgentBase: refactor adding operations for transformation to separate function to ensure presence of Owner/DN/Group in dict entries
  • RequestTaskAgent no longer sets shifterProxy by default.

Plus some refactoring and minor fixes

Addresses #3695

@coveralls
Copy link

coveralls commented Jun 14, 2018

Coverage Status

Coverage increased (+0.003%) to 20.607% when pulling dc8ddf9 on andresailer:multiVoSquashed into 6057929 on DIRACGrid:rel-v6r20.

@@ -110,6 +111,27 @@ def execute(self):
"""

operationsOnTransformationDict = {}
owner, ownerGroup, ownerDN = None, None, None
# getting the credentials for submission
if getProxyInfo(False, False)['OK']: # there is a shifterProxy
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you name the parameters please ?

owner, ownerGroup, ownerDN = None, None, None
# getting the credentials for submission
if getProxyInfo(False, False)['OK']: # there is a shifterProxy
res = getProxyInfo(False, False)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could call that only once for the test and the result ?


if not requestClient:
self.requestClient = ReqClient()
self.requestClient = ReqClient(useCertificates=useCertificates,
delegatedDN=ownerDN,
Copy link
Contributor

Choose a reason for hiding this comment

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

Pfiou :-) Have you seen this actually used somewhere, or do you just trust the comments I've put a couple of years ago ? :D

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact I am not sure how this works.
Do you set the ownerDN and the ownerGroup of the Request somewhere ? If not, it tries to get it from proxyInfo, which would not do anything now if I understand properly.
However, with the rpcStub fix that was merged lately, I am not sure of what will happen with these delegatedDN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean these comments? 🤣

:param delegatedDN: Not clear what it can be used for.
:param delegatedGroup: Not clear what it can be used for.

I saw those, laughed, looked at git blame praise, tried it out and saw that it worked. Otherwise I would have given up on this route.

I also thought about the stub changes and rebased my developments yesterday, things are still working.

The ownerDN/group of the requests are (already) set in the RequestTasks TaskManager

def _assignRequestToTask(self, oRequest, taskDict, transID, taskID, ownerDN, ownerGroup):
"""set ownerDN and group to request, and add the request to taskDict if it is
valid, otherwise remove the task from the taskDict
:param oRequest: Request
:param dict taskDict: dictionary of tasks, modified in this function
:param int transID: Transformation ID
:param int taskID: Task ID
:param str ownerDN: certificate DN used for the requests
:param str onwerGroup: dirac group used for the requests
:returns: None
"""
oRequest.RequestName = self._transTaskName(transID, taskID)
oRequest.OwnerDN = ownerDN
oRequest.OwnerGroup = ownerGroup

Copy link
Contributor

Choose a reason for hiding this comment

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

:D yes, these.
Good that it works. Not sure what you are praising though ! :D

@andresailer andresailer force-pushed the multiVoSquashed branch 2 times, most recently from fbcc138 to c3655cc Compare June 18, 2018 15:07
@fstagni
Copy link
Contributor

fstagni commented Jun 19, 2018

The looks OK to me, but you need to add info here: https://github.com/DIRACGrid/DIRAC/wiki/DIRAC-v6r21 and in the documentation, both the technical docu of the agents and in the /docs part.

@andresailer
Copy link
Contributor Author

andresailer commented Jun 20, 2018

both the technical docu of the agents and in the /docs part.

What do you mean with technical doc ? configuration parameters etc?

@andresailer
Copy link
Contributor Author

@andresailer
Copy link
Contributor Author

For proper working of the RTA we need a fix for #3727

@fstagni
Copy link
Contributor

fstagni commented Jun 20, 2018

Yes I meant what you have done: config parameters inside the code.

@andresailer andresailer force-pushed the multiVoSquashed branch 2 times, most recently from 9fd701c to e278278 Compare June 21, 2018 12:06
@andresailer
Copy link
Contributor Author

Added the config like parameter descriptions for direct comparison, will remove the other one later on.
(moving from table to this is more effort than manipulating the table in emacs)

https://diracsailer.readthedocs.io/en/multivosquashed/CodeDocumentation/TransformationSystem/Agent/RequestTaskAgent.html (clear your cache if you don't see it)

Also realised that the doc was wrong two options are actually UpdateFilesStatus and UpdateTasksStatus...

@@ -254,12 +247,28 @@ def _getClients(self):
return {'TransformationClient': threadTransformationClient,
'TaskManager': threadTaskManager}

def _getDelegatedClients(self, ownerDN=None, ownerGroup=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you simply modify _getClients() instead of adding one more function? For extensions is one less thing to extend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, moved the parameters into _getClients

@andresailer
Copy link
Contributor Author

@andresailer
Copy link
Contributor Author

@atsareg any comment for the layout of the parameters? table vs config like structure?

@andresailer andresailer changed the title [integration] MultiVO TransformationSystem [v6r21] MultiVO TransformationSystem Jun 25, 2018
@fstagni
Copy link
Contributor

fstagni commented Jul 5, 2018

Will be rebased on v6r20

@fstagni fstagni changed the title [v6r21] MultiVO TransformationSystem [WIP][v6r21] MultiVO TransformationSystem Jul 5, 2018
@andresailer andresailer changed the base branch from integration to rel-v6r20 July 5, 2018 10:50
@andresailer andresailer requested a review from zmathe as a code owner July 5, 2018 10:50
TaskManagerAgentBase: add option ShifterCredentials to set the credentials to use for all submissions, this is single VO only
TaskManagerAgentBase: WorkflowTasks/RequestTasks: pass ownerDN and ownerGroup parameter to all the submission clients if using shifterProxy ownerDN and ownerGroup are None thus reproducing the original behaviour
TaskManagerAgentBase: refactor adding operations for transformation to separate function to ensure presence of Owner/DN/Group in dict entries
execute is running after _execute(?), so we need to find ShifterCredentials in initialize
@andresailer andresailer changed the title [WIP][v6r21] MultiVO TransformationSystem [v6r20] MultiVO TransformationSystem Jul 5, 2018
@andresailer
Copy link
Contributor Author

@atsareg atsareg merged commit b8d9fc7 into DIRACGrid:rel-v6r20 Jul 6, 2018
@andresailer andresailer deleted the multiVoSquashed branch July 6, 2018 15:32
@fstagni fstagni mentioned this pull request Nov 2, 2019
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.

5 participants