Add WMWorkload.validateSiteLisitsUpdate method. #12260

todor-ivanov · 2025-02-11T16:24:10Z

Status

ready

Description

During the validation of the user provided request_args we have a step to mandatory check against the spec arguments from here:

WMCore/src/python/WMCore/WMSpec/WMWorkload.py

Line 2074 in 0283414

validateArgumentsUpdate(schema, argumentDefinition, optionKey=None)

and subsequently the siteLists from here:

WMCore/src/python/WMCore/WMSpec/WMWorkloadTools.py

Line 294 in 0283414

validateSiteLists(arguments)

Unfortunately the so defined function at WMWorloadTools.validateSiteLists sets the sitelists to empty lists by default (even in the case they are completely missing from the so provided by the user request_args):

WMCore/src/python/WMCore/WMSpec/WMWorkloadTools.py

Lines 247 to 259 in 0283414

    
           def validateSiteLists(arguments): 
        
               whiteList = arguments.get("SiteWhitelist", []) 
        
               blackList = arguments.get("SiteBlacklist", []) 
        
               whiteList = makeList(whiteList) 
        
               blackList = makeList(blackList) 
        
               res = (set(whiteList) & set(blackList)) 
        
               if len(res): 
        
                   msg = "Validation failed: The same site cannot be white and blacklisted: %s" % list(res) 
        
                   raise WMSpecFactoryException(msg) 
        
               # store the properly formatted values (list instead of string) 
        
               arguments["SiteWhitelist"] = whiteList 
        
               arguments["SiteBlacklist"] = blackList 
        
               return

This causes the following behavior:

During the call to validate a dictionary of arguments provided by the user which is missing the siteWhitelist or SiteBlacklist at ReqMgr2 - here:

WMCore/src/python/WMCore/ReqMgr/Utils/Validation.py

Line 116 in 0283414

workload.validateArgumentsPartialUpdate(request_args)

The siteLists are set to empty lists before we enter the sequence of steps to update both the request at ReqMgr and all the WorkQueuElements.

Here is a printout of this effect produced by an interactive debugging session again together with @mapellidario:

[11/Feb/2025:15:34:51]  Updating request "tivanov_TaskChain_LumiMask_multiRun_SiteListsTest_v7_241210_093449_7256" with these user-provided args: {'RequestPriority': 700}
Before Validation:{'RequestPriority': 700}
After  Validation:{'RequestPriority': 700, 'SiteWhitelist': [], 'SiteBlacklist': []}
Calling _handleNoStatusUpdate
[11/Feb/2025:15:34:51]  Handling request: tivanov_TaskChain_LumiMask_multiRun_SiteListsTest_v7_241210_093449_7256 with CurrentRequest status: running-open
[11/Feb/2025:15:34:51]  reqArgs: {'RequestPriority': 700, 'SiteWhitelist': []}

This way the so produced empty lists enter as argument difference in reqArgsDiff and are considered as a change to empty SiteWhilist and SiteBlacklist and upon that updated in the workflow spec. Causing the so reported bug.

Here is the result after applying the current patch:

[11/Feb/2025:15:52:11]  Updating request "tivanov_TaskChain_LumiMask_multiRun_SiteListsTest_v7_241210_093449_7256" with these user-provided args: {'RequestPriority': 900}
Before Validation:{'RequestPriority': 900}
After  Validation:{'RequestPriority': 900}
Calling _handleNoStatusUpdate
[11/Feb/2025:15:52:11]  Handling request: tivanov_TaskChain_LumiMask_multiRun_SiteListsTest_v7_241210_093449_7256 with CurrentRequest status: running-open
[11/Feb/2025:15:52:11]  reqArgs: {'RequestPriority': 900}

It is obvious now that the Request._handleNoStatusUpdate method is called without the empty siteLists

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

YES

Related PRs

The PR introducing the bug was: #12099

External dependencies / deployment changes

N/A

dmwm-bot · 2025-02-11T16:36:41Z

Jenkins results:

Python3 Unit tests: succeeded
- 2 changes in unstable tests
Python3 Pylint check: failed
- 4 warnings and errors that must be fixed
- 1 warnings
- 68 comments to review
Pycodestyle check: succeeded
- 18 comments to review

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

dmwm-bot · 2025-02-11T16:44:41Z

Jenkins results:

Python3 Unit tests: succeeded
- 3 changes in unstable tests
Python3 Pylint check: failed
- 4 warnings and errors that must be fixed
- 1 warnings
- 68 comments to review
Pycodestyle check: succeeded
- 18 comments to review

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

amaltaro

Thank you for providing this fix. In addition to the comment along the code, I also have a comment on the WMWorkloadTools, but I'd rather have it here than along the code. So here it goes.

Even though that looks the way to go, I am not sure whether that is the correct system-wide fix.

When a workflow is created or assigned, the workflow factory is loaded and arguments are:

set to a default value
validated the data type
validated the content
usually following the StdBase + sub-class spec. For the site lists, we use this definition for instance:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/StdSpecs/StdBase.py#L1147

so I am afraid that with this change, we will be either causing an error somewhere else or setting these to None as default.

I'd suggest to test/investigate those cases as well.

amaltaro · 2025-02-11T20:33:52Z

src/python/Utils/Utilities.py

@@ -33,6 +33,8 @@ def makeList(stringList):
    throws a ValueError if the input is not well formed.
    If the stringList is already of type list, then return it untouched.
    """
+    # NOTE: In case of a change to this function beware of side effects which


Given that the Utils package is supposed to be agnostic of any WMCore-specific logic, I would suggest to remove this comment, please.

todor-ivanov · 2025-02-12T10:17:02Z

ok this:

When a workflow is created or assigned, the workflow factory is loaded and arguments are:

set to a default value

validated the data type

validated the content
usually following the StdBase + sub-class spec. For the site lists, we use this definition for instance:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/StdSpecs/StdBase.py#L1147

Tells the whole story. This function has been already developed under the presumption there is no previously set parameters at the workflow. In addition to that, it will not only NOT work for updates of already existing parameters, but it would rather miss eventual SiteBlacklist vs. SiteWhitelists conflicts between the sets of already existing + new arguments. Because in the current version of this function it checks for such conflicts only in among the site lists provided with the call but not with the already existing ones.

I am going to change the code. And will not touch the old one in any way. It would be better in my opinion.

dmwm-bot · 2025-02-12T12:57:59Z

Jenkins results:

Python3 Unit tests: succeeded
- 4 changes in unstable tests
Python3 Pylint check: failed
- 5 warnings and errors that must be fixed
- 1 warnings
- 81 comments to review
Pycodestyle check: succeeded
- 16 comments to review

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

todor-ivanov · 2025-02-12T13:37:32Z

And here follow the printouts from Reqmgr's logs and the HTTP headers returned for a set of calls, generated with an external script to a patched Reqmgr instance:

Changing only the priority of a workflow: