From fd3cd66e1eec61ba05dfe38c084cd7c5b937e828 Mon Sep 17 00:00:00 2001 From: Todor Ivanov Date: Wed, 21 Aug 2024 13:24:08 +0200 Subject: [PATCH 1/2] Add SiteList actions to transient statuses in ReqMgr2 Add proper checks for allowed request properties changes && Stop reducing request_args only to RequestPriority for noStatusTransition actions Extend allowed arguments including stat_keys and RequestStatus && Call the relevant modifiers from _handleNoStatusUpdate Fix missing RequestName from request_args on mutipple calls of validate_request_update_args Add proper log messages for Sitelists changes Remove redundant validation calls && Update reqdb couch with single action && Move reduceReport to Utils. Typo Remove forgotten commented lines of code Review comments Source files pylint fixes Review comments --- .../ReqMgr/DataStructs/RequestStatus.py | 23 +++++-- src/python/WMCore/ReqMgr/Service/Request.py | 59 ++++++++++++----- src/python/WMCore/ReqMgr/Utils/Validation.py | 63 ++++++++++++++++--- 3 files changed, 115 insertions(+), 30 deletions(-) diff --git a/src/python/WMCore/ReqMgr/DataStructs/RequestStatus.py b/src/python/WMCore/ReqMgr/DataStructs/RequestStatus.py index 68ccde18b2..fa99c0d7fa 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,10 @@ 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') + if not allowedKeys == 'all_attributes': + allowedKeys.extend(ALLOWED_ACTIONS_ALL_STATUS) + 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..caa9d4ed67 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,25 +405,51 @@ 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 + reqArgs = deepcopy(request_args) + + if not reqArgs: + cherrypy.log("Nothing to be changed at this stage") + 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) + + if reqArgsNothandled: + 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 def _handleAssignmentApprovedTransition(self, workload, request_args, dn): diff --git a/src/python/WMCore/ReqMgr/Utils/Validation.py b/src/python/WMCore/ReqMgr/Utils/Validation.py index e1cdea84ee..cab4964611 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,25 +81,30 @@ 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) + request = reqmgr_db_service.getRequestByNames(request_name) + request = request[request_name] + reqArgsDiff = _validate_request_allowed_args(request, request_args) + # validate the status - 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: + if "RequestStatus" in reqArgsDiff: + validate_state_transition(reqmgr_db_service, request_name, reqArgsDiff["RequestStatus"]) + if reqArgsDiff["RequestStatus"] in STATES_ALLOW_ONLY_STATE_TRANSITION: # if state change doesn't allow other transition nothing else to validate args_only_status = {} - args_only_status["RequestStatus"] = request_args["RequestStatus"] - args_only_status["cascade"] = request_args.get("cascade", False) + args_only_status["RequestStatus"] = reqArgsDiff["RequestStatus"] + args_only_status["cascade"] = reqArgsDiff.get("cascade", False) return workload, args_only_status - elif request_args["RequestStatus"] == 'assigned': - workload.validateArgumentForAssignment(request_args) + elif reqArgsDiff["RequestStatus"] == 'assigned': + workload.validateArgumentForAssignment(reqArgsDiff) - validate_request_priority(request_args) + validate_request_priority(reqArgsDiff) - return workload, request_args + return workload, reqArgsDiff def validate_request_priority(reqArgs): From 901463d05f71df5d4295b3cf8aba4b0dd58e3209 Mon Sep 17 00:00:00 2001 From: Todor Ivanov Date: Fri, 6 Sep 2024 17:41:07 +0200 Subject: [PATCH 2/2] Unit tests Unit tests Unit tests pylint fixes Unit tests - remove tests for reduceReport --- .../WMCore_t/ReqMgr_t/Utils_t/Validation_t.py | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) 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..5d70179234 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,27 @@ 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) + print(f"reqArgsDiff: {reqArgsDiff}") + print(f"expectedReqArgs: {expectedReqArgs}") + self.assertDictEqual(reqArgsDiff, expectedReqArgs) + print("===============================") + + if __name__ == '__main__': unittest.main()