-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adding design updates to handle block device with kopia #6590
Conversation
/kind changelog-not-required |
design/volume-snapshot-data-movement/volume-snapshot-data-movement.md
Outdated
Show resolved
Hide resolved
design/volume-snapshot-data-movement/volume-snapshot-data-movement.md
Outdated
Show resolved
Hide resolved
f6f1312
to
b21aef6
Compare
design/volume-snapshot-data-movement/volume-snapshot-data-movement.md
Outdated
Show resolved
Hide resolved
design/volume-snapshot-data-movement/volume-snapshot-data-movement.md
Outdated
Show resolved
Hide resolved
b21aef6
to
c853c89
Compare
I got the backup and restore to work in the implementation this design is based on: There is still a lot of work remaining to be done in the implementation. |
@Lyndon-Li Please comment on the PR where I attempt your proposal if you would like I think that @dzaninovic brings up good points about the reader, but we can use the virutalfs streaming file to ensure that Kopia handles it like a streaming file. I think that for restoration, the performance impacts of writing zeros could be a real concern. I think that we really should consider making a wrapper for the output like here: Like I said in the comment on the PR I am planning on updating the design with the above. If it makes sense please let me know so that I can adjust when updating tomorrow :) |
@shawn-hurley Let me comment on your changes in catalogicsoftware#2 here below, so that we can make the discussion centrally. |
1. For the IO optimization, like DIO: For all these complications, a new uploader -- block uploader is more suitable. On the other hand, I am not saying that the current Conclusively:
|
2. For skipping zero bytes during restore: Moreover, detecting zero bytes are not an easy task either. Simply enumerating each byte one by one is not a good choice, think about how much CPU it takes for large disk. Actually, detecting and applying zero bytes involve backup repo and storage side support, we don't need to consider it in Phase-0 and even the initial version of block level backup doesn't need to support it. |
3. check target file is a block device from OS level |
I believe the answer here, IMO, is to just use Golang's built-in reader from os. File. This seems to work. @dzaninovic, if we need to add the performance bits, I think it does make sense to make this part of a more full feature block uploader that can also handle CBT IMO. @Lyndon-Li just making sure, but you are ok with this? @dzaninovic does this work for you? |
I am fine with having optimizations added later in separate PRs and use the simple way for now if that is acceptable. |
This can be a non-default option that user can set if they know that disks are provisioned with all zeroes. |
c466d34
to
572d601
Compare
@sseago @shubham-pampattiwar @Lyndon-Li @dzaninovic @weshayutin Quick ping to inform folks that the design is updated based on the conversations. Please take a look and let me know what y'all think! |
I've read through this design, which looks good and the interface change pr 6608 which is also looking good. |
The current design looks good to me. |
Personally, I don't think we can use this as an optional method. It is too dangerous since all-zero provision is hard to config, detect and not all upper level scenarios direct to a new provision; and it is not a good bargain to detect zero data by per-byte enumeration. |
I reverted catalogicsoftware#2 where virtualfs.StreamingFileFromReader() was attempted because it was causing a backup failure. time="2023-08-09T21:31:47Z" level=error msg="Async fs backup data path failed" dataupload=backup1-cv4mr error="Failed to run kopia backup: Failed to upload the kopia snapshot for si default@default:block1/pvc-raw: unsupported source: default@default:block1/pvc-raw" error.file="/go/pkg/mod/github.com/kopia/[email protected]/snapshot/snapshotfs/upload.go:1291" error.function="github.com/kopia/kopia/snapshot/snapshotfs.(*Uploader).Upload" logSource="pkg/controller/data_upload_controller.go:328" By looking at the Kopia code I can see that it fails because fs.StreamingFile is not considered in the switch statement. @shawn-hurley will attempt to use this instead so if that is successful and more preferable than my original solution we will use that: In any case design will have to change to account for this change so don't merge it yet. |
Signed-off-by: Shawn Hurley <[email protected]>
ebaf316
572d601
to
ebaf316
Compare
Codecov Report
@@ Coverage Diff @@
## main #6590 +/- ##
==========================================
- Coverage 60.18% 60.17% -0.02%
==========================================
Files 242 242
Lines 25640 25679 +39
==========================================
+ Hits 15432 15452 +20
- Misses 9135 9150 +15
- Partials 1073 1077 +4 |
@Lyndon-Li @sseago @shubham-pampattiwar I have once again updated the design. We have to do some specific things during the restore to handle the block device and how things work from the node. We still use the virtualfs to save the data into the kopia repository. Please let me now what y'all think @dzaninovic My new virtualfs PR should work for restore now! |
I merged your code and confirmed that md5 of restored data is correct. |
@shawn-hurley from your statement:
It looks like these code have fixed some problems, without of which the restore will fail. I didn't get what the problems are. So if it is true, could you share some more details? |
The most significant change is that the built-in output will try and create a directory where the block device is. This fails, so we check to make sure that the file is a block device, and if it is, we skip the creation of the directory because it exists. Another change is the writing of the blocks. When using the default |
#6608 merged |
"codecov/project — 60.18% (-0.01%) compared to bb96c21" |
Thank you for contributing to Velero!
Please add a summary of your change
This will add the design elements for handling block device's. We are making a small change to the VGDP provider interface so that the data type can be based on and used by data movers.
I don't believe this is an API change that is user-facing or even plugin-facing. Can someone help me make sure?
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.