-
Notifications
You must be signed in to change notification settings - Fork 182
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.
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
Metadata Syncer for vsphere CSI Driver #54
Metadata Syncer for vsphere CSI Driver #54
Conversation
Welcome @RaunakShah! |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Hi @RaunakShah. Thanks for your PR. I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Signed the CLA |
/ok-to-test |
68a9c79
to
ad76722
Compare
/assign @codenrhoden |
Other than the small nit that was pointed out, code-wise this looks pretty good! Awesome job! I can't really comment on whether or not this is doing what is intended... but will rely on others to make that determination. /lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just the nit from divyen to address and I'm curious about the k/k
dependency...
go.mod
Outdated
k8s.io/api v0.0.0-20190831074750-7364b6bdad65 | ||
k8s.io/apimachinery v0.0.0-20190831074630-461753078381 | ||
k8s.io/client-go v0.0.0-20190831074946-3fe2abece89e | ||
k8s.io/klog v0.4.0 | ||
k8s.io/sample-controller v0.0.0-20190831080103-b17b22266fdc | ||
k8s.io/kubernetes v1.11.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll file an issue to follow-up on this later, but this is a really old version of k/k to pull in. It's outside of the "n-2" support policy. The vSphere CPI recently updated it's dependency to 1.15.0, and I know it was a ton of work to do so. At some point the same thing should happen here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And actually, as I've looked at the new *.go files, the only direct imports of k8s.io/*
packages are of klog
, client-go
, and api
. So I'm wondering why kubernetes
is being added as a direct dependency here at all... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I had some tests as part of this PR earlier and overlooked removing it when i removed the tests. Since you've pointed it out, maybe we'll look at that internally and try to resolve any issues ASAP. Thanks!
ad19813
to
6763819
Compare
/retest |
6763819
to
25600fe
Compare
Thanks @divyenpatel @dvonthenen @codenrhoden for the swift reviews! Ive rebased all commits into one based on your comments |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: codenrhoden, RaunakShah 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
The goal of Metadata Syncer is to push vSphere container volume metadata (PV labels, PVC labels, the name of the Pod consuming container volume) from Kubernetes Cluster to vSphere.
This metadata will also be presented to VC or Storage Admin on vSphere UI.
For this design we have taken into consideration, that CSI is container orchestrator agnostic, so we cannot directly use Kubernetes objects in the CSI controller/node plugin code. We will be running separate container in the controller plugin pod, to update metadata for CNS Volumes.
We call this container as "Metadata Syncer".
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Integration, unit and e2e tests for this container will be added in the following PRs. This PR only adds the informer code to perform these updates.
Release note: