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

Refactor BackupItemAction to backupitemaction/v1 #4943

Merged

Conversation

phuongatemc
Copy link
Contributor

Signed-off-by: Hoang, Phuong [email protected]

Thank you for contributing to Velero!

Please add a summary of your change

Refactor BackupItemAction proto to backupitemaction/v1 and related code.

Does your change fix a particular issue?

This is part of the implementation for Plugin Versioning Design https://github.com/vmware-tanzu/velero/blob/main/design/plugin-versioning.md

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.

@github-actions github-actions bot requested review from dsu-igeek and sseago May 27, 2022 21:22
@blackpiglet blackpiglet requested review from blackpiglet and removed request for dsu-igeek June 16, 2022 11:28
@reasonerjt reasonerjt requested review from reasonerjt and ywk253100 and removed request for blackpiglet June 24, 2022 02:34
hack/update-proto.sh Outdated Show resolved Hide resolved
@@ -1,6 +1,71 @@
// Code generated by protoc-gen-go. DO NOT EDIT.
// source: DeleteItemAction.proto

/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using a different version of protoc with https://github.com/vmware-tanzu/velero/blob/main/hack/build-image/Dockerfile#L48 as there are some unexpected changes?
Please also update the version if the answer is yes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ywk253100 I just regenerated these via make update and assuming the build-image dockerfile is the one used, the DeleteItemAction changes are the same for me as above. Perhaps it was something to do with the way the file was originally generated? Checking git history, the other generated plugin files have been modified more recently than this one. Perhaps something changed in a dependency somewhere along the way, or perhaps this particular file was generated manually instead of via make update? In any case, running make update right now on DeleteItemAction does result in these changes. I even removed my local protoc file just in case it was being used instead of the one in the Docker image, but the changes still came through.

@@ -18,6 +18,9 @@ HACK_DIR=$(dirname "${BASH_SOURCE}")

echo "Updating plugin proto"

protoc pkg/plugin/proto/backupitemaction/v1/*.proto --go_out=plugins=grpc:pkg/plugin/generated/ -I pkg/plugin/proto/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured out how to get the output files to generate properly. Replace the above line with this:

protoc pkg/plugin/proto/backupitemaction/v1/*.proto --go_out=plugins=grpc:pkg/plugin/generated/ --go_opt=module=github.com/vmware-tanzu/velero/pkg/plugin/generated -I pkg/plugin/proto/

The required addition is --go_opt=module=github.com/vmware-tanzu/velero/pkg/plugin/generated which tells protoc-gen-go to strip the module prefix from the go_package because we're already under the module root dir.

@@ -18,6 +18,9 @@ HACK_DIR=$(dirname "${BASH_SOURCE}")

echo "Updating plugin proto"

protoc pkg/plugin/proto/backupitemaction/v1/*.proto --go_out=plugins=grpc:pkg/plugin/generated/ -I pkg/plugin/proto/
protoc pkg/plugin/proto/*.proto --go_out=plugins=grpc:pkg/plugin/generated/ -I pkg/plugin/proto/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the above BIA code to generate properly locally, I had to add go_package to Shared.proto. For this protoc command to work on the other top level unversioned files, I needed that as well. Once added, the this protoc line needs the same --go_opt=module=github.com/vmware-tanzu/velero/pkg/plugin/generated as the first one. The required go_package line to all files in pkg/plugin/generated is this:

option go_package = "github.com/vmware-tanzu/velero/pkg/plugin/generated";

protoc pkg/plugin/proto/*.proto --go_out=plugins=grpc:pkg/plugin/generated/ -I pkg/plugin/proto/
cp -rf pkg/plugin/generated/github.com/vmware-tanzu/velero/pkg/plugin/generated/* pkg/plugin/generated
rm -rf pkg/plugin/generated/github.com

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the above changes, the cp and rm lines can be removed.

@blackpiglet
Copy link
Contributor

Verified on my environment according to @sseago comments.

Need to --go_opt=module=github.com/vmware-tanzu/velero/pkg/plugin/generated in protoc command, and remove option go_package = "github.com/vmware-tanzu/velero/pkg/plugin/generated"; from proto files.

Hoang, Phuong added 3 commits August 25, 2022 17:10
Signed-off-by: Hoang, Phuong <[email protected]>
Signed-off-by: Hoang, Phuong <[email protected]>
@sseago sseago force-pushed the refactor_plugin_biav1 branch from b567413 to b483093 Compare August 25, 2022 21:12
@sseago
Copy link
Collaborator

sseago commented Aug 25, 2022

Rebased to main to resolve conflicts, and fixed the hack/update-*.sh files to make sure protoc generation happens before crd generation.

@sseago sseago force-pushed the refactor_plugin_biav1 branch from b483093 to 50e3fde Compare August 25, 2022 21:18
If generating protoc go files from scratch, `make update` fails if
CRD generation happens first, since the protoc-generated
files are imported by the api go files.
protoc generation needs to happen earlier.

Signed-off-by: Scott Seago <[email protected]>
@sseago sseago force-pushed the refactor_plugin_biav1 branch from 50e3fde to 6b83530 Compare August 25, 2022 21:23
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2022

Codecov Report

Merging #4943 (5a5a4c1) into main (e849441) will not change coverage.
The diff coverage is 44.00%.

@@           Coverage Diff           @@
##             main    #4943   +/-   ##
=======================================
  Coverage   41.03%   41.03%           
=======================================
  Files         231      231           
  Lines       19645    19645           
=======================================
  Hits         8062     8062           
  Misses      10993    10993           
  Partials      590      590           
Impacted Files Coverage Δ
pkg/plugin/framework/action_resolver.go 6.89% <0.00%> (ø)
pkg/plugin/framework/backup_item_action.go 0.00% <0.00%> (ø)
pkg/plugin/framework/backup_item_action_client.go 0.00% <0.00%> (ø)
pkg/plugin/framework/delete_item_action_server.go 0.00% <0.00%> (ø)
pkg/plugin/framework/item_snapshotter_client.go 0.00% <ø> (ø)
pkg/plugin/framework/item_snapshotter_server.go 0.00% <0.00%> (ø)
pkg/plugin/framework/restore_item_action_server.go 0.00% <0.00%> (ø)
pkg/plugin/clientmgmt/manager.go 81.32% <50.00%> (ø)
pkg/plugin/framework/backup_item_action_server.go 51.94% <57.14%> (ø)
pkg/backup/backup.go 79.49% <100.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sseago
Copy link
Collaborator

sseago commented Aug 25, 2022

Updated to newer protobug/protoc-gen-go since the module option isn't supported in the ancient 1.0.0 protoc-gen-go we're currently using. This is breaking tests now, so I'll need to fix some things now. Looks like some changes to generated code may need to be reflected in the velero code that references it.

@sseago sseago force-pushed the refactor_plugin_biav1 branch from 0000706 to 5a5a4c1 Compare August 26, 2022 01:28
@github-actions github-actions bot added the Dependencies Pull requests that update a dependency file label Aug 26, 2022
@sseago
Copy link
Collaborator

sseago commented Aug 29, 2022

I have tested this PR with our existing OADP/OpenShift backup item action plugins. The only thing I had to do for this to work was to rebuild the image with the velero deps updated to include the code changes in this PR.

Using the new code with an old plugin image (based on incompatible velero dependencies) will result in an error like the below when trying to run a backup:

time="2022-08-29T18:53:11Z" level=error msg="backup failed" controller=backup error="rpc error: code = Unimplemented desc = unknown service v1.BackupItemAction" key=openshift-adp/bia-test logSource="pkg/controller/backup_controller.go:301"

Signed-off-by: Scott Seago <[email protected]>
@sseago sseago force-pushed the refactor_plugin_biav1 branch from 9c4026d to 91ac570 Compare August 29, 2022 20:55
@shubham-pampattiwar shubham-pampattiwar merged commit 94a9a7c into vmware-tanzu:main Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Pull requests that update a dependency file has-changelog has-unit-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants