-
Notifications
You must be signed in to change notification settings - Fork 1.5k
OCPBUGS-74625: gcp: skip AI zones #10269
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
OCPBUGS-74625: gcp: skip AI zones #10269
Conversation
|
/retitle OCPBUGS-74625: gcp: skip AI zones |
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-74625, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
91eb6fe to
7129a45
Compare
|
With local debug output enabled (not included in the PR), AI zones are skipped: Installing to Installing to |
wking
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
/approve |
| logrus.Warnf("Found zone %s, but it does not follow expected zone naming convention", zone) | ||
| return false | ||
| } | ||
| return strings.Contains(parts[2], "ai") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, the current GCP docs say:
Make a GET request to the zones.list method. To match zones containing ai in their name, use the filter query parameter with the regular expression
name eq '.*-ai.*'.
That's not quite what you're doing here. I'm fine with how you have it, but aligning with the regexp GCP officially suggests might be more future-proof. It's also a bit mind-boggling that GCP's official recommendation is to use a zone-name regexp and not some more structured property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From GCP docs on regions and zones:
Standard zone
A standard zone name contains two parts: the region and the zone in the region. For example, the fully qualified name for zone a in region us-central1 is us-central1-a.
AI zone
AI zones follow an extended naming convention to differentiate them from non-AI zones. For an AI zone, the variable consists of three parts: the string ai (to identify it as an AI zone), a number (indicating its deployment group), and a letter (indicating the shared software update schedule). For example, the fully qualified name for AI zone ai2b in region us-west4 is us-west4-ai2b. This AI zone shares its deployment rollout wave with the standard us-west4-b zone.
I think the convention is <region>-<zone>. For standard zone, the <zone> part is just the zone ID (e.g. a, b). For AI zone, the <zone> part is ai<number><letter> (e.g. ai2b).
I guess the current approach looks good, right? GCP doc is just "confusing" :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the <region part seems to follow: <geography>-<area><number> too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking I missed that part in the docs (it is hidden in a tab).
Based on that, I have updated the PR to use strings.Contains to search for the substring -ai which is the equivalent of this regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the current approach looks good, right? GCP doc is just "confusing" :D
The approach would work... the updated substring match based on the regex is more "official" and a little simpler so I'm good with that!
Good find on those docs, I didn't see that section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, sounds good to me!
Filter out AI zones when discovering zones in the region. AI zones do not have quota for general compute resources, so we should not provision nodes there by default.
7129a45 to
fee6f94
Compare
wking
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified later |
|
@patrickdillon: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/verified later @jianli-wei @patrickdillon |
|
@patrickdillon: This PR has been marked to be verified later by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-gcp-ovn |
|
/verified by jiwei Mark as verified, according to my yesterday's testing (see comment-1 and comment-2), and the updates to the function |
|
@jianli-wei: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5314736
into
openshift:main
|
@patrickdillon: Jira Issue Verification Checks: Jira Issue OCPBUGS-74625 Jira Issue OCPBUGS-74625 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
GCP introduced AI zones, which should be skipped when provisioning general compute instances.