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

fix: temp exclude SKUs without compatible VM image #617

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tallaxes
Copy link
Collaborator

@tallaxes tallaxes commented Dec 13, 2024

Fixes #616

Description

Exclude from consideration VM SKUs that don't have a compatible (= tagged for NVMe) VM image in community image galleries - for now, until the VM images are properly tagged. The implementation only allows VM SKUs that support SCSI (defined as either declaring SCSI support in their DiskControllerType, or not declaring anything at all).

Possible alternative implementations (which we may consider in the future):

  • Internally assign corresponding capabilities (e.g. karpenter.azure.com/controller-disk-type) to both instance types (derived from VM SKUs) and images (either statically or derived from image definition) and rely on requirements compatibility, and dynamically take VM SKUs without compatible image support out of consideration. This would have a side benefit of being able to select controller-disk-type.
  • Detect persistent / non-retriable failure to provision certain VM SKUs and take them out of consideration.
  • Use explicit VM SKU allowlist

How was this change tested?

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note

Exclude from consideration VM SKUs that don't have a compatible (= tagged for NVMe) VM image

@tallaxes tallaxes added area/vm-images Issues or PRs related to VM images or image galleries area/provisioning Issues or PRs related to provisioning (instance provider) labels Dec 13, 2024
@tallaxes tallaxes self-assigned this Dec 13, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 12307396129

Details

  • 240 of 240 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 94.257%

Totals Coverage Status
Change from base Build 12167425051: 0.03%
Covered Lines: 37354
Relevant Lines: 39630

💛 - Coveralls

@tallaxes tallaxes requested a review from Copilot December 13, 2024 00:53

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • hack/codegen.sh: Language not supported
Comments suppressed due to low confidence (2)

pkg/providers/instancetype/instancetypes.go:278

  • [nitpick] The variable name 'useSIG' is ambiguous. It should be renamed to 'useSharedImageGallery' for better clarity.
useSIG := false // replace with options.FromContext(ctx).UseSIG when available

pkg/providers/instancetype/instancetypes.go:413

  • The function 'isCompatibleImageAvailable' should have test coverage to ensure its correctness.
func isCompatibleImageAvailable(sku *skewer.SKU, useSIG bool) bool {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/provisioning Issues or PRs related to provisioning (instance provider) area/vm-images Issues or PRs related to VM images or image galleries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SKU-image/disk incompatibility
2 participants