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

Feature/add provider interface for block mode #6608

Conversation

shawn-hurley
Copy link
Contributor

@shawn-hurley shawn-hurley commented Aug 4, 2023

Thank you for contributing to Velero!

Please add a summary of your change

Adding the block mode to the interfaces, to be included in 1.12.

This will allow us to solve the issue of block support in a Z-stream without breaking the API's in a Z-stream.

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch 3 times, most recently from 9fa8dc6 to 4e3f1ed Compare August 4, 2023 17:48
@weshayutin
Copy link
Contributor

Thank you @shawn-hurley! @draghuram @dzaninovic FYI :)

@dzaninovic
Copy link
Contributor

I will refactor my code to adjust for these changes.

@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch from 4e3f1ed to 405b70a Compare August 7, 2023 15:17
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2023

Codecov Report

Merging #6608 (284f7c8) into main (411bd54) will decrease coverage by 0.16%.
The diff coverage is 78.57%.

❗ Current head 284f7c8 differs from pull request most recent head 563a16c. Consider uploading reports for the commit 563a16c to get more accurate results

@@            Coverage Diff             @@
##             main    #6608      +/-   ##
==========================================
- Coverage   60.31%   60.16%   -0.16%     
==========================================
  Files         242      242              
  Lines       25924    25735     -189     
==========================================
- Hits        15637    15484     -153     
+ Misses       9186     9167      -19     
+ Partials     1101     1084      -17     
Files Changed Coverage Δ
pkg/uploader/provider/provider.go 100.00% <ø> (ø)
pkg/uploader/types.go 100.00% <ø> (ø)
pkg/datapath/file_system.go 31.09% <40.00%> (-0.44%) ⬇️
pkg/uploader/kopia/snapshot.go 80.00% <100.00%> (+0.28%) ⬆️
pkg/uploader/provider/kopia.go 85.71% <100.00%> (+0.68%) ⬆️
pkg/uploader/provider/restic.go 80.55% <100.00%> (+1.14%) ⬆️

... and 5 files with indirect coverage changes

sseago
sseago previously approved these changes Aug 7, 2023
@sseago
Copy link
Collaborator

sseago commented Aug 7, 2023

Note for other maintainers/reviewers: This PR just implements the API change, adding volMode to RunBackup(), but errors out as unsupported if Block is specified. The hope is to get the API change into 1.12 before anyone is actually using this API, but then we can add the kopia implementation bits (Restic will continue to error out on block mode) in a future release. Making the API change later increases the likelihood that it will break someone's implementation of it, but at the moment there are no implementations of this API outside of Velero core code.

@dzaninovic
Copy link
Contributor

@Lyndon-Li
Copy link
Contributor

Per my understanding, the changes in this PR won't take effect under the current code context.

As you can see from the controller and the exposer , the snapshot volume mode has been hard-coded to FileSystem, and the AccessPoint has been hard-coded to ByPath. So with the current code changes only, the data path won't call the uploader with PersistentVolumeBlock as the volMode.
I am not sure there is anywhere else in the current code that conflicts with the block mode volumes/snapshots. This has never been tested.

If so, what is the purpose to have this separate PR, why don't we submit the entire functioning code in one PR later when they are ready?
If we want to block data mover from processing block mode PVCs (so that we could get a clearer error message), I think we should check it before taking snapshot or before submitting DataUpload CR. cc @blackpiglet

@sseago
Copy link
Collaborator

sseago commented Aug 9, 2023

@Lyndon-Li "If so, what is the purpose to have this separate PR, why don't we submit the entire functioning code in one PR later when they are ready?"

One reason is that this introduces an API change to RunBackup and RunRestore, so it's better to introduce the API change in 1.12 now (before anyone is using it). Ideally we wouldn't have API changes in 1.12.x, when we're ready to submit the block-mode functionality, and while it may be unlikely that anyone outside of velero core will be using these APIs by then, it's not impossible. It's probably better to add it now (even though there is no new functionality associated with volMode=block) rather than in a patch release.

@shawn-hurley
Copy link
Contributor Author

If so, what is the purpose to have this separate PR, why don't we submit the entire functioning code in one PR later when they are ready?

The biggest reason is that we want the API changes for the 1.12.0 release. This will make sure that we don't add a breaking API change for folks who are attempting to use the VGDP (I think that is the right acronym that I have been reading?) Please let me know if this makes sense.

Also note, I will add code around when ByPath or ByBlock should be set.

@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch 2 times, most recently from d58a9fb to e72216b Compare August 9, 2023 18:43
@weshayutin
Copy link
Contributor

Thanks for the progress!

@Lyndon-Li
Copy link
Contributor

@shawn-hurley Please help to fix the CI errors. Thanks!

@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch from e72216b to 284f7c8 Compare August 10, 2023 13:09
@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch from 284f7c8 to 7695423 Compare August 15, 2023 18:50
@shawn-hurley
Copy link
Contributor Author

@Lyndon-Li @sseago @shubham-pampattiwar @mateusoliveira43

To get in, just pinging folks for awareness that I have updated and it should be ready to go!

@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch from 7695423 to f4ccf40 Compare August 15, 2023 19:12
@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch from 420a44e to af4f05a Compare August 15, 2023 19:18
@shubham-pampattiwar
Copy link
Collaborator

@shawn-hurley please squash all the commits and/or remove the design commit.

@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch from af4f05a to 4805140 Compare August 15, 2023 19:32
Signed-off-by: Shawn Hurley <shawn@hurley.page>
@shawn-hurley shawn-hurley force-pushed the feature/add-provider-interface-blockmod branch from 4805140 to 563a16c Compare August 15, 2023 19:33
@shawn-hurley shawn-hurley changed the title Feature/add provider interface blockmod Feature/add provider interface for block mode Aug 16, 2023
@reasonerjt reasonerjt merged commit 0b30adb into vmware-tanzu:main Aug 16, 2023
24 checks passed
@dzaninovic dzaninovic mentioned this pull request Aug 18, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants