bugfixes for BIAv2 controller work

Signed-off-by: Scott Seago <[email protected]>

design/biav2-design.md

@@ -34,6 +34,7 @@ message ExecuteResponse {
     bytes item = 1;
     repeated generated.ResourceIdentifier additionalItems = 2;
     string operationID = 3;
+    repeated generated.ResourceIdentifier itemsToUpdate = 4;
 }
 ```
 The BackupItemAction service gets two new rpc methods:
@@ -78,6 +79,19 @@ message OperationProgress {
 }
 ```
 
+In addition to the two new rpc methods added to the BackupItemAction interface, there is also a new `Name()` method. This one is only actually used internally by Velero to get the name that the plugin was registered with, but it still must be defined in a plugin which implements BackupItemActionV2 in order to implement the interface. It doesn't really matter what it returns, though, as this particular method is not delegated to the plugin via RPC calls. The new (and modified) interface methods for `BackupItemAction` are as follows:
+```
+type BackupItemAction interface {
+...
+	Name() string
+...
+	Execute(item runtime.Unstructured, backup *api.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, string, []velero.ResourceIdentifier, error)
+	Progress(operationID string, backup *api.Backup) (velero.OperationProgress, error)
+	Cancel(operationID string, backup *api.Backup) error
+...
+}
+```
+
 A new PluginKind, `BackupItemActionV2`, will be created, and the backup process will be modified to use this plugin kind.
 See [Plugin Versioning](plugin-versioning.md) for more details on implementation plans, including v1 adapters, etc.
 

pkg/controller/async_backup_operations_controller.go

@@ -30,8 +30,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/event"
-	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/itemoperation"
@@ -168,17 +166,7 @@ func NewAsyncBackupOperationsReconciler(
 func (c *asyncBackupOperationsReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	s := kube.NewPeriodicalEnqueueSource(c.logger, mgr.GetClient(), &velerov1api.BackupList{}, c.frequency, kube.PeriodicalEnqueueSourceOption{})
 	return ctrl.NewControllerManagedBy(mgr).
-		For(&velerov1api.Backup{}, builder.WithPredicates(predicate.Funcs{
-			UpdateFunc: func(ue event.UpdateEvent) bool {
-				return false
-			},
-			DeleteFunc: func(de event.DeleteEvent) bool {
-				return false
-			},
-			GenericFunc: func(ge event.GenericEvent) bool {
-				return false
-			},
-		})).
+		For(&velerov1api.Backup{}, builder.WithPredicates(kube.FalsePredicate{})).
 		Watches(s, nil).
 		Complete(c)
 }

pkg/controller/backup_controller.go

@@ -766,18 +766,14 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error {
 	case logCounter.GetCount(logrus.ErrorLevel) > 0:
 		if inProgressOperations {
 			backup.Status.Phase = velerov1api.BackupPhaseWaitingForPluginOperationsPartiallyFailed
-		} else if backup.Status.AsyncBackupItemOperationsAttempted > 0 {
-			backup.Status.Phase = velerov1api.BackupPhaseFinalizingAfterPluginOperationsPartiallyFailed
 		} else {
-			backup.Status.Phase = velerov1api.BackupPhasePartiallyFailed
+			backup.Status.Phase = velerov1api.BackupPhaseFinalizingAfterPluginOperationsPartiallyFailed
 		}
 	default:
 		if inProgressOperations {
 			backup.Status.Phase = velerov1api.BackupPhaseWaitingForPluginOperations
-		} else if backup.Status.AsyncBackupItemOperationsAttempted > 0 {
-			backup.Status.Phase = velerov1api.BackupPhaseFinalizingAfterPluginOperations
 		} else {
-			backup.Status.Phase = velerov1api.BackupPhaseCompleted
+			backup.Status.Phase = velerov1api.BackupPhaseFinalizingAfterPluginOperations
 		}
 	}
 	// Mark completion timestamp before serializing and uploading.

pkg/controller/backup_controller_test.go

@@ -605,7 +605,7 @@ func TestProcessBackupCompletions(t *testing.T) {
 		backupExists             bool
 		existenceCheckError      error
 	}{
-		// Completed
+		// FinalizingAfterPluginOperations
 		{
 			name:                     "backup with no backup location gets the default",
 			backup:                   defaultBackup().Result(),
@@ -633,12 +633,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.True(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -669,12 +668,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.False(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -708,12 +706,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.True(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -745,12 +742,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.False(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					Expiration:          &metav1.Time{now.Add(10 * time.Minute)},
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					Expiration:     &metav1.Time{now.Add(10 * time.Minute)},
+					StartTimestamp: &timestamp,
 				},
 			},
 		},
@@ -782,12 +778,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.True(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -820,12 +815,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.False(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -858,12 +852,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.True(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -896,12 +889,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.True(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -934,12 +926,11 @@ func TestProcessBackupCompletions(t *testing.T) {
 					DefaultVolumesToFsBackup: boolptr.False(),
 				},
 				Status: velerov1api.BackupStatus{
-					Phase:               velerov1api.BackupPhaseCompleted,
-					Version:             1,
-					FormatVersion:       "1.1.0",
-					StartTimestamp:      &timestamp,
-					CompletionTimestamp: &timestamp,
-					Expiration:          &timestamp,
+					Phase:          velerov1api.BackupPhaseFinalizingAfterPluginOperations,
+					Version:        1,
+					FormatVersion:  "1.1.0",
+					StartTimestamp: &timestamp,
+					Expiration:     &timestamp,
 				},
 			},
 		},
@@ -1087,13 +1078,16 @@ func TestProcessBackupCompletions(t *testing.T) {
 
 			// Ensure we have a CompletionTimestamp when uploading and that the backup name matches the backup in the object store.
 			// Failures will display the bytes in buf.
-			hasNameAndCompletionTimestamp := func(info persistence.BackupInfo) bool {
+			hasNameAndCompletionTimestampIfCompleted := func(info persistence.BackupInfo) bool {
 				buf := new(bytes.Buffer)
 				buf.ReadFrom(info.Metadata)
 				return info.Name == test.backup.Name &&
-					strings.Contains(buf.String(), `"completionTimestamp": "2006-01-02T22:04:05Z"`)
+					(!(strings.Contains(buf.String(), `"phase": "Completed"`) ||
+						strings.Contains(buf.String(), `"phase": "Failed"`) ||
+						strings.Contains(buf.String(), `"phase": "PartiallyFailed"`)) ||
+						strings.Contains(buf.String(), `"completionTimestamp": "2006-01-02T22:04:05Z"`))
 			}
-			backupStore.On("PutBackup", mock.MatchedBy(hasNameAndCompletionTimestamp)).Return(nil)
+			backupStore.On("PutBackup", mock.MatchedBy(hasNameAndCompletionTimestampIfCompleted)).Return(nil)
 
 			// add the test's backup to the informer/lister store
 			require.NotNil(t, test.backup)

pkg/plugin/velero/backupitemaction/v2/backup_item_action.go

@@ -29,7 +29,10 @@ import (
 
 // BackupItemAction is an actor that performs an operation on an individual item being backed up.
 type BackupItemAction interface {
-	// Name returns the name of this BIA
+	// Name returns the name of this BIA. Plugins which implement this interface must defined Name,
+	// but its content is unimportant, as it won't actually be called via RPC. Velero's plugin infrastructure
+	// will implement this directly rather than delegating to the RPC plugin in order to return the name
+	// that the plugin was registered under. The plugins must implement the method to complete the interface.
 	Name() string
 
 	// AppliesTo returns information about which resources this action should be invoked for.

site/content/docs/main/api-types/backup.md

@@ -33,6 +33,10 @@ spec:
   # CSI VolumeSnapshot status turns to ReadyToUse during creation, before
   # returning error as timeout. The default value is 10 minute.
   csiSnapshotTimeout: 10m
+  # ItemOperationTimeout specifies the time used to wait for
+  # asynchronous BackupItemAction operations
+  # The default value is 1 hour.
+  csiSnapshotTimeout: 1h
   # Array of namespaces to include in the backup. If unspecified, all namespaces are included.
   # Optional.
   includedNamespaces:
@@ -146,7 +150,8 @@ status:
   expiration: null
   # The current phase.
   # Valid values are New, FailedValidation, InProgress, WaitingForPluginOperations,
-  # WaitingForPluginOperationsPartiallyFailed, Completed, PartiallyFailed, Failed.
+  # WaitingForPluginOperationsPartiallyFailed, FinalizingafterPluginOperations,
+  # FinalizingAfterPluginOperationsPartiallyFailed, Completed, PartiallyFailed, Failed.
   phase: ""
   # An array of any validation errors encountered.
   validationErrors: null
@@ -158,6 +163,12 @@ status:
   volumeSnapshotsAttempted: 2
   # Number of volume snapshots that Velero successfully created for this backup.
   volumeSnapshotsCompleted: 1
+  # Number of attempted async BackupItemAction operations for this backup.
+  asyncBackupIemOperationsAttempted: 2
+  # Number of async BackupItemAction operations that Velero successfully completed for this backup.
+  asyncBackupIemOperationsCompleted: 1
+  # Number of async BackupItemAction operations that ended in failure for this backup.
+  asyncBackupIemOperationsFailed: 0
   # Number of warnings that were logged by the backup.
   warnings: 2
   # Number of errors that were logged by the backup.