diff --git a/src/python/WMCore/ReqMgr/DataStructs/RequestStatus.py b/src/python/WMCore/ReqMgr/DataStructs/RequestStatus.py index 68ccde18b2..d6e9c216fb 100644 --- a/src/python/WMCore/ReqMgr/DataStructs/RequestStatus.py +++ b/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 diff --git a/src/python/WMCore/ReqMgr/Service/Request.py b/src/python/WMCore/ReqMgr/Service/Request.py index d167ecbdb5..5cc404db42 100644 --- a/src/python/WMCore/ReqMgr/Service/Request.py +++ b/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 diff --git a/src/python/WMCore/ReqMgr/Utils/Validation.py b/src/python/WMCore/ReqMgr/Utils/Validation.py index e1cdea84ee..a76a1e4fcf 100644 --- a/src/python/WMCore/ReqMgr/Utils/Validation.py +++ b/src/python/WMCore/ReqMgr/Utils/Validation.py @@ -6,6 +6,7 @@ from future.utils import viewitems, viewvalues from hashlib import md5 +from copy import deepcopy from Utils.PythonVersion import PY3 from Utils.Utilities import encodeUnicodeToBytesConditional @@ -13,7 +14,7 @@ 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): diff --git a/test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py b/test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py index b147041a0e..6696d480b6 100644 --- a/test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py +++ b/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()