Add support for customName (custom container DID) #11765
Conversation
|
Jenkins results:
|
vkuznet
changed the title
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
vkuznet
force-pushed
the
fix-issue-11734
branch
from
fcd9512
to
8d1fb77
Compare
|
Jenkins results:
|
|
Alan, Todor, I made initial changes which are ready to review. I see one Rucio test failing which I think is not related to this PR. Please review and provide your feedback of what else need to be done. |
There was a problem hiding this comment.
Valentin, this is looking good in general. I did leave a couple of comments along the code though.
Comparing these changes against what has been requested in the ticket, I think you covered it all but the new group scope that we need to have someone from Rucio to create it for us.
I also wanted to point out that the adoption of the custom container was still somehow uncertain, in the tickets and as we discussed these developments. I do think it will be the best option though.
src/python/WMCore/MicroService/MSPileup/DataStructs/MSPileupObj.py
Outdated
Show resolved
Hide resolved
|
Alan, also please clarify if something needs to be done in WMCore codebase for new group scope you mentioned. I also see on original ticket mention of Rucio DID and lexicon rules. But for both items I interpreted that it is outside of WMCore codebase and will be done externally. Please advise if I'm wrong. |
|
Jenkins results:
|
src/python/WMCore/MicroService/MSPileup/DataStructs/MSPileupObj.py
Outdated
Show resolved
Hide resolved
For the new group scope, it will be just a matter of having one of the CMS Rucio developers to create it in Rucio. After that, we can simply start creating DIDs against the new scope. As far as I can see, scope has already been parameterized in all of the methods that we might need it: |
|
Jenkins results:
|
| @@ -18,6 +18,7 @@ | |||
| "deactivatedOn": int, seconds since epoch in GMT timezone (service-based) | |||
| "active": boolean, (mandatory) | |||
| "pileupSize": integer, current size of the pileup in bytes (service-based) | |||
| "customName": string, custom container DID (opitonal) | |||
There was a problem hiding this comment.
Please fix this typo as well (opitonal)
vkuznet
force-pushed
the
fix-issue-11734
branch
from
ac29481
to
ebc3841
Compare
|
Jenkins results:
|
|
Jenkins results:
|
vkuznet
force-pushed
the
fix-issue-11734
branch
from
ebc3841
to
3bff311
Compare
|
Jenkins results:
|
There was a problem hiding this comment.
@vkuznet Valentin, these changes look good to me. However, we still have to contact Eric/Rucio developers to decide which scope we want to create for this new data. Perhap we could use the DMWM mattermost channel for that discussion (or DM Dev Forum)?
And with that, it just occurred to me that, whenever we are dealing with a custom pileup dataset name, we need to also provide the new scope name to the Rucio queries. In other words, instead of using the standard cms scope, we should pass the scope used for customName pileups. For that, I would suggest to have a separate commit in this PR not to change what has already been reviewed.
|
Jenkins results:
|
|
Thank you for providing these changes, Valentin. The only options I see though are:
Any thoughts? |
|
I understand your concern and I was trying to find best solution which I implemented as option 1. I doubt that option 2 is feasible since it will require a-prior knowledge of dataset presence, setting up MongoDB backend for testing, etc. I see it much more expensive. The changes I made to code are minimal and if you want they can be further separated into production and mock functions, i.e. |
|
I thought the mocking was isolated to the Pycurl module, but I see it also changes the other MSPileup module. Why do you think we need to setup MongoDB? I see no difference between the mock rucio or real rucio, given that none of those depend on MongoDB. On what concerns finding a pileup dataset, that's easily achievable by: Said that, I am inclined to removing emulation code from the actual production baseline code and run a unit test as integration. Once you are happy with the result, just flag the unit test as integration not to have it executed by Jenkins. |
|
The mocking is required since we need to test |
|
Oh, I didn't see that there was a mongodb record update within that function. At the moment we do not have MongoDB in our test setup. A few alternatives for that would be:
I would suggest to do that in a future ticket/pull request though. UPDATE: I still would like to have the mocking-related code - within the core code - removed from this PR. |
|
ok, so the plan would be:
Is this plan correct? BTW, we already have mocking for MongoDB, and my unit test which I provided does that. |
|
Yes, that's the plan with the following corrections/complement:
we should also remove this emulation-related code: https://github.com/dmwm/WMCore/pull/11765/files#diff-f6418aca4acfed87f5215da3ab07dcbf5c33fbcdc5b2617135eb20c676a1bf25R69 , which I failed to find in a specific commit.
it will follow a release candidate model with the usual pre-production validation of the WM ecosystem. |
|
ok, mock code which emulates the rucio token has been removed. And, I adjusted unit test to not check Please have a look and proceed with this PR review (or merge). Then I can move towards testbed testing. |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
author Valentin Kuznetsov <vkuznet@gmail.com> 1697051710 -0400 committer Valentin Kuznetsov <vkuznet@gmail.com> 1700501711 -0500 gpgsig -----BEGIN PGP SIGNATURE----- iHUEABEIAB0WIQROzG8FXytELxEiq36vnSwZCcnsGQUCZVuYzwAKCRCvnSwZCcns GUU4AQCIVcEWuhGRgiCBz0B2/3iMqZzU7wJ9wHLfUZLT55Ml9AD/T1sgPzYDwwML HKayNiZqBYGCoAclikaA1zmNwmDgRNY= =CZww -----END PGP SIGNATURE----- Add support for customName (custom container DID) Add new mockup data to use custom rucio scope modify pileupSizeTask to return containers (used in unit test) Allow usage of mock tokens Added rucioParams to mock API as it is used in tests Remove mock codebase which emulates rucio token initialize datasetSizes dict
vkuznet
force-pushed
the
fix-issue-11734
branch
from
3517c41
to
d93f903
Compare
|
Jenkins results:
|
|
Alan, commits are squashed and I fixed |
|
Alan, for the record, the commit name was auto-generated after I squashed changes and re-order commits. Turns out the order was important and I resolved different conflicts and final commit message was auto-generated by github. I did not want to mess more with it, as it provided reference to previous commits in order they were committed. Thanks for final approval and merge though. I'm glad we made it before holidays. |
|
@amaltaro , I tried to run integration test and encountered the following error. I took one dataset from https://cmsweb-testbed.cern.ch/ms-pileup/data/pileup and put it into integration test, when I run it I see the following exception: So, to proceed I think we need some dataset associated with |
|
@vkuznet Valentin, now that we have upgraded central services, could you please add I was about to create a GH issue to fix a KeyError exception in WorkflowUpdater code, but it's likely better if we just make everything consistent and update the pileup documents in production as well (all the testbed documents already contain that attribute). |
Fixes #11734
Status
In development
Description
Add support for new customName attribute (custon container DID)
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
External dependencies / deployment changes