-
Notifications
You must be signed in to change notification settings - Fork 145
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
feat: add smb file restore from snapshot #1466
feat: add smb file restore from snapshot #1466
Conversation
Hi @umagnus. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
/ok-to-test |
f41df5e
to
60e9312
Compare
7f6efab
to
236d63a
Compare
8a26a08
to
6ba13ec
Compare
pkg/azurefile/controllerserver.go
Outdated
for { | ||
select { | ||
case <-timeTick: | ||
jobState, percent, err := getAzcopyJob(dstFileShareName) |
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.
can you combine the logic with copy volume, the select ... case
logic is quite similar
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 think you could use same function for both restoreFromSansphot and copyVolume
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.
It's because srcPath is different from copy volume and snapshot has other logic for different storage account, should we combine the logic just for azcopy?
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.
combine the logic in a copyFileShareByAzcopy
func
test/e2e/testsuites/dynamically_provisioned_volume_snapshot_tester.go
Outdated
Show resolved
Hide resolved
test/e2e/testsuites/dynamically_provisioned_volume_snapshot_tester.go
Outdated
Show resolved
Hide resolved
bc90501
to
39033de
Compare
/retest |
1 similar comment
/retest |
2ced8e0
to
3374e63
Compare
/retest |
63b1b7d
to
931c2bd
Compare
/test pull-azurefile-csi-driver-e2e-capz |
d68ca98
to
11f9a5c
Compare
/retest |
1 similar comment
/retest |
/test pull-azurefile-csi-driver-e2e-capz |
/retest |
/test pull-azurefile-csi-driver-e2e-capz |
1 similar comment
/test pull-azurefile-csi-driver-e2e-capz |
test.Run(ctx, cs, snapshotrcs, ns) | ||
}) | ||
|
||
ginkgo.It("should create a pod, write to its pv, take a volume snapshot, overwrite data in original pv, create another pod from the snapshot, and read unaltered original data from original pv[file.csi.azure.com]", func(ctx ginkgo.SpecContext) { |
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 think we could remove this test case to make test pass first
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.
done, remove it
ccad31e
to
7049605
Compare
/test pull-azurefile-csi-driver-e2e-capz |
@umagnus: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
/retest |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, umagnus 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 |
pls cherrypick to release-1.30, it may depend on #1657 merge first |
/cherrypick release-1.30 |
@andyzhangx: #1466 failed to apply on top of branch "release-1.30":
In response to this:
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. |
Would this gets cherry-picked to 1.29 as well? |
@kaovilai no, we don't have plan to support this feature on csi driver v1.29 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
feat: add smb file restore from snapshot by using azcopy
Which issue(s) this PR fixes:
Fixes #136
Requirements:
Special notes for your reviewer:
enable different storageclass smb file restore, but only be enabled in the same sub because csi cannot get different subs id in sourceVolumeContent snapshotId, but it can get account name in it. Also add e2e test case
should create a pod, write to its pv, take a volume snapshot, overwrite data in original pv, create another pod from the snapshot, use another storage class, and read unaltered original data from original pv[file.csi.azure.com]
to test this case.csi logs
Release note: