-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add lifetime to RucioContainers shipping to disk sites #10950
Conversation
Jenkins results:
|
25ca441
to
0a891b0
Compare
Jenkins results:
|
@jhonatanamado Jhonatan, can you please link the relevant T0 changes to this PR? This will allow us to see all the required changes to have this feature fully functional. This unit test: need to be looked at as well. |
Hi @amaltaro, Sorry, indeed I forgot to add the relevant changes to the T0 code. Please check the following dmwm/T0#4642 About the unit test, I will look at it asap |
0a891b0
to
6fd97e5
Compare
Jenkins results:
|
Hi @amaltaro , Alan, the changes were tested with a replay without issues. Let me know if you have other suggestion. |
@jhonatanamado @germanfgv could you please clarify whether this issue is supposed to fix #10558 as well? Or is #10558 actually requesting some additional rucio rule parameters? |
Hi @amaltaro ,I did not remember that issue because I installed a cronjob to clean those 'block-level' rules but indeed, with some minor modifications in |
@jhonatanamado oh, I thought it was the same request. If it's slightly different, then I think working on that in a different PR will be the best. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhonatanamado these changes are looking good in general. However, I made some comments/requests along the code for your consideration. Thanks
@@ -49,6 +49,7 @@ def __init__(self, logger=None, dbi=None, params=None): | |||
subscribed INTEGER DEFAULT 0, | |||
phedex_group VARCHAR(100), | |||
delete_blocks INTEGER, | |||
dataset_lifetime INTEGER DEFAULT 46656000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jhonatan, if you are actually setting dataset_lifetime
for every dataset from the RunConfigAPI, then having a default value of 0
looks more reasonable to me.
In the RucioInjector code, we could even have a check like:
if dataset_lifetime > 0:
kwargs['lifetime'....
such that when it's 0, nothing happens. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, is more reasonable. I also modified RucioInjector in the clause of add that parameter
@@ -1079,6 +1079,9 @@ def getWorkloadCreateArgs(): | |||
"MergedLFNBase": {"default": "/store/data"}, | |||
"UnmergedLFNBase": {"default": "/store/unmerged"}, | |||
"DeleteFromSource": {"default": False, "type": strToBool}, | |||
|
|||
# Rucio rule subscription lifetime (used in ContainerRules by T0) | |||
"DatasetLifetime": {"default": 18*30*24*60*60, "type": int, "optional": True}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make it an assignment only parameter? Just like the other PhEDEx-related parameters that we have encoded in the wofkflow?
6fd97e5
to
b3f6010
Compare
Jenkins results:
|
b3f6010
to
8c7673a
Compare
Jenkins results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jhonatan, thanks for providing these changes. There are still a few items that require attention, please check comments along the code. Thanks
8c7673a
to
c982bfb
Compare
Jenkins results:
|
Add a new lifetime parameter for each RucioContainer rule that is subscribed to disk site
c982bfb
to
634c629
Compare
Jenkins results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for applying those changes, Jhonatan. It looks good to go now.
@jhonatanamado Jhonatan, if you want to run a fresh and clean replay with this code, I have just cut tag |
Great, thanks for the tag @amaltaro. I will do it and give to you a feedback on how it goes. |
Add a new lifetime parameter for each RucioContainer rule that is subscribed to disk site
Fixes #10746
Status
Tested with a replay
Description
For a given dataset (Rucio Container rule) processed by Tier0 and shipping to disk sites, add a new attribute of lifetime to the rucio container rule only if its subscribed at Disk sites
Is it backward compatible (if not, which system it affects?)
MAYBE
Related PRs
First discussed in #10747
External dependencies / deployment changes
No