Common library for CNS CSI driver

[divyenpatel]

What this PR does / why we need it:
As part of vSAN 67u3, we have released APIs for Cloud Native Storage.
We have a vSphere CNS CSI driver which uses these new APIs.

In this PR, we are adding a common library required for new CNS CSI driver.

Common library mainly contains

• VolumeManager to call CNS APIs (CreateVolume, AttachVolume, DetachVolume, DeleteVolume, UpdateVolumeMetadata, QueryVolume and QueryAllVolume)
• NodeManager containing implementation for interfaces to cache and obtain kubernetes node details.
• Library functions to obtain information about vSphere Managed objects (datacenters, datastores, hosts, SPBM, and node virtual machines). These library functions will be used by CNS CSI controller and node plugin for volume provisioning and volume attach/detach operations.

In the follow up PRs we will be publishing code for CNS CSI driver.

Special notes for your reviewer:

• In this PR we are using specific commit of govmomi - vmware/govmomi@8bdc2d6
• This commit contains the CNS go binding, which is used in this PR. When govmomi has new release available including CNS go bindings, we will update go.mod file.

Release note:

common code for vSphere CNS CSI Driver

[k8s-ci-robot]

Welcome @divyenpatel!

It looks like this is your first PR to kubernetes-sigs/vsphere-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/vsphere-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[codenrhoden]

I tried to review this with a view towards package structure and consistency with other VMW out-of-tree K8s projects. I do not attempt to validate any code/functionality directly related to CNS. This PR is already coming from the experts in that area.

I have an overarching concern that the common libraries from the CPI project were not used, and appear to be copied into this project with this PR. That will only lead to long-term divergence between the CPI and CSI, and those two projects are inextricably linked. We need a common experience across all projects and products, and that can only be done when things like config file parsing and vCenter session management are shared between the two. I would very much like to see any needed enhancements to the common libraries made in the CPI repo, rather than having that functionality duplicated here.

If an enhancement is made to the CPI code, we want CSI to get it as well w/o having to copy the code.

I'm also concerned that there are zero _test.go files in this PR. Even files that were copied from the CPI repo (and then enhanced) did not bring the unit tests with them.

[codenrhoden]

This package seems like it was based off of https:/kubernetes/cloud-provider-vsphere/tree/master/pkg/common/kubernetes.

Why not enhance that common package?

[codenrhoden]

/assign
/assign @frapposelli
/assign @dvonthenen
/assign @andrewsykim

[codenrhoden]

I know this is just the first part of a huge amount of effort that went into this. Very exciting to see!

One question I had along those lines that may be relevant to the future PRs. Is there any possibility of preserving more git authorship history? I ask because right now all the credit goes to @divyenpatel for this 3000+ lines of code. I know many contributed, and some of those people may like to find their way into the OWNERS file in the future. Showing continued authorship and contributions helps with that.

[dvonthenen]

Just as an FYI, there are some Linux operating systems that don't use the normalized UUID. Photon v2 as an example. You need to protect yourself against both. That why in the CPI we have a function called "shakeOutNodeIDLookup".

Since the CPI already does this same functionality and we already have a ListNodes capability in which it retrieves all the nodes in that k8s cluster that you can call through gRPC, would it be helpful to just call that instead of duplicating the effort considering CSI is already dependent on the CPI because of the node labels and zones functionality?

https:/kubernetes/cloud-provider-vsphere/blob/master/pkg/cloudprovider/vsphere/server/server.go#L74

[dvonthenen]

Posted my feedback and I didn't want to duplicate what @codenrhoden posted in his review, but I would definitely have to echo the comments made in it.

[divyenpatel]

Thank you @dvonthenen @codenrhoden for detailed review. Let me address your review comments.

[andrewsykim]

Adding a few more comments on top of @codenrhoden's

[divyenpatel]

addressed some of the review comments as part of this commit - d1d96a4

I will mark them as resolved and will work on rest of the comments.

[divyenpatel]

One question I had along those lines that may be relevant to the future PRs. Is there any possibility of preserving more git authorship history? I ask because right now all the credit goes to @divyenpatel for this 3000+ lines of code. I know many contributed, and some of those people may like to find their way into the OWNERS file in the future. Showing continued authorship and contributions helps with that.

@codenrhoden yes that is the plan. After we are done with merging common code required for CNS controller, metadata syncer and full sync. Authors will generate PRs for their own components.

We have assigned tasks to each contributors to address this authorship issue.

I will be generating PR for controller.
@RaunakShah will be generating PR for metadata syncer container.
@lipingxue will be generating PR for full sync.
@yuyiying and @chethanv28 will be generating PRs for e2e tests and framework.

CC: Manoj Sundararajan - @manojvs157

[manojvs157]

One question I had along those lines that may be relevant to the future PRs. Is there any possibility of preserving more git authorship history? I ask because right now all the credit goes to @divyenpatel for this 3000+ lines of code. I know many contributed, and some of those people may like to find their way into the OWNERS file in the future. Showing continued authorship and contributions helps with that.

@codenrhoden yes that is the plan. After we are done with merging common code required for CNS controller, metadata syncer and full sync. Authors will generate PRs for their own components.

We have assigned tasks to each contributors to address this authorship issue.

I will be generating PR for controller.
@RaunakShah will be generating PR for metadata syncer container.
@lipingxue will be generating PR for full sync.
@yuyiying and @chethanv28 will be generating PRs for e2e tests and framework.

CC: Manoj Sundararajan - @manojvs157

Thanks for bringing that up @codenrhoden. Yes, we have a plan for individuals that contributed to various areas in CSI to generate future PRs largely inline with where their contributions have been. @divyenpatel has contributed towards most of what is part of this PR.

[codenrhoden]

From https:/kubernetes-sigs/vsphere-csi-driver/pull/47/files#r318841162

@codenrhoden pbm.go is not used anywhere in the cloud-provider-vsphere repository.

I think, if we keep the common code which is actually used in this repository locally in this repo, it would be good. I agree, We need to minimize duplicated code and remove dead code from cloud-provider-vsphere repository as well.

I think even though cloud-provider-vsphere and vsphere-csi-driver is used in conjunction, from CI/CD perspective, we will have challenges to validate driver functionally when some one changes code in the CCM repository.

I think this is the main conversation that needs to be resolved. It may be true that PBM code is not used in the CPI, and should live here instead. I think that should be resolved with @frapposelli
and @dvonthenen.

But I strongly believe the config package should be common. That has already been enhanced beyond what was copied here for support of specifying IPv4/6 per vCenter connection. It also has tests to ensure backwards compatibility.

I think we need to work out what can/should be shared between CPI and CSI. In my mind the biggest two are config (parsing vsphere.conf from a file or from a K8s config map, reading creds from K8s secrets), and session management. Do you see anything unique about what CNS would require when it comes to authenticating with vCenter and managing connections? I know that there is a separate CNS client that gets created through govmomi, but that seems like a single additional thing that could be built on top. Am I missing anything?

/cc @frapposelli @andrewsykim @dvonthenen

[andrewsykim]

I think we need to work out what can/should be shared between CPI and CSI. In my mind the biggest two are config (parsing vsphere.conf from a file or from a K8s config map, reading creds from K8s secrets), and session management.

I think of the 3, most importantly we want the config types to be shared since those are user facing APIs that we really can't break. Managing the types in two places seems error prone. I think anything else - extracting creds and session management should be shared ideally but we can iterate on that in follow-up PRs. Thoughts?

[codenrhoden]

I think of the 3, most importantly we want the config types to be shared since those are user facing APIs that we really can't break. Managing the types in two places seems error prone. I think anything else - extracting creds and session management should be shared ideally but we can iterate on that in follow-up PRs. Thoughts?

yes, I think iterating here is okay, but that the config stuff is a hard requirement. But I definitely want others opinion. One point of clarification, though, you said "config types". I don't think it should be just the types... I think it should be the whole package to include logic and tests.

[andrewsykim]

One point of clarification, though, you said "config types". I don't think it should be just the types... I think it should be the whole package to include logic and tests.

agreed

[codenrhoden]

@divyenpatel I see three things to take care of before merging this.

1. let's come to an agreement with @andrewsykim on the .gitignore stuff... I don't have a strong opinion on it.
2. The file copyright issue
3. Once all the above is fixed, squash all the commits into one. We can't make the commit with the file containing a VMW copyright.

[dvonthenen]

/lgtm

[codenrhoden]

/approve

I'll be opening issues to track some of the future improvements and de-duplication that can be done.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codenrhoden, divyenpatel

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 [codenrhoden]

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

[divyenpatel]

Thanks @codenrhoden @dvonthenen @andrewsykim

Regarding the config file, the only addition is the Kubernetes cluster-id, which we can add to https:/kubernetes/cloud-provider-vsphere/tree/master/pkg/common/config and refer it from there. Let me address that in separate PR.

Also for de-duplication of common code, I will revisit this code and make sure we don't have duplicated common code here in this repository. vclib we can directly reuse from cloud-provider-vsphere repository and the cns specific common code we can keep in this repository as it is only going to be consumed by the driver.