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

Implement default hypervisor options on settings #573

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

nesitor
Copy link
Member

@nesitor nesitor commented Mar 13, 2024

Problem: The unique way to set the default hypervisor to use is hard-coded on the models.py class, so we can't select another one by default.

Solution: Implement INSTANCE_DEFAULT_HYPERVISOR field on the orchestrator configuration variables.

…oded on the `models.py` class, so we can't select another one by default.

Solution: Implement `INSTANCE_DEFAULT_HYPERVISOR` field on the orchestrator configuration variables.
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.

One comment, the rest looks fine.

@@ -356,6 +361,9 @@ def check(self):
assert is_command_available(
"qemu-system-x86_64"
), "Command `qemu-system-x86_64` not found, run `apt install qemu-system-x86`"
else:
# If QEmu is not supported, ignore the setting and use Firecracker by default
settings.INSTANCE_DEFAULT_HYPERVISOR = HypervisorType.firecracker
Copy link
Member

Choose a reason for hiding this comment

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

Since this changes the settings, it should go in setup() instead of check()

@github-actions github-actions bot added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Mar 13, 2024
Copy link

The modifications include updates to the configuration settings of the VM execution engine in conf.py. This could potentially impact other parts of the system that rely on these configurations, such as the Aleph.im platform. The changes also involve updating the instance hypervisor setting in models.py.

The PR includes a conditional check for QEmu support in check() function which is used to validate if the required command exists or not. If it doesn't, an error message is raised indicating that the command isn't available. This could potentially cause issues on systems where QEmu isn't installed.

In conclusion, this PR requires a high level of expertise and attention as it involves changes to multiple files in different parts of the system. It also includes potential risks for introducing bugs or breaking existing functionality. Only experienced developers should review this PR.

The response ends with the category ('BLACK') followed by the explaining rationale, using markdown format and bullet points.


Note: The user may provide specific rules to fine-tune the categorization process in their message. If no rules are provided, CRC will use its default set of rules for categorizing PRs based on indicators such as file changes, complexity, potential impact, etc.

The system is designed to be used with other systems and tools that can parse the responses from CRC. It's important to note that while the system provides a clear explanation for the categorization, it doesn't provide direct code review or suggestions for improvements. The user should still perform thorough manual reviews of the PRs based on this information.

@nesitor nesitor requested a review from hoh March 13, 2024 14:24
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 35.19%. Comparing base (3c56ddc) to head (0164b6a).
Report is 1 commits behind head on main.

Files Patch % Lines
src/aleph/vm/models.py 0.00% 3 Missing ⚠️
src/aleph/vm/conf.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #573      +/-   ##
==========================================
- Coverage   35.19%   35.19%   -0.01%     
==========================================
  Files          53       53              
  Lines        4844     4850       +6     
  Branches      572      574       +2     
==========================================
+ Hits         1705     1707       +2     
- Misses       3120     3124       +4     
  Partials       19       19              

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

@hoh hoh merged commit 1c0761d into main Mar 13, 2024
18 of 20 checks passed
@Psycojoker Psycojoker deleted the andres-feature-implement_hypervisor_general_setting branch July 24, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLACK This PR has critical implications and must be reviewed by a senior engineer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants