Skip to content
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

fix vsphere backward compatibility issue #409

Conversation

divyenpatel
Copy link
Member

@divyenpatel divyenpatel commented Oct 2, 2020

What this PR does / why we need it:
This PR is fixing the v2.0.0 backward compatibility issue observed on the vSphere 67 release.

Which issue this PR fixes
fixes #408

Special notes for your reviewer:
Testing is done.

@marunachalam has helped verify this fix on vSphere 67 latest patch as well as vSphere 7.0
All full sync related issues are resolved on vSphere 67 vCenter.

Test logs
VC 6.7- https://gist.github.com/marunachalam/cc257dfa77479f08a4ba2af24d802791
VC 7.0 - https://gist.github.com/marunachalam/b86d999e757fa3f72678f342949f86a2

Release note:

fix vsphere backward compatibility issue

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 2, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2020
@divyenpatel divyenpatel force-pushed the fix-backward-compatibility-in-syncer branch from b01edf7 to 85b9cbc Compare October 2, 2020 22:29
@svcbot-qecnsdp
Copy link

Can one of the admins verify this patch?

@divyenpatel divyenpatel force-pushed the fix-backward-compatibility-in-syncer branch from 85b9cbc to 1f6b354 Compare October 2, 2020 22:32
pkg/syncer/fullsync.go Outdated Show resolved Hide resolved
@chethanv28
Copy link
Collaborator

I have only minor comments.
Please run Vanilla pipeline(with latest VC) and update the results in the PR. With this the testing section will be complete w.r.t all VC versions

pkg/csi/service/vanilla/controller.go Outdated Show resolved Hide resolved
pkg/syncer/fullsync.go Outdated Show resolved Hide resolved
@chethanv28
Copy link
Collaborator

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chethanv28, 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:

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

@divyenpatel divyenpatel force-pushed the fix-backward-compatibility-in-syncer branch 2 times, most recently from 3e27dd4 to 1e9e320 Compare October 7, 2020 00:10
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 7, 2020
@SandeepPissay
Copy link
Contributor

The change looks good except for a minor comment above. Can you run E2E tests? It will be nice if we test this code works fine on vSphere 6.7U3 before merge. Btw, can you also squash commit this PR?

@divyenpatel divyenpatel force-pushed the fix-backward-compatibility-in-syncer branch from 731db74 to 2f4ddef Compare October 7, 2020 03:08
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 7, 2020
@marunachalam
Copy link
Contributor

@SandeepPissay I have executed the Full sync delete volume test on 67u3 and 7.0 setup with images provided by @divyenpatel, here are the logs.
7.0_full_sync_delete_vol.txt
67u3_full_sync_delete_vol.txt

@divyenpatel
Copy link
Member Author

jtest block-vanilla

@divyenpatel
Copy link
Member Author

jtest file-vanilla

@svcbot-qecnsdp
Copy link

Started Vanilla block pipeline...

@svcbot-qecnsdp
Copy link

Started vanilla file pipeline...

@svcbot-qecnsdp
Copy link

File vanilla build status: FAILURE 
Stage before exit: make 

@svcbot-qecnsdp
Copy link

Block vanilla build status: FAILURE 
Stage before exit: testbed-deploy 

@sashrith
Copy link
Contributor

sashrith commented Oct 7, 2020

jtest block-vanilla

@sashrith
Copy link
Contributor

sashrith commented Oct 7, 2020

jtest file-vanilla

@svcbot-qecnsdp
Copy link

Started vanilla file pipeline...

@svcbot-qecnsdp
Copy link

Started Vanilla block pipeline...

@svcbot-qecnsdp
Copy link

Block vanilla build status: FAILURE 
Stage before exit: testbed-deploy 

@svcbot-qecnsdp
Copy link

File vanilla build status: FAILURE 
Stage before exit: make 

@SandeepPissay
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2020
@k8s-ci-robot k8s-ci-robot merged commit 6c555c4 into kubernetes-sigs:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-2.1.0-candidate release-2.1.0-cherry-picked size/M Denotes a PR that changes 30-99 lines, ignoring generated files. v2.0.1 Candidate for v2.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full sync in vSphere CSI Driver 2.0.0 will not delete unused volume in vSphere 67u3
8 participants