From 1ba65dbe280964a0111be224e89e3b3557b69691 Mon Sep 17 00:00:00 2001 From: Jose Caballero Bejar Date: Thu, 25 Jun 2026 13:50:46 +0100 Subject: [PATCH 1/6] Add a new flag "live_migration" This flag is needed for both cases: - when the entire hypervisor.drain workflow is executed - when only the server.migrate action is executed Because of that, we need to add the flag in the YAML configuration file for both Actions (hypervisor.drain and server.migrate), and also in the Orchesta YAML configuration that connects those two Actions. The behavior of this flag is as follows: * only affects the migration of ACTIVE Servers. If the Server is already SHUTOFF, an unconditional Cold Migration will be performed * for ACTIVE Servers * if the flag is set to True (default value), then a Live Migration will be attempted * if the flag is set to False, a Cold Migration will be performed --- actions/hypervisor.drain.yaml | 5 +++++ actions/server.migrate.yaml | 5 +++++ actions/workflows/hypervisor.drain.workflow.yaml | 2 ++ 3 files changed, 12 insertions(+) diff --git a/actions/hypervisor.drain.yaml b/actions/hypervisor.drain.yaml index fbcd50ecc..539880dcf 100644 --- a/actions/hypervisor.drain.yaml +++ b/actions/hypervisor.drain.yaml @@ -21,3 +21,8 @@ parameters: description: Reason for draining the hypervisor required: true type: string + live_migration: + description: Decides if an ACTIVE Server should go under Live Migration (default) or Cold Migration. A Cold Migration will shutdown ACTIVE Servers, and therefore ths flag does not affect SHUTOFF Servers. + required: true + type: boolean + default: true diff --git a/actions/server.migrate.yaml b/actions/server.migrate.yaml index 2e5930b63..fd4603e35 100644 --- a/actions/server.migrate.yaml +++ b/actions/server.migrate.yaml @@ -34,4 +34,9 @@ parameters: type: boolean required: true default: true + live_migration: + description: Decides if an ACTIVE Server should go under Live Migration (default) or Cold Migration. A Cold Migration will shutdown ACTIVE Servers, and therefore this flag does not affect SHUTOFF Servers. + required: true + type: boolean + default: true runner_type: python-script diff --git a/actions/workflows/hypervisor.drain.workflow.yaml b/actions/workflows/hypervisor.drain.workflow.yaml index 4028322a8..483553454 100644 --- a/actions/workflows/hypervisor.drain.workflow.yaml +++ b/actions/workflows/hypervisor.drain.workflow.yaml @@ -6,6 +6,7 @@ input: - cloud_account - hypervisor_name - disabled_reason + - live_migration: true vars: - servers: null @@ -48,3 +49,4 @@ tasks: cloud_account: <% ctx().cloud_account %> server_id: <% item(server_id) %> snapshot: <% not item(server_name).startsWith("amphora-") %> + live_migration: <% ctx().live_migration %> From 640c19bcbbcd656a1f629057bdc224c443cf553f Mon Sep 17 00:00:00 2001 From: Jose Caballero Bejar Date: Thu, 25 Jun 2026 14:35:54 +0100 Subject: [PATCH 2/6] Implement the new logic for Cold Migrations We decide whether a Live Migration or a Cold Migration is performed on ACTIVE Servers based on the value of the new flag "live_migration". SHUTOFF Servers are still handled with a Cold Migration. Some refactoring to make the code more readable: * the step for both Live Migration and Cold Migration have been moved to their own functions * the check for Servers in a status different than ACTIVE and SHUTOFF has been moved outside the original can_be_migrated() function and moved into the new decision tree. This way, the can_be_migrated() function only checks the properties of the Server's Flavor. --- lib/apis/openstack_api/openstack_server.py | 47 ++++++++++++++----- .../openstack_api/test_openstack_server.py | 20 ++++++-- 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/lib/apis/openstack_api/openstack_server.py b/lib/apis/openstack_api/openstack_server.py index 208e1a74b..f44db11c6 100644 --- a/lib/apis/openstack_api/openstack_server.py +++ b/lib/apis/openstack_api/openstack_server.py @@ -15,20 +15,39 @@ def can_be_migrated(server: Server): raise ValueError( f"Attempted to move GPU or FPGA flavor, {server.flavor.name}, which is not allowed!" ) - if server.status not in ["ACTIVE", "SHUTOFF"]: - raise ValueError( - f"Server status: {server.status}. The server must be ACTIVE or SHUTOFF to be migrated" - ) if server.flavor.vcpus > 60: raise ValueError( f"Attempted to move flavor with greater than 60 cores, {server.flavor.name}, which is not allowed!" ) +def _cold_migration( + conn: Connection, + server: Server, + dest_host: Optional[str] = None, +) -> None: + conn.compute.migrate_server(server=server.id, host=dest_host) + conn.compute.wait_for_server(server, status="VERIFY_RESIZE", wait=3600) + conn.compute.confirm_server_resize(server.id) + wait_for_migration_status(conn, server.id, "confirmed") + + +def _live_migration( + conn: Connection, + server: Server, + dest_host: Optional[str] = None, +) -> None: + conn.compute.live_migrate_server( + server=server.id, host=dest_host, block_migration=True + ) + wait_for_migration_status(conn, server.id, "completed") + + def snapshot_and_migrate_server( conn: Connection, server_id: str, snapshot: bool, + live_migration: bool, dest_host: Optional[str] = None, ) -> None: """ @@ -37,24 +56,26 @@ def snapshot_and_migrate_server( :param server_id: Server ID to migrate :param server_status: Status of machine to migrate - must be ACTIVE or SHUTOFF :param flavor_name: Server flavor name + :param live_migration: decides if an ACTIVE Server should go under Cold Migration or Live Migration :param dest_host: Optional host to migrate to, otherwise chosen by scheduler """ server = conn.compute.get_server(server_id) - can_be_migrated(server) if snapshot: snapshot_server(conn=conn, server_id=server_id) time.sleep(10) # Ensure server task status has updated after snapshot logger.info("Migrating server: %s", server.id) if server.status == "SHUTOFF": - conn.compute.migrate_server(server=server_id, host=dest_host) - conn.compute.wait_for_server(server, status="VERIFY_RESIZE", wait=3600) - conn.compute.confirm_server_resize(server_id) - wait_for_migration_status(conn, server_id, "confirmed") - if server.status == "ACTIVE": - conn.compute.live_migrate_server( - server=server_id, host=dest_host, block_migration=True + _cold_migration(conn, server, dest_host) + elif server.status == "ACTIVE": + if live_migration: + can_be_migrated(server) + _live_migration(conn, server, dest_host) + else: + _cold_migration(conn, server, dest_host) + else: + raise ValueError( + f"Server status: {server.status}. The server must be ACTIVE or SHUTOFF to be migrated" ) - wait_for_migration_status(conn, server_id, "completed") logger.info("Migration completed of server: %s", server.id) diff --git a/tests/lib/apis/openstack_api/test_openstack_server.py b/tests/lib/apis/openstack_api/test_openstack_server.py index f7fd373de..b5f04715c 100644 --- a/tests/lib/apis/openstack_api/test_openstack_server.py +++ b/tests/lib/apis/openstack_api/test_openstack_server.py @@ -34,6 +34,7 @@ def test_active_migration( mock_connection = MagicMock() mock_server_id = "server1" mock_server = MagicMock() + mock_server.id = mock_server_id mock_server.flavor.name = "l3.nano" mock_server.flavor.vcpus = 2 mock_server.status = "ACTIVE" @@ -43,6 +44,7 @@ def test_active_migration( server_id=mock_server_id, dest_host=dest_host, snapshot=True, + live_migration=True, ) mock_snapshot_server.assert_called_once_with( conn=mock_connection, server_id=mock_server_id @@ -69,6 +71,7 @@ def test_shutoff_migration( mock_connection = MagicMock() mock_server_id = "server1" mock_server = MagicMock() + mock_server.id = mock_server_id mock_server.status = "SHUTOFF" mock_server.flavor.name = "l3.nano" mock_server.flavor.vcpus = 2 @@ -79,6 +82,7 @@ def test_shutoff_migration( server_id=mock_server_id, dest_host=dest_host, snapshot=True, + live_migration=False, ) mock_snapshot_server.assert_called_once_with( conn=mock_connection, server_id=mock_server_id @@ -90,7 +94,9 @@ def test_shutoff_migration( mock_connection.compute.wait_for_server.assert_called_once_with( mock_server, status="VERIFY_RESIZE", wait=3600 ) - mock_connection.compute.confirm_server_resize(mock_server_id) + mock_connection.compute.confirm_server_resize.assert_called_once_with( + mock_server_id + ) mock_wait_for_migration_status.assert_called_once_with( mock_connection, mock_server_id, "confirmed" ) @@ -107,6 +113,7 @@ def test_active_migration_failed_wait_for_status( mock_connection = MagicMock() mock_server_id = "server1" mock_server = MagicMock() + mock_server.id = mock_server_id mock_server.flavor.name = "l3.nano" mock_server.flavor.vcpus = 2 mock_server.status = "ACTIVE" @@ -119,6 +126,7 @@ def test_active_migration_failed_wait_for_status( server_id=mock_server_id, dest_host=None, snapshot=True, + live_migration=True, ) mock_snapshot_server.assert_called_once_with( conn=mock_connection, server_id=mock_server_id @@ -141,6 +149,7 @@ def test_no_snapshot_migration(mock_wait_for_migration_status, mock_snapshot_ser mock_connection = MagicMock() mock_server_id = "server1" mock_server = MagicMock() + mock_server.id = mock_server_id mock_server.flavor.name = "l3.nano" mock_server.flavor.vcpus = 2 mock_server.status = "ACTIVE" @@ -150,6 +159,7 @@ def test_no_snapshot_migration(mock_wait_for_migration_status, mock_snapshot_ser conn=mock_connection, server_id=mock_server_id, snapshot=False, + live_migration=True, ) mock_snapshot_server.assert_not_called() mock_connection.compute.live_migrate_server.assert_called_once_with( @@ -168,6 +178,7 @@ def test_migration_fail(mock_snapshot_server): mock_connection = MagicMock() mock_server_id = "server1" mock_server = MagicMock() + mock_server.id = mock_server_id mock_server.flavor.name = "l3.nano" mock_server.flavor.vcpus = 2 mock_server.status = "ERROR" @@ -180,7 +191,8 @@ def test_migration_fail(mock_snapshot_server): snapshot_and_migrate_server( conn=mock_connection, server_id=mock_server_id, - snapshot=True, + snapshot=False, + live_migration=True, ) mock_snapshot_server.assert_not_called() mock_connection.compute.live_migrate_server.assert_not_called() @@ -231,6 +243,7 @@ def test_raise_invalid_migration(flavor_name, flavor_vcpu): mock_connection = MagicMock() mock_server_id = "server1" mock_server = MagicMock() + mock_server.id = mock_server_id mock_server.flavor.name = flavor_name mock_server.flavor.vcpus = flavor_vcpu mock_server.status = "ACTIVE" @@ -241,7 +254,8 @@ def test_raise_invalid_migration(flavor_name, flavor_vcpu): conn=mock_connection, server_id=mock_server_id, dest_host=None, - snapshot=True, + snapshot=False, + live_migration=True, ) From 5e00a96d8086dcf93c6ad72100c69b972b1f5b52 Mon Sep 17 00:00:00 2001 From: David Fairbrother Date: Tue, 30 Jun 2026 19:43:01 +0100 Subject: [PATCH 3/6] BUG: Fix hang when migration fails but server state does not change Noticed while testing cold migration. Openstack's wait for status does not allow us to specify invalid states. This means if we ask for it to go to "VERIFY_RESIZE" it will spend the entire timeout (previously 1 hour) waiting. Instead we should check the migration status for the error (as the VM will appear ACTIVE/SHUTDOWN the entire time unless there's a serious error), then confirm the VM moves to VERIFY_RESIZE on the happy path. Add some extra test cases in-case we ever have it a VM completes a migration but somehow ends up ACTIVE or ERROR when the migration is finished. This is extremely unlikely, but play defensive in this case. --- lib/apis/openstack_api/openstack_server.py | 44 +++++--- .../openstack_api/test_openstack_server.py | 104 +++++++++++++++++- 2 files changed, 129 insertions(+), 19 deletions(-) diff --git a/lib/apis/openstack_api/openstack_server.py b/lib/apis/openstack_api/openstack_server.py index f44db11c6..e2f997beb 100644 --- a/lib/apis/openstack_api/openstack_server.py +++ b/lib/apis/openstack_api/openstack_server.py @@ -27,9 +27,23 @@ def _cold_migration( dest_host: Optional[str] = None, ) -> None: conn.compute.migrate_server(server=server.id, host=dest_host) - conn.compute.wait_for_server(server, status="VERIFY_RESIZE", wait=3600) + + # Using conn.compute.wait_for_server will wait even if our migration transitions to error + # causing a hang until timeout so we must wait for the migration then wait for OpenStack + # to update the status after.... + wait_for_migration_status(conn, server.id, "finished") + + # We're just waiting for Nova to update as the migration has completed here + conn.compute.wait_for_server(server, status="VERIFY_RESIZE", wait=10) + + if server.status.casefold() != "VERIFY_RESIZE".casefold(): + raise RuntimeError( + f"Migration caused VM to enter unexpected state {server.status}" + " instead of 'VERIFY_RESIZE'." + ) + + logger.info("Confirming resize for %s", server.id) conn.compute.confirm_server_resize(server.id) - wait_for_migration_status(conn, server.id, "confirmed") def _live_migration( @@ -64,18 +78,22 @@ def snapshot_and_migrate_server( snapshot_server(conn=conn, server_id=server_id) time.sleep(10) # Ensure server task status has updated after snapshot logger.info("Migrating server: %s", server.id) - if server.status == "SHUTOFF": - _cold_migration(conn, server, dest_host) - elif server.status == "ACTIVE": - if live_migration: - can_be_migrated(server) - _live_migration(conn, server, dest_host) - else: + + server_status = server.status.casefold() + + match server_status: + case "shutoff": _cold_migration(conn, server, dest_host) - else: - raise ValueError( - f"Server status: {server.status}. The server must be ACTIVE or SHUTOFF to be migrated" - ) + case "active": + if live_migration: + can_be_migrated(server) + _live_migration(conn, server, dest_host) + else: + _cold_migration(conn, server, dest_host) + case _: + raise ValueError( + f"Server status: {server.status}. The server must be ACTIVE or SHUTOFF to be migrated" + ) logger.info("Migration completed of server: %s", server.id) diff --git a/tests/lib/apis/openstack_api/test_openstack_server.py b/tests/lib/apis/openstack_api/test_openstack_server.py index b5f04715c..27a9a6c22 100644 --- a/tests/lib/apis/openstack_api/test_openstack_server.py +++ b/tests/lib/apis/openstack_api/test_openstack_server.py @@ -1,6 +1,7 @@ import itertools from datetime import datetime -from unittest.mock import MagicMock, patch +from typing import Tuple, Any +from unittest.mock import MagicMock, patch, PropertyMock import pytest from apis.openstack_api.openstack_server import ( @@ -59,6 +60,55 @@ def test_active_migration( mock_connection.compute.migrate_server.assert_not_called() +def _migration_side_effect_generator( + mock_server, mock_connection, return_values: Tuple[Any, Any] +) -> None: + """ + Instruments a fake server to always return the first value of a tuple until migrate is called + after it will always return the second value of the tuple + """ + + class MigrateAPIFake: + def __init__(self): + self.called = False + + def callable_migration(self, *_, **__): + self.called = True + + def return_val(self, *_, **__): + return return_values[0] if not self.called else return_values[1] + + api_fake = MigrateAPIFake() + + # Patch in a side effect of calling migrate_server will cause mock_server.status to update to VERIFY_RESIZE + mock_connection.compute.migrate_server.side_effect = api_fake.callable_migration + type(mock_server).status = PropertyMock(side_effect=api_fake.return_val) + mock_connection.compute.get_server.return_value = mock_server + + +@patch("apis.openstack_api.openstack_server.snapshot_server") +@patch("apis.openstack_api.openstack_server.wait_for_migration_status") +def test_active_cold_migration(_, __): + """ + Tests that active VMs are also be migratable + """ + mock_connection = MagicMock() + mock_server = MagicMock() + + _migration_side_effect_generator( + mock_server, mock_connection, return_values=("ACTIVE", "VERIFY_RESIZE") + ) + snapshot_and_migrate_server( + conn=mock_connection, + server_id="", + snapshot=False, + live_migration=False, + ) + + # If we see migrate server called we can assume it's done the same calls as shutoff any rely on that unit test + mock_connection.compute.migrate_server.assert_called_once() + + @pytest.mark.parametrize("dest_host", [None, "hv01"]) @patch("apis.openstack_api.openstack_server.snapshot_server") @patch("apis.openstack_api.openstack_server.wait_for_migration_status") @@ -72,11 +122,16 @@ def test_shutoff_migration( mock_server_id = "server1" mock_server = MagicMock() mock_server.id = mock_server_id - mock_server.status = "SHUTOFF" mock_server.flavor.name = "l3.nano" mock_server.flavor.vcpus = 2 - mock_connection.compute.get_server.return_value = mock_server + _migration_side_effect_generator( + mock_server, + mock_connection, + # Mixed case intentional + return_values=("SHUToff", "VERIFY_RESIZE"), + ) + snapshot_and_migrate_server( conn=mock_connection, server_id=mock_server_id, @@ -84,6 +139,7 @@ def test_shutoff_migration( snapshot=True, live_migration=False, ) + mock_snapshot_server.assert_called_once_with( conn=mock_connection, server_id=mock_server_id ) @@ -92,23 +148,59 @@ def test_shutoff_migration( server=mock_server_id, host=dest_host ) mock_connection.compute.wait_for_server.assert_called_once_with( - mock_server, status="VERIFY_RESIZE", wait=3600 + mock_server, status="VERIFY_RESIZE", wait=10 ) mock_connection.compute.confirm_server_resize.assert_called_once_with( mock_server_id ) mock_wait_for_migration_status.assert_called_once_with( - mock_connection, mock_server_id, "confirmed" + mock_connection, mock_server_id, "finished" ) +@patch("apis.openstack_api.openstack_server.snapshot_server") +@patch("apis.openstack_api.openstack_server.wait_for_migration_status") +def test_invalid_status_after_migration(_, __): + """ + Tests if the migration is marked finished but the VM moves into an error or unknown state instead of VERIFY_RESIZE + """ + mock_connection = MagicMock() + mock_server = MagicMock() + + _migration_side_effect_generator( + mock_server, mock_connection, return_values=("ACTIVE", "ERROR") + ) + with pytest.raises(RuntimeError): + snapshot_and_migrate_server( + conn=mock_connection, + server_id="", + snapshot=False, + live_migration=False, + ) + + mock_connection.compute.migrate_server.assert_called_once() + mock_connection.compute.confirm_resize.assert_not_called() + + # Also confirm we error on a weird state like ACTIVE which migrate should not go to + _migration_side_effect_generator( + mock_server, mock_connection, return_values=("ACTIVE", "ACTIVE") + ) + with pytest.raises(RuntimeError): + snapshot_and_migrate_server( + conn=mock_connection, + server_id="", + snapshot=False, + live_migration=False, + ) + + @patch("apis.openstack_api.openstack_server.snapshot_server") @patch("apis.openstack_api.openstack_server.wait_for_migration_status") def test_active_migration_failed_wait_for_status( mock_wait_for_migration_status, mock_snapshot_server ): """ - Test migration when the the status of migration raises an error + Test migration when the status of migration raises an error """ mock_connection = MagicMock() mock_server_id = "server1" From eeaf104eefda96e6f1adeb20eaec2294eecf8b46 Mon Sep 17 00:00:00 2001 From: David Fairbrother Date: Wed, 1 Jul 2026 10:25:17 +0100 Subject: [PATCH 4/6] BUG: Skip any migration or snapshot steps if we're in an invalid state Skips any migration steps, as we would previously take a snapshot - force the user to wait for multiple hours then fail when we knew up-front it wouldn't work. Instead provide rapid feedback that we can't do an error state or we can't migrate before doing any work --- lib/apis/openstack_api/openstack_server.py | 26 ++++++----- .../openstack_api/test_openstack_server.py | 43 ++++++++++++++++++- 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/lib/apis/openstack_api/openstack_server.py b/lib/apis/openstack_api/openstack_server.py index e2f997beb..c91cc53ca 100644 --- a/lib/apis/openstack_api/openstack_server.py +++ b/lib/apis/openstack_api/openstack_server.py @@ -10,7 +10,7 @@ logger = logging.getLogger(__name__) -def can_be_migrated(server: Server): +def check_can_be_live_migrated(server: Server): if server.flavor.name.startswith("g-") or server.flavor.name.startswith("f-"): raise ValueError( f"Attempted to move GPU or FPGA flavor, {server.flavor.name}, which is not allowed!" @@ -74,26 +74,30 @@ def snapshot_and_migrate_server( :param dest_host: Optional host to migrate to, otherwise chosen by scheduler """ server = conn.compute.get_server(server_id) - if snapshot: - snapshot_server(conn=conn, server_id=server_id) - time.sleep(10) # Ensure server task status has updated after snapshot - logger.info("Migrating server: %s", server.id) - server_status = server.status.casefold() match server_status: case "shutoff": - _cold_migration(conn, server, dest_host) + # Can't live migrate something that isn't 'alive' + logger.info("Server %s is shutoff, forcing cold migration", server.id) + live_migration = False case "active": if live_migration: - can_be_migrated(server) - _live_migration(conn, server, dest_host) - else: - _cold_migration(conn, server, dest_host) + check_can_be_live_migrated(server) case _: raise ValueError( f"Server status: {server.status}. The server must be ACTIVE or SHUTOFF to be migrated" ) + + if snapshot: + snapshot_server(conn=conn, server_id=server_id) + time.sleep(10) # Ensure server task status has updated after snapshot + logger.info("Migrating server: %s", server.id) + + if live_migration: + _live_migration(conn, server, dest_host) + else: + _cold_migration(conn, server, dest_host) logger.info("Migration completed of server: %s", server.id) diff --git a/tests/lib/apis/openstack_api/test_openstack_server.py b/tests/lib/apis/openstack_api/test_openstack_server.py index 27a9a6c22..0d0236e3b 100644 --- a/tests/lib/apis/openstack_api/test_openstack_server.py +++ b/tests/lib/apis/openstack_api/test_openstack_server.py @@ -1,7 +1,7 @@ import itertools from datetime import datetime from typing import Tuple, Any -from unittest.mock import MagicMock, patch, PropertyMock +from unittest.mock import MagicMock, patch, PropertyMock, NonCallableMock import pytest from apis.openstack_api.openstack_server import ( @@ -465,6 +465,47 @@ def test_wait_for_migration_status_error(bad_state): ) +@patch("apis.openstack_api.openstack_server.snapshot_server") +@patch("apis.openstack_api.openstack_server._live_migration") +@patch("apis.openstack_api.openstack_server._cold_migration") +def test_we_skip_all_steps_for_error(cold_migration, live_migration, snapshot_method): + """ + Ensures that we don't attempt to perform any work at all if a server is in an invalid state + as the snapshot step especially can take multiple hours when we already know up-front we'll fail + """ + mock_server = NonCallableMock(return_value="error") + mock_connection = MagicMock() + mock_connection.compute.get_server.return_value = mock_server + + def _assert_no_migration_steps(): + snapshot_method.assert_not_called() + cold_migration.assert_not_called() + live_migration.assert_not_called() + + snapshot_method.reset_mock() + cold_migration.reset_mock() + live_migration.reset_mock() + + # Ensure no combination of flags bypasses these checks + with pytest.raises(ValueError): + snapshot_and_migrate_server( + conn=mock_connection, server_id="", snapshot=True, live_migration=False + ) + _assert_no_migration_steps() + + with pytest.raises(ValueError): + snapshot_and_migrate_server( + conn=mock_connection, server_id="", snapshot=False, live_migration=False + ) + _assert_no_migration_steps() + + with pytest.raises(ValueError): + snapshot_and_migrate_server( + conn=mock_connection, server_id="", snapshot=False, live_migration=True + ) + _assert_no_migration_steps() + + def test_build_server(): """ Test build server on a given hypervisor From 5a173b149c72ce2df827d087eb20eba1bfceb0c9 Mon Sep 17 00:00:00 2001 From: DavidFair Date: Wed, 1 Jul 2026 10:51:29 +0100 Subject: [PATCH 5/6] Revert "BUG: Skip any migration or snapshot steps if we're in an invalid state" --- lib/apis/openstack_api/openstack_server.py | 26 +++++------ .../openstack_api/test_openstack_server.py | 43 +------------------ 2 files changed, 12 insertions(+), 57 deletions(-) diff --git a/lib/apis/openstack_api/openstack_server.py b/lib/apis/openstack_api/openstack_server.py index c91cc53ca..e2f997beb 100644 --- a/lib/apis/openstack_api/openstack_server.py +++ b/lib/apis/openstack_api/openstack_server.py @@ -10,7 +10,7 @@ logger = logging.getLogger(__name__) -def check_can_be_live_migrated(server: Server): +def can_be_migrated(server: Server): if server.flavor.name.startswith("g-") or server.flavor.name.startswith("f-"): raise ValueError( f"Attempted to move GPU or FPGA flavor, {server.flavor.name}, which is not allowed!" @@ -74,30 +74,26 @@ def snapshot_and_migrate_server( :param dest_host: Optional host to migrate to, otherwise chosen by scheduler """ server = conn.compute.get_server(server_id) + if snapshot: + snapshot_server(conn=conn, server_id=server_id) + time.sleep(10) # Ensure server task status has updated after snapshot + logger.info("Migrating server: %s", server.id) + server_status = server.status.casefold() match server_status: case "shutoff": - # Can't live migrate something that isn't 'alive' - logger.info("Server %s is shutoff, forcing cold migration", server.id) - live_migration = False + _cold_migration(conn, server, dest_host) case "active": if live_migration: - check_can_be_live_migrated(server) + can_be_migrated(server) + _live_migration(conn, server, dest_host) + else: + _cold_migration(conn, server, dest_host) case _: raise ValueError( f"Server status: {server.status}. The server must be ACTIVE or SHUTOFF to be migrated" ) - - if snapshot: - snapshot_server(conn=conn, server_id=server_id) - time.sleep(10) # Ensure server task status has updated after snapshot - logger.info("Migrating server: %s", server.id) - - if live_migration: - _live_migration(conn, server, dest_host) - else: - _cold_migration(conn, server, dest_host) logger.info("Migration completed of server: %s", server.id) diff --git a/tests/lib/apis/openstack_api/test_openstack_server.py b/tests/lib/apis/openstack_api/test_openstack_server.py index 0d0236e3b..27a9a6c22 100644 --- a/tests/lib/apis/openstack_api/test_openstack_server.py +++ b/tests/lib/apis/openstack_api/test_openstack_server.py @@ -1,7 +1,7 @@ import itertools from datetime import datetime from typing import Tuple, Any -from unittest.mock import MagicMock, patch, PropertyMock, NonCallableMock +from unittest.mock import MagicMock, patch, PropertyMock import pytest from apis.openstack_api.openstack_server import ( @@ -465,47 +465,6 @@ def test_wait_for_migration_status_error(bad_state): ) -@patch("apis.openstack_api.openstack_server.snapshot_server") -@patch("apis.openstack_api.openstack_server._live_migration") -@patch("apis.openstack_api.openstack_server._cold_migration") -def test_we_skip_all_steps_for_error(cold_migration, live_migration, snapshot_method): - """ - Ensures that we don't attempt to perform any work at all if a server is in an invalid state - as the snapshot step especially can take multiple hours when we already know up-front we'll fail - """ - mock_server = NonCallableMock(return_value="error") - mock_connection = MagicMock() - mock_connection.compute.get_server.return_value = mock_server - - def _assert_no_migration_steps(): - snapshot_method.assert_not_called() - cold_migration.assert_not_called() - live_migration.assert_not_called() - - snapshot_method.reset_mock() - cold_migration.reset_mock() - live_migration.reset_mock() - - # Ensure no combination of flags bypasses these checks - with pytest.raises(ValueError): - snapshot_and_migrate_server( - conn=mock_connection, server_id="", snapshot=True, live_migration=False - ) - _assert_no_migration_steps() - - with pytest.raises(ValueError): - snapshot_and_migrate_server( - conn=mock_connection, server_id="", snapshot=False, live_migration=False - ) - _assert_no_migration_steps() - - with pytest.raises(ValueError): - snapshot_and_migrate_server( - conn=mock_connection, server_id="", snapshot=False, live_migration=True - ) - _assert_no_migration_steps() - - def test_build_server(): """ Test build server on a given hypervisor From 039b7e1309ce7ae87d8affab819d6130101e89c8 Mon Sep 17 00:00:00 2001 From: DavidFair Date: Wed, 1 Jul 2026 10:57:33 +0100 Subject: [PATCH 6/6] Revert "BUG: Skip any migration or snapshot steps if we're in an invalid state"