Feature: Control of balance for instances #462

1yam · 2023-08-03T15:50:54Z

Added functionality to check the current balance and compute costs for instances.

odesenfans

Good job, a few things to change here and there to keep you busy but the logic seems sound. Needs more unit tests though!

odesenfans · 2023-08-03T21:27:28Z

deployment/migrations/versions/0017_f9fa39b6bdef_vm_instances.py

@@ -129,7 +129,7 @@ def recreate_cost_views():
                                           free_disk.included_disk_space,
                                           0) AS additional_disk_space) additional_disk,
                 LATERAL ( SELECT CASE
-                                      WHEN vms.persistent THEN 2000
+                                      WHEN COALESCE(vms.persistent, true) THEN 2000
                                      ELSE 200
                                      END AS base_compute_unit_price) bcp,
                 LATERAL ( SELECT 1 + vms.environment_internet::integer AS compute_unit_price_multiplier) m,


The changes to this migration script will have to be put in a separate migration script that recreates the VM cost view (views must be recreated entirely, you cannot just add a column). Create a new empty migration script with alembic revision -m "fix VM cost view" and then take the pieces that you need from this migration script to drop the vm cost view and recreate it.

The changes to this file must be reverted now.

src/aleph/handlers/content/content_handler.py

odesenfans · 2023-08-03T21:31:04Z

src/aleph/handlers/content/vm.py

@@ -298,6 +354,24 @@ class VmMessageHandler(ContentHandler):

    """

+    async def check_balance(self, session: DbSession, message: MessageDb):


Missing return value type (-> None).

odesenfans · 2023-08-03T21:33:52Z

src/aleph/handlers/content/vm.py

@@ -289,6 +301,50 @@ class HasParent(Protocol):
            )


+def get_extra_storage(content: InstanceContent, session: DbSession) -> int:
+    volumes: int = 0
+    for i in content.volumes:


Rename i to volume.

odesenfans · 2023-08-03T21:44:04Z

src/aleph/handlers/content/vm.py

@@ -289,6 +301,50 @@ class HasParent(Protocol):
            )


+def get_extra_storage(content: InstanceContent, session: DbSession) -> int:
+    volumes: int = 0


Rename to total_volume_size. It would be better if this function returned the size in bytes, and then let the caller do whatever it wants (convert to MB, MiB, Watts, I don't care).

odesenfans · 2023-08-03T21:48:02Z

src/aleph/handlers/content/vm.py

+    return volumes
+
+
+def get_additional_storage_price(content, session: DbSession) -> float:


Always use decimals when manipulating prices.

odesenfans · 2023-08-03T21:52:07Z

src/aleph/handlers/message_handler.py

@@ -316,6 +315,7 @@ async def process(
            session=session, pending_message=pending_message
        )
        content_handler = self.get_content_handler(message.type)
+        await content_handler.check_balance(session=session, message=message)


This should definitely be called after check_dependencies (otherwise we might look for volumes that don't exist) and probably after check_permissions (we should reduce the amount of computation done before checking permissions).

src/aleph/types/message_status.py

odesenfans · 2023-08-03T21:54:09Z

tests/message_processing/test_process_instances.py

@@ -2,6 +2,7 @@
 import itertools


I 'd like to see more tests:

unit tests for the code that computes the cost of a single InstanceContent object.

a test here that tries to process an instance without having enough tokens.

Where are the additional tests?

odesenfans · 2023-08-03T21:54:35Z

src/aleph/db/accessors/vms.py

@@ -113,3 +113,14 @@ def refresh_vm_version(session: DbSession, vm_hash: str) -> None:
    )
    session.execute(delete(VmVersionDb).where(VmVersionDb.vm_hash == vm_hash))
    session.execute(upsert_stmt)
+
+
+def get_total_cost_for_address(session: DbSession, address: str) -> int:


The return value should be a Decimal (costs can be fractional).

…of sql request

odesenfans

A lot of my comments from the first review still need to be addressed.

odesenfans · 2023-08-08T15:47:50Z

deployment/migrations/versions/0017_f9fa39b6bdef_vm_instances.py

@@ -129,7 +129,7 @@ def recreate_cost_views():
                                           free_disk.included_disk_space,
                                           0) AS additional_disk_space) additional_disk,
                 LATERAL ( SELECT CASE
-                                      WHEN vms.persistent THEN 2000
+                                      WHEN COALESCE(vms.persistent, true) THEN 2000
                                      ELSE 200
                                      END AS base_compute_unit_price) bcp,
                 LATERAL ( SELECT 1 + vms.environment_internet::integer AS compute_unit_price_multiplier) m,


The changes to this file must be reverted now.

odesenfans · 2023-08-08T15:48:59Z

deployment/migrations/versions/0018_7bcb8e5fe186_fix_vm_cost_view.py

+def downgrade() -> None:
+    op.execute("drop view costs_view")
+    op.execute("drop view vm_costs_view")
+    op.execute("drop view vm_volumes_files_view")


That's not really what the downgrade should do. A downgrade should bring the view back to its previous version. I'll fix it myself, just FYI.

src/aleph/handlers/content/content_handler.py

odesenfans · 2023-08-08T15:50:25Z

src/aleph/handlers/content/vm.py

@@ -243,7 +255,7 @@ def add_ref_to_check(_volume):


 def check_parent_volumes_size_requirements(
-    session: DbSession, content: ExecutableContent
+        session: DbSession, content: ExecutableContent


I see a few changes in spacing in the PR, I don't understand why. Are you using the PyCharm formatter instead of black?

odesenfans · 2023-08-08T15:50:49Z

src/aleph/handlers/content/vm.py

+    total_volume_size: int = 0
+    for volume in content.volumes:
+        if hasattr(volume, "ref") and volume.ref:
+            pin_file = get_message_file_pin(session=session, item_hash=volume.ref)


As discussed, this should be get_file_tag.

src/aleph/types/message_status.py

odesenfans · 2023-08-08T15:52:07Z

tests/message_processing/test_process_instances.py

@@ -2,6 +2,7 @@
 import itertools


Where are the additional tests?

Co-authored-by: Olivier Desenfans <[email protected]>

…1yam-vm-balance-check

odesenfans and others added 2 commits July 27, 2023 23:38

Do not merge: fix cost view migration

64b4341

Feature: Control of balance for Instances

6d21807

Added functionality to check the current balance and compute costs for instances.

odesenfans requested changes Aug 3, 2023

View reviewed changes

1yam added 16 commits August 7, 2023 15:18

Fix: price calculations in db views

4a42b7a

Fix: return type Decimal

5dce8d1

Refactor: rename get_extra_storage to get_volume_size

0035df4

Refactor: functions get_volume_size

5d19d62

Fix: mypy

c58f02d

Add new test with no balance

00be932

Fix: Add ERROR code 5 (Insufficient balances) in db migrations

7681e22

Fix: Unit test

10404a5

Test

db7f847

Fix: Launch control balance only on message post

eb2c7ea

Fix: Test error with no balance

67cf926

Fix: No balance test

bb3e11e

Fix: control message type

2cff692

Fix: re add __version__

9f1738a

Refractor: get_volum_size use get_message_file_pin functions instead …

5a43c29

…of sql request

Fix: mypy error

64d00b0

odesenfans requested changes Aug 8, 2023

View reviewed changes

odesenfans and others added 7 commits August 8, 2023 21:37

Internal: reenable AVAX signature unit tests (#461)

0175169

Update src/aleph/handlers/content/content_handler.py

b326c4f

Co-authored-by: Olivier Desenfans <[email protected]>

Merge branch '1yam-vm-balance-check' of github.com:1yam/pyaleph into …

68eb9e0

…1yam-vm-balance-check

Fix: type error functions return Decimal instead of int

9dfc207

Refactor: move cost calcul & test for cost

377ae1a

Refactor type : ExecutableContent instead of ExecutableContent

6a46672

Fix

fbf68a5

1yam requested a review from odesenfans August 8, 2023 21:06

odesenfans added 2 commits August 21, 2023 17:04

Fix: formatting issues

c3225d7

Fix: compute included storage in GiB instead of GB

f2af477

odesenfans added 2 commits August 21, 2023 23:07

Fix: insufficient balance error format

b2334dd

mib

98b8129

odesenfans approved these changes Aug 22, 2023

View reviewed changes

odesenfans changed the title ~~Feature: Control of balance for Instances~~ Feature: Control of balance for instances Aug 22, 2023

odesenfans merged commit 4c50268 into aleph-im:master Aug 22, 2023
2 checks passed

odesenfans deleted the 1yam-vm-balance-check branch August 22, 2023 09:47

1yam mentioned this pull request Aug 31, 2023

Feature : Balance Check persistent VM #469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Control of balance for instances #462

Feature: Control of balance for instances #462

1yam commented Aug 3, 2023 •

edited

Loading

odesenfans left a comment

odesenfans Aug 3, 2023

odesenfans Aug 8, 2023

odesenfans Aug 3, 2023

odesenfans Aug 3, 2023

odesenfans Aug 3, 2023

odesenfans Aug 3, 2023

odesenfans Aug 3, 2023

odesenfans Aug 3, 2023

odesenfans Aug 8, 2023

odesenfans Aug 3, 2023

odesenfans left a comment

odesenfans Aug 8, 2023

odesenfans Aug 8, 2023

odesenfans Aug 8, 2023

odesenfans Aug 8, 2023

odesenfans Aug 8, 2023

		@@ -298,6 +354,24 @@ class VmMessageHandler(ContentHandler):

		"""

		async def check_balance(self, session: DbSession, message: MessageDb):

		return volumes


		def get_additional_storage_price(content, session: DbSession) -> float:

Feature: Control of balance for instances #462

Feature: Control of balance for instances #462

Conversation

1yam commented Aug 3, 2023 • edited Loading

odesenfans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odesenfans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1yam commented Aug 3, 2023 •

edited

Loading