feat(FIR-10741): Defaulting to cheapest engine type (#125)

Author: ptiurin

src/firebolt/service/engine.py

@@ -1,6 +1,7 @@
 from logging import getLogger
 from typing import List, Optional, Union
 
+from firebolt.common.exception import FireboltError
 from firebolt.common.urls import (
     ACCOUNT_ENGINE_BY_NAME_URL,
     ACCOUNT_ENGINE_URL,
@@ -107,7 +108,7 @@ def create(
         region: Union[str, Region, None] = None,
         engine_type: Union[str, EngineType] = EngineType.GENERAL_PURPOSE,
         scale: int = 2,
-        spec: str = "i3.4xlarge",
+        spec: Optional[str] = None,
         auto_stop: int = 20,
         warmup: Union[str, WarmupMethod] = WarmupMethod.PRELOAD_INDEXES,
         description: str = "",
@@ -121,7 +122,8 @@ def create(
             engine_type: The engine type. GENERAL_PURPOSE or DATA_ANALYTICS
             scale: The number of compute instances on the engine.
                 The scale can be any int from 1 to 128.
-            spec: The AWS EC2 instance type.
+            spec: Firebolt instance type. If not set will default to
+                the cheapest instance.
             auto_stop: The amount of time (in minutes)
             after which the engine automatically stops.
             warmup: The warmup method that should be used.
@@ -162,9 +164,19 @@ def create(
             ),
         )
 
-        instance_type_key = self.resource_manager.instance_types.get_by_name(
-            instance_type_name=spec
-        ).key
+        if spec:
+            instance_type_key = self.resource_manager.instance_types.get_by_name(
+                instance_type_name=spec
+            ).key
+        else:
+            instance_type = (
+                self.resource_manager.instance_types.cheapest_instance_in_region(region)
+            )
+            if not instance_type:
+                raise FireboltError(
+                    f"No suitable default instances found in region {region}"
+                )
+            instance_type_key = instance_type.key
 
         engine_revision = EngineRevision(
             specification=EngineRevisionSpecification(

src/firebolt/service/instance_type.py

@@ -3,6 +3,7 @@
 from firebolt.common.urls import INSTANCE_TYPES_URL
 from firebolt.common.util import cached_property
 from firebolt.model.instance_type import InstanceType, InstanceTypeKey
+from firebolt.model.region import Region
 from firebolt.service.base import BaseService
 
 
@@ -41,6 +42,31 @@ def instance_types_by_name(self) -> Dict[InstanceTypeLookup, InstanceType]:
             for i in self.instance_types
         }
 
+    def cheapest_instance_in_region(self, region: Region) -> Optional[InstanceType]:
+        # Get only awailable instances in region
+        response = self.client.get(
+            url=INSTANCE_TYPES_URL,
+            params={"page.first": 5000, "filter.id_region_id_eq": region.key.region_id},
+        )
+        instance_types = [
+            InstanceType.parse_obj(i["node"]) for i in response.json()["edges"]
+        ]
+        # Filter out instances without storage
+        instance_list = [
+            i
+            for i in instance_types
+            if i.storage_size_bytes and i.storage_size_bytes != "0"
+        ]
+        if not instance_list:
+            return None
+        cheapest = min(
+            instance_list,
+            key=lambda x: x.price_per_hour_cents
+            if x.price_per_hour_cents
+            else float("Inf"),
+        )
+        return cheapest
+
     def get_by_key(self, instance_type_key: InstanceTypeKey) -> InstanceType:
         """Get an instance type by key."""
 

tests/unit/service/conftest.py

@@ -22,6 +22,7 @@
 from firebolt.model.database import Database, DatabaseKey
 from firebolt.model.engine import Engine, EngineKey, EngineSettings
 from firebolt.model.instance_type import InstanceType, InstanceTypeKey
+from firebolt.model.region import Region
 from tests.unit.util import list_to_paginated_response
 
 
@@ -55,6 +56,8 @@ def instance_type_1(provider, region_1) -> InstanceType:
             instance_type_id="instance_type_id_1",
         ),
         name="i3.4xlarge",
+        price_per_hour_cents=10,
+        storage_size_bytes=0,
     )
 
 
@@ -67,12 +70,35 @@ def instance_type_2(provider, region_2) -> InstanceType:
             instance_type_id="instance_type_id_2",
         ),
         name="i3.8xlarge",
+        price_per_hour_cents=20,
+        storage_size_bytes=500,
     )
 
 
 @pytest.fixture
-def mock_instance_types(instance_type_1, instance_type_2) -> List[InstanceType]:
-    return [instance_type_1, instance_type_2]
+def instance_type_3(provider, region_2) -> InstanceType:
+    return InstanceType(
+        key=InstanceTypeKey(
+            provider_id=provider.provider_id,
+            region_id=region_2.key.region_id,
+            instance_type_id="instance_type_id_2",
+        ),
+        name="i3.8xlarge",
+        price_per_hour_cents=30,
+        storage_size_bytes=500,
+    )
+
+
+@pytest.fixture
+def cheapest_instance(instance_type_2) -> InstanceType:
+    return instance_type_2
+
+
+@pytest.fixture
+def mock_instance_types(
+    instance_type_1, instance_type_2, instance_type_3
+) -> List[InstanceType]:
+    return [instance_type_1, instance_type_2, instance_type_3]
 
 
 @pytest.fixture
@@ -130,11 +156,58 @@ def do_mock(
     return do_mock
 
 
+@pytest.fixture
+def instance_type_region_1_callback(
+    instance_type_region_1_url: str, mock_instance_types
+) -> Callable:
+    def do_mock(
+        request: httpx.Request = None,
+        **kwargs,
+    ) -> Response:
+        assert request.url == instance_type_region_1_url
+        return Response(
+            status_code=httpx.codes.OK,
+            json=list_to_paginated_response(mock_instance_types),
+        )
+
+    return do_mock
+
+
+@pytest.fixture
+def instance_type_empty_callback() -> Callable:
+    def do_mock(
+        request: httpx.Request = None,
+        **kwargs,
+    ) -> Response:
+        return Response(
+            status_code=httpx.codes.OK,
+            json=list_to_paginated_response([]),
+        )
+
+    return do_mock
+
+
 @pytest.fixture
 def instance_type_url(settings: Settings) -> str:
     return f"https://{settings.server}{INSTANCE_TYPES_URL}?page.first=5000"
 
 
+@pytest.fixture
+def instance_type_region_1_url(settings: Settings, region_1: Region) -> str:
+    return (
+        f"https://{settings.server}{INSTANCE_TYPES_URL}?page.first=5000&"
+        f"filter.id_region_id_eq={region_1.key.region_id}"
+    )
+
+
+@pytest.fixture
+def instance_type_region_2_url(settings: Settings, region_2: Region) -> str:
+    return (
+        f"https://{settings.server}{INSTANCE_TYPES_URL}?page.first=5000&"
+        f"filter.id_region_id_eq={region_2.key.region_id}"
+    )
+
+
 @pytest.fixture
 def engine_callback(engine_url: str, mock_engine) -> Callable:
     def do_mock(

tests/unit/service/test_engine.py

@@ -4,9 +4,10 @@
 from pytest_httpx import HTTPXMock
 
 from firebolt.common import Settings
-from firebolt.common.exception import NoAttachedDatabaseError
+from firebolt.common.exception import FireboltError, NoAttachedDatabaseError
 from firebolt.model.engine import Engine
 from firebolt.model.instance_type import InstanceType
+from firebolt.model.region import Region
 from firebolt.service.manager import ResourceManager
 
 
@@ -16,8 +17,8 @@ def test_engine_create(
     auth_url: str,
     provider_callback: Callable,
     provider_url: str,
-    instance_type_callback: Callable,
-    instance_type_url: str,
+    instance_type_region_1_callback: Callable,
+    instance_type_region_1_url: str,
     region_callback: Callable,
     region_url: str,
     settings: Settings,
@@ -32,7 +33,9 @@ def test_engine_create(
 ):
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(provider_callback, url=provider_url)
-    httpx_mock.add_callback(instance_type_callback, url=instance_type_url)
+    httpx_mock.add_callback(
+        instance_type_region_1_callback, url=instance_type_region_1_url
+    )
     httpx_mock.add_callback(account_id_callback, url=account_id_url)
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(region_callback, url=region_url)
@@ -44,14 +47,44 @@ def test_engine_create(
     assert engine.name == engine_name
 
 
+def test_engine_create_no_available_types(
+    httpx_mock: HTTPXMock,
+    auth_callback: Callable,
+    auth_url: str,
+    provider_callback: Callable,
+    provider_url: str,
+    instance_type_empty_callback: Callable,
+    instance_type_region_2_url: str,
+    settings: Settings,
+    mock_instance_types: List[InstanceType],
+    engine_name: str,
+    account_id_callback: Callable,
+    account_id_url: str,
+    engine_url: str,
+    region_2: Region,
+):
+    httpx_mock.add_callback(auth_callback, url=auth_url)
+    httpx_mock.add_callback(provider_callback, url=provider_url)
+    httpx_mock.add_callback(
+        instance_type_empty_callback, url=instance_type_region_2_url
+    )
+    httpx_mock.add_callback(account_id_callback, url=account_id_url)
+    httpx_mock.add_callback(auth_callback, url=auth_url)
+
+    manager = ResourceManager(settings=settings)
+
+    with raises(FireboltError):
+        manager.engines.create(name=engine_name, region=region_2)
+
+
 def test_engine_no_attached_database(
     httpx_mock: HTTPXMock,
     auth_callback: Callable,
     auth_url: str,
     provider_callback: Callable,
     provider_url: str,
-    instance_type_callback: Callable,
-    instance_type_url: str,
+    instance_type_region_1_callback: Callable,
+    instance_type_region_1_url: str,
     region_callback: Callable,
     region_url: str,
     settings: Settings,
@@ -72,7 +105,9 @@ def test_engine_no_attached_database(
 ):
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(provider_callback, url=provider_url)
-    httpx_mock.add_callback(instance_type_callback, url=instance_type_url)
+    httpx_mock.add_callback(
+        instance_type_region_1_callback, url=instance_type_region_1_url
+    )
     httpx_mock.add_callback(account_id_callback, url=account_id_url)
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(region_callback, url=region_url)
@@ -92,8 +127,8 @@ def test_engine_start_binding_to_missing_database(
     auth_url: str,
     provider_callback: Callable,
     provider_url: str,
-    instance_type_callback: Callable,
-    instance_type_url: str,
+    instance_type_region_1_callback: Callable,
+    instance_type_region_1_url: str,
     region_callback: Callable,
     region_url: str,
     settings: Settings,
@@ -112,7 +147,9 @@ def test_engine_start_binding_to_missing_database(
 ):
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(provider_callback, url=provider_url)
-    httpx_mock.add_callback(instance_type_callback, url=instance_type_url)
+    httpx_mock.add_callback(
+        instance_type_region_1_callback, url=instance_type_region_1_url
+    )
     httpx_mock.add_callback(account_id_callback, url=account_id_url)
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(region_callback, url=region_url)
@@ -133,8 +170,8 @@ def test_get_connection(
     auth_url: str,
     provider_callback: Callable,
     provider_url: str,
-    instance_type_callback: Callable,
-    instance_type_url: str,
+    instance_type_region_1_callback: Callable,
+    instance_type_region_1_url: str,
     region_callback: Callable,
     region_url: str,
     settings: Settings,
@@ -154,7 +191,9 @@ def test_get_connection(
 ):
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(provider_callback, url=provider_url)
-    httpx_mock.add_callback(instance_type_callback, url=instance_type_url)
+    httpx_mock.add_callback(
+        instance_type_region_1_callback, url=instance_type_region_1_url
+    )
     httpx_mock.add_callback(account_id_callback, url=account_id_url)
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(region_callback, url=region_url)
@@ -178,8 +217,8 @@ def test_attach_to_database(
     provider_url: str,
     region_callback: Callable,
     region_url: str,
-    instance_type_callback: Callable,
-    instance_type_url: str,
+    instance_type_region_1_callback: Callable,
+    instance_type_region_1_url: str,
     settings: Settings,
     account_id_callback: Callable,
     account_id_url: str,
@@ -200,7 +239,9 @@ def test_attach_to_database(
 ):
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(provider_callback, url=provider_url)
-    httpx_mock.add_callback(instance_type_callback, url=instance_type_url)
+    httpx_mock.add_callback(
+        instance_type_region_1_callback, url=instance_type_region_1_url
+    )
     httpx_mock.add_callback(account_id_callback, url=account_id_url)
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(bindings_callback, url=bindings_url)