Skip to content

satisfy volume cloning topology requirements when choosing zone for CreateVolume #1150

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 17, 2023
Merged

satisfy volume cloning topology requirements when choosing zone for CreateVolume #1150

merged 1 commit into from
Mar 17, 2023

Conversation

amacaskill
Copy link
Member

@amacaskill amacaskill commented Feb 17, 2023 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:
Flakey unit tests discovered an issue in the way volume cloning chooses the zone to clone the volume into. The test fails when the random logic picks a source zone that does not satisfy the volume cloning limitations.

Volume cloning should satisfy the following topology requirements:

  1. For zonal->zonal cloning, the zone of the clone must match the zone of the source disk
  2. For zonal->regional cloning, one of the clone's replicated zones must match the source disk zone.
    • The zonal to regional disk clone is broken for pantheon gcloud, so I am not sure what will happen (if the clone regional disk replica zones gets overrideen or not in the event that neither replicazone matches the source disk zone, so we will just follow the documentation and enforce this.
  3. For regional->regional cloning, the source disk replicated zones must match the destination replicated zones. If the replicated zones do not match, gce will override the clone replicated zones to match the source replicated zones, so nothing is needed here to validate before Creating the disk.

Which issue(s) this PR fixes:
Fixes #1137

Does this PR introduce a user-facing change?:

It is no longer necessary to specify allowedTopologies for zonal -> zonal cloning and zonal/regional -> regional cloning to ensure that the clone zone is compatible with the GCE volume cloning requirements.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 17, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 17, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2023
@amacaskill amacaskill marked this pull request as draft February 17, 2023 17:17
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 17, 2023
@amacaskill amacaskill force-pushed the volume-cloning-fix branch 4 times, most recently from 6c92046 to 831b978 Compare February 17, 2023 21:32
@amacaskill amacaskill changed the title [WIP] satisfy volume cloning topology requirements when choosing zone for CreateVolume satisfy volume cloning topology requirements when choosing zone for CreateVolume Feb 17, 2023
@amacaskill amacaskill marked this pull request as ready for review February 17, 2023 21:33
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2023
@k8s-ci-robot k8s-ci-robot requested a review from leiyiz February 17, 2023 21:33
@amacaskill amacaskill force-pushed the volume-cloning-fix branch 2 times, most recently from 3472a21 to 2fcd98e Compare February 18, 2023 02:02
@amacaskill
Copy link
Member Author

/assign @saikat-royc

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 18, 2023
@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 8, 2023
msau42
msau42 reviewed Mar 10, 2023
View reviewed changes
Copy link
Contributor

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

Great work, these are really comprehensive test cases!

pkg/gce-pd-csi-driver/controller_test.go
},
{
name: "success regional disk clone of regional source disk",
name: "success zonal -> zonal cloning, req = allowedTopologies, pref = req w/ randomly selected zone as first element: immediate binding w/ allowedTopologies",
Copy link
Contributor

Choose a reason for hiding this comment

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

Note on test case naming, the preferred order can be different or the same for both delayed and immediate binding. By the time we get here, I don't think there's a way to distinguish between immediate + delayed binding.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was mentioning immediate binding / delayed binding so that I could be sure I covered all of the cases in the external provisioner topology support table: https://github.com/kubernetes-csi/external-provisioner#topology-support, but if that doesn't make sense. I can remove the last sections of these test case names. So for this one. remove , immediate binding w/ allowedTopologies

@amacaskill amacaskill force-pushed the volume-cloning-fix branch 2 times, most recently from 138bf50 to a4ee611 Compare March 13, 2023 17:08
@amacaskill
Copy link
Member Author

/retest

@amacaskill amacaskill requested a review from msau42 March 13, 2023 18:02
@amacaskill
Copy link
Member Author

/retest

@amacaskill amacaskill force-pushed the volume-cloning-fix branch 2 times, most recently from 0d0be04 to 5f1922e Compare March 13, 2023 21:43
@amacaskill
Copy link
Member Author

/retest

@amacaskill amacaskill force-pushed the volume-cloning-fix branch 2 times, most recently from f022295 to 7aec082 Compare March 13, 2023 22:55
@msau42 msau42 mentioned this pull request Mar 14, 2023
Merged
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 16, 2023
@msau42
Copy link
Contributor

msau42 commented Mar 17, 2023

/lgtm
/approve

Did I already mention how awesome this is? :-)

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amacaskill, msau42

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 91a66ec into kubernetes-sigs:master Mar 17, 2023
@amacaskill amacaskill mentioned this pull request Apr 11, 2023
Merged
@sunnylovestiramisu sunnylovestiramisu mentioned this pull request May 26, 2023
Merged
k8s-ci-robot added a commit that referenced this pull request May 26, 2023
@k8s-ci-robot
Merge pull request #1240 from sunnylovestiramisu/automated-cherry-pic…
ddcd135 
…k-of-#1232-#1227-upstream-release-1.9

Automated cherry pick of #1150 #1232 #1227 on release-1.9.
@sunnylovestiramisu sunnylovestiramisu mentioned this pull request May 30, 2023
Merged
k8s-ci-robot added a commit that referenced this pull request Jun 1, 2023
@k8s-ci-robot
Merge pull request #1244 from sunnylovestiramisu/automated-cherry-pic…
256c7c3 
…k-of-#1150-#1232-#1227-upstream-release-1.8

Automated cherry pick of #1150: fix bug where volume cloning topology requirements are
#1232: Use errors.As so we can detect wrapped errors, and check for
#1227: Adding new metric pdcsi_operation_errors to fetch error
@sunnylovestiramisu sunnylovestiramisu mentioned this pull request Jul 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcary mattcary Awaiting requested review from mattcary

@saikat-royc saikat-royc Awaiting requested review from saikat-royc

@leiyiz leiyiz Awaiting requested review from leiyiz

@msau42 msau42 Awaiting requested review from msau42

Assignees

@msau42 msau42

@saikat-royc saikat-royc

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flaky unit test
4 participants
@amacaskill @msau42 @k8s-ci-robot @saikat-royc