Skip to content
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: Allow User to control their VM #124

Merged
merged 34 commits into from
Jul 5, 2024
Merged

Feature: Allow User to control their VM #124

merged 34 commits into from
Jul 5, 2024

Conversation

1yam
Copy link
Collaborator

@1yam 1yam commented Jun 12, 2024

Summary
This feature introduces new capabilities for users to manage their virtual machines (VMs) directly. The following functionalities are included:

  • Notify instances
  • Stop VMs
  • Reboot VMs
  • Erase VMs
  • Expire VMs
  • Get Logs

Copy link

Explanation:
The PR adds a new file vmclient.py to the project. This file includes a class VmClient that interacts with a virtual machine via HTTP requests. The class has methods to perform various operations on the virtual machine, such as starting, stopping, rebooting, erasing, expiring, and notifying about the allocation of a virtual machine. This level of complexity is suitable for a 'RED' review as it involves adding new functionality and modifying existing code, which may require a deep understanding of the project architecture.

Highlights:

  • Addition of a new file vmclient.py
  • Addition of a new class VmClient
  • Implementation of various methods for interacting with a virtual machine

Marksdown:

### Categorization: RED

#### New File: vmclient.py
The PR adds a new file `vmclient.py`. This file includes a class `VmClient` that interacts with a virtual machine via HTTP requests.

#### New Class: VmClient
The PR adds a new class `VmClient`. This class has methods to perform various operations on the virtual machine, such as starting, stopping, rebooting, erasing, expiring, and notifying about the allocation of a virtual machine.

#### Implementation of methods for interacting with a virtual machine
The PR implements various methods for interacting with a virtual machine, such as starting, stopping, rebooting, erasing, expiring, and notifying about the allocation of a virtual machine.

The response ends with the categorization ('RED'), followed by the highlights and markdown formatting.

@github-actions github-actions bot added the RED This PR is complex and may require more time to review. label Jun 12, 2024
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@214db7c). Learn more about missing BASE report.
Report is 3 commits behind head on main.

Current head c6c8cab differs from pull request most recent head 2b3b63f

Please upload reports for the commit 2b3b63f to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #124   +/-   ##
=======================================
  Coverage        ?   84.15%           
=======================================
  Files           ?       27           
  Lines           ?     1130           
  Branches        ?      188           
=======================================
  Hits            ?      951           
  Misses          ?      176           
  Partials        ?        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@hoh hoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments.

My main remarks are on changing the pattern from callbacks to async generator, and to add typing annotation.

pyproject.toml Outdated Show resolved Hide resolved
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
@1yam 1yam marked this pull request as ready for review June 19, 2024 10:37
@1yam 1yam requested a review from hoh June 19, 2024 10:37
Copy link

Failed to retrieve llama text: POST 504:

504 Gateway Time-out


The server didn't respond in time.

src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
@hoh
Copy link
Member

hoh commented Jun 19, 2024

Can you add tests on the new code ?

Fix

Fix: using real path server instead of fake server for test

Fix: create playload
@Psycojoker
Copy link
Collaborator

Isn't this whole PR missing any form of documentation '-'?

hoh and others added 8 commits June 27, 2024 12:28
When "using bundled libsecp256k1", the setup using `/tmp/venv/bin/hatch run testing:test` fails to proceed on Python 3.12.

That library `secp256k1` has been unmaintained for more than 2 years now (0.14.0, Nov 6, 2021), and seems to not support Python 3.12.

The error in the logs:
```
File "/tmp/pip-build-env-ye8d6ort/overlay/lib/python3.12/site-packages/setuptools/_distutils/dist.py", line 862, in get_command_obj
 cmd_obj = self.command_obj[command] = klass(self)
                                                ^^^^^^^^^^^
      TypeError: 'NoneType' object is not callable
      [end of output]
```

See failing CI run:
https://github.com/aleph-im/aleph-sdk-python/actions/runs/9613634583/job/26516767722
@olethanh olethanh changed the title Feature: Allow User to control there VM Feature: Allow User to control their VM Jul 3, 2024
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
1yam and others added 2 commits July 4, 2024 11:00
@olethanh olethanh self-requested a review July 4, 2024 09:54
Copy link
Contributor

@olethanh olethanh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait before merging. I have encountered a few bug while testing them again the CRN.
I will provide fix for them

tests/unit/test_vmclient.py Outdated Show resolved Hide resolved
src/aleph/sdk/client/vmclient.py Outdated Show resolved Hide resolved
* Problem: A user cannot initialize an already created confidential VM.

Solution: Implement `VmConfidentialClient` class to be able to initialize and interact with confidential VMs.

* Problem: Auth was not working

Corrections:
 *  Measurement type returned was missing field needed for validation of measurements
 * Port number was not handled correctly in authentifaction
 * Adapt to new auth protocol where domain is moved to the operation field (While keeping compat with the old format)
 * Get measurement was not working since signed with the wrong method
 * inject_secret was not sending a json
 * Websocked auth was sending a twice serialized json
 * update 'vendorized' aleph-vm auth file from source

Co-authored-by: Olivier Le Thanh Duong <olivier@lethanh.be>
@olethanh olethanh self-requested a review July 4, 2024 20:33
Copy link
Contributor

@olethanh olethanh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done the modification in the andres-feature-implement_confidential_vm_initialization branch and merge dinto this one.

For me we can merge this PR. there are still some possible amelioration on the test and doc but these can be done in a sparate PR

@Psycojoker
Copy link
Collaborator

there are still some possible amelioration on the test and doc but these can be done in a separate PR

Have you open a clickup ticket for that?

@olethanh olethanh merged commit e1704ef into main Jul 5, 2024
14 of 15 checks passed
@olethanh
Copy link
Contributor

olethanh commented Jul 5, 2024

@Psycojoker https://app.clickup.com/24395970/v/l/6-901502681758-1

@Psycojoker Psycojoker deleted the 1yam-vm-client branch July 24, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RED This PR is complex and may require more time to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants