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

Make GPU instance not selectable to regular VM customers #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dakshgup
Copy link
Member

Make GPU instance not selectable to regular VM customers via APIs

The APIs allow all vm size family to be selectable. In the web, we don't
visualize them anyway, however, in the APIs, they were selectable. With
this commit, we perform size validation in the API and only allow
visible vm_sizes to be picked.

Disallow gpu instance in web route

Validate boot-image in web

Without this check, customers can edit the parameter via inspect element
and use postgres base image as a base image for a VM. We do the same for
APIs.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Pull Request Summary

Title: Make GPU instance not selectable to regular VM customers

  • Validation Updates: Added validation logic to ensure GPU instances are not selectable via APIs and web routes, aligning API behavior with the web interface.
  • Migration and Model Changes: Removed migration files and methods related to boot image paths and sizes, impacting how boot images are managed.
  • Configuration Adjustments: Updated billing rates and provider visibility settings to support the restriction of GPU instances.
  • Test Suite Modifications: Adjusted and removed tests to reflect the new validation logic and the removal of certain functionalities.

Potential Pitfalls:

  • The removal of boot image management methods and related tests could lead to undetected issues if not thoroughly reviewed.

Comments

Gemfile.lock

  • Line 114: Downgrading minitest from 5.23.0 to 5.22.3 could potentially introduce issues if there were bug fixes or features in the newer version that your tests rely on.
  • Line 181: Downgrading regexp_parser from 2.9.2 to 2.9.1 could affect any functionality that relies on regular expression parsing, especially if there were important fixes or improvements in the newer version.

bin/ci

  • Lines 12 - 18: The removal of the conditional check for options[:test_cases] means that both encrypted and unencrypted VM tests will always run. Ensure this is the intended behavior.
  • Lines 51 - 54: The --test-cases option has been removed from the command line arguments. If this was used in any CI configurations or scripts, they will need to be updated accordingly.

model/boot_image.rb

  • Line 14: The remove_boot_image method has been removed. Ensure that no other parts of the codebase rely on this method for boot image removal.
  • Line 14: The path method has been removed. Verify that no other parts of the codebase depend on this method to access the boot image path.

prog/download_boot_image.rb

  • Lines 21 - 23: The comment here is useful for future reference but ensure that the logic for determining URLs is robust enough to handle all cases.

prog/remove_boot_image.rb

  • Line 1: The deletion of prog/remove_boot_image.rb removes the functionality for removing boot images. Ensure that this functionality is no longer required or has been replaced elsewhere in the codebase.

prog/vm/nexus.rb

  • Line 17: The exclude_host_ids parameter has been removed from the assemble method. Ensure that any dependent code or documentation is updated accordingly.
  • Line 102: The exclude_host_ids parameter has been removed from the stack creation. Ensure that this change is reflected in any related logic or documentation.
  • Line 191: The host_exclusion_filter parameter has been removed from the allocation logic. Ensure that this change is reflected in any related logic or documentation.

rhizome/host/lib/boot_image.rb

  • Line 31: This error message could be more descriptive to help with debugging. Consider including the name of the image and the context of the failure.

routes/api/project/location/vm.rb

  • Lines 45 - 49: Good addition of size validation to prevent GPU instances from being selectable via the API.

scheduling/allocator.rb

  • Line 8: The host_exclusion_filter parameter has been removed from the allocate method. Ensure that this change does not unintentionally affect any existing functionality that relies on excluding specific hosts.
  • Line 21: The host_exclusion_filter parameter has been removed from the Request struct. Verify that no other parts of the codebase depend on this parameter.
  • Line 94: The logic for excluding hosts based on the host_exclusion_filter has been removed. Ensure that this does not lead to unintended host selections.
  • Line 256: The ordering of boot images has been adjusted to remove the nulls: :last option. Confirm that this change does not affect the intended boot image selection.

spec/model/boot_image_spec.rb

  • Line 1: Deleting this test file removes coverage for the #remove_boot_image method in the BootImage model. Ensure that this functionality is covered elsewhere or add new tests if necessary.

spec/prog/download_boot_image_spec.rb

  • Line 38: The nap(15) expectation should be wrapped in an expect block to properly test the behavior.
  • Line 69: The nap(15) expectation should be wrapped in an expect block to properly test the behavior.

spec/prog/remove_boot_image_spec.rb

  • Line 1: The deletion of this test file removes coverage for the Prog::RemoveBootImage class. Ensure that the functionality is either deprecated or covered by other tests to avoid potential issues.

spec/prog/vm/nexus_spec.rb

  • Line 352: Removed the host_exclusion_filter parameter from the allocation call, which simplifies the allocation logic.
  • Line 367: Removed the host_exclusion_filter parameter from the allocation call for GitHub runners.
  • Line 386: Removed the host_exclusion_filter parameter from the allocation call when forcing a specific host.
  • Line 433: Removed the test case for excluding hosts, as this functionality is no longer supported.
  • Line 451: Removed the test case for failing when the same host is forced and excluded, as host exclusion is no longer supported.

spec/routes/api/project/location/vm_spec.rb

  • Lines 189 - 198: Good addition of test case to validate VM size and ensure GPU instances are not selectable.
  • Lines 200 - 210: Great job adding a test case to verify successful VM creation without specifying a VM size.

spec/scheduling/allocator_spec.rb

  • Line 43: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 57: The allocation request parameters have been modified to exclude certain filters. Verify that these filters are not required for other allocation logic.
  • Line 98: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 126: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 155: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 172: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 190: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 206: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 222: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 238: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 269: The allocation request parameters have been modified to exclude certain filters. Verify that these filters are not required for other allocation logic.
  • Line 463: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.
  • Line 476: The allocation request parameters have been modified to exclude certain filters. Verify that these filters are not required for other allocation logic.
  • Line 624: The 'size_gib' attribute has been removed from BootImage creation. Ensure that this change is consistent with the rest of the codebase and that no other parts of the code rely on this attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant