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

[Compute] Fix #23341: az vm list-skus: Fix filtering out VM sizes that are available regionally when they are restricted in all zones #23457

Merged
merged 8 commits into from
Aug 30, 2022

Conversation

yanzhudd
Copy link
Contributor

@yanzhudd yanzhudd commented Aug 8, 2022

Description

In az vm list-skus command, fix filtering out VM sizes that are available regionally when they are restricted in all zones
Close #23341

Testing Guide

History Notes

[Compute] Fix #23341: az vm list-skus: Fix filtering out VM sizes that are available regionally when they are restricted in all zones


This checklist is used to make sure that common guidelines for a pull request are followed.

@ghost ghost requested a review from yonzhan August 8, 2022 03:15
@ghost ghost added the Auto-Assign Auto assign by bot label Aug 8, 2022
@ghost ghost assigned zhoxing-ms Aug 8, 2022
@ghost ghost added this to the Aug 2022 (2022-09-06) milestone Aug 8, 2022
@ghost ghost added the Compute az vm/vmss/image/disk/snapshot label Aug 8, 2022
@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Aug 8, 2022

Please add test case and rerun some tests like #18939

if restriction['type'] == 'Location' and (
result['locationInfo'][0]['location'] in (restriction['restrictionInfo']['locations'] or [])):
is_restrict_location = True
if is_restrict_zone and is_restrict_location:

Choose a reason for hiding this comment

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

I think it is possible for a VM sku to be restricted in region but not to have zonal restrictions listed. So I think the SKU should be considered restricted if it has a "Location" restriction OR all zones restricted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is possible for a VM sku to be restricted in region but not to have zonal restrictions listed. So I think the SKU should be considered restricted if it has a "Location" restriction OR all zones restricted.

Thanks. We modified the code again according to your comment. Could you please help to confirm it again?

Choose a reason for hiding this comment

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

I thought about this a bit more and think we may want to bring in Azure Compute team into conversation here to double-check.

If we are filtering out VM sizes from the list based on the restriction types, we may need to make the filtering logic also dependent on the --zone parameter which for this CLI command is described as "--zone: Show skus supporting availability zones. Allowed values: false, true"

I put together the following table to see if it makes sense to you and to Azure Compute:

Restriction Types VM SKU Availability is_available?
Location=null, Zone=null Yes, regional or any zone TRUE
Location=null, Zone=1,2 Yes, regional or zone 3 TRUE
Location=null, Zone=1,2,3 Yes, but only regional if (--zone parameter) then FALSE else TRUE
Location=eastus2, Zone=null No, since region is restricted all zones are restricted FALSE
Location=eastus2, Zone=1,2 No, since region is restricted all zones are restricted FALSE
Location=eastus2, Zone=1,2,3 No, since region is restricted all zones are restricted FALSE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this a bit more and think we may want to bring in Azure Compute team into conversation here to double-check.

If we are filtering out VM sizes from the list based on the restriction types, we may need to make the filtering logic also dependent on the --zone parameter which for this CLI command is described as "--zone: Show skus supporting availability zones. Allowed values: false, true"

I put together the following table to see if it makes sense to you and to Azure Compute:

Restriction Types VM SKU Availability is_available?
Location=null, Zone=null Yes, regional or any zone TRUE
Location=null, Zone=1,2 Yes, regional or zone 3 TRUE
Location=null, Zone=1,2,3 Yes, but only regional if (--zone parameter) then FALSE else TRUE
Location=eastus2, Zone=null No, since region is restricted all zones are restricted FALSE
Location=eastus2, Zone=1,2 No, since region is restricted all zones are restricted FALSE
Location=eastus2, Zone=1,2,3 No, since region is restricted all zones are restricted FALSE

Hello @grizzlytheodore, could you please help to confirm this logic about filtering out VM sizes from the list based on the restriction types? Thanks.

Hello @chasewilson, if the relationship mentioned above is confirmed, we would modify the logic about filtering out VM sizes from the list based on the restriction types.

Copy link
Contributor Author

@yanzhudd yanzhudd Aug 16, 2022

Choose a reason for hiding this comment

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

I thought about this a bit more and think we may want to bring in Azure Compute team into conversation here to double-check.

If we are filtering out VM sizes from the list based on the restriction types, we may need to make the filtering logic also dependent on the --zone parameter which for this CLI command is described as "--zone: Show skus supporting availability zones. Allowed values: false, true"

I put together the following table to see if it makes sense to you and to Azure Compute:

Restriction Types VM SKU Availability is_available?
Location=null, Zone=null Yes, regional or any zone TRUE
Location=null, Zone=1,2 Yes, regional or zone 3 TRUE
Location=null, Zone=1,2,3 Yes, but only regional if (--zone parameter) then FALSE else TRUE
Location=eastus2, Zone=null No, since region is restricted all zones are restricted FALSE
Location=eastus2, Zone=1,2 No, since region is restricted all zones are restricted FALSE
Location=eastus2, Zone=1,2,3 No, since region is restricted all zones are restricted FALSE

@travismaier, could you please help to confirm this logic about filtering out VM sizes from the list based on the restriction types? Thanks.

@yonzhan
Copy link
Collaborator

yonzhan commented Aug 9, 2022

Compute

Comment on lines 137 to 139
# This SKU is not available only if zonal restriction and all zones are restricted
# and skus supporting availability zones are required to show
# or regional restriction and the region is restricted
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please refine this comment to make it more readable?

@zhoxing-ms zhoxing-ms merged commit aa48f64 into Azure:dev Aug 30, 2022
@yanzhudd yanzhudd deleted the vm_list_skus_bug branch August 31, 2022 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot Compute az vm/vmss/image/disk/snapshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

az vm list-skus filtering out VM sizes that are available regionally when they are restricted in all zones
5 participants