v0.2.0.alpha driver initial code push #2

davidz627 · 2018-06-11T22:22:05Z

All the code written so far for the driver

davidz627 · 2018-06-11T22:27:19Z

A full review probably isn't possible here. But if you're scrolling through and see any glaring issues feel free to put a comment or add an issue to the repo after this gets merged.

saad-ali · 2018-06-12T23:50:39Z

Gopkg.lock

+  name = "github.com/container-storage-interface/spec"
+  packages = ["lib/go/csi/v0"]
+  revision = "35d9f9d77954980e449e52c3f3e43c21bd8171f5"
+  version = "v0.2.0"


0.3.0 is out. Do you want to do that update separately?

yes, I'll do it in a future push. This is just the current working code.

saad-ali · 2018-06-13T00:51:07Z

deploy/docker/Dockerfile

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM fedora:26


Confirm with @tallclair what base image we should use.

This is a TODO. Wont fix for initial commit.

saad-ali · 2018-06-13T00:53:54Z

deploy/kubernetes/controller.yaml

+    app: csi-gce-pd
+  ports:
+    - name: dummy
+      port: 12345


nit: add a comments explaining the purpose at the top of the file, e.g.:

# The dummy service is required for creating a StatefulSet

I'm not sure you actually need a service object. I launched the plugin without the service object and it was fine

weird, we've had this discussion before and I remember trying specifically and having it fail. I will test again

saad-ali · 2018-06-13T00:55:43Z

deploy/kubernetes/controller.yaml

+          volumeMounts:
+            - name: socket-dir
+              mountPath: /csi
+        - name: csi-attacher


nit: csi-attacher -> csi-external-attacher for consistency?

the image is named csi-attacher. But i'll change it for consistency.

modified the csi-external-provisioner to csi-provisioner instead for provisioner

msau42 · 2018-06-13T02:02:42Z

Makefile

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+STAGINGIMAGE=gcr.io/dyzz-csi-staging/csi/gce-driver


Can you rename all the references to "gce-pd" instead of just "gce"

msau42 · 2018-06-13T02:04:26Z

deploy/kubernetes/README.md

+Step 1 (Create Credentials):
+Create Service Account Credential JSON on GCP:
+    - TODO: Add detailed steps on how to do this
+    - Requires both Compute Owner and Cloud Project Owner permissions


That's pretty broad permissions, there are no reduced scopes for just pd?

theres a "compute-admin" and a "compute-storage-admin" scope. However when I tried those for some reason there were not enough permissions to Attach. Everything else was fine, just not Attach. I will open an issue on this repo to revisit that and either it will work, or I will open a bug against the compute scopes because I think "Attach" should definitely be covered under "compute-admin"

msau42 · 2018-06-13T02:10:06Z

Can you separate out vendor from the rest? That could make this slightly easier to review :-)

davidz627 · 2018-06-13T17:49:30Z

@msau42 seperated out, its still a 4000+ line commit :P

msau42

For the gce-> gce-pd naming change, I meant everywhere in the project. package, file, directory, type names etc.

msau42 · 2018-06-13T20:22:23Z

pkg/gce-cloud-provider/gce.go

+	if err != nil {
+		return nil, err
+	}
+	service.UserAgent = fmt.Sprintf("GCE CSI Driver/%s (%s %s)", "0.2.0", runtime.GOOS, runtime.GOARCH)


Is there a way to make this version number parameterized so you don't have to modify the code everytime you bump the version? I don't think there's a downward api for container image name though.

added a TODO, don't think its super important, bumping the version is a very manual process and theres no way to get it from the Spec itself AFAIK

msau42 · 2018-06-13T20:22:50Z

pkg/gce-cloud-provider/gce.go

+}
+
+func getProjectAndZoneFromMetadata() (string, string, error) {
+	result, err := metadata.Get("instance/zone")


I think there's a metadata.Zone() call

msau42 · 2018-06-13T21:30:06Z

deploy/kubernetes/controller.yaml

+    app: csi-gce-pd
+  ports:
+    - name: dummy
+      port: 12345


I'm not sure you actually need a service object. I launched the plugin without the service object and it was fine

msau42 · 2018-06-13T21:32:43Z

deploy/kubernetes/setup.yaml

+    namespace: default
+roleRef:
+  kind: ClusterRole
+  name: system:csi-external-provisioner


There's a bug and this role doesn't actually have all the permissions needed. So as a workaround, I had to create a custom clusterrole with just persistentvolumes.get and bind to that too.

I see, the bug was hidden from me because external-attacher has PV GET and I have them bundled together in the controller service account

msau42 · 2018-06-13T21:34:39Z

pkg/gce-csi-driver/controller.go

+
+const (
+	// MaxVolumeSize is the maximum standard and ssd size of 64TB
+	MaxVolumeSize       int64 = 64000000000000


Maybe you can define these in terms of 1000's to make it easier to read

msau42 · 2018-06-13T21:35:35Z

pkg/gce-csi-driver/controller.go

+	DiskTypeStandard = "pd-standard"
+	diskTypeDefault  = DiskTypeStandard
+
+	diskTypePersistent = "PERSISTENT"


Seems like there's two kinds of "disk types". Can you rename one of them to be disambiguate them?

msau42 · 2018-06-13T21:37:09Z

pkg/gce-csi-driver/controller.go

+		capBytes = tcap
+	}
+	// Too small, default
+	if capBytes < DefaultVolumeSize {


cap < min volume size?

msau42 · 2018-06-13T21:39:55Z

pkg/gce-csi-driver/controller.go

+
+func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) {
+	// TODO: Only allow deletion of volumes that were created by the driver
+	// Assuming ID is of form {project}/{zone}/{id}


Do you need project in the id?

comment was obsolete

msau42 · 2018-06-13T21:44:34Z

pkg/utils/utils.go

+	return splitId[0], splitId[1], nil
+}
+
+func CombineVolumeId(zone, name string) string {


You probably need to add something for repd in the volume id

will do in the future

we're not supporting backwards compatibility yet? I wonder how difficult it would be to change the volume handle format later

I will make sure the volume handle is stable before going to beta. I think its fine for alpha (?) @saad-ali

Ya, it's fine for alpha. Update the README file in the root of the repo indicating current status is alpha, and what the implication of alpha are (potentially backwards compatibility breaking changes).

msau42 · 2018-06-14T18:11:46Z

pkg/gce-csi-driver/node.go

+	Mounter mountmanager.Mounter
+}
+
+func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) {


Publish and Unpublish should add some locking in case it gets called multiple times for the same mount. Kubernetes theoretically synchronizes this for the plugin, but it's not a requirement in the CSI spec.

And similarly for Stage/Unstage

just locked all of the node calls for now, I'll add a TODO to make them more fine grained and only lock mutually exclusive calls with each other in the future

some rudimentary node call locking, changed volume size representations, removed dummy service from statefulset definition.

davidz627 · 2018-06-18T17:55:44Z

ping @msau42 @saad-ali for approval

msau42 · 2018-06-18T18:22:56Z

/lgtm

Pull request from sigs head to forked head

Add OpenShift Dockerfile

davidz627 assigned saad-ali and msau42 Jun 11, 2018

k8s-ci-robot requested a review from saad-ali June 11, 2018 22:22

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 11, 2018

davidz627 force-pushed the feature/GCEDriverCode branch 2 times, most recently from 6f85a0f to 1cf9e6b Compare June 12, 2018 22:53

saad-ali reviewed Jun 13, 2018

View reviewed changes

davidz627 force-pushed the feature/GCEDriverCode branch from 1cf9e6b to 9688b10 Compare June 13, 2018 01:07

msau42 reviewed Jun 13, 2018

View reviewed changes

davidz627 added 2 commits June 13, 2018 10:44

v0.2.0.alpha driver initial code push

40e0211

Vendored Packages

9c3112c

davidz627 force-pushed the feature/GCEDriverCode branch from 9688b10 to 9c3112c Compare June 13, 2018 17:48

msau42 reviewed Jun 13, 2018

View reviewed changes

davidz627 force-pushed the feature/GCEDriverCode branch from 9abe2c2 to a30cd65 Compare June 13, 2018 20:43

msau42 reviewed Jun 13, 2018

View reviewed changes

msau42 reviewed Jun 14, 2018

View reviewed changes

davidz627 force-pushed the feature/GCEDriverCode branch 2 times, most recently from 8b53c4c to 56ce990 Compare June 14, 2018 18:19

Makefile s/gce/gce-pd, added TODO, changed metadata.Zone() call, added

f3b2119

some rudimentary node call locking, changed volume size representations, removed dummy service from statefulset definition.

davidz627 force-pushed the feature/GCEDriverCode branch from 56ce990 to f3b2119 Compare June 15, 2018 00:01

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

davidz627 merged commit e0fdcf1 into kubernetes-sigs:master Jun 18, 2018

davidz627 deleted the feature/GCEDriverCode branch June 18, 2018 18:25

k8s-ci-robot pushed a commit that referenced this pull request Aug 12, 2020

Merge pull request #2 from kubernetes-sigs/master

4473873

Pull request from sigs head to forked head

jsafrane pushed a commit to jsafrane/gcp-compute-persistent-disk-csi-driver that referenced this pull request Nov 11, 2020

Merge pull request kubernetes-sigs#2 from jsafrane/add-dockerfile

3e2b23e

Add OpenShift Dockerfile

v0.2.0.alpha driver initial code push #2

v0.2.0.alpha driver initial code push #2

Conversation

davidz627 commented Jun 11, 2018

davidz627 commented Jun 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 Jun 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

msau42 commented Jun 13, 2018

davidz627 commented Jun 13, 2018

msau42 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

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

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 Jun 18, 2018

msau42 commented Jun 18, 2018

davidz627 Jun 13, 2018 •

edited

Loading