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 %> diff --git a/lib/apis/openstack_api/openstack_server.py b/lib/apis/openstack_api/openstack_server.py index 208e1a74b..e2f997beb 100644 --- a/lib/apis/openstack_api/openstack_server.py +++ b/lib/apis/openstack_api/openstack_server.py @@ -15,20 +15,53 @@ 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) + + # 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) + + +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 +70,30 @@ 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 - ) - wait_for_migration_status(conn, server_id, "completed") + + server_status = server.status.casefold() + + match server_status: + case "shutoff": + _cold_migration(conn, server, dest_host) + 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 f7fd373de..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 ( @@ -34,6 +35,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 +45,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 @@ -57,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") @@ -69,17 +121,25 @@ def test_shutoff_migration( mock_connection = MagicMock() mock_server_id = "server1" mock_server = MagicMock() - mock_server.status = "SHUTOFF" + mock_server.id = mock_server_id 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, dest_host=dest_host, snapshot=True, + live_migration=False, ) + mock_snapshot_server.assert_called_once_with( conn=mock_connection, server_id=mock_server_id ) @@ -88,25 +148,64 @@ 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_connection.compute.confirm_server_resize(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" 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 +218,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 +241,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 +251,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 +270,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 +283,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 +335,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 +346,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, )