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