Skip to content

[Release-1.7] cherry pick of #1150 satisfy volume cloning topology requirements when choosing zone for CreateVolume #1301

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

Closed
wants to merge 2 commits into from
Closed

[Release-1.7] cherry pick of #1150 satisfy volume cloning topology requirements when choosing zone for CreateVolume #1301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb2f27d
fix bug where volume cloning topology requirements are ignored when c…
amacaskill Feb 17, 2023
3e2fe55
Update go.mod
sunnylovestiramisu Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ require (
k8s.io/apimachinery v0.22.0
k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible
k8s.io/component-base v0.22.0
k8s.io/klog/v2 v2.60.1
k8s.io/klog/v2 v2.80.1
k8s.io/kubernetes v1.22.0
k8s.io/mount-utils v0.22.0
k8s.io/test-infra v0.0.0-20200115230622-70a5174aa78d
k8s.io/utils v0.0.0-20210707171843-4b05e18ac7d9
k8s.io/utils v0.0.0-20230711102312-30195339c3c7
)

require google.golang.org/protobuf v1.26.0

require (
github.com/Microsoft/go-winio v0.4.16 // indirect
github.com/beorn7/perks v1.0.1 // indirect
Expand Down Expand Up @@ -65,7 +67,6 @@ require (
golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac // indirect
golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
google.golang.org/appengine v1.6.6 // indirect
google.golang.org/protobuf v1.26.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
Expand Down
8 changes: 4 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,6 @@ github.com/soheilhy/cmux v0.1.4/go.mod h1:IM3LyeVVIOuxMH7sFAkER9+bJ4dT7Ms6E4xg4k
github.com/soheilhy/cmux v0.1.5/go.mod h1:T7TcVDs9LWfQgPlPsdngu6I6QIoyIFZDDC6sNE1GqG0=
github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B0CQ=
github.com/spf13/afero v1.2.2 h1:5jhuqJyZCZf2JRofRvN/nIFgIWNzPa3/Vz8mYylgbWc=
github.com/spf13/afero v1.2.2/go.mod h1:9ZxEEn6pIJ8Rxe320qSDBk6AsU0r9pR7Q4OcevTdifk=
github.com/spf13/cast v1.3.0/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE=
github.com/spf13/cobra v0.0.3/go.mod h1:1l0Ry5zgKvJasoi3XT1TypsSe7PqH0Sj9dhYf7v3XqQ=
Expand Down Expand Up @@ -1218,8 +1217,8 @@ k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE=
k8s.io/klog/v2 v2.2.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=
k8s.io/klog/v2 v2.4.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=
k8s.io/klog/v2 v2.9.0/go.mod h1:hy9LJ/NvuK+iVyP4Ehqva4HxZG/oXyIS3n3Jmire4Ec=
k8s.io/klog/v2 v2.60.1 h1:VW25q3bZx9uE3vvdL6M8ezOX79vA2Aq1nEWLqNQclHc=
k8s.io/klog/v2 v2.60.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=
k8s.io/klog/v2 v2.80.1 h1:atnLQ121W371wYYFawwYx1aEY2eUfs4l3J72wtgAwV4=
k8s.io/klog/v2 v2.80.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=
k8s.io/kube-aggregator v0.22.0/go.mod h1:zHTepg0Q4tKzru7Pwg1QYHWrU/wrvIXM8hUdDAH66qg=
k8s.io/kube-controller-manager v0.22.0/go.mod h1:E/EYMoCj8bbPRmu19JF4B9QLyQL8Tywg+9Q/rg+F80U=
k8s.io/kube-openapi v0.0.0-20180731170545-e3762e86a74c/go.mod h1:BXM9ceUBTj2QnfH2MK1odQs778ajze1RxcmP6S8RVVc=
Expand All @@ -1245,8 +1244,9 @@ k8s.io/test-infra v0.0.0-20200115230622-70a5174aa78d/go.mod h1:d8SKryJBXAwfCFVL4
k8s.io/utils v0.0.0-20181019225348-5e321f9a457c/go.mod h1:8k8uAuAQ0rXslZKaEWd0c3oVhZz7sSzSiPnVZayjIX0=
k8s.io/utils v0.0.0-20190506122338-8fab8cb257d5/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
k8s.io/utils v0.0.0-20201110183641-67b214c5f920/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
k8s.io/utils v0.0.0-20210707171843-4b05e18ac7d9 h1:imL9YgXQ9p7xmPzHFm/vVd/cF78jad+n4wK1ABwYtMM=
k8s.io/utils v0.0.0-20210707171843-4b05e18ac7d9/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
k8s.io/utils v0.0.0-20230711102312-30195339c3c7 h1:ZgnF1KZsYxWIifwSNZFZgNtWE89WI5yiP5WwlfDoIyc=
k8s.io/utils v0.0.0-20230711102312-30195339c3c7/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
knative.dev/pkg v0.0.0-20191101194912-56c2594e4f11/go.mod h1:pgODObA1dTyhNoFxPZTTjNWfx6F0aKsKzn+vaT9XO/Q=
modernc.org/cc v1.0.0/go.mod h1:1Sk4//wdnYJiUIxnW8ddKpaOJCF37yAdqYnkxUpaYxw=
modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
Expand Down
132 changes: 126 additions & 6 deletions pkg/gce-pd-csi-driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"k8s.io/apimachinery/pkg/util/uuid"
"k8s.io/client-go/util/flowcontrol"
"k8s.io/klog/v2"
"k8s.io/utils/strings/slices"

"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute"
Expand Down Expand Up @@ -107,6 +108,14 @@ type workItem struct {
unpublishReq *csi.ControllerUnpublishVolumeRequest
}

// locationRequirements are additional location topology requirements that must be respected when creating a volume.
type locationRequirements struct {
srcVolRegion string
srcVolZone string
srcReplicationType string
cloneReplicationType string
}

var _ csi.ControllerServer = &GCEControllerServer{}

const (
Expand Down Expand Up @@ -151,6 +160,44 @@ func isDiskReady(disk *gce.CloudDisk) (bool, error) {
return false, nil
}

// cloningLocationRequirements returns additional location requirements to be applied to the given create volume requests topology.
// If the CreateVolumeRequest will use volume cloning, location requirements in compliance with the volume cloning limitations
// will be returned: https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/volume-cloning#limitations.
func cloningLocationRequirements(req *csi.CreateVolumeRequest, cloneReplicationType string) (*locationRequirements, error) {
if !useVolumeCloning(req) {
return nil, nil
}
// If we are using volume cloning, this will be set.
volSrc := req.VolumeContentSource.GetVolume()
volSrcVolID := volSrc.GetVolumeId()

_, sourceVolKey, err := common.VolumeIDToKey(volSrcVolID)
if err != nil {
return nil, fmt.Errorf("volume ID is invalid: %w", err)
}

isZonalSrcVol := sourceVolKey.Type() == meta.Zonal
if isZonalSrcVol {
region, err := common.GetRegionFromZones([]string{sourceVolKey.Zone})
if err != nil {
return nil, fmt.Errorf("failed to get region from zones: %w", err)
}
sourceVolKey.Region = region
}

srcReplicationType := replicationTypeNone
if !isZonalSrcVol {
srcReplicationType = replicationTypeRegionalPD
}

return &locationRequirements{srcVolZone: sourceVolKey.Zone, srcVolRegion: sourceVolKey.Region, srcReplicationType: srcReplicationType, cloneReplicationType: cloneReplicationType}, nil
}

// useVolumeCloning returns true if the create volume request should be created with volume cloning.
func useVolumeCloning(req *csi.CreateVolumeRequest) bool {
return req.VolumeContentSource != nil && req.VolumeContentSource.GetVolume() != nil
}

func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
var err error
// Validate arguments
Expand Down Expand Up @@ -186,12 +233,21 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
if multiWriter {
gceAPIVersion = gce.GCEAPIVersionBeta
}

var locationTopReq *locationRequirements
if useVolumeCloning(req) {
locationTopReq, err = cloningLocationRequirements(req, params.ReplicationType)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "failed to get location requirements: %v", err.Error())
}
}

// Determine the zone or zones+region of the disk
var zones []string
var volKey *meta.Key
switch params.ReplicationType {
case replicationTypeNone:
zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 1)
zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 1, locationTopReq)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to pick zones for disk: %v", err.Error())
}
Expand All @@ -201,7 +257,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
volKey = meta.ZonalKey(name, zones[0])

case replicationTypeRegionalPD:
zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 2)
zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 2, locationTopReq)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to pick zones for disk: %v", err.Error())
}
Expand Down Expand Up @@ -1382,7 +1438,29 @@ func diskIsAttachedAndCompatible(deviceName string, instance *compute.Instance,
return false, nil
}

func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int) ([]string, error) {
// pickZonesInRegion will remove any zones that are not in the given region.
func pickZonesInRegion(region string, zones []string) []string {
refinedZones := []string{}
for _, zone := range zones {
if strings.Contains(zone, region) {
refinedZones = append(refinedZones, zone)
}
}
return refinedZones
}

func prependZone(zone string, zones []string) []string {
newZones := []string{zone}
for i := 0; i < len(zones); i++ {
// Do not add a zone if it is equal to the zone that is already prepended to newZones.
if zones[i] != zone {
newZones = append(newZones, zones[i])
}
}
return newZones
}

func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int, locationTopReq *locationRequirements) ([]string, error) {
reqZones, err := getZonesFromTopology(top.GetRequisite())
if err != nil {
return nil, fmt.Errorf("could not get zones from requisite topology: %w", err)
Expand All @@ -1392,6 +1470,39 @@ func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int) ([]string
return nil, fmt.Errorf("could not get zones from preferred topology: %w", err)
}

if locationTopReq != nil {
srcVolZone := locationTopReq.srcVolZone
switch locationTopReq.cloneReplicationType {
// For zonal -> zonal cloning, the source disk zone must match the destination disk zone.
case replicationTypeNone:
// If the source volume zone is not in the topology requirement, we return an error.
if !slices.Contains(prefZones, srcVolZone) && !slices.Contains(reqZones, srcVolZone) {
volumeCloningReq := fmt.Sprintf("clone zone must match source disk zone: %s", srcVolZone)
return nil, fmt.Errorf("failed to find zone from topology %v: %s", top, volumeCloningReq)
}
return []string{srcVolZone}, nil
// For zonal or regional -> regional disk cloning, the source disk region must match the destination disk region.
case replicationTypeRegionalPD:
srcVolRegion := locationTopReq.srcVolRegion
prefZones = pickZonesInRegion(srcVolRegion, prefZones)
reqZones = pickZonesInRegion(srcVolRegion, reqZones)

if len(prefZones) == 0 && len(reqZones) == 0 {
volumeCloningReq := fmt.Sprintf("clone zone must reside in source disk region %s", srcVolRegion)
return nil, fmt.Errorf("failed to find zone from topology %v: %s", top, volumeCloningReq)
}

// For zonal -> regional disk cloning, one of the replicated zones must match the source zone.
if locationTopReq.srcReplicationType == replicationTypeNone {
if !slices.Contains(prefZones, srcVolZone) && !slices.Contains(reqZones, srcVolZone) {
volumeCloningReq := fmt.Sprintf("one of the replica zones of the clone must match the source disk zone: %s", srcVolZone)
return nil, fmt.Errorf("failed to find zone from topology %v: %s", top, volumeCloningReq)
}
prefZones = prependZone(srcVolZone, prefZones)
}
}
}

if numZones <= len(prefZones) {
return prefZones[0:numZones], nil
} else {
Expand Down Expand Up @@ -1450,16 +1561,25 @@ func getZoneFromSegment(seg map[string]string) (string, error) {
return zone, nil
}

func pickZones(ctx context.Context, gceCS *GCEControllerServer, top *csi.TopologyRequirement, numZones int) ([]string, error) {
func pickZones(ctx context.Context, gceCS *GCEControllerServer, top *csi.TopologyRequirement, numZones int, locationTopReq *locationRequirements) ([]string, error) {
var zones []string
var err error
if top != nil {
zones, err = pickZonesFromTopology(top, numZones)
zones, err = pickZonesFromTopology(top, numZones, locationTopReq)
if err != nil {
return nil, fmt.Errorf("failed to pick zones from topology: %w", err)
}
} else {
zones, err = getDefaultZonesInRegion(ctx, gceCS, []string{gceCS.CloudProvider.GetDefaultZone()}, numZones)
existingZones := []string{gceCS.CloudProvider.GetDefaultZone()}
// We set existingZones to the source volume zone so that for zonal -> zonal cloning, the clone is provisioned
// in the same zone as the source volume, and for zonal -> regional, one of the replicated zones will always
// be the zone of the source volume. For regional -> regional cloning, the srcVolZone will not be set, so we
// just use the default zone.
if locationTopReq != nil && locationTopReq.srcReplicationType == replicationTypeNone {
existingZones = []string{locationTopReq.srcVolZone}
}
// If topology is nil, then the Immediate binding mode was used without setting allowedTopologies in the storageclass.
zones, err = getDefaultZonesInRegion(ctx, gceCS, existingZones, numZones)
if err != nil {
return nil, fmt.Errorf("failed to get default %v zones in region: %w", numZones, err)
}
Expand Down
Loading