MSOutput: read RelVal output data policy from configuration #11024

amaltaro · 2022-03-07T02:22:35Z

Fixes #10106

Status

ready

Description

This PR provides the following:

remove the hard-coded RelVal output logic in place of a configurable schema (defined within the MSOutput service configuration)
validate the policy data structure against is data type, RSE names, and datatier names
each RSE destination will get a copy of the output data

In addition to this, it also removes an obsolete use of the tapePledges configuration attribute.

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

None

External dependencies / deployment changes

Deployment changes: dmwm/deployment#1130
services_config: https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/131 and https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/130

amaltaro · 2022-03-07T02:25:20Z

@haozturk @bbilin could you please review the RelVal output data placement policy defined in this PR: dmwm/deployment#1130

I am planning to change the implementation of this PR, but I would appreciate if you could confirm that the RelVal policy is sound. Thanks

cmsdmwmbot · 2022-03-07T02:33:54Z

Jenkins results:

Python3 Unit tests: succeeded
- 8 tests added
- 2 changes in unstable tests
Python3 Pylint check: succeeded
Pylint py3k check: succeeded
Pycodestyle check: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/12848/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2022-03-07T17:13:08Z

Jenkins results:

Python3 Unit tests: succeeded
- 9 tests added
- 2 changes in unstable tests
Python3 Pylint check: succeeded
Pylint py3k check: succeeded
Pycodestyle check: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/12851/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2022-03-07T19:54:44Z

Jenkins results:

Python3 Unit tests: succeeded
- 10 tests added
Python3 Pylint check: succeeded
Pylint py3k check: succeeded
Pycodestyle check: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/12853/artifact/artifacts/PullRequestReport.html

bbilin · 2022-03-07T19:59:40Z

@amaltaro sorry for the latency in replying.

For the moment we are running all the relval chains at CERN, so there is no need to add FNAL to the rules. We would hence suggest for all the tiers you mention to be stored only at CERN.

Many thanks,

B. for PdmV

cmsdmwmbot · 2022-03-07T21:34:28Z

Jenkins results:

Python3 Unit tests: succeeded
- 10 tests added
- 1 changes in unstable tests
Python3 Pylint check: succeeded
Pylint py3k check: succeeded
Pycodestyle check: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/12854/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2022-03-07T21:41:31Z

Jenkins results:

Python3 Unit tests: succeeded
- 10 tests added
- 1 changes in unstable tests
Python3 Pylint check: succeeded
Pylint py3k check: succeeded
Pycodestyle check: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/12855/artifact/artifacts/PullRequestReport.html

todor-ivanov

Thanks for this PR @amaltaro. I do like the idea for separating the whole RelVal policy generation as a separate class. I've left few comments inline, which may be worth taking a look. None of them is a showstopper. Mostly requests for adding few lines for code documentation.

todor-ivanov · 2022-03-07T21:47:11Z

src/python/WMCore/MicroService/MSOutput/MSOutput.py

+        self.msConfig.setdefault("dbsUrl", "https://cmsweb-prod.cern.ch/dbs/prod/global/DBSReader")
+        allDBSDatatiers = getDataTiers(self.msConfig['dbsUrl'])
+        allDiskRSEs = self.rucio.evaluateRSEExpression("*", returnTape=False)
+        self.relvalPolicy = RelValPolicy(self.msConfig.get("relvalPolicy", []),


I'd say this configuration check here:
self.msConfig.get("relvalPolicy", [])

would be good to have as a separate default assignment like the rest msConfig fields.

It would be good also to have a simple Note either in the docstring or as an inline comment about what should this list contain.

todor-ivanov · 2022-03-07T21:54:57Z

src/python/WMCore/MicroService/MSOutput/RelValPolicy.py

+    This module will contain the RelVal output data placement policy, where
+    destinations will be decided according to the dataset datatier.
+    """
+    def __init__(self, policyDesc, listDatatiers, listRSEs, logger=None):


Could you please add a doc string or a note explaining what would be the expected input to the init method?

todor-ivanov · 2022-03-07T21:59:18Z

src/python/WMCore/MicroService/MSOutput/RelValPolicy.py

+        :param policyDesc: list of dictionaries with the policy definition
+        :param validDBSTiers: list with existent DBS datatiers
+        :param validDiskRSEs: list with existent Rucio Disk RSEs
+        :return: raise an exception if any validation fails


If the method does not return a value, I would format this line as:
No return value. The method raises an exception if any validation fails instead
or something similar.

todor-ivanov · 2022-03-07T22:04:01Z

src/python/WMCore/MicroService/MSOutput/RelValPolicy.py

+            msg = "The RelVal output data placement policy is not in the expected data type. "
+            msg += "Type expected: list, while the current data type is: {}. ".format(type(policyDesc))
+            msg += "This critical ERROR must be fixed."
+            raise RelValPolicyException(msg) from None


Once this is raised during the parameter validation process here, is there a place in the MSOutput module code to handle this exception, or it is left to be treated as a general exception and just the proper messages to be logged?

In case it fails, it's supposed to fail the MSOutput object initialization. Meaning, the service will crash until someone spots the problem.

That's fair.
Thanks

todor-ivanov · 2022-03-07T22:05:06Z

src/python/WMCore/MicroService/MSOutput/RelValPolicy.py

+            # validate the datatier
+            if not isinstance(item['datatier'], str):
+                msg = "The 'datatier' parameter must be a string, not {}.".format(type(item['datatier']))
+                raise RelValPolicyException(msg) from None


Same question as above. And also for the few similar cases bellow.

validate the RSE name as well Create RelValPolicy module; revert MSOutput changes fix __str__ method to use json.dumps instead set number of copies according to number of destinations fix attribute name apply Todors suggestions

remake unit tests added stringification unit test

cmsdmwmbot · 2022-03-08T02:19:47Z

Jenkins results:

Python3 Unit tests: succeeded
- 10 tests added
- 1 changes in unstable tests
Python3 Pylint check: succeeded
Pylint py3k check: succeeded
Pycodestyle check: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/12856/artifact/artifacts/PullRequestReport.html

amaltaro · 2022-03-08T02:41:20Z

Thanks for the review, Todor. I think I either answered or fixed the comments/requests you made.

If it looks good to you, could you please buy us some time and:

merge this PR
cut a new WMCore tag (I guess it's 2.0.1.pre5)
make a new cmsdist pull request with that tag
request that cmsdist PR - and all of the deployment/services_config - in the usual HG2203 requests gitlab ticket
?

todor-ivanov

This is looking pretty good.
Thanks @amaltaro

amaltaro · 2022-03-14T14:35:39Z

FYI I have also updated the documentation: https://github.com/dmwm/WMCore/wiki/ReqMgr2-MicroService-Output#relval-disk-policy
@haozturk

haozturk · 2022-03-15T09:31:29Z

perfect, thanks Alan!

amaltaro added PR: Do not merge yet PR: Work in progress labels Mar 7, 2022

amaltaro force-pushed the fix-10106 branch from 208cbad to 724885d Compare March 7, 2022 16:57

amaltaro requested a review from todor-ivanov March 7, 2022 19:33

amaltaro force-pushed the fix-10106 branch from 724885d to 771da3e Compare March 7, 2022 19:45

bbilin mentioned this pull request Mar 7, 2022

Define MSOutput policy for RelVal; remove legacy tapePledges dmwm/deployment#1130

Merged

amaltaro force-pushed the fix-10106 branch from 771da3e to 35f3ceb Compare March 7, 2022 21:21

amaltaro removed the PR: Work in progress label Mar 7, 2022

amaltaro force-pushed the fix-10106 branch from 35f3ceb to e985080 Compare March 7, 2022 21:33

todor-ivanov requested changes Mar 7, 2022

View reviewed changes

amaltaro added 2 commits March 7, 2022 21:06

unit tests and pylint

de788f3

remake unit tests added stringification unit test

amaltaro force-pushed the fix-10106 branch from e985080 to de788f3 Compare March 8, 2022 02:07

amaltaro removed the PR: Do not merge yet label Mar 8, 2022

amaltaro requested a review from todor-ivanov March 8, 2022 02:39

todor-ivanov approved these changes Mar 8, 2022

View reviewed changes

todor-ivanov merged commit a1da43b into dmwm:master Mar 8, 2022

amaltaro added the PR: Update documentation label Mar 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSOutput: read RelVal output data policy from configuration #11024

MSOutput: read RelVal output data policy from configuration #11024

amaltaro commented Mar 7, 2022 •

edited

Loading

amaltaro commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

bbilin commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

todor-ivanov left a comment

todor-ivanov Mar 7, 2022

todor-ivanov Mar 7, 2022

todor-ivanov Mar 7, 2022

todor-ivanov Mar 7, 2022

todor-ivanov Mar 7, 2022

amaltaro Mar 8, 2022

todor-ivanov Mar 8, 2022

todor-ivanov Mar 7, 2022

cmsdmwmbot commented Mar 8, 2022

amaltaro commented Mar 8, 2022

todor-ivanov left a comment

amaltaro commented Mar 14, 2022

haozturk commented Mar 15, 2022

MSOutput: read RelVal output data policy from configuration #11024

MSOutput: read RelVal output data policy from configuration #11024

Conversation

amaltaro commented Mar 7, 2022 • edited Loading

Status

Description

Is it backward compatible (if not, which system it affects?)

Related PRs

External dependencies / deployment changes

amaltaro commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

bbilin commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

cmsdmwmbot commented Mar 7, 2022

todor-ivanov left a 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

cmsdmwmbot commented Mar 8, 2022

amaltaro commented Mar 8, 2022

todor-ivanov left a comment

Choose a reason for hiding this comment

amaltaro commented Mar 14, 2022

haozturk commented Mar 15, 2022

amaltaro commented Mar 7, 2022 •

edited

Loading