Merge pull request vmware-tanzu#4943 from phuongatemc/refactor_plugin…

…_biav1

Refactor BackupItemAction to backupitemaction/v1

changelogs/unreleased/4943-phuongatemc

@@ -0,0 +1 @@
+Refactor BackupItemAction proto and related code to backupitemaction/v1 package.  This is part of implementation of the plugin version design https://github.com/vmware-tanzu/velero/blob/main/design/plugin-versioning.md

go.mod

@@ -42,6 +42,7 @@ require (
 	golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
 	google.golang.org/api v0.74.0
 	google.golang.org/grpc v1.45.0
+	google.golang.org/protobuf v1.28.0
 	k8s.io/api v0.22.2
 	k8s.io/apiextensions-apiserver v0.22.2
 	k8s.io/apimachinery v0.22.2
@@ -133,7 +134,6 @@ require (
 	gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
 	google.golang.org/genproto v0.0.0-20220324131243-acbaeb5b85eb // indirect
-	google.golang.org/protobuf v1.28.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/ini.v1 v1.66.2 // indirect
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect

hack/build-image/Dockerfile

@@ -45,11 +45,11 @@ RUN go get golang.org/x/tools/cmd/goimports@11e9d9cc0042e6bd10337d4d2c3e5d929550
 # get protoc compiler and golang plugin
 WORKDIR /root
 RUN apt-get update && apt-get install -y unzip
-RUN wget --quiet https://github.com/protocolbuffers/protobuf/releases/download/v3.9.1/protoc-3.9.1-linux-x86_64.zip && \
-    unzip protoc-3.9.1-linux-x86_64.zip && \
+RUN wget --quiet https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-linux-x86_64.zip && \
+    unzip protoc-3.14.0-linux-x86_64.zip && \
     mv bin/protoc /usr/bin/protoc && \
     chmod +x /usr/bin/protoc
-RUN go get github.com/golang/protobuf/protoc-gen-go@v1.0.0
+RUN go get github.com/golang/protobuf/protoc-gen-go@v1.4.3
 
 # get goreleaser
 RUN wget --quiet https://github.com/goreleaser/goreleaser/releases/download/v0.120.8/goreleaser_Linux_x86_64.tar.gz && \

hack/update-proto.sh → hack/update-2proto.sh

@@ -18,6 +18,8 @@ HACK_DIR=$(dirname "${BASH_SOURCE}")
 
 echo "Updating plugin proto"
 
-protoc pkg/plugin/proto/*.proto --go_out=plugins=grpc:pkg/plugin/generated/ -I pkg/plugin/proto/
+echo protoc --version
+protoc pkg/plugin/proto/*.proto --go_out=plugins=grpc:pkg/plugin/generated/ --go_opt=module=github.com/vmware-tanzu/velero/pkg/plugin/generated -I pkg/plugin/proto/
+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/
 
 echo "Updating plugin proto - done!"

hack/verify-fmt.sh

@@ -15,4 +15,4 @@
 # limitations under the License.
 
 HACK_DIR=$(dirname "${BASH_SOURCE[0]}")
-"${HACK_DIR}"/update-fmt.sh --verify
+"${HACK_DIR}"/update-1fmt.sh --verify

hack/verify-generated-crd-code.sh

@@ -16,7 +16,7 @@
 
 HACK_DIR=$(dirname "${BASH_SOURCE}")
 
-${HACK_DIR}/update-generated-crd-code.sh
+${HACK_DIR}/update-3generated-crd-code.sh
 
 # ensure no changes to generated CRDs
 if ! git diff --exit-code config/crd/v1/crds/crds.go >/dev/null; then

hack/verify-generated-issue-template.sh

@@ -27,7 +27,7 @@ cleanup() {
 }
 
 echo "Verifying generated Github issue template"
-${HACK_DIR}/update-generated-issue-template.sh ${OUT_TMP_FILE} > /dev/null
+${HACK_DIR}/update-4generated-issue-template.sh ${OUT_TMP_FILE} > /dev/null
 output=$(echo "`diff ${ISSUE_TEMPLATE_FILE} ${OUT_TMP_FILE}`")
 
 if [[ -n "${output}" ]] ; then

pkg/backup/backup.go

@@ -45,6 +45,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/kuberesource"
 	"github.com/vmware-tanzu/velero/pkg/plugin/framework"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
+	biav1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/backupitemaction/v1"
 	"github.com/vmware-tanzu/velero/pkg/podexec"
 	"github.com/vmware-tanzu/velero/pkg/podvolume"
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
@@ -62,7 +63,7 @@ const BackupFormatVersion = "1.1.0"
 type Backupper interface {
 	// Backup takes a backup using the specification in the velerov1api.Backup and writes backup and log data
 	// to the given writers.
-	Backup(logger logrus.FieldLogger, backup *Request, backupFile io.Writer, actions []velero.BackupItemAction, volumeSnapshotterGetter VolumeSnapshotterGetter) error
+	Backup(logger logrus.FieldLogger, backup *Request, backupFile io.Writer, actions []biav1.BackupItemAction, volumeSnapshotterGetter VolumeSnapshotterGetter) error
 	BackupWithResolvers(log logrus.FieldLogger, backupRequest *Request, backupFile io.Writer,
 		backupItemActionResolver framework.BackupItemActionResolver, itemSnapshotterResolver framework.ItemSnapshotterResolver,
 		volumeSnapshotterGetter VolumeSnapshotterGetter) error
@@ -170,7 +171,7 @@ type VolumeSnapshotterGetter interface {
 // back up individual resources that don't prevent the backup from continuing to be processed) are logged
 // to the backup log.
 func (kb *kubernetesBackupper) Backup(log logrus.FieldLogger, backupRequest *Request, backupFile io.Writer,
-	actions []velero.BackupItemAction, volumeSnapshotterGetter VolumeSnapshotterGetter) error {
+	actions []biav1.BackupItemAction, volumeSnapshotterGetter VolumeSnapshotterGetter) error {
 	backupItemActions := framework.NewBackupItemActionResolver(actions)
 	itemSnapshotters := framework.NewItemSnapshotterResolver(nil)
 	return kb.BackupWithResolvers(log, backupRequest, backupFile, backupItemActions, itemSnapshotters,
@@ -206,16 +207,19 @@ func (kb *kubernetesBackupper) BackupWithResolvers(log logrus.FieldLogger,
 	var err error
 	backupRequest.ResourceHooks, err = getResourceHooks(backupRequest.Spec.Hooks.Resources, kb.discoveryHelper)
 	if err != nil {
+		log.WithError(errors.WithStack(err)).Debugf("Error from getResourceHooks")
 		return err
 	}
 
 	backupRequest.ResolvedActions, err = backupItemActionResolver.ResolveActions(kb.discoveryHelper)
 	if err != nil {
+		log.WithError(errors.WithStack(err)).Debugf("Error from backupItemActionResolver.ResolveActions")
 		return err
 	}
 
 	backupRequest.ResolvedItemSnapshotters, err = itemSnapshotterResolver.ResolveActions(kb.discoveryHelper)
 	if err != nil {
+		log.WithError(errors.WithStack(err)).Debugf("Error from itemSnapshotterResolver.ResolveActions")
 		return err
 	}
 
@@ -238,6 +242,7 @@ func (kb *kubernetesBackupper) BackupWithResolvers(log logrus.FieldLogger,
 	if kb.resticBackupperFactory != nil {
 		resticBackupper, err = kb.resticBackupperFactory.NewBackupper(ctx, backupRequest.Backup)
 		if err != nil {
+			log.WithError(errors.WithStack(err)).Debugf("Error from NewBackupper")
 			return errors.WithStack(err)
 		}
 	}

pkg/backup/backup_test.go

@@ -47,6 +47,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/discovery"
 	"github.com/vmware-tanzu/velero/pkg/kuberesource"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
+	biav1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/backupitemaction/v1"
 	"github.com/vmware-tanzu/velero/pkg/podvolume"
 	"github.com/vmware-tanzu/velero/pkg/test"
 	testutil "github.com/vmware-tanzu/velero/pkg/test"
@@ -1360,7 +1361,7 @@ func TestBackupActionsRunForCorrectItems(t *testing.T) {
 				h.addItems(t, resource)
 			}
 
-			actions := []velero.BackupItemAction{}
+			actions := []biav1.BackupItemAction{}
 			for action := range tc.actions {
 				actions = append(actions, action)
 			}
@@ -1386,7 +1387,7 @@ func TestBackupWithInvalidActions(t *testing.T) {
 		name         string
 		backup       *velerov1.Backup
 		apiResources []*test.APIResource
-		actions      []velero.BackupItemAction
+		actions      []biav1.BackupItemAction
 	}{
 		{
 			name: "action with invalid label selector results in an error",
@@ -1402,7 +1403,7 @@ func TestBackupWithInvalidActions(t *testing.T) {
 					builder.ForPersistentVolume("baz").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				new(recordResourcesAction).ForLabelSelector("=invalid-selector"),
 			},
 		},
@@ -1420,7 +1421,7 @@ func TestBackupWithInvalidActions(t *testing.T) {
 					builder.ForPersistentVolume("baz").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&appliesToErrorAction{},
 			},
 		},
@@ -1482,7 +1483,7 @@ func TestBackupActionModifications(t *testing.T) {
 		name         string
 		backup       *velerov1.Backup
 		apiResources []*test.APIResource
-		actions      []velero.BackupItemAction
+		actions      []biav1.BackupItemAction
 		want         map[string]unstructuredObject
 	}{
 		{
@@ -1493,7 +1494,7 @@ func TestBackupActionModifications(t *testing.T) {
 					builder.ForPod("ns-1", "pod-1").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				modifyingActionGetter(func(item *unstructured.Unstructured) {
 					item.SetLabels(map[string]string{"updated": "true"})
 				}),
@@ -1510,7 +1511,7 @@ func TestBackupActionModifications(t *testing.T) {
 					builder.ForPod("ns-1", "pod-1").ObjectMeta(builder.WithLabels("should-be-removed", "true")).Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				modifyingActionGetter(func(item *unstructured.Unstructured) {
 					item.SetLabels(nil)
 				}),
@@ -1527,7 +1528,7 @@ func TestBackupActionModifications(t *testing.T) {
 					builder.ForPod("ns-1", "pod-1").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				modifyingActionGetter(func(item *unstructured.Unstructured) {
 					item.Object["spec"].(map[string]interface{})["nodeName"] = "foo"
 				}),
@@ -1545,7 +1546,7 @@ func TestBackupActionModifications(t *testing.T) {
 					builder.ForPod("ns-1", "pod-1").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				modifyingActionGetter(func(item *unstructured.Unstructured) {
 					item.SetName(item.GetName() + "-updated")
 					item.SetNamespace(item.GetNamespace() + "-updated")
@@ -1586,7 +1587,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 		name         string
 		backup       *velerov1.Backup
 		apiResources []*test.APIResource
-		actions      []velero.BackupItemAction
+		actions      []biav1.BackupItemAction
 		want         []string
 	}{
 		{
@@ -1599,7 +1600,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 					builder.ForPod("ns-3", "pod-3").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&pluggableAction{
 					selector: velero.ResourceSelector{IncludedNamespaces: []string{"ns-1"}},
 					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
@@ -1631,7 +1632,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 					builder.ForPod("ns-3", "pod-3").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&pluggableAction{
 					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
 						additionalItems := []velero.ResourceIdentifier{
@@ -1661,7 +1662,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 					builder.ForPersistentVolume("pv-2").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&pluggableAction{
 					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
 						additionalItems := []velero.ResourceIdentifier{
@@ -1694,7 +1695,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 					builder.ForPersistentVolume("pv-2").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&pluggableAction{
 					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
 						additionalItems := []velero.ResourceIdentifier{
@@ -1724,7 +1725,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 					builder.ForPersistentVolume("pv-2").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&pluggableAction{
 					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
 						additionalItems := []velero.ResourceIdentifier{
@@ -1755,7 +1756,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 					builder.ForPersistentVolume("pv-2").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&pluggableAction{
 					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
 						additionalItems := []velero.ResourceIdentifier{
@@ -1785,7 +1786,7 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 					builder.ForPod("ns-3", "pod-3").Result(),
 				),
 			},
-			actions: []velero.BackupItemAction{
+			actions: []biav1.BackupItemAction{
 				&pluggableAction{
 					selector: velero.ResourceSelector{IncludedNamespaces: []string{"ns-1"}},
 					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {

pkg/controller/backup_controller_test.go

@@ -49,7 +49,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
 	"github.com/vmware-tanzu/velero/pkg/plugin/framework"
 	pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks"
-	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
+	biav1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/backupitemaction/v1"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	"github.com/vmware-tanzu/velero/pkg/util/logging"
@@ -59,7 +59,7 @@ type fakeBackupper struct {
 	mock.Mock
 }
 
-func (b *fakeBackupper) Backup(logger logrus.FieldLogger, backup *pkgbackup.Request, backupFile io.Writer, actions []velero.BackupItemAction, volumeSnapshotterGetter pkgbackup.VolumeSnapshotterGetter) error {
+func (b *fakeBackupper) Backup(logger logrus.FieldLogger, backup *pkgbackup.Request, backupFile io.Writer, actions []biav1.BackupItemAction, volumeSnapshotterGetter pkgbackup.VolumeSnapshotterGetter) error {
 	args := b.Called(logger, backup, backupFile, actions, volumeSnapshotterGetter)
 	return args.Error(0)
 }
@@ -843,7 +843,7 @@ func TestProcessBackupCompletions(t *testing.T) {
 			pluginManager.On("GetBackupItemActions").Return(nil, nil)
 			pluginManager.On("CleanupClients").Return(nil)
 			pluginManager.On("GetItemSnapshotters").Return(nil, nil)
-			backupper.On("Backup", mock.Anything, mock.Anything, mock.Anything, []velero.BackupItemAction(nil), pluginManager).Return(nil)
+			backupper.On("Backup", mock.Anything, mock.Anything, mock.Anything, []biav1.BackupItemAction(nil), pluginManager).Return(nil)
 			backupper.On("BackupWithResolvers", mock.Anything, mock.Anything, mock.Anything, framework.BackupItemActionResolver{}, framework.ItemSnapshotterResolver{}, pluginManager).Return(nil)
 			backupStore.On("BackupExists", test.backupLocation.Spec.StorageType.ObjectStorage.Bucket, test.backup.Name).Return(test.backupExists, test.existenceCheckError)
 

pkg/plugin/clientmgmt/manager.go

@@ -20,12 +20,12 @@ import (
 	"strings"
 	"sync"
 
-	v1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/item_snapshotter/v1"
-
 	"github.com/sirupsen/logrus"
 
 	"github.com/vmware-tanzu/velero/pkg/plugin/framework"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
+	biav1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/backupitemaction/v1"
+	isv1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/item_snapshotter/v1"
 )
 
 // Manager manages the lifecycles of plugins.
@@ -37,10 +37,10 @@ type Manager interface {
 	GetVolumeSnapshotter(name string) (velero.VolumeSnapshotter, error)
 
 	// GetBackupItemActions returns all backup item action plugins.
-	GetBackupItemActions() ([]velero.BackupItemAction, error)
+	GetBackupItemActions() ([]biav1.BackupItemAction, error)
 
 	// GetBackupItemAction returns the backup item action plugin for name.
-	GetBackupItemAction(name string) (velero.BackupItemAction, error)
+	GetBackupItemAction(name string) (biav1.BackupItemAction, error)
 
 	// GetRestoreItemActions returns all restore item action plugins.
 	GetRestoreItemActions() ([]velero.RestoreItemAction, error)
@@ -55,10 +55,10 @@ type Manager interface {
 	GetDeleteItemAction(name string) (velero.DeleteItemAction, error)
 
 	// GetItemSnapshotter returns the item snapshotter plugin for name
-	GetItemSnapshotter(name string) (v1.ItemSnapshotter, error)
+	GetItemSnapshotter(name string) (isv1.ItemSnapshotter, error)
 
 	// GetItemSnapshotters returns all item snapshotter plugins
-	GetItemSnapshotters() ([]v1.ItemSnapshotter, error)
+	GetItemSnapshotters() ([]isv1.ItemSnapshotter, error)
 
 	// CleanupClients terminates all of the Manager's running plugin processes.
 	CleanupClients()
@@ -166,10 +166,10 @@ func (m *manager) GetVolumeSnapshotter(name string) (velero.VolumeSnapshotter, e
 }
 
 // GetBackupItemActions returns all backup item actions as restartableBackupItemActions.
-func (m *manager) GetBackupItemActions() ([]velero.BackupItemAction, error) {
+func (m *manager) GetBackupItemActions() ([]biav1.BackupItemAction, error) {
 	list := m.registry.List(framework.PluginKindBackupItemAction)
 
-	actions := make([]velero.BackupItemAction, 0, len(list))
+	actions := make([]biav1.BackupItemAction, 0, len(list))
 
 	for i := range list {
 		id := list[i]
@@ -186,7 +186,7 @@ func (m *manager) GetBackupItemActions() ([]velero.BackupItemAction, error) {
 }
 
 // GetBackupItemAction returns a restartableBackupItemAction for name.
-func (m *manager) GetBackupItemAction(name string) (velero.BackupItemAction, error) {
+func (m *manager) GetBackupItemAction(name string) (biav1.BackupItemAction, error) {
 	name = sanitizeName(name)
 
 	restartableProcess, err := m.getRestartableProcess(framework.PluginKindBackupItemAction, name)
@@ -264,7 +264,7 @@ func (m *manager) GetDeleteItemAction(name string) (velero.DeleteItemAction, err
 	return r, nil
 }
 
-func (m *manager) GetItemSnapshotter(name string) (v1.ItemSnapshotter, error) {
+func (m *manager) GetItemSnapshotter(name string) (isv1.ItemSnapshotter, error) {
 	name = sanitizeName(name)
 
 	restartableProcess, err := m.getRestartableProcess(framework.PluginKindItemSnapshotter, name)
@@ -276,10 +276,10 @@ func (m *manager) GetItemSnapshotter(name string) (v1.ItemSnapshotter, error) {
 	return r, nil
 }
 
-func (m *manager) GetItemSnapshotters() ([]v1.ItemSnapshotter, error) {
+func (m *manager) GetItemSnapshotters() ([]isv1.ItemSnapshotter, error) {
 	list := m.registry.List(framework.PluginKindItemSnapshotter)
 
-	actions := make([]v1.ItemSnapshotter, 0, len(list))
+	actions := make([]isv1.ItemSnapshotter, 0, len(list))
 
 	for i := range list {
 		id := list[i]