Topology support #77

davidz627 · 2018-07-23T22:29:34Z

The driver will return the node toplogy on GetNodeInfo and supports simple Requisite topology specification on the create volume call.

Also the testing infrastructure was changed to support topology.

Fixes: #13

/assign @msau42
/assign @verult

k8s-ci-robot · 2018-07-23T22:29:35Z

@davidz627: GitHub didn't allow me to assign the following users: verult.

Note that only kubernetes-sigs members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

The driver will return the node toplogy on GetNodeInfo and supports simple Requisite topology specification on the create volume call.

Also the testing infrastructure was changed to support topology.

Part of: #13

/assign @msau42
/assign @verult

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 kubernetes/test-infra repository.

k8s-ci-robot · 2018-07-23T22:29:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627

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:

~~OWNERS~~ [davidz627]

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

msau42 · 2018-07-23T23:13:52Z

pkg/gce-pd-csi-driver/controller.go

+	// TODO: Support preferred toplogies.
+	if req.GetAccessibilityRequirements() != nil {
+		reqTop := req.GetAccessibilityRequirements().GetRequisite()
+		if reqTop == nil || len(reqTop) == 0 {


I don't think requisite is strictly required if preferences is specified

msau42 · 2018-07-23T23:17:05Z

pkg/gce-pd-csi-driver/controller.go

+		if reqTop[0].GetSegments() == nil {
+			return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("CreateVolume requisite toplogies specified specified but no segments"))
+		}
+		// Currently just pick the 0th requisite toplogy. This is a valid strategy but


if no preferences are specified, then yeah cycling or random may be better

msau42 · 2018-07-23T23:19:44Z

pkg/gce-pd-csi-driver/controller.go

+		}
+		// Currently just pick the 0th requisite toplogy. This is a valid strategy but
+		// we may want to randomly cycle in the future (?)
+		zoneSeg, ok := reqTop[0].GetSegments()["zone"]


the topologyKey should be a constant that is also well-defined in the documentation (because users will have to set it in the storageclass). I think the current thought was something like "com.google.topology/zone"

Since this key could be used across GCP products, we should get more review on this constant, but that could be a separate discussion

msau42 · 2018-07-23T23:21:51Z

pkg/gce-pd-csi-driver/controller.go

+			configuredZone = zoneSeg
+		} else {
+			return nil, status.Error(codes.InvalidArgument,
+				fmt.Sprintf("CreateVolume requisite topology specified but could not find zone in segment: %v", reqTop[0].GetSegments()))


We may want to have even stricter validation and error if other topology keys that we don't understand are specified.

FYI, I proposed to add this restriction to CSI in this PR: container-storage-interface/spec#257 .

msau42 · 2018-07-24T00:01:13Z

pkg/gce-pd-csi-driver/controller_test.go

+					},
+				},
+			},
+			expVol: &csi.Volume{


The returned volume should also have topology set right?

msau42 · 2018-07-24T00:03:03Z

pkg/gce-pd-csi-driver/controller_test.go

+			},
+		},
+		{
+			name: "success with topology overwrite zone in params",


I think we actually wanted to error in this case because we don't just want to drop/ignore user config

davidz627 · 2018-07-25T21:37:59Z

@msau42 comments addressed

davidz627 · 2018-07-25T22:00:32Z

@msau42 sorry this PR really balooned but everything was kind of intertwined :(

The good news is the commits are split "decently"

verult

Reviewed the core logic, mostly looks good!

verult · 2018-07-25T23:03:46Z

pkg/gce-pd-csi-driver/controller.go

+		}
+		// Currently just pick the 0th requisite toplogy. This is a valid strategy but
+		// we may want to randomly cycle in the future (?)
+		zoneSeg, ok := reqTop[0].GetSegments()["zone"]


Since this key could be used across GCP products, we should get more review on this constant, but that could be a separate discussion

verult · 2018-07-26T00:35:46Z

pkg/gce-pd-csi-driver/controller.go

+			configuredZone = zoneSeg
+		} else {
+			return nil, status.Error(codes.InvalidArgument,
+				fmt.Sprintf("CreateVolume requisite topology specified but could not find zone in segment: %v", reqTop[0].GetSegments()))


FYI, I proposed to add this restriction to CSI in this PR: container-storage-interface/spec#257 .

verult · 2018-07-26T00:53:54Z

pkg/gce-pd-csi-driver/controller.go

 	for k, v := range req.GetParameters() {
 		if k == "csiProvisionerSecretName" || k == "csiProvisionerSecretNamespace" {
 			// These are hardcoded secrets keys required to function but not needed by GCE PD
 			continue
 		}
 		switch strings.ToLower(k) {
-		case "type":
+		case common.ParameterKeyType:


Are zone & type parameters kept here because of in-tree migration?

Yes, we want to continue to support Parameters

verult · 2018-07-26T01:02:19Z

pkg/gce-pd-csi-driver/controller.go

@@ -341,12 +368,23 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
 	// TODO: Factor out the volume capability functionality and use as validation in all other functions as well
 	glog.V(5).Infof("Using default ValidateVolumeCapabilities")
 	// Validate Arguments
+	if req.GetVolumeCapabilities() == nil || len(req.GetVolumeCapabilities()) == 0 {


What about AccessibleTopology?

verult · 2018-07-26T01:06:34Z

pkg/gce-pd-csi-driver/controller.go

+	// Pick the preferred toplogy in order
+	if len(prefTop) != 0 {
+		if prefTop[0].GetSegments() == nil {
+			return "", fmt.Errorf("preferred toplogies specified specified but no segments")


"specified" duplicated, and a couple of "toplogy" and "toplogies" in this file

verult · 2018-07-26T01:15:01Z

pkg/gce-pd-csi-driver/controller.go

+		}
+		return zone, nil
+	} else if len(reqTop) != 0 {
+		r := rand.Intn(len(reqTop))


The in-tree plugin has a round-robin mechanism to choose zones (here). Could we do something similar?

we are not guaranteed to have a monotonically increasing id/name for the CSI Driver. Using a hash strategy could have unwanted behavior. We would have to actually keep track of which we've done or not to get round-robin going. I think a random pick is actually our best option here

From a backwards compatibility standpoint for in-tree migration, do we need to support this though? cc @saad-ali

This does not affect migration. There are two potential cases for the migration path:

The whole PD lifecycle is migrated to CSI, then CreateVolume is called and we pick a zone in whatever method we want (random here), all future references to this volume just continue to use the zone picked

PD is precreated in some zone, this logic is never used, we just use whatever zone was used in the creation (the round robin method)

I don't think theres any point after creation where we make assumptions on "how the zone was picked". so therefore it can't create any issues.

If a user created a statefulset:

Before the migration, the PDs would be evenly spread across zones

After the migration, the PDs would be randomly spread across zones

The question is, do we want to retain the old spreading behavior for backwards compatibility? Or should we just declare multi-zone in-tree provisioning broken and require users to used delayed provisioning going forward?

verult · 2018-07-26T01:17:10Z

pkg/gce-pd-csi-driver/controller.go

+
+	if len(configuredZone) == 0 {
+		// Default to zone that the driver is in
+		configuredZone = gceCS.CloudProvider.GetZone()


The zone-picking strategy should be consistent, regardless of whether zones are specified through parameters or AccessibilityRequirement

what do you mean by this? consistent with what?

I think what @verult means is that once you enable the topology capability, then you should always get an accessibility requirement. So if you run into a case where there is no zone specified, then you should error.

I believe accessibility requirements can be unset:

// If this field is not specified and the SP has the // ACCESSIBILITY_CONSTRAINTS plugin capability, the SP MAY choose // where the provisioned volume is accessible from.

The current logic is:

Only one of Parameters or AccessibilityRequirements may be set. Error if both are set.

If neither are set (valid according to CSI Spec) default to zone that driver is in

This logic makes sense to me according to the Spec. Please let me know if you think it should be changed and we can discuss

verult · 2018-07-26T01:24:16Z

pkg/gce-cloud-provider/metadata/metadata.go

+	"cloud.google.com/go/compute/metadata"
+)
+
+type MetadataService interface {


Comments, especially around the intended purpose of the interface in order to limit its unwanted expansion. I'm worried about future developers adding arbitrary methods here.

nit: "Service" gives me an impression of a long running process, and it's already an overloaded term in the container world

davidz627 · 2018-07-27T20:51:48Z

addressed comments.

msau42 · 2018-07-28T00:16:54Z

pkg/common/constants.go

+	ParameterKeyType = "type"
+
+	// Keys for Topology
+	TopologyKeyZone = "com.google.gcp-compute-persistent-disk-csi-driver.topology/zone"


I think we should not put the driver name in the key. Filestore driver should be able to share this key too.

I thought we didn't want collisions between topology keys? I may be misunderstanding what this is used for exactly

We want to be able to share topology keys amongst drivers from the same vendor.

To avoid proliferation of labels on Node objects

To allow users to use a common set of topology labels in their Pods.

msau42 · 2018-07-28T00:27:09Z

pkg/gce-pd-csi-driver/controller.go

 			configuredZone = v
 		default:
 			return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid option %q", k))
 		}
 	}

+	// Topology overwrites Storage Class Params if specified


I think we should not allow both zone/zones and AllowedTopologies to be specified at the same time. But I'm not sure if we can actually detect that because csi provisioner will always fill in the requisite topologies, even when AllowedTopologies is not specified. cc @verult

So I think the next best option would be to use the zone/zones parameters because that's what the user specified, so we shouldn't just ignore it.

msau42 · 2018-07-28T00:29:39Z

pkg/gce-pd-csi-driver/controller.go

+
+	if len(configuredZone) == 0 {
+		// Default to zone that the driver is in
+		configuredZone = gceCS.CloudProvider.GetZone()


I think what @verult means is that once you enable the topology capability, then you should always get an accessibility requirement. So if you run into a case where there is no zone specified, then you should error.

msau42 · 2018-07-28T00:33:33Z

pkg/gce-pd-csi-driver/controller.go

+		}
+		return zone, nil
+	} else if len(reqTop) != 0 {
+		r := rand.Intn(len(reqTop))


From a backwards compatibility standpoint for in-tree migration, do we need to support this though? cc @saad-ali

msau42 · 2018-07-28T00:34:59Z

pkg/gce-pd-csi-driver/controller.go

+
+	// Pick the preferred topology in order
+	if len(prefTop) != 0 {
+		if prefTop[0].GetSegments() == nil {


You could have a common function to pick extract a zone from a topology, to simplify the preference vs required logic.

picking a zone from topology is different between preference vs required so it doesn't make sense to me to abstract this out, I may be misunderstanding your suggestion

davidz627 · 2018-07-30T21:14:12Z

@msau42 hoping to get this in by today if it looks good to you, I think all open questions have been resolved. Let me squash commits before your lgtm though, there are currently too many

msau42 · 2018-07-30T23:24:23Z

lgtm, do you want to squash?

Refactor common utils and added metadata service. Refactor of e2e testing framework to accomidate parallel tests.

davidz627 · 2018-07-31T00:59:23Z

@msau42 rebased, thanks!

msau42 · 2018-07-31T01:00:51Z

/lgtm

k8s-ci-robot assigned msau42 Jul 23, 2018

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 23, 2018

k8s-ci-robot requested review from msau42 and saad-ali July 23, 2018 22:29

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 23, 2018

msau42 reviewed Jul 23, 2018

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 23, 2018

msau42 reviewed Jul 24, 2018

View reviewed changes

davidz627 changed the title ~~Basic topology support for the driver~~ Topology support Jul 25, 2018

verult suggested changes Jul 26, 2018

View reviewed changes

davidz627 force-pushed the feature/realTopology branch from 8ce2bdf to b3cd61d Compare July 27, 2018 20:51

davidz627 mentioned this pull request Jul 27, 2018

fix node deployment to include GOOGLE_APPLICATION_CREDENTIALS envvar #79

Closed

davidz627 force-pushed the feature/realTopology branch 3 times, most recently from c2c0f0b to 6934ae8 Compare July 27, 2018 23:03

msau42 reviewed Jul 28, 2018

View reviewed changes

davidz627 force-pushed the feature/realTopology branch from e814f05 to 583c125 Compare July 30, 2018 20:04

Added topology support to driver. Updated CSI Test dep to v0.3.0.

f11eada

Refactor common utils and added metadata service. Refactor of e2e testing framework to accomidate parallel tests.

davidz627 force-pushed the feature/realTopology branch from 583c125 to f11eada Compare July 31, 2018 00:59

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2018

k8s-ci-robot merged commit da1b1c0 into kubernetes-sigs:master Jul 31, 2018

davidz627 deleted the feature/realTopology branch July 31, 2018 17:14

Topology support #77

Topology support #77

Conversation

davidz627 commented Jul 23, 2018 • edited Loading

k8s-ci-robot commented Jul 23, 2018

k8s-ci-robot commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Jul 25, 2018

davidz627 commented Jul 25, 2018 • edited Loading

verult left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Jul 30, 2018

msau42 commented Jul 30, 2018

davidz627 commented Jul 31, 2018

msau42 commented Jul 31, 2018

davidz627 commented Jul 23, 2018 •

edited

Loading

davidz627 commented Jul 25, 2018 •

edited

Loading

davidz627 Jul 27, 2018 •

edited

Loading