Regional PD Implementation [Removing parameter "zone"] #84

davidz627 · 2018-08-08T18:55:51Z

Fixes: #7

This is the full regional PD implementation.

BREAKING CHANGES:

It includes REMOVAL of the parameter "zone"
It ADDS the parameter "replication-type"
NodeID format has been changed to: {zone}/{nodeNameFromGCE}
VolumeID format has been changed to fully qualified format:
projects/{project}/zones/{zone}/disks/{name}
or
projects/{project}/regions/{region}/disks/{name}

Also snuck in,
Fixes: #53

/assign @msau42
/assign @verult

k8s-ci-robot · 2018-08-08T18:55:55Z

[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-08-08T19:59:57Z

pkg/common/utils.go

-func SplitZoneNameId(id string) (string, string, error) {
+func VolumeIDToKey(id string) (*meta.Key, error) {
+	splitId := strings.Split(id, "/")
+	if len(splitId) != 6 {


Can you make the total and each element index constants?

msau42 · 2018-08-08T20:00:25Z

pkg/common/utils.go

+	}
+}
+
+func NodeIDToZoneAndName(id string) (string, string, error) {
 	splitId := strings.Split(id, "/")
 	if len(splitId) != 2 {


msau42 · 2018-08-08T20:01:11Z

pkg/common/utils.go

+		return "", fmt.Errorf("no zones specified")
+	}
+	zone := zones[0]
+	splitZone := strings.Split(zone, "-")


Add a comment with the expected syntax

msau42 · 2018-08-08T20:05:22Z

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

+		remainingZones := reqSet.Difference(prefSet)
+
+		if remainingZones.Len() < remainingNumZones {
+			return nil, fmt.Errorf("need %v zones in preferred and requisite topologies to satisfy, only have %v unique zones", numZones, reqSet.Union(prefSet).Len())


message could be simplified a little bit to say "need %v zones from topology, only got %v unique zones"

msau42 · 2018-08-08T20:07:08Z

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

+		return nil, fmt.Errorf("failed to insert regional disk: %v", err)
+	}
+
+	glog.Infof("Completed creation of disk %v", name)


Add log levels to all the Info statements

msau42 · 2018-08-08T20:11:00Z

Just did a quick scan review. Will do more in-depth later.

Nit: we're not actually deprecating 'zone', we're outright removing it

davidz627 · 2018-08-09T01:34:01Z

/retest

davidz627 · 2018-08-13T20:55:01Z

@msau42 addressed comments :)
still can't figure out this e2e test failure :(

msau42 · 2018-08-13T21:09:54Z

pkg/common/utils.go

+	} else if splitId[volIDToplogyKey] == "regions" {
+		return meta.RegionalKey(splitId[volIDDiskNameValue], splitId[volIDToplogyValue]), nil
+	} else {
+		return nil, fmt.Errorf("could not get id components, expected either zones or regions, got: %v", splitId[2])


2 => volIDTopologyKey

msau42 · 2018-08-13T21:13:33Z

pkg/common/utils.go

+)
+
+const (
+	volIDFmt = "projects/%s/zones/%s/disks/{}"


is this used anywhere?

msau42 · 2018-08-13T21:14:04Z

pkg/common/utils.go

+const (
+	volIDFmt = "projects/%s/zones/%s/disks/{}"
+
+	volIDToplogyKey    = 2


Add a comment above here with the syntax

msau42 · 2018-08-13T21:16:16Z

pkg/common/utils.go

@@ -31,14 +46,41 @@ func GbToBytes(Gb int64) int64 {
 	return Gb * 1024 * 1024 * 1024
 }

-func SplitZoneNameId(id string) (string, string, error) {
+func VolumeIDToKey(id string) (*meta.Key, error) {


unit tests?

msau42 · 2018-08-13T21:54:40Z

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

-		zone, err := getZoneFromSegment(prefTop[0].GetSegments())
+		// Add the remaining number of zones into the set
+		// TODO: Maybe we should pick a random set to get
+		nSlice, err := pickRandNFromSlice(remainingZones.List(), remainingNumZones)


I think we actually want the first zone to be random. But the second zone should always be +1 from the first (sorted)

msau42 · 2018-08-13T21:56:05Z

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

-	return cloud.zone
+func (cloud *CloudProvider) GetDisk(ctx context.Context, key *meta.Key) (*CloudDisk, error) {
+	switch key.Type() {
+	case meta.Zonal:


Instead of having case statements for the two types of disks, I'm wondering if it would be simpler to define a generic Disk interface, and have a zonal implementation, and a regional implementation.

I think there are merits to both ways, although the current method I still like better for 3 reasons:

This isn't just an interface for disks, there are some other "types" of calls as well that wouldn't cleanly fit in a disk interface and I would rather just maintain 1 interface for "cloud provider"

Some calls are not different between regional/zonal, I believe in go since there is no inheritance we would just have to copy/paste that code if there were two types of disks

It's already like this and would take some effort to change. Without a strong reason otherwise it would be better to spend that effort elsewhere.

davidz627 · 2018-08-14T21:01:35Z

/hold
Will squash commits before merging

davidz627 · 2018-08-14T21:18:24Z

@msau42 addressed comments

msau42 · 2018-08-14T22:21:11Z

lgtm

picking random zones just pick 1 zone randomly and the others are picked consecutively

davidz627 · 2018-08-14T23:19:04Z

/hold cancel
@msau42 for lgtm

msau42 · 2018-08-14T23:19:54Z

/lgtm

jingxu97 · 2018-08-20T21:03:23Z

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

+	}
+
+	glog.V(4).Infof("Completed creation of disk %v", name)
+	disk, err := cloudProvider.GetDisk(ctx, meta.ZonalKey(name, diskZone))


now for each creation, there is an extra cloud provider GET API calls. Why not just use insertDisk which can return the disk object directly? In previous version, there is no GET call after the CREATE call, right?

insertDisk doesn't return a disk object AFAIK. It returns an operation which we can poll for completion of the disk create operation, but getting the disk itself will be a seperate operation still.

We required this change not strictly because of RePD but because we wanted to have volume ID be the disk selfLink of the form projects/{project}/zones|regions/{zone|region}/disks/{diskname} and to get the self-link we need to get the disk

davidz627 added 3 commits August 8, 2018 11:51

Dep updates: compute beta and meta key

a40d2a5

Regional PD Testing Changes

8ab1572

Regional PD Deployment Script Changes

14ab26f

k8s-ci-robot assigned msau42 and verult Aug 8, 2018

k8s-ci-robot requested review from saad-ali and verult August 8, 2018 18:55

k8s-ci-robot added 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 8, 2018

msau42 reviewed Aug 8, 2018

View reviewed changes

davidz627 changed the title ~~Regional PD Implementation [Deprecating parameter "zone"]~~ Regional PD Implementation [Removing parameter "zone"] Aug 8, 2018

davidz627 force-pushed the feature/RePDNoParams branch 7 times, most recently from f8ba1c2 to 8cae16b Compare August 9, 2018 20:27

davidz627 mentioned this pull request Aug 10, 2018

Improve cloud provider layering #70

Closed

davidz627 force-pushed the feature/RePDNoParams branch from 8cae16b to 172c91c Compare August 13, 2018 20:54

msau42 reviewed Aug 13, 2018

View reviewed changes

davidz627 force-pushed the feature/RePDNoParams branch 4 times, most recently from d37ed65 to b56cdaa Compare August 14, 2018 17:26

davidz627 force-pushed the feature/RePDNoParams branch from b56cdaa to 6dd5ca7 Compare August 14, 2018 17:29

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2018

davidz627 force-pushed the feature/RePDNoParams branch from 8f5ea6d to e6cce81 Compare August 14, 2018 21:18

Regional PD Implementation, added unit tests for common utilities. When

2c284d4

picking random zones just pick 1 zone randomly and the others are picked consecutively

davidz627 force-pushed the feature/RePDNoParams branch from e6cce81 to 2c284d4 Compare August 14, 2018 23:18

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2018

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

k8s-ci-robot merged commit 29bb01d into kubernetes-sigs:master Aug 14, 2018

davidz627 deleted the feature/RePDNoParams branch August 14, 2018 23:29

jingxu97 reviewed Aug 20, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regional PD Implementation [Removing parameter "zone"] #84

Regional PD Implementation [Removing parameter "zone"] #84

davidz627 commented Aug 8, 2018 •

edited

Loading

k8s-ci-robot commented Aug 8, 2018

msau42 Aug 8, 2018

msau42 Aug 8, 2018

msau42 Aug 8, 2018

msau42 Aug 8, 2018

msau42 Aug 8, 2018

msau42 commented Aug 8, 2018

davidz627 commented Aug 9, 2018

davidz627 commented Aug 13, 2018 •

edited

Loading

msau42 Aug 13, 2018

msau42 Aug 13, 2018

msau42 Aug 13, 2018

msau42 Aug 13, 2018

msau42 Aug 13, 2018

msau42 Aug 13, 2018

davidz627 Aug 14, 2018

davidz627 commented Aug 14, 2018

davidz627 commented Aug 14, 2018

msau42 commented Aug 14, 2018

davidz627 commented Aug 14, 2018

msau42 commented Aug 14, 2018

jingxu97 Aug 20, 2018

davidz627 Aug 21, 2018

Regional PD Implementation [Removing parameter "zone"] #84

Regional PD Implementation [Removing parameter "zone"] #84

Conversation

davidz627 commented Aug 8, 2018 • edited Loading

k8s-ci-robot commented Aug 8, 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

msau42 commented Aug 8, 2018

davidz627 commented Aug 9, 2018

davidz627 commented Aug 13, 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

davidz627 commented Aug 14, 2018

davidz627 commented Aug 14, 2018

msau42 commented Aug 14, 2018

davidz627 commented Aug 14, 2018

msau42 commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Aug 8, 2018 •

edited

Loading

davidz627 commented Aug 13, 2018 •

edited

Loading