Revert "BUG: Skip any migration or snapshot steps if we're in an invalid state"

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)
 
 

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