-
Notifications
You must be signed in to change notification settings - Fork 179
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
Fullsync for vsphere CSI Driver #61
Conversation
Welcome @lipingxue! |
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 @lipingxue. 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. |
@lipingxue looks like you need to sign the CLA |
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.
Just one small bit of feedback so far. Will ok-to-test
once the CLA is in place.
I just signed CLA. |
we'll see if the CLA bot picks it up! |
@lipingxue Your commit is under your personal address. We'll want it to be under your VMware address.
Once you set your config correctly, you can change your single commit with:
|
@codenrhoden It seems that CLA bot does not pick up my CLA. I signed the CLA using my personal email and the email account I used for my github account is my personal email. Does it matter? |
@codenrhoden Ignore my previous update. cncf-cla is marded as "Yes" now. |
I know you said "ignore", but just for posterity -- yes it matters. The commits need to be with your VMware address for several reasons. I see that they are updated now, so thank you! /ok-to-test |
Other than the comments @codenrhoden made, this seems pretty good to me! Awesome! |
@lipingxue It looks like there are just a couple fixes to make re: You can test this locally by running either |
This looks good to me! Please squash your commits and this is good to go. |
Fullsync for vsphere CSI Driver Address comments from Travis. Address comments from Travis and Divyen. Fix the fmt check.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: codenrhoden, lipingxue 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 |
…ncy-openshift-4.14-vmware-vsphere-syncer Updating vmware-vsphere-syncer images to be consistent with ART
What this PR does / why we need it:
The goal for FullSync is to fetch data from CNS and comparing it with the local PV/PVC information in Kubernetes cluster to identify what volumes need to be created/updated/deleted in CNS. FullSync is designed to handle the case when Metadata Syncer cannot push down the data to vSphere as expected. Those cases includes, CNS goes down, csi driver goes down etc.
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 FullSync will be added in the following PRs.
Release note: