Separate state transition validation from nonState transition valdations

src/python/WMCore/ReqMgr/DataStructs/RequestStatus.py

@@ -128,10 +128,10 @@
                             "NonCustodialSites", "Override",
                             "SubscriptionPriority"],
     "assigned": ["RequestPriority"],
-    "staging": ["RequestPriority"],
+    "staging": ["RequestPriority", "SiteWhitelist", "SiteBlacklist"],
     "staged": ["RequestPriority"],
-    "acquired": ["RequestPriority"],
-    "running-open": ["RequestPriority"],
+    "acquired": ["RequestPriority", "SiteWhitelist", "SiteBlacklist"],
+    "running-open": ["RequestPriority", "SiteWhitelist", "SiteBlacklist"],
     "running-closed": ["RequestPriority"],
     "failed": [],
     "force-complete": [],
@@ -146,6 +146,18 @@
     "rejected-archived": [],
 }
 
+# NOTE: We need to Explicitly add RequestStatus to reqArgsDiff, since it is
+#       missing from the ALLOWED_ACTIONS_FOR_STATUS mapping. The alternative
+#       would be to add it as allowed action to every state.
+#       The same applies to few more, such as all the keys needed for the
+#       workqueue_stat_validation() calls, but we do this only during
+#       request parameters validation.
+ALLOWED_ACTIONS_ALL_STATUS = ["RequestStatus"]
+
+# NOTE: We need to explicitly add all stat keys during validation
+#       They are needed for the workqueue_stat_validation() calls
+ALLOWED_STAT_KEYS = ['total_jobs', 'input_lumis', 'input_events', 'input_num_files']
+
 # Workflow state transition automatically controlled by ReqMgr2
 ### NOTE: order of this list matters and it's used for status transition
 AUTO_TRANSITION = {"staged": ["acquired", "running-open", "running-closed", "completed"],
@@ -184,7 +196,8 @@ def get_modifiable_properties(status=None):
     TODO: Currently gets the result from hardcoded list. change to get from configuration or db
     """
     if status:
-        return ALLOWED_ACTIONS_FOR_STATUS.get(status, 'all_attributes')
+        allowedKeys = ALLOWED_ACTIONS_FOR_STATUS.get(status, 'all_attributes')
+        return allowedKeys
     else:
         return ALLOWED_ACTIONS_FOR_STATUS
 

src/python/WMCore/ReqMgr/Service/Request.py

@@ -10,6 +10,7 @@
 import traceback
 
 import cherrypy
+from copy import deepcopy
 
 import WMCore.ReqMgr.Service.RegExp as rx
 from WMCore.Database.CMSCouch import CouchError
@@ -404,24 +405,56 @@ def _handleNoStatusUpdate(self, workload, request_args, dn):
         """
         For no-status update, we only support the following parameters:
          1. RequestPriority
-         2. Global workqueue statistics, while acquiring a workflow
+         2. SiteWhitelist
+         3. SiteBlacklist
+         4. Global workqueue statistics, while acquiring a workflow
+        As Global workqueue statistics updates are exclusive to the rest of the
+        parameters. Meaning if it is about to be updated all the rest of the
+        request_args will be ignored.
         """
-        if 'RequestPriority' in request_args:
-            # Yes, we completely ignore any other arguments posted by the user (web UI case)
-            request_args = {'RequestPriority': request_args['RequestPriority']}
-            validate_request_priority(request_args)
-            # must update three places: GQ elements, workload_cache and workload spec
-            self.gq_service.updatePriority(workload.name(), request_args['RequestPriority'])
-            report = self.reqmgr_db_service.updateRequestProperty(workload.name(), request_args, dn)
-            workload.setPriority(request_args['RequestPriority'])
-            workload.saveCouchUrl(workload.specUrl())
-            cherrypy.log('Updated priority of "{}" to: {}'.format(workload.name(), request_args['RequestPriority']))
-        elif workqueue_stat_validation(request_args):
-            report = self.reqmgr_db_service.updateRequestStats(workload.name(), request_args)
-            cherrypy.log('Updated workqueue statistics of "{}", with:  {}'.format(workload.name(), request_args))
-        else:
-            msg = "There are invalid arguments for no-status update: %s" % request_args
-            raise InvalidSpecParameterValue(msg)
+        reqArgs = deepcopy(request_args)
+
+        if not reqArgs:
+            cherrypy.log(f"Nothing to be changed at this stage for {workload.name()}")
+            return 'OK'
+
+        if workqueue_stat_validation(reqArgs):
+            report = self.reqmgr_db_service.updateRequestStats(workload.name(), reqArgs)
+            cherrypy.log('Updated workqueue statistics of "{}", with:  {}'.format(workload.name(), reqArgs))
+            return report
+
+        reqArgsNothandled = []
+        for reqArg in reqArgs:
+            if reqArg == 'RequestPriority':
+                validate_request_priority(reqArgs)
+                # must update three places: GQ elements, workload_cache and workload spec
+                self.gq_service.updatePriority(workload.name(), reqArgs['RequestPriority'])
+                workload.setPriority(reqArgs['RequestPriority'])
+                cherrypy.log('Updated priority of "{}" to: {}'.format(workload.name(), reqArgs['RequestPriority']))
+            elif reqArg == "SiteWhitelist":
+                workload.setSiteWhitelist(reqArgs["SiteWhitelist"])
+                cherrypy.log('Updated SiteWhitelist of "{}", with:  {}'.format(workload.name(), reqArgs['SiteWhitelist']))
+            elif reqArg == "SiteBlacklist":
+                workload.setSiteBlacklist(reqArgs["SiteBlacklist"])
+                cherrypy.log('Updated SiteBlacklist of "{}", with:  {}'.format(workload.name(), reqArgs['SiteBlacklist']))
+            else:
+                reqArgsNothandled.append(reqArg)
+                cherrypy.log("Unhandled argument for no-status update: %s" % reqArg)
+
+        reqStatus = self.reqmgr_db_service.getRequestByNames(workload.name())[workload.name()]['RequestStatus']
+        cherrypy.log(f"CurrentRequest status: {reqStatus}")
+        if reqArgsNothandled:
+            if reqStatus == 'assignment-approved':
+                cherrypy.log(f"Handling assignment-approved arguments differently!")
+                self._handleAssignmentStateTransition(workload, request_args, dn)
+            else:
+                msg = "There were unhandled arguments left for no-status update: %s" % reqArgsNothandled
+                raise InvalidSpecParameterValue(msg)
+
+        # Commit the changes of the current workload object to the database:
+        workload.saveCouchUrl(workload.specUrl())
+
+        report = self.reqmgr_db_service.updateRequestProperty(workload.name(), reqArgs, dn)
 
         return report
 

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

@@ -6,14 +6,15 @@
 from future.utils import viewitems, viewvalues
 
 from hashlib import md5
+from copy import deepcopy
 
 from Utils.PythonVersion import PY3
 from Utils.Utilities import encodeUnicodeToBytesConditional
 from WMCore.Lexicon import procdataset
 from WMCore.REST.Auth import authz_match
 from WMCore.ReqMgr.DataStructs.Request import initialize_request_args, initialize_clone
 from WMCore.ReqMgr.DataStructs.RequestError import InvalidStateTransition, InvalidSpecParameterValue
-from WMCore.ReqMgr.DataStructs.RequestStatus import check_allowed_transition, STATES_ALLOW_ONLY_STATE_TRANSITION
+from WMCore.ReqMgr.DataStructs.RequestStatus import check_allowed_transition, get_modifiable_properties, STATES_ALLOW_ONLY_STATE_TRANSITION, ALLOWED_STAT_KEYS
 from WMCore.ReqMgr.Tools.cms import releases, architectures, dashboardActivities
 from WMCore.Services.DBS.DBS3Reader import getDataTiers
 from WMCore.WMFactory import WMFactory
@@ -22,11 +23,48 @@
 from WMCore.WMSpec.WMWorkloadTools import loadSpecClassByType, setArgumentsWithDefault
 from WMCore.Cache.GenericDataCache import GenericDataCache, MemoryCacheStruct
 
+
 def workqueue_stat_validation(request_args):
     stat_keys = ['total_jobs', 'input_lumis', 'input_events', 'input_num_files']
     return set(request_args.keys()) == set(stat_keys)
 
 
+def _validate_request_allowed_args(reqArgs, newReqArgs):
+    """
+    Compares two request configuration dictionaries and returns the difference
+    between them, but only at the first level (no recursive operations are attempted).
+    Returns the key/value pairs taken from the newReqArgs only if they are allowed
+    for the given request status.
+    :param reqArgs:    A dictionary with the current request definition
+    :param newReqArgs: A dictionary with user-provided request parameter changes
+    :return:           A dictionary reflecting the difference between the above two
+    NOTE:    This is asymmetrical operation/comparison, where newReqArs' values
+             are considered to take precedence but not the other way around. They
+             are in fact checked if they differ from the values at reqArgs and only those
+             items which differ are returned.
+    """
+    reqArgsDiff = {}
+    status = reqArgs["RequestStatus"]
+
+    # Checking all keys that differ:
+    reqArgsDiffKeys = []
+    for key in newReqArgs:
+        if key in reqArgs and newReqArgs[key] == reqArgs[key]:
+            continue
+        else:
+            reqArgsDiffKeys.append(key)
+
+    allowedKeys = deepcopy(get_modifiable_properties(status))
+    allowedKeys.extend(ALLOWED_STAT_KEYS)
+
+    # Filter out all fields which are not allowed for the given source status:
+    for key in reqArgsDiffKeys:
+        if key in allowedKeys:
+            reqArgsDiff[key] = newReqArgs[key]
+
+    return reqArgsDiff
+
+
 def validate_request_update_args(request_args, config, reqmgr_db_service, param):
     """
     param and safe structure is RESTArgs structure: named tuple
@@ -43,11 +81,15 @@ def validate_request_update_args(request_args, config, reqmgr_db_service, param)
     """
     # this needs to be deleted for validation
     request_name = request_args.pop("RequestName")
+
     couchurl = '%s/%s' % (config.couch_host, config.couch_reqmgr_db)
     workload = WMWorkloadHelper()
     workload.loadSpecFromCouch(couchurl, request_name)
 
     # validate the status
+    # NOTE: For state transition validation we do not consider request parameter
+    #       difference, but we rather act against the request parameter changes
+    #       as they have been requested
     if "RequestStatus" in request_args:
         validate_state_transition(reqmgr_db_service, request_name, request_args["RequestStatus"])
         if request_args["RequestStatus"] in STATES_ALLOW_ONLY_STATE_TRANSITION:
@@ -58,10 +100,14 @@ def validate_request_update_args(request_args, config, reqmgr_db_service, param)
             return workload, args_only_status
         elif request_args["RequestStatus"] == 'assigned':
             workload.validateArgumentForAssignment(request_args)
-
-    validate_request_priority(request_args)
-
-    return workload, request_args
+        validate_request_priority(request_args)
+        return workload, request_args
+    else:
+        request = reqmgr_db_service.getRequestByNames(request_name)
+        request = request[request_name]
+        reqArgsDiff = _validate_request_allowed_args(request, request_args)
+        validate_request_priority(reqArgsDiff)
+        return workload, reqArgsDiff
 
 
 def validate_request_priority(reqArgs):

test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py

@@ -8,8 +8,11 @@
 import unittest
 
 from WMCore.ReqMgr.DataStructs.RequestError import InvalidSpecParameterValue
+from WMCore.ReqMgr.DataStructs.RequestStatus import ACTIVE_STATUS, get_modifiable_properties
 from WMCore.ReqMgr.Utils.Validation import (validateOutputDatasets,
-                                            validate_request_priority)
+                                            validate_request_priority,
+                                            _validate_request_allowed_args)
+from WMCore.WMSpec.StdSpecs.StdBase import StdBase
 
 
 class ValidationTests(unittest.TestCase):
@@ -74,5 +77,28 @@ def testRequestPriorityValidation(self):
                 validate_request_priority(reqArgs)
 
 
+    def testValidateRequestAllowedArgs(self):
+        """
+        Tests the `_validate_request_allowed_args` functions, which validates two pairs
+        of request arguments and returns the difference between them and on top of that
+        applies a filter of allowed parameters changes per status
+        :return: nothing, raises an exception if there are problems
+        """
+        defReqArgs = StdBase.getWorkloadAssignArgs()
+        newReqArgs = {key: None for key in defReqArgs.keys()}
+
+        for status in ACTIVE_STATUS:
+            # NOTE: We need to add the RequestStatus artificially and assign it
+            #       to the currently tested active status
+            defReqArgs["RequestStatus"] = status
+            expectedReqArgs = {key: None for key in get_modifiable_properties(status)}
+            reqArgsDiff = _validate_request_allowed_args(defReqArgs, newReqArgs)
+            reqArgsDiff.pop("RequestStatus")
+            print(f"reqArgsDiff: {reqArgsDiff}")
+            print(f"expectedReqArgs: {expectedReqArgs}")
+            self.assertDictEqual(reqArgsDiff, expectedReqArgs)
+            print("===============================")
+
+
 if __name__ == '__main__':
     unittest.main()