OCPBUGS-122133: gcp: skip disk type check when the permission is denied - #10862
Conversation
Add 500-type errors, such as 503 Service Unavailable to the graceful handling of the disk type validation. The disk type validation is supposed to prevent known failures, but if for some reason the API call fails we do not want that in and of itself to be fatal. That was the original design of the validation, this commit just adds 503 to that handling. (cherry picked from commit 9fb3564)
The disk type availability check queries compute.diskTypes.get once per zone. Service accounts holding only the documented minimum install permissions do not have that permission, so the lookup returns 403 and the install fails before any resource is created: platform.gcp.defaultMachinePlatform.diskType: Invalid value: "pd-ssd": googleapi: Error 403: Required 'compute.diskTypes.get' permission The disk type is valid; only the check is unavailable. Reading a disk type is not needed to install, so treat a denial the same way as a backend error and skip the check rather than report a bad value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@barbacbd: This pull request references Jira Issue OCPBUGS-122133, which is invalid:
Comment 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughDisk type availability validation now reports only fatal client errors. Permission failures, server errors, and non-API failures produce warnings and allow validation to continue. Tests cover error classification and warning output. ChangesGCP disk type validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to GCP disk-type validation now allows installation to continue when availability cannot be verified because of permission, server, or lookup failures, while retaining validation for actionable client errors. The change is covered by targeted error-classification tests and is ready to merge. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/cc @patrickdillon |
|
/jira refresh |
|
@barbacbd: This pull request references Jira Issue OCPBUGS-122133, which is valid. The bug has been moved to the POST state. 7 validation(s) were run on this bug
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. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@barbacbd: This pull request references Jira Issue OCPBUGS-122133, which is valid. 7 validation(s) were run on this bug
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. |
|
Scheduling tests matching the |
|
/approve |
|
/approve On behalf of Bruno, since owner file is not updated in 4.22 :D |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rochacbruno, tthvo 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 |
|
/test e2e-gcd-ovn-private-techpreview |
|
@barbacbd: 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. |
|
/verified by unit |
|
@barbacbd: 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. |
|
/label backport-risk-assessed |
cdaf698
into
openshift:release-4.22
|
@barbacbd: Jira Issue Verification Checks: Jira Issue OCPBUGS-122133 Jira Issue OCPBUGS-122133 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. |
The disk type availability check queries compute.diskTypes.get once per
zone. Service accounts holding only the documented minimum install
permissions do not have that permission, so the lookup returns 403 and
the install fails before any resource is created:
The disk type is valid; only the check is unavailable. Reading a disk
type is not needed to install, so treat a denial the same way as a
backend error and skip the check rather than report a bad value.
This PR also includes the backport for 9fb3564.
Summary by CodeRabbit