-
Notifications
You must be signed in to change notification settings - Fork 39.6k
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
Fixes the regression of GCEPD not provisioning correctly on GKE alpha clusters. #59447
Conversation
/release-note-none |
@@ -398,9 +325,12 @@ func (manager *gceServiceManager) getDiskSourceURI(disk *GCEDisk) (string, error | |||
|
|||
func (manager *gceServiceManager) getDiskTypeURI( | |||
diskRegion string, diskZoneInfo zoneType, diskType string) (string, error) { | |||
getProjectsAPIEndpoint := manager.getProjectsAPIEndpoint() | |||
|
|||
var getProjectsAPIEndpoint string | |||
if manager.gce.AlphaFeatureGate.Enabled(AlphaFeatureGCEDisk) { |
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.
Since this method is used by both CreateDiskOnCloudProvider(...)
and CreateRegionalDiskOnCloudProvider(...)
, we should probably not key off AlphaFeatureGate.Enabled(AlphaFeatureGCEDisk)
alone, right? That would force standard disks that are running on alpha cluster to use the alpha api?
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.
Agreed, better to pass that info in from the CreateDisk...
methods.
Addressed comments, and added basic attach unit test. |
Test issue: #59518 |
/lgtm Could you manually test and verify that RePD is working (create, attach, mount, etc.). And also verify that in a project that is not whitelisted for GCE alpha that standard PD works (create, attach, mount), but RePD fails? |
/retest Review the full test history for this PR. Silence the bot with an |
Legitimate verify-bazel failure:
|
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
/lgtm cancel |
@saad-ali I've done the following tests manually for multizone clusters, with DiskAlphaAPI both set and unset, and single-zone cluster with alpha flag unset:
|
Awesome, one more thing to run the same set of tests against an alpha cluster on a project that is not whitelisted for the GCE API (to verify the regression is fixed). /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saad-ali, verult The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these OWNERS Files:
Approvers can indicate their approval by writing |
Once merged, this should be cherry picked to 1.9 and 1.8 branches. |
/kind bug |
[MILESTONENOTIFIER] Milestone Pull Request Needs Approval @cblecker @saad-ali @verult @kubernetes/sig-storage-misc Action required: This pull request must have the Pull Request Labels
|
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
…-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #59447: Fixes the regression of GCEPD not provisioning correctly on Cherry pick of #59447 on release-1.9. #59447: Fixes the regression of GCEPD not provisioning correctly on ```release-note Bug fix: Clusters with GCE feature 'DiskAlphaAPI' enabled were unable to dynamically provision GCE PD volumes. ```
…-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #59447: Fixes the regression of GCEPD not provisioning correctly on Cherry pick of #59447 on release-1.8. #59447: Fixes the regression of GCEPD not provisioning correctly on ```release-note Bug fix: Clusters with GCE feature 'DiskAlphaAPI' enabled were unable to dynamically provision GCE PD volumes. ```
Fixes the regression by better distinguishing between single-zone and multi-zone PDs.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #59047
Special notes for your reviewer: All PD and dynamic provisioning e2e tests pass, manually verified provision, delete, attach, and detach of both single-zone and multi-zone PDs. Will create e2e tests for multizone PDs in a separate PR.
Release note:
/sig storage
/assign @saad-ali