Core: allow move_rule to override all options Fix rucio#4995

Before this commit, `move_rule` could only override two attributes of the new
rule. After the commit, `move_rule` can override every field of the new rule with
the argument `override`.

bin/rucio

@@ -1443,10 +1443,16 @@ def move_rule(args):
     Update a rule.
     """
     client = get_client(args)
+
+    override = {}
+    if args.activity:
+        override['activity'] = args.activity
+    if args.source_replica_expression:
+        override['source_replica_expression'] = None if args.source_replica_expression.lower() == "none" else args.source_replica_expression
+
     print(client.move_replication_rule(rule_id=args.rule_id,
                                        rse_expression=args.rse_expression,
-                                       activity=args.activity,
-                                       source_replica_expression=args.source_replica_expression))
+                                       override=override))
     return SUCCESS
 
 
@@ -2524,8 +2530,8 @@ You can filter by account::
     move_rule_parser.set_defaults(function=move_rule)
     move_rule_parser.add_argument(dest='rule_id', action='store', help='Rule id')
     move_rule_parser.add_argument(dest='rse_expression', action='store', help='RSE expression of new rule')
-    move_rule_parser.add_argument('--activity', dest='activity', action='store', help='Update activity for moved rule')
-    move_rule_parser.add_argument('--source-replica-expression', dest='source_replica_expression', action='store', help='Update source-replica-expression for moved rule')
+    move_rule_parser.add_argument('--activity', dest='activity', action='store', help='Update activity for moved rule.')
+    move_rule_parser.add_argument('--source-replica-expression', dest='source_replica_expression', action='store', help='Update source-replica-expression for moved rule. Use "None" to remove the old value.')
 
     # The list-rses command
     list_rses_parser = subparsers.add_parser('list-rses', help='Show the list of all the registered Rucio Storage Elements (RSEs).')

lib/rucio/api/rule.py

@@ -317,31 +317,26 @@ def examine_replication_rule(rule_id, issuer, vo='def', session=None):
 
 
 @transactional_session
-def move_replication_rule(rule_id, rse_expression, activity, source_replica_expression, issuer, vo='def', session=None):
+def move_replication_rule(rule_id, rse_expression, override, issuer, vo='def', session=None):
     """
     Move a replication rule to another RSE and, once done, delete the original one.
 
     :param rule_id:                    Rule to be moved.
     :param rse_expression:             RSE expression of the new rule.
-    :param activity:                   Activity of the new rule.
-    :param source_replica_expression:  Source-Replica-Expression of the new rule.
+    :param override:                   Configurations to update for the new rule.
     :param session:                    The DB Session.
     :param vo:                         The VO to act on.
     :raises:                           RuleNotFound, RuleReplaceFailed, InvalidRSEExpression, AccessDenied
     """
     kwargs = {
         'rule_id': rule_id,
         'rse_expression': rse_expression,
-        'activity': activity,
-        'source_replica_expression': source_replica_expression
+        'override': override,
+        'session': session,
     }
     if is_multi_vo(session=session) and not has_permission(issuer=issuer, vo=vo, action='access_rule_vo', kwargs=kwargs, session=session):
         raise AccessDenied('Account %s can not access rules at other VOs.' % (issuer))
     if not has_permission(issuer=issuer, vo=vo, action='move_rule', kwargs=kwargs, session=session):
         raise AccessDenied('Account %s can not move this replication rule.' % (issuer))
 
-    return rule.move_rule(rule_id=rule_id,
-                          rse_expression=rse_expression,
-                          activity=activity,
-                          source_replica_expression=source_replica_expression,
-                          session=session)
+    return rule.move_rule(**kwargs)

lib/rucio/client/ruleclient.py

@@ -157,14 +157,13 @@ def reduce_replication_rule(self, rule_id, copies, exclude_expression=None):
         exc_cls, exc_msg = self._get_exception(headers=r.headers, status_code=r.status_code, data=r.content)
         raise exc_cls(exc_msg)
 
-    def move_replication_rule(self, rule_id, rse_expression, activity, source_replica_expression):
+    def move_replication_rule(self, rule_id, rse_expression, override):
         """
         Move a replication rule to another RSE and, once done, delete the original one.
 
         :param rule_id:                    Rule to be moved.
         :param rse_expression:             RSE expression of the new rule.
-        :param activity:                   Activity of the new rule.
-        :param source_replica_expression:  Source-Replica-Expression of the new rule.
+        :param override:                   Configurations to update for the new rule.
         :raises:                           RuleNotFound, RuleReplaceFailed
         """
 
@@ -173,8 +172,7 @@ def move_replication_rule(self, rule_id, rse_expression, activity, source_replic
         data = dumps({
             'rule_id': rule_id,
             'rse_expression': rse_expression,
-            'activity': activity,
-            'source_replica_expression': source_replica_expression
+            'override': override,
         })
         r = self._send_request(url, type_='POST', data=data)
         if r.status_code == codes.created:

lib/rucio/core/rule.py

@@ -16,7 +16,7 @@
 # Authors:
 # - Vincent Garonne <vincent.garonne@cern.ch>, 2012-2018
 # - Mario Lassnig <mario.lassnig@cern.ch>, 2013-2020
-# - Martin Barisits <martin.barisits@cern.ch>, 2013-2021
+# - Martin Barisits <martin.barisits@cern.ch>, 2013-2022
 # - Cedric Serfon <cedric.serfon@cern.ch>, 2014-2021
 # - David Cameron <david.cameron@cern.ch>, 2014
 # - Joaquín Bogado <jbogado@linti.unlp.edu.ar>, 2014-2018
@@ -52,6 +52,7 @@
 from datetime import datetime, timedelta
 from re import match
 from string import Template
+from typing import Dict, Any, Optional
 
 from dogpile.cache.api import NO_VALUE
 from six import string_types
@@ -1418,17 +1419,18 @@ def reduce_rule(rule_id, copies, exclude_expression=None, session=None):
 
 
 @transactional_session
-def move_rule(rule_id, rse_expression, activity=None, source_replica_expression=None, session=None):
+def move_rule(rule_id: str, rse_expression: str, override: Optional[Dict[str, Any]] = None, session=None):
     """
     Move a replication rule to another RSE and, once done, delete the original one.
 
     :param rule_id:                    Rule to be moved.
     :param rse_expression:             RSE expression of the new rule.
-    :param activity:                   Activity of the new rule.
-    :param source_replica_expression:  Source-Replica-Expression of the new rule.
+    :param override:                   Configurations to update for the new rule.
     :param session:                    The DB Session.
     :raises:                           RuleNotFound, RuleReplaceFailed, InvalidRSEExpression
     """
+    override = override or {}
+
     try:
         rule = session.query(models.ReplicationRule).filter_by(id=rule_id).one()
 
@@ -1444,22 +1446,34 @@ def move_rule(rule_id, rse_expression, activity=None, source_replica_expression=
 
         notify = {RuleNotification.YES: 'Y', RuleNotification.CLOSE: 'C', RuleNotification.PROGRESS: 'P'}.get(rule.notification, 'N')
 
-        new_rule_id = add_rule(dids=[{'scope': rule.scope, 'name': rule.name}],
-                               account=rule.account,
-                               copies=rule.copies,
-                               rse_expression=rse_expression,
-                               grouping=grouping,
-                               weight=rule.weight,
-                               lifetime=lifetime,
-                               locked=rule.locked,
-                               subscription_id=rule.subscription_id,
-                               source_replica_expression=source_replica_expression if source_replica_expression else rule.source_replica_expression,
-                               activity=activity if activity else rule.activity,
-                               notify=notify,
-                               purge_replicas=rule.purge_replicas,
-                               ignore_availability=rule.ignore_availability,
-                               comment=rule.comments,
-                               session=session)
+        options = {
+            'dids': [{'scope': rule.scope, 'name': rule.name}],
+            'account': rule.account,
+            'copies': rule.copies,
+            'rse_expression': rse_expression,
+            'grouping': grouping,
+            'weight': rule.weight,
+            'lifetime': lifetime,
+            'locked': rule.locked,
+            'subscription_id': rule.subscription_id,
+            'source_replica_expression': rule.source_replica_expression,
+            'activity': rule.activity,
+            'notify': notify,
+            'purge_replicas': rule.purge_replicas,
+            'ignore_availability': rule.ignore_availability,
+            'comment': rule.comments,
+            'session': session,
+        }
+
+        for key in override:
+            if key in ['dids', 'session']:
+                raise UnsupportedOperation('Not allowed to override option %s' % key)
+            elif key not in options:
+                raise UnsupportedOperation('Non-valid override option %s' % key)
+            else:
+                options[key] = override[key]
+
+        new_rule_id = add_rule(**options)
 
         session.flush()
 

lib/rucio/tests/test_rule.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# Copyright 2013-2021 CERN
+# Copyright 2013-2022 CERN
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -31,7 +31,7 @@
 # - Dimitrios Christidis <dimitrios.christidis@cern.ch>, 2021
 # - Simon Fayer <simon.fayer05@imperial.ac.uk>, 2021
 # - David Población Criado <david.poblacion.criado@cern.ch>, 2021
-# - Joel Dierkes <joel.dierkes@cern.ch>, 2021
+# - Joel Dierkes <joel.dierkes@cern.ch>, 2021-2022
 
 import json
 import random
@@ -931,7 +931,7 @@ def test_move_rule_with_arguments(self):
 
         activity = "No User Subscriptions"
         source_replica_expression = self.rse3 + "|" + self.rse1
-        rule_id2 = move_rule(rule_id, self.rse3, activity=activity, source_replica_expression=source_replica_expression)
+        rule_id2 = move_rule(rule_id, self.rse3, override={'activity': activity, 'source_replica_expression': source_replica_expression})
 
         assert(get_rule(rule_id2)['state'] == RuleState.REPLICATING)
         assert(get_rule(rule_id)['child_rule_id'] == rule_id2)
@@ -1546,3 +1546,21 @@ def test_detach_dataset_lock_removal_shared_file(did_client, did_factory, root_a
     dataset_locks = list(get_dataset_locks(**dataset_internal))
     print("Dataset locks after detach: {0}".format(dataset_locks))
     assert(len([d for d in dataset_locks if d["rule_id"] == rule_id]) == 0)
+
+
+def test_move_rule_invalid_argument(vo, rse_factory, mock_scope, root_account):
+    rse, rse_id = rse_factory.make_rse()
+    new_rse, new_rse_id = rse_factory.make_rse()
+    files = create_files(3, mock_scope, [rse_id])
+    dataset = 'dataset_' + str(uuid())
+    add_did(mock_scope, dataset, DIDType.DATASET, root_account)
+    attach_dids(mock_scope, dataset, files, root_account)
+
+    rule_id = add_rule(dids=[{'scope': mock_scope, 'name': dataset}], account=root_account, copies=1, rse_expression=rse, grouping='DATASET', weight=None, lifetime=None, locked=False, subscription_id=None)[0]
+    assert(get_rule(rule_id)['state'] == RuleState.OK)
+
+    with pytest.raises(UnsupportedOperation):
+        _ = move_rule(rule_id, new_rse, override={'session': 17})
+
+    with pytest.raises(UnsupportedOperation):
+        _ = move_rule(rule_id, new_rse, override={'xX_MyFirstStreetName_Xx': 17})

lib/rucio/web/rest/flaskapi/v1/rules.py

@@ -18,6 +18,7 @@
 # - Thomas Beermann <thomas.beermann@cern.ch>, 2021
 # - Radu Carpa <radu.carpa@cern.ch>, 2021
 # - Joel Dierkes <joel.dierkes@cern.ch>, 2021-2022
+# - Martin Barisits <martin.barisits@cern.ch>, 2022
 
 from json import dumps
 
@@ -571,13 +572,20 @@ def post(self, rule_id):
         parameters = json_parameters()
         rse_expression = param_get(parameters, 'rse_expression')
         rule_id = param_get(parameters, 'rule_id', default=rule_id)
+        override = param_get(parameters, 'override', default={})
+
+        # For backwards-compatibility, deprecate in the future.
         activity = param_get(parameters, 'activity', default=None)
+        if activity and 'activity' not in override:
+            override['activity'] = activity
         source_replica_expression = param_get(parameters, 'source_replica_expression', default=None)
+        if source_replica_expression and 'source_replica_expression' not in override:
+            override['source_replica_expression'] = source_replica_expression
+
         try:
             rule_ids = move_replication_rule(rule_id=rule_id,
                                              rse_expression=rse_expression,
-                                             activity=activity,
-                                             source_replica_expression=source_replica_expression,
+                                             override=override,
                                              issuer=request.environ.get('issuer'),
                                              vo=request.environ.get('vo'))
         except RuleReplaceFailed as error: