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

@@ -26,12 +26,10 @@ import (
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/clock"
+	clocks "k8s.io/utils/clock"
 	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"
@@ -133,7 +131,7 @@ func (m *BackupItemOperationsMap) UpdateForBackup(backupStore persistence.Backup
 type asyncBackupOperationsReconciler struct {
 	client.Client
 	logger            logrus.FieldLogger
-	clock             clock.Clock
+	clock             clocks.WithTickerAndDelayedExecution
 	frequency         time.Duration
 	itemOperationsMap *BackupItemOperationsMap
 	newPluginManager  func(logger logrus.FieldLogger) clientmgmt.Manager
@@ -152,7 +150,7 @@ func NewAsyncBackupOperationsReconciler(
 	abor := &asyncBackupOperationsReconciler{
 		Client:            client,
 		logger:            logger,
-		clock:             clock.RealClock{},
+		clock:             clocks.RealClock{},
 		frequency:         frequency,
 		itemOperationsMap: &BackupItemOperationsMap{operations: make(map[string]*operationsForBackup)},
 		newPluginManager:  newPluginManager,
@@ -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/async_backup_operations_controller_test.go

@@ -28,7 +28,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/clock"
+	testclocks "k8s.io/utils/clock/testing"
 	ctrl "sigs.k8s.io/controller-runtime"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -51,7 +51,7 @@ var (
 	bia           = &biav2mocks.BackupItemAction{}
 )
 
-func mockAsyncBackupOperationsReconciler(fakeClient kbclient.Client, fakeClock *clock.FakeClock, freq time.Duration) (*asyncBackupOperationsReconciler, *BackupItemOperationsMap) {
+func mockAsyncBackupOperationsReconciler(fakeClient kbclient.Client, fakeClock *testclocks.FakeClock, freq time.Duration) (*asyncBackupOperationsReconciler, *BackupItemOperationsMap) {
 	abor, biaMap := NewAsyncBackupOperationsReconciler(
 		logrus.StandardLogger(),
 		fakeClient,
@@ -65,7 +65,7 @@ func mockAsyncBackupOperationsReconciler(fakeClient kbclient.Client, fakeClock *
 }
 
 func TestAsyncBackupOperationsReconcile(t *testing.T) {
-	fakeClock := clock.NewFakeClock(time.Now())
+	fakeClock := testclocks.NewFakeClock(time.Now())
 	metav1Now := metav1.NewTime(fakeClock.Now())
 
 	defaultBackupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "default").Result()

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/controller/backup_finalizer_controller.go

@@ -26,7 +26,7 @@ import (
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/clock"
+	clocks "k8s.io/utils/clock"
 	ctrl "sigs.k8s.io/controller-runtime"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -42,7 +42,7 @@ import (
 // backupFinalizerReconciler reconciles a Backup object
 type backupFinalizerReconciler struct {
 	client            kbclient.Client
-	clock             clock.Clock
+	clock             clocks.WithTickerAndDelayedExecution
 	backupper         pkgbackup.Backupper
 	newPluginManager  func(logrus.FieldLogger) clientmgmt.Manager
 	metrics           *metrics.ServerMetrics
@@ -53,7 +53,7 @@ type backupFinalizerReconciler struct {
 // NewBackupFinalizerReconciler initializes and returns backupFinalizerReconciler struct.
 func NewBackupFinalizerReconciler(
 	client kbclient.Client,
-	clock clock.Clock,
+	clock clocks.WithTickerAndDelayedExecution,
 	backupper pkgbackup.Backupper,
 	newPluginManager func(logrus.FieldLogger) clientmgmt.Manager,
 	backupStoreGetter persistence.ObjectBackupStoreGetter,

pkg/controller/backup_finalizer_controller_test.go

@@ -30,7 +30,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/clock"
+	testclocks "k8s.io/utils/clock/testing"
 	ctrl "sigs.k8s.io/controller-runtime"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -45,7 +45,7 @@ import (
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 )
 
-func mockBackupFinalizerReconciler(fakeClient kbclient.Client, fakeClock *clock.FakeClock) (*backupFinalizerReconciler, *fakeBackupper) {
+func mockBackupFinalizerReconciler(fakeClient kbclient.Client, fakeClock *testclocks.FakeClock) (*backupFinalizerReconciler, *fakeBackupper) {
 	backupper := new(fakeBackupper)
 	return NewBackupFinalizerReconciler(
 		fakeClient,
@@ -58,7 +58,7 @@ func mockBackupFinalizerReconciler(fakeClient kbclient.Client, fakeClock *clock.
 	), backupper
 }
 func TestBackupFinalizerReconcile(t *testing.T) {
-	fakeClock := clock.NewFakeClock(time.Now())
+	fakeClock := testclocks.NewFakeClock(time.Now())
 	metav1Now := metav1.NewTime(fakeClock.Now())
 
 	defaultBackupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "default").Result()

pkg/controller/backup_sync_controller_test.go

@@ -30,10 +30,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/clock"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
 	core "k8s.io/client-go/testing"
+	testclocks "k8s.io/utils/clock/testing"
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlClient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -157,7 +157,7 @@ func numBackups(c ctrlClient.WithWatch, ns string) (int, error) {
 
 var _ = Describe("Backup Sync Reconciler", func() {
 	It("Test Backup Sync Reconciler basic function", func() {
-		fakeClock := clock.NewFakeClock(time.Now())
+		fakeClock := testclocks.NewFakeClock(time.Now())
 		type cloudBackupData struct {
 			backup               *velerov1api.Backup
 			podVolumeBackups     []*velerov1api.PodVolumeBackup