-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature : Balance Check persistent VM #469
Conversation
src/aleph/handlers/content/vm.py
Outdated
@@ -320,15 +320,19 @@ class VmMessageHandler(ContentHandler): | |||
""" | |||
|
|||
async def check_balance(self, session: DbSession, message: MessageDb) -> None: | |||
if message.type != MessageType.instance: | |||
if not (message.type == MessageType.instance) and not (message.type == MessageType.program): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_get_vm_content()
called below already performs this check, you can just remove this line.
src/aleph/handlers/content/vm.py
Outdated
if isinstance(content, ProgramContent): | ||
return | ||
|
||
if message.type == MessageType.program: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easier for mypy to figure out what's going on if you use if isinstance(content, ProgramContent)
.
src/aleph/handlers/content/vm.py
Outdated
current_balance = ( | ||
get_total_balance(address=content.address, session=session) or 0 | ||
get_total_balance(address=content.address, session=session) or 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting, please apply black
to all your PRs when opening them. Whitespace changes are just noise in the Git history.
src/aleph/services/cost.py
Outdated
@@ -12,9 +13,8 @@ | |||
|
|||
|
|||
def _get_file_from_ref( | |||
session: DbSession, ref: str, use_latest: bool | |||
session: DbSession, ref: str, use_latest: bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, please format your PRs with black
. It seems like you have a different formatter running automatically.
src/aleph/services/cost.py
Outdated
if file is None: | ||
raise RuntimeError(f"Could not find entry in file tags for {volume.ref}.") | ||
total_volume_size += Decimal(file.size) | ||
if hasattr(volume, "ref"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add this condition? ref_volumes
is defined as "all the volumes having a ref
field" so this seems unneeded.
src/aleph/services/cost.py
Outdated
cpu: Decimal = Decimal(content.resources.vcpus) | ||
memory: Decimal = Decimal(math.ceil(content.resources.memory / 2000)) | ||
compute_unit_multiplier = cpu if cpu >= memory else memory | ||
environment_internet: Decimal = Decimal(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity and similarity with the view, could you change this to:
- Compute the number of compute units in a function
- Instead of
environment_internet
, could you add another function to return a "compute unit multiplier" based on the content of the message? Also you can use integers here, conversion to decimal will be automatic.
src/aleph/services/cost.py
Outdated
@@ -77,7 +78,18 @@ def get_additional_storage_price( | |||
def compute_cost(session: DbSession, content: ExecutableContent) -> Decimal: | |||
is_microvm = isinstance(content, ProgramContent) and not content.on.persistent | |||
compute_unit_cost: Decimal = Decimal("200.0") if is_microvm else Decimal("2000.0") | |||
cpu: Decimal = Decimal(content.resources.vcpus) | |||
memory: Decimal = Decimal(math.ceil(content.resources.memory / 2000)) | |||
compute_unit_multiplier = cpu if cpu >= memory else memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to nb_compute_units
.
src/aleph/services/cost.py
Outdated
|
||
return (compute_unit_cost * content.resources.vcpus) + get_additional_storage_price( | ||
compute_units_required = compute_unit_multiplier | ||
compute_unit_multiplier = environment_internet or 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the comment above, this can be simplified to
compute_unit_multiplier = 1
if content.environment.internet:
compute_unit_multiplier += 1
Based on : #462
Solutions:
- Add check of the balance for VM persistent
- Fix View who did not show Persistent VM