From 4d0224f11fc42febf3adfe81af47a8ddf034fa30 Mon Sep 17 00:00:00 2001 From: jgilaber Date: Fri, 24 Oct 2025 15:01:33 +0200 Subject: [PATCH] Fix zone migration to accept dst_pool or dst_type The zone migration strategy now properly validates that audits specify either dst_pool OR dst_type for volume operations, but not both. This should prevent having both volume migration and retype operations for a volume in the same action plan. The schema validation now uses 'oneOf' and 'not' to reject audit that specify both dst_pool and dst_type simultaneously. Updated tests to cover the new validation logic including: - Valid configurations with dst_pool only - Valid configurations with dst_type only - Invalid configurations with both dst_pool and dst_type - Multiple storage pools with mixed valid configurations The change also adds docstrings for the zone migration audit api tests. Generated-By: claude-code (claude-sonnet-4-5) Closes-Bug: 2128056 Change-Id: Ief35d465615036323475eb38329f8c1c188f94b1 Signed-off-by: jgilaber --- ...ration_schema_change-92a9ec8631870c84.yaml | 7 + .../strategy/strategies/zone_migration.py | 11 +- watcher/tests/api/v1/test_audits.py | 165 +++++++++++++++--- 3 files changed, 162 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/zone_migration_schema_change-92a9ec8631870c84.yaml diff --git a/releasenotes/notes/zone_migration_schema_change-92a9ec8631870c84.yaml b/releasenotes/notes/zone_migration_schema_change-92a9ec8631870c84.yaml new file mode 100644 index 000000000..df8d9c075 --- /dev/null +++ b/releasenotes/notes/zone_migration_schema_change-92a9ec8631870c84.yaml @@ -0,0 +1,7 @@ +--- +upgrade: + - | + The Zone Migration Strategy now accepts audits with either 'dst_type' or + 'dst_pool' as input, but not with both. The strategy won't support retyping + and migrating a volume in the same action plan, so the user should choose + the input according to the desired operation. diff --git a/watcher/decision_engine/strategy/strategies/zone_migration.py b/watcher/decision_engine/strategy/strategies/zone_migration.py index 77ff00765..85aa01fef 100644 --- a/watcher/decision_engine/strategy/strategies/zone_migration.py +++ b/watcher/decision_engine/strategy/strategies/zone_migration.py @@ -132,7 +132,16 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy): "type": "string" } }, - "required": ["src_pool", "dst_type"], + "oneOf": [ + { + "required": ["src_pool", "dst_pool"], + "not": {"required": ["dst_type"]} + }, + { + "required": ["src_pool", "dst_type"], + "not": {"required": ["dst_pool"]} + } + ], "additionalProperties": False } }, diff --git a/watcher/tests/api/v1/test_audits.py b/watcher/tests/api/v1/test_audits.py index 05a192ab4..487da1e47 100644 --- a/watcher/tests/api/v1/test_audits.py +++ b/watcher/tests/api/v1/test_audits.py @@ -860,7 +860,7 @@ class TestPost(TestPostBase): response = self.post_json('/audits', audit_dict, expect_errors=True) self.assertEqual('application/json', response.content_type) self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) - assert not mock_trigger_audit.called + mock_trigger_audit.assert_not_called() @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_parameters_no_predefined_strategy( @@ -879,7 +879,7 @@ class TestPost(TestPostBase): 'parameter spec in predefined strategy') self.assertTrue(response.json['error_message']) self.assertIn(expected_error_msg, response.json['error_message']) - assert not mock_trigger_audit.called + mock_trigger_audit.assert_not_called() @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_parameters_no_schema( @@ -902,7 +902,7 @@ class TestPost(TestPostBase): 'parameter spec in predefined strategy') self.assertTrue(response.json['error_message']) self.assertIn(expected_error_msg, response.json['error_message']) - assert not mock_trigger_audit.called + mock_trigger_audit.assert_not_called() @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_with_parameter_not_allowed( @@ -925,7 +925,7 @@ class TestPost(TestPostBase): expected_error_msg = 'Audit parameter fake2 are not allowed' self.assertTrue(response.json['error_message']) self.assertIn(expected_error_msg, response.json['error_message']) - assert not mock_trigger_audit.called + mock_trigger_audit.assert_not_called() @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_with_missing_parameter( @@ -949,7 +949,7 @@ class TestPost(TestPostBase): "Invalid parameters for strategy: 'fake1' is a required property") self.assertTrue(response.json['error_message']) self.assertIn(expected_error_msg, response.json['error_message']) - assert not mock_trigger_audit.called + mock_trigger_audit.assert_not_called() @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') @mock.patch('oslo_utils.timeutils.utcnow') @@ -1040,7 +1040,7 @@ class TestPost(TestPostBase): expected_error_msg = 'Request not acceptable.' self.assertTrue(response.json['error_message']) self.assertIn(expected_error_msg, response.json['error_message']) - assert not mock_trigger_audit.called + mock_trigger_audit.assert_not_called() @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_with_force_false(self, mock_trigger_audit): @@ -1094,7 +1094,7 @@ class TestPost(TestPostBase): expected_msg = 'A valid goal or audit_template_id must be provided' self.assertTrue(response.json['error_message']) self.assertIn(expected_msg, response.json['error_message']) - assert not mock_trigger_audit.called + mock_trigger_audit.assert_not_called() class TestDelete(api_base.FunctionalTest): @@ -1257,6 +1257,13 @@ class TestAuditZoneMigration(TestPostBase): @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_zone_migration_without_dst_pool(self, mock_trigger_audit): + """Verify zone migration audit with dst_type instead of dst_pool. + + Tests that an audit can be created with storage pool parameters + containing src_pool, src_type, and dst_type (without dst_pool). + This validates the schema allows dst_type as an alternative to + dst_pool for specifying the destination. + """ mock_trigger_audit.return_value = mock.ANY zm_params = { 'storage_pools': [ @@ -1275,12 +1282,17 @@ class TestAuditZoneMigration(TestPostBase): @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_zone_migration_without_src_pool(self, mock_trigger_audit): + """Verify zone migration audit rejects missing src_pool. + + Tests that an audit creation fails with BAD_REQUEST when storage + pool parameters contain dst_pool and src_type but lack src_pool. + This validates the schema correctly enforces src_pool as required. + """ mock_trigger_audit.return_value = mock.ANY zm_params = { 'storage_pools': [ {"dst_pool": "dst_pool_name", - "src_type": "src_type_name", - "dst_type": "dst_type_name"} + "src_type": "src_type_name"} ] } @@ -1288,16 +1300,26 @@ class TestAuditZoneMigration(TestPostBase): response = self.post_json('/audits', audit_input_dict, expect_errors=True) - self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) self.assertEqual("application/json", response.content_type) - expected_error_msg = ("'src_pool' is a required property") - self.assertTrue(response.json['error_message']) - self.assertIn(expected_error_msg, response.json['error_message']) - assert not mock_trigger_audit.called + self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) + expected_error_msgs = ( + "is not valid under any of the given schemas", + "Failed validating 'oneOf' in schema" + ) + for expected_error_msg in expected_error_msgs: + self.assertIn(expected_error_msg, response.json['error_message']) + mock_trigger_audit.assert_not_called() @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_zone_migration_without_dst_type(self, mock_trigger_audit): + """Verify zone migration audit with dst_pool instead of dst_type. + + Tests that an audit can be created with storage pool parameters + containing src_pool, src_type, and dst_pool (without dst_type). + This validates the schema allows dst_pool as an alternative to + dst_type for specifying the destination. + """ mock_trigger_audit.return_value = mock.ANY zm_params = { 'storage_pools': [ @@ -1312,21 +1334,24 @@ class TestAuditZoneMigration(TestPostBase): response = self.post_json('/audits', audit_input_dict, expect_errors=True) self.assertEqual("application/json", response.content_type) - self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) - expected_error_msg = ("'dst_type' is a required property") - self.assertTrue(response.json['error_message']) - self.assertIn(expected_error_msg, response.json['error_message']) - assert not mock_trigger_audit.called + self.assertEqual(HTTPStatus.CREATED, response.status_int) @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') def test_create_audit_zone_migration_without_src_type(self, mock_trigger_audit): + """Verify zone migration audit accepts optional src_type. + + Tests that an audit can be created with storage pool parameters + containing dst_pool and src_pool (without src_type). This validates + the schema allows src_type to be optional when both pool names are + specified. + """ mock_trigger_audit.return_value = mock.ANY zm_params = { 'storage_pools': [ {"dst_pool": "dst_pool_name", "src_pool": "src_pool_name", - "dst_type": "dst_type_name"} + } ] } @@ -1335,3 +1360,103 @@ class TestAuditZoneMigration(TestPostBase): response = self.post_json('/audits', audit_input_dict) self.assertEqual("application/json", response.content_type) self.assertEqual(HTTPStatus.CREATED, response.status_int) + + @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') + def test_create_audit_zone_migration_with_dst_pool_dst_type( + self, mock_trigger_audit + ): + """Verify zone migration rejects both destination parameters at once. + + Tests that an audit creation fails with BAD_REQUEST when both + dst_type and dst_pool are passed in the input parameters. This + validates the schema correctly enforces that the two destination + parameters are mutually exclusive. + """ + mock_trigger_audit.return_value = mock.ANY + zm_params = { + 'storage_pools': [ + {"dst_type": "dst_type_name", + "dst_pool": "dst_pool_name"} + ] + } + + audit_input_dict = self._prepare_audit_params(zm_params) + + response = self.post_json('/audits', audit_input_dict, + expect_errors=True) + self.assertEqual("application/json", response.content_type) + self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) + expected_error_msgs = ( + "is not valid under any of the given schemas", + "Failed validating 'oneOf' in schema" + ) + for expected_error_msg in expected_error_msgs: + self.assertIn(expected_error_msg, response.json['error_message']) + mock_trigger_audit.assert_not_called() + + @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') + def test_create_audit_zone_migration_with_multiple_valid_inputs( + self, mock_trigger_audit + ): + """Verify zone migration audit with multiple storage pools. + + Tests that an audit can be created with multiple storage pool + entries, each with valid parameter combinations. This validates + the schema correctly handles arrays of storage pool configurations + with varying valid parameter combinations. + """ + mock_trigger_audit.return_value = mock.ANY + zm_params = { + 'storage_pools': [ + {"src_pool": "src_pool_name", + "src_type": "src_type_name", + "dst_type": "dst_type_name"}, + {"src_pool": "src_pool_name", + "src_type": "src_type_name2", + "dst_pool": "dst_type_name"}, + ] + } + + audit_input_dict = self._prepare_audit_params(zm_params) + + response = self.post_json('/audits', audit_input_dict) + self.assertEqual("application/json", response.content_type) + self.assertEqual(HTTPStatus.CREATED, response.status_int) + + @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') + def test_create_audit_zone_migration_second_storage_pool_invalid( + self, mock_trigger_audit + ): + """Verify zone migration rejects invalid entry in multiple pools. + + Tests that an audit creation fails with BAD_REQUEST when one of + the storage pool entries has an invalid parameter combination, + even when other entries are valid. This validates the schema + enforces validation on all array elements. + """ + mock_trigger_audit.return_value = mock.ANY + zm_params = { + 'storage_pools': [ + {"src_pool": "src_pool_name", + "src_type": "src_type_name", + "dst_type": "dst_type_name"}, + {"src_pool": "src_pool_name", + "dst_type": "dst_type_name2", + "dst_pool": "dst_type_name"}, + ] + } + + audit_input_dict = self._prepare_audit_params(zm_params) + + response = self.post_json( + '/audits', audit_input_dict, expect_errors=True + ) + self.assertEqual("application/json", response.content_type) + self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) + expected_error_msgs = ( + "is not valid under any of the given schemas", + "Failed validating 'oneOf' in schema" + ) + for expected_error_msg in expected_error_msgs: + self.assertIn(expected_error_msg, response.json['error_message']) + mock_trigger_audit.assert_not_called()