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

feat: supporting additional handing for TotalRegionalCores quota error, and adding better customer facing error messages on nodeclaim #45

Merged
merged 16 commits into from
Dec 2, 2023

Conversation

Bryce-Soghigian
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian commented Nov 23, 2023

Fixes #

Description
This PR adds support for TotalRegionalCores errors and makes it more apparent for the customer that this is the root cause of the problem for why their scale up stopped.

This also modifies the code to use a less generic check for subscription level quota errors, and specifically only marking instances as unavailable if we see a SKUFamily quota error instead of just all subscription level quota errors.

We also are only logging the Azure Errors and are returning better customer facing errors in this case since they will be viewed in the nodeclaim.

How was this change tested?

  • Scale Up observing better errors.
  • make presubmit

Does this change impact docs?

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

Release Note


@Bryce-Soghigian
Copy link
Collaborator Author

@juan-lee Let me know if you think this leads to a better experience in more clearly communicating you hit TotalRegionCores errors!

Copy link
Collaborator Author

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

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

/test

Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

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

LGTM, mostly minor feedback in comments. Could we also have tests for these cases? Could follow example in 58586db

pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
@Bryce-Soghigian Bryce-Soghigian force-pushed the bsoghigian/total-regional-cores branch from 535a1b7 to 99d8a74 Compare November 28, 2023 06:27
@Bryce-Soghigian Bryce-Soghigian force-pushed the bsoghigian/total-regional-cores branch from f734a32 to 089d0a7 Compare December 1, 2023 10:01
Copy link
Collaborator Author

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

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

/test

@Bryce-Soghigian Bryce-Soghigian marked this pull request as ready for review December 1, 2023 10:01
pkg/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Show resolved Hide resolved
Copy link
Collaborator Author

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

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

/test

Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

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

/test

@tallaxes tallaxes merged commit 20da22d into main Dec 2, 2023
7 checks passed
@tallaxes tallaxes deleted the bsoghigian/total-regional-cores branch December 2, 2023 01:33
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.

2 participants