instance user scripts fix

coriolis/api/v1/deployments.py

@@ -80,9 +80,6 @@ class DeploymentsController(api_wsgi.Controller):
             'instance_osmorphing_minion_pool_mappings', {})
         user_scripts = deployment.get('user_scripts', {})
         api_utils.validate_user_scripts(user_scripts)
-        user_scripts = api_utils.normalize_user_scripts(
-            user_scripts, deployment.get("instances", []))
-
         return (
             transfer_id, force, clone_disks, skip_os_morphing,
             instance_osmorphing_minion_pool_mappings,

coriolis/api/v1/transfers.py

@@ -124,8 +124,6 @@ class TransferController(api_wsgi.Controller):
 
         user_scripts = transfer.get('user_scripts', {})
         api_utils.validate_user_scripts(user_scripts)
-        user_scripts = api_utils.normalize_user_scripts(
-            user_scripts, instances)
 
         # NOTE(aznashwan): we validate the destination environment for the
         # import provider before appending the 'storage_mappings' parameter
@@ -368,8 +366,6 @@ class TransferController(api_wsgi.Controller):
 
         user_scripts = merged_body['user_scripts']
         api_utils.validate_user_scripts(user_scripts)
-        merged_body['user_scripts'] = api_utils.normalize_user_scripts(
-            user_scripts, transfer.get('instances', []))
 
         return merged_body
 

coriolis/api/v1/utils.py

@@ -117,22 +117,6 @@ def validate_user_scripts(user_scripts):
     return user_scripts
 
 
-def normalize_user_scripts(user_scripts, instances):
-    """ Removes instance user_scripts if said instance is not one of the
-        selected instances for the replica/migration """
-    if user_scripts is None:
-        user_scripts = {}
-    for instance in list(user_scripts.get('instances', {}).keys()):
-        if instance not in instances:
-            LOG.warn("Removing provided instance '%s' from user_scripts body "
-                     "because it's not included in one of the selected "
-                     "instances for this replica/migration: %s",
-                     instance, instances)
-            user_scripts['instances'].pop(instance, None)
-
-    return user_scripts
-
-
 def validate_instances_list_for_transfer(instances):
     if not instances:
         raise exception.InvalidInput(

coriolis/conductor/rpc/server.py

@@ -1664,6 +1664,21 @@ class ConductorServerEndpoint(object):
             db_api.set_action_last_execution_status(
                 ctxt, deployment_id, error_status)
 
+    def _normalize_user_scripts(self, user_scripts, instances):
+        """ Removes instance user_scripts if said instance is not one of the
+            selected instances for the replica/migration """
+        if user_scripts is None:
+            user_scripts = {}
+        for instance in list(user_scripts.get('instances', {}).keys()):
+            if instance not in instances:
+                LOG.warn("Removing provided instance '%s' from user_scripts "
+                         "body because it's not included in one of the "
+                         "selected instances for this replica/migration: %s",
+                         instance, instances)
+                user_scripts['instances'].pop(instance, None)
+
+        return user_scripts
+
     @transfer_synchronized
     def deploy_transfer_instances(
             self, ctxt, transfer_id, force=False, wait_for_execution=None,
@@ -1683,6 +1698,7 @@ class ConductorServerEndpoint(object):
             init_status = constants.EXECUTION_STATUS_PENDING
 
         instances = transfer.instances
+        user_scripts = self._normalize_user_scripts(user_scripts, instances)
 
         deployment = models.Deployment()
         deployment.id = str(uuid.uuid4())

coriolis/tests/api/v1/test_transfers.py

@@ -119,13 +119,11 @@ class TransferControllerTestCase(test_base.CoriolisBaseTestCase):
     @mock.patch.object(api_utils, 'validate_network_map')
     @mock.patch.object(endpoints_api.API, 'validate_target_environment')
     @mock.patch.object(api_utils, 'validate_user_scripts')
-    @mock.patch.object(api_utils, 'normalize_user_scripts')
     @mock.patch.object(api_utils, 'validate_storage_mappings')
     @ddt.file_data('data/transfers_validate_create_body.yml')
     def test_validate_create_body(
         self,
         mock_validate_storage_mappings,
-        mock_normalize_user_scripts,
         mock_validate_user_scripts,
         mock_validate_target_environment,
         mock_validate_network_map,
@@ -147,7 +145,6 @@ class TransferControllerTestCase(test_base.CoriolisBaseTestCase):
         instances = transfer.get('instances')
         storage_mappings = transfer.get('storage_mappings')
         mock_validate_instances_list_for_transfer.return_value = instances
-        mock_normalize_user_scripts.return_value = user_scripts
 
         if exception_raised:
             self.assertRaisesRegex(
@@ -178,8 +175,6 @@ class TransferControllerTestCase(test_base.CoriolisBaseTestCase):
             mock_validate_target_environment.assert_called_once_with(
                 ctxt, destination_endpoint_id, destination_environment)
             mock_validate_user_scripts.assert_called_once_with(user_scripts)
-            mock_normalize_user_scripts.assert_called_once_with(
-                user_scripts, instances)
             mock_validate_storage_mappings.assert_called_once_with(
                 storage_mappings)
 
@@ -368,7 +363,6 @@ class TransferControllerTestCase(test_base.CoriolisBaseTestCase):
         mock_get_updated_user_scripts.assert_called_once_with(
             "mock_scripts", "mock_new_scripts")
 
-    @mock.patch.object(api_utils, 'normalize_user_scripts')
     @mock.patch.object(api_utils, 'validate_user_scripts')
     @mock.patch.object(api_utils, 'validate_storage_mappings')
     @mock.patch.object(api_utils, 'validate_network_map')
@@ -387,7 +381,6 @@ class TransferControllerTestCase(test_base.CoriolisBaseTestCase):
         mock_validate_network_map,
         mock_validate_storage_mappings,
         mock_validate_user_scripts,
-        mock_normalize_user_scripts,
         config,
         expected_result
     ):
@@ -398,8 +391,6 @@ class TransferControllerTestCase(test_base.CoriolisBaseTestCase):
         id = mock.sentinel.id
         mock_get_transfer.return_value = transfer
         mock_get_merged_transfer_values.return_value = transfer_body
-        mock_normalize_user_scripts.return_value = transfer_body[
-            'user_scripts']
 
         result = testutils.get_wrapped_function(
             self.transfers._validate_update_body)(
@@ -429,8 +420,6 @@ class TransferControllerTestCase(test_base.CoriolisBaseTestCase):
             transfer_body['storage_mappings'])
         mock_validate_user_scripts.assert_called_once_with(
             transfer_body['user_scripts'])
-        mock_normalize_user_scripts.assert_called_once_with(
-            transfer_body['user_scripts'], transfer['instances'])
 
     @mock.patch.object(api.API, 'get_transfer')
     @ddt.file_data('data/transfers_validate_update_body_raises.yml')

coriolis/tests/api/v1/test_utils.py

@@ -223,43 +223,6 @@ class UtilsTestCase(test_base.CoriolisBaseTestCase):
             user_scripts
         )
 
-    def test_normalize_user_scripts(
-        self
-    ):
-        user_scripts = {
-            'instances': {
-                "mock_instance_1": "mock_value_1",
-                "mock_instance_2": "mock_value_2"
-            }
-        }
-        instances = ["mock_instance_2", "mock_instance_3"]
-        expected_result = {
-            'instances': {
-                "mock_instance_2": "mock_value_2"
-            }
-        }
-
-        with self.assertLogs('coriolis.api.v1.utils', level='WARN'):
-            result = utils.normalize_user_scripts(user_scripts, instances)
-
-        self.assertEqual(
-            expected_result,
-            result
-        )
-
-    def test_normalize_user_scripts_none(
-        self
-    ):
-        user_scripts = None
-        instances = None
-
-        result = utils.normalize_user_scripts(user_scripts, instances)
-
-        self.assertEqual(
-            {},
-            result
-        )
-
     @ddt.file_data('data/utils_validate_instances_list_for_transfer.yml')
     def test_validate_instances_list_for_transfer(
         self,

coriolis/tests/conductor/rpc/test_server.py

@@ -2023,6 +2023,31 @@ class ConductorServerEndpointTestCase(test_base.CoriolisBaseTestCase):
             to_dict=False
         )
 
+    def test_normalize_user_scripts(self):
+        user_scripts = {
+            'instances': {
+                "mock_instance_1": "mock_value_1",
+                "mock_instance_2": "mock_value_2"
+            }
+        }
+        instances = ["mock_instance_2", "mock_instance_3"]
+
+        expected_result = {
+            'instances': {
+                "mock_instance_2": "mock_value_2"
+            }
+        }
+
+        with self.assertLogs('coriolis.conductor.rpc.server', level='WARNING'):
+            result = self.server._normalize_user_scripts(user_scripts,
+                                                         instances)
+
+        self.assertEqual(expected_result, result)
+
+    def test_normalize_user_scripts_none(self):
+        result = self.server._normalize_user_scripts(None, None)
+        self.assertEqual({}, result)
+
     @mock.patch.object(server.ConductorServerEndpoint, '_get_deployment')
     def test_get_deployment(self, mock_get_deployment):
         result = testutils.get_wrapped_function(self.server.get_deployment)(
@@ -2216,6 +2241,8 @@ class ConductorServerEndpointTestCase(test_base.CoriolisBaseTestCase):
         mock_get_available_providers.assert_called_once_with(
             mock.sentinel.context)
 
+    @mock.patch.object(server.ConductorServerEndpoint,
+                       '_normalize_user_scripts')
     @mock.patch.object(server.ConductorServerEndpoint,
                        '_validate_deployment_inputs')
     @mock.patch.object(models, 'Deployment')
@@ -2226,7 +2253,10 @@ class ConductorServerEndpointTestCase(test_base.CoriolisBaseTestCase):
     def test_deploy_transfer_instances(
             self, mock_get_deployment, mock_execute_deployment,
             mock_get_transfer, mock_add_deployment, mock_deployment_model,
-            mock_validate_deployment_inputs):
+            mock_validate_deployment_inputs, mock_normalize_user_scripts):
+
+        mock_normalize_user_scripts.return_value = {'instances': {}}
+
         transfer_mock = mock.MagicMock()
         transfer_mock.instance_osmorphing_minion_pool_mappings = {
             mock.sentinel.instance1: mock.sentinel.pool1}
@@ -2253,7 +2283,7 @@ class ConductorServerEndpointTestCase(test_base.CoriolisBaseTestCase):
                 trust_id=None)
 
         self.assertEqual(mock_get_deployment.return_value, result)
-        self.assertEqual(mock.sentinel.user_scripts, deployment.user_scripts)
+        self.assertEqual({'instances': {}}, deployment.user_scripts)
         self.assertEqual(True, deployment.clone_disks)
         self.assertEqual(False, deployment.skip_os_morphing)
         self.assertEqual(
@@ -2263,6 +2293,8 @@ class ConductorServerEndpointTestCase(test_base.CoriolisBaseTestCase):
         self.assertTrue(
             transfer_mock.destination_environment is not
             deployment.destination_environment)
+        mock_normalize_user_scripts.assert_called_once_with(
+            mock.sentinel.user_scripts, transfer_mock.instances)
         mock_get_transfer.assert_called_once_with(
             mock.sentinel.ctxt, mock.sentinel.transfer_id,
             include_task_info=True)