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

Fixes the regression of GCEPD not provisioning correctly on GKE alpha clusters. #59447

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

verult
Copy link
Contributor

@verult verult commented Feb 7, 2018

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:

Bug fix: Clusters with GCE feature 'DiskAlphaAPI' enabled were unable to dynamically provision GCE PD volumes.

/sig storage
/assign @saad-ali

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 7, 2018
@verult
Copy link
Contributor Author

verult commented Feb 7, 2018

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 7, 2018
@@ -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) {
Copy link
Member

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?

Copy link
Contributor Author

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.

@verult
Copy link
Contributor Author

verult commented Feb 7, 2018

Addressed comments, and added basic attach unit test.

@verult
Copy link
Contributor Author

verult commented Feb 8, 2018

Test issue: #59518

@saad-ali
Copy link
Member

saad-ali commented Feb 8, 2018

/lgtm
/approve

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?

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 8, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@cblecker
Copy link
Member

cblecker commented Feb 8, 2018

Legitimate verify-bazel failure:

I0208 05:12:20.879] Verifying verify-bazel.sh
I0208 05:12:20.894] 
I0208 05:12:20.896] +++ Running case: verify.bazel 
I0208 05:12:20.897] +++ working dir: /go/src/k8s.io/kubernetes
I0208 05:12:20.899] +++ command: bash "hack/make-rules/../../hack/verify-bazel.sh"
I0208 05:12:39.293] diff -Naupr /go/src/k8s.io/kubernetes/pkg/volume/gce_pd/BUILD /tmp/verify-bazel.fFjFv4/go/src/k8s.io/kubernetes/pkg/volume/gce_pd/BUILD
I0208 05:12:39.293] --- /go/src/k8s.io/kubernetes/pkg/volume/gce_pd/BUILD	2018-02-08 05:08:43.249678444 +0000
I0208 05:12:39.294] +++ /tmp/verify-bazel.fFjFv4/go/src/k8s.io/kubernetes/pkg/volume/gce_pd/BUILD	2018-02-08 05:12:33.130579654 +0000
I0208 05:12:39.294] @@ -43,6 +43,7 @@ go_test(
I0208 05:12:39.295]      embed = [":go_default_library"],
I0208 05:12:39.295]      importpath = "k8s.io/kubernetes/pkg/volume/gce_pd",
I0208 05:12:39.296]      deps = [
I0208 05:12:39.296] +        "//pkg/kubelet/apis:go_default_library",
I0208 05:12:39.297]          "//pkg/util/mount:go_default_library",
I0208 05:12:39.297]          "//pkg/volume:go_default_library",
I0208 05:12:39.297]          "//pkg/volume/testing:go_default_library",
I0208 05:12:39.298] 
I0208 05:12:39.298] Run ./hack/update-bazel.sh
I0208 05:12:40.146] +++ exit code: 1
I0208 05:12:40.150] +++ error: 1
I0208 05:12:40.170] FAILED   verify-bazel.sh	20s

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@cblecker
Copy link
Member

cblecker commented Feb 8, 2018

/lgtm cancel
Removing LGTM to stop retesting loop

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2018
@verult
Copy link
Contributor Author

verult commented Feb 8, 2018

@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:

  • (Create) Dynamic provisioning with RePD storage class and Delete reclaim policy.
  • (Attach & mount) Running a pod referencing the PVC created above.
  • (Detach & unmount) Deleting the previous pod
  • (Delete) Deleting the PVC created above.
    And all of the above are done using the default gce-pd storage class as well.

@saad-ali
Copy link
Member

saad-ali commented Feb 9, 2018

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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2018
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@saad-ali saad-ali added this to the v1.9 milestone Feb 9, 2018
@saad-ali
Copy link
Member

saad-ali commented Feb 9, 2018

Once merged, this should be cherry picked to 1.9 and 1.8 branches.

@cblecker
Copy link
Member

cblecker commented Feb 9, 2018

/kind bug
/priority important-soon

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Feb 9, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@cblecker @saad-ali @verult @kubernetes/sig-storage-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 6 days, the pull request will be moved out of the v1.9 milestone.

Pull Request Labels
  • sig/storage: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit a35accc into kubernetes:master Feb 9, 2018
@k8s-cherrypick-bot
Copy link

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.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 1, 2018
k8s-github-robot pushed a commit that referenced this pull request Mar 14, 2018
…-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.
```
k8s-github-robot pushed a commit that referenced this pull request Mar 14, 2018
…-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.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/needs-approval priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic Volume Provisioning not working on GKE alpha clusters
7 participants