Replace old lister, update status using r.Client.Update, fix restic c…

…reds file

Signed-off-by: F. Gold <fgold@vmware.com>

go.mod

@@ -19,7 +19,7 @@ require (
 	github.com/hashicorp/go-plugin v0.0.0-20190610192547-a1bc61569a26
 	github.com/joho/godotenv v1.3.0
 	github.com/kubernetes-csi/external-snapshotter/client/v4 v4.0.0
-	github.com/onsi/ginkgo v1.16.4
+	github.com/onsi/ginkgo v1.16.5
 	github.com/onsi/gomega v1.16.0
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.11.0

go.sum

@@ -520,8 +520,9 @@ github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+W
 github.com/onsi/ginkgo v1.11.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
 github.com/onsi/ginkgo v1.12.1/go.mod h1:zj2OWP4+oCPe1qIXoGWkgMRwljMUYCdkwsT2108oapk=
 github.com/onsi/ginkgo v1.14.0/go.mod h1:iSB4RoI2tjJc9BBv4NKIKWKya62Rps+oPG/Lv9klQyY=
-github.com/onsi/ginkgo v1.16.4 h1:29JGrr5oVBm5ulCWet69zQkzWipVXIol6ygQUe/EzNc=
 github.com/onsi/ginkgo v1.16.4/go.mod h1:dX+/inL/fNMqNlz0e9LfyB9TswhZpCVdJM/Z6Vvnwo0=
+github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE=
+github.com/onsi/ginkgo v1.16.5/go.mod h1:+E8gABHa3K6zRBolWtd+ROzc/U5bkGt0FwiG042wbpU=
 github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA=
 github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
 github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY=

pkg/apis/velero/v1/pod_volume_backup_types.go

@@ -127,12 +127,12 @@ type PodVolumeBackup struct {
 	Status PodVolumeBackupStatus `json:"status,omitempty"`
 }
 
-// TODO(2.0) After converting all resources to use the runttime-controller client,
+// TODO(2.0) After converting all resources to use the runtime-controller client,
 // the k8s:deepcopy marker will no longer be needed and should be removed.
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 // +kubebuilder:object:root=true
-// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackup,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackup/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackups,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackups/status,verbs=get;update;patch
 
 // PodVolumeBackupList is a list of PodVolumeBackups.
 type PodVolumeBackupList struct {

pkg/cmd/cli/restic/server.go

@@ -219,7 +219,6 @@ func (s *resticServer) run() {
 
 		PvLister:  s.kubeInformerFactory.Core().V1().PersistentVolumes().Lister(),
 		PvcLister: s.kubeInformerFactory.Core().V1().PersistentVolumeClaims().Lister(),
-		PvbLister: s.veleroInformerFactory.Velero().V1().PodVolumeBackups().Lister(),
 	}
 	if err := pvbReconciler.SetupWithManager(s.mgr); err != nil {
 		s.logger.Fatal(err, "unable to create controller", "controller", controller.PodVolumeBackup)

pkg/controller/pod_volume_backup_controller.go

@@ -28,17 +28,14 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/clock"
 	corev1listers "k8s.io/client-go/listers/core/v1"
-	"sigs.k8s.io/cluster-api/util/patch"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/vmware-tanzu/velero/internal/credentials"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
-	listers "github.com/vmware-tanzu/velero/pkg/generated/listers/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/metrics"
 	"github.com/vmware-tanzu/velero/pkg/restic"
 	"github.com/vmware-tanzu/velero/pkg/util/filesystem"
@@ -65,11 +62,10 @@ type PodVolumeBackupReconciler struct {
 
 	PvLister  corev1listers.PersistentVolumeLister
 	PvcLister corev1listers.PersistentVolumeClaimLister
-	PvbLister listers.PodVolumeBackupLister
 }
 
-// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackup,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackup/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackups,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=velero.io,resources=podvolumebackups/status,verbs=get;update;patch
 func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := r.Log.WithFields(logrus.Fields{
 		"controller":      "podvolumebackup",
@@ -82,7 +78,6 @@ func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 			log.Debug("Unable to find PodVolumeBackup")
 			return ctrl.Result{}, nil
 		}
-
 		return ctrl.Result{}, errors.Wrap(err, "getting PodVolumeBackup")
 	}
 
@@ -93,21 +88,7 @@ func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		)
 	}
 
-	// Initialize patch helper.
-	patchHelper, err := patch.NewHelper(&pvb, r.Client)
-	if err != nil {
-		return ctrl.Result{}, errors.Wrap(err, "getting a patch helper to update this resource")
-	}
-
-	defer func() {
-		// Attempt to Patch the pvb object and status after each reconciliation.
-		if err := patchHelper.Patch(ctx, &pvb); err != nil {
-			log.WithError(err).Error("update PodVolumeBackup status")
-			return
-		}
-	}()
-
-	log.Info("Pod volume backup starting")
+	log.Info("PodVolumeBackup starting")
 
 	// Only process items for this node.
 	if pvb.Spec.Node != r.NodeName {
@@ -116,7 +97,7 @@ func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 
 	// Only process new items.
 	if pvb.Status.Phase != "" && pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseNew {
-		log.Debug("Pod volume backup is not new, not processing")
+		log.Debug("PodVolumeBackup is not new, not processing")
 		return ctrl.Result{}, nil
 	}
 
@@ -127,23 +108,28 @@ func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	pvb.Status.StartTimestamp = &metav1.Time{Time: r.Clock.Now()}
 
 	var pod corev1.Pod
-	if err := r.Client.Get(ctx, req.NamespacedName, &pod); err != nil {
-		return r.updateStatusFailed(&pvb, err, fmt.Sprintf("getting pod %s/%s", pvb.Spec.Pod.Namespace, pvb.Spec.Pod.Name))
+	podNamespacedName := client.ObjectKey{
+		Namespace: pvb.Spec.Pod.Namespace,
+		Name:      pvb.Spec.Pod.Name,
+	}
+	if err := r.Client.Get(ctx, podNamespacedName, &pod); err != nil {
+		return r.updateStatusToFailed(ctx, &pvb, err, fmt.Sprintf("getting pod %s/%s", pvb.Spec.Pod.Namespace, pvb.Spec.Pod.Name))
 	}
 
 	var resticDetails resticDetails
-	resticCmd, err := r.buildResticCommand(log, req.Namespace, &pvb, &pod, &resticDetails)
+	resticCmd, err := r.buildResticCommand(ctx, log, &pvb, &pod, &resticDetails)
 	if err != nil {
-		return r.updateStatusFailed(&pvb, err, "building restic command")
+		return r.updateStatusToFailed(ctx, &pvb, err, "building Restic command")
 	}
+	defer os.Remove(resticDetails.credsFile)
 
 	var emptySnapshot bool
 	stdout, stderr, err := r.ResticExec.RunBackup(resticCmd, log, r.updateBackupProgressFunc(&pvb, log))
 	if err != nil {
 		if strings.Contains(stderr, "snapshot is empty") {
 			emptySnapshot = true
 		} else {
-			return r.updateStatusFailed(&pvb, err, fmt.Sprintf("running restic backup, stderr=%s", stderr))
+			return r.updateStatusToFailed(ctx, &pvb, err, fmt.Sprintf("running Restic backup, stderr=%s", stderr))
 		}
 	}
 	log.Debugf("Ran command=%s, stdout=%s, stderr=%s", resticCmd.String(), stdout, stderr)
@@ -156,7 +142,7 @@ func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 
 		snapshotID, err = r.ResticExec.GetSnapshotID(cmd)
 		if err != nil {
-			return r.updateStatusFailed(&pvb, err, "getting snapshot id")
+			return r.updateStatusToFailed(ctx, &pvb, err, "getting snapshot id")
 		}
 	}
 
@@ -175,14 +161,19 @@ func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	r.Metrics.ObserveResticOpLatency(r.NodeName, req.Name, resticCmd.Command, backupName, latencySeconds)
 	r.Metrics.RegisterResticOpLatencyGauge(r.NodeName, req.Name, resticCmd.Command, backupName, latencySeconds)
 	r.Metrics.RegisterPodVolumeBackupDequeue(r.NodeName)
-	log.Info("Pod volume backup completed")
+	log.Info("PodVolumeBackup completed")
 
+	if err := r.Client.Update(ctx, &pvb); err != nil {
+		log.WithError(err).Error("updating PodVolumeBackup resource")
+	}
 	return ctrl.Result{}, nil
 }
 
 // SetupWithManager registers the PVB controller.
 func (r *PodVolumeBackupReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	return ctrl.NewControllerManagedBy(mgr).For(&velerov1api.PodVolumeBackup{}).Complete(r)
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&velerov1api.PodVolumeBackup{}).
+		Complete(r)
 }
 
 func (r *PodVolumeBackupReconciler) singlePathMatch(path string) (string, error) {
@@ -198,28 +189,33 @@ func (r *PodVolumeBackupReconciler) singlePathMatch(path string) (string, error)
 	return matches[0], nil
 }
 
-// getParentSnapshot finds the most recent completed pod volume backup for the
-// specified PVC and returns its restic snapshot ID. Any errors encountered are
+// getParentSnapshot finds the most recent completed PodVolumeBackup for the
+// specified PVC and returns its Restic snapshot ID. Any errors encountered are
 // logged but not returned since they do not prevent a backup from proceeding.
-func getParentSnapshot(log logrus.FieldLogger, pvcUID, backupStorageLocation string, pvbListers listers.PodVolumeBackupNamespaceLister) string {
+func (r *PodVolumeBackupReconciler) getParentSnapshot(ctx context.Context, log logrus.FieldLogger, pvbNamespace, pvcUID, bsl string) string {
 	log = log.WithField("pvcUID", pvcUID)
-	log.Infof("Looking for most recent completed pod volume backup for this PVC")
+	log.Infof("Looking for most recent completed PodVolumeBackup for this PVC")
 
-	pvcBackups, err := pvbListers.List(labels.SelectorFromSet(map[string]string{velerov1api.PVCUIDLabel: pvcUID}))
-	if err != nil {
-		log.WithError(errors.WithStack(err)).Error("listing pod volume backups for PVC")
-		return ""
+	listOpts := &client.ListOptions{
+		Namespace: pvbNamespace,
+	}
+	matchingLabels := client.MatchingLabels(map[string]string{velerov1api.PVCUIDLabel: pvcUID})
+	matchingLabels.ApplyToList(listOpts)
+
+	var pvbList velerov1api.PodVolumeBackupList
+	if err := r.Client.List(ctx, &pvbList, listOpts); err != nil {
+		log.WithError(errors.WithStack(err)).Error("getting list of podvolumebackups for this PVC")
 	}
 
-	// Go through all the pod volume backups for the PVC and look for the most
+	// Go through all the podvolumebackups for the PVC and look for the most
 	// recent completed one to use as the parent.
-	var mostRecentBackup *velerov1api.PodVolumeBackup
-	for _, backup := range pvcBackups {
-		if backup.Status.Phase != velerov1api.PodVolumeBackupPhaseCompleted {
+	var mostRecentPVB *velerov1api.PodVolumeBackup
+	for _, pvb := range pvbList.Items {
+		if pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseCompleted {
 			continue
 		}
 
-		if backupStorageLocation != backup.Spec.BackupStorageLocation {
+		if bsl != pvb.Spec.BackupStorageLocation {
 			// Check the backup storage location is the same as spec in order to
 			// support backup to multiple backup-locations. Otherwise, there exists
 			// a case that backup volume snapshot to the second location would
@@ -230,22 +226,22 @@ func getParentSnapshot(log logrus.FieldLogger, pvcUID, backupStorageLocation str
 			continue
 		}
 
-		if mostRecentBackup == nil || backup.Status.StartTimestamp.After(mostRecentBackup.Status.StartTimestamp.Time) {
-			mostRecentBackup = backup
+		if mostRecentPVB == nil || pvb.Status.StartTimestamp.After(mostRecentPVB.Status.StartTimestamp.Time) {
+			mostRecentPVB = &pvb
 		}
 	}
 
-	if mostRecentBackup == nil {
-		log.Info("No completed pod volume backup found for PVC")
+	if mostRecentPVB == nil {
+		log.Info("No completed PodVolumeBackup found for PVC")
 		return ""
 	}
 
 	log.WithFields(map[string]interface{}{
-		"parentPodVolumeBackup": mostRecentBackup.Name,
-		"parentSnapshotID":      mostRecentBackup.Status.SnapshotID,
-	}).Info("Found most recent completed pod volume backup for PVC")
+		"parentPodVolumeBackup": mostRecentPVB.Name,
+		"parentSnapshotID":      mostRecentPVB.Status.SnapshotID,
+	}).Info("Found most recent completed PodVolumeBackup for PVC")
 
-	return mostRecentBackup.Status.SnapshotID
+	return mostRecentPVB.Status.SnapshotID
 }
 
 // updateBackupProgressFunc returns a func that takes progress info and patches
@@ -256,10 +252,14 @@ func (r *PodVolumeBackupReconciler) updateBackupProgressFunc(pvb *velerov1api.Po
 	}
 }
 
-func (r *PodVolumeBackupReconciler) updateStatusFailed(pvb *velerov1api.PodVolumeBackup, err error, msg string) (ctrl.Result, error) {
+func (r *PodVolumeBackupReconciler) updateStatusToFailed(ctx context.Context, pvb *velerov1api.PodVolumeBackup, err error, msg string) (ctrl.Result, error) {
 	pvb.Status.Phase = velerov1api.PodVolumeBackupPhaseFailed
 	pvb.Status.Message = msg
 	pvb.Status.CompletionTimestamp = &metav1.Time{Time: r.Clock.Now()}
+
+	if err := r.Client.Update(ctx, pvb); err != nil {
+		return ctrl.Result{}, errors.Wrap(err, "updating PodVolumeBackup resource with failed status")
+	}
 	return ctrl.Result{}, errors.Wrap(err, msg)
 }
 
@@ -269,7 +269,7 @@ type resticDetails struct {
 	path                  string
 }
 
-func (r *PodVolumeBackupReconciler) buildResticCommand(log *logrus.Entry, ns string, pvb *velerov1api.PodVolumeBackup, pod *corev1.Pod, details *resticDetails) (*restic.Command, error) {
+func (r *PodVolumeBackupReconciler) buildResticCommand(ctx context.Context, log *logrus.Entry, pvb *velerov1api.PodVolumeBackup, pod *corev1.Pod, details *resticDetails) (*restic.Command, error) {
 	volDir, err := kube.GetVolumeDirectory(pod, pvb.Spec.Volume, r.PvcLister, r.PvLister)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting volume directory name")
@@ -287,9 +287,8 @@ func (r *PodVolumeBackupReconciler) buildResticCommand(log *logrus.Entry, ns str
 	// Temporary credentials.
 	details.credsFile, err = r.CredsFileStore.Path(restic.RepoKeySelector())
 	if err != nil {
-		return nil, errors.Wrap(err, "creating temporary restic credentials file")
+		return nil, errors.Wrap(err, "creating temporary Restic credentials file")
 	}
-	defer os.Remove(details.credsFile)
 
 	cmd := restic.BackupCommand(pvb.Spec.RepoIdentifier, details.credsFile, path, pvb.Spec.Tags)
 
@@ -302,7 +301,7 @@ func (r *PodVolumeBackupReconciler) buildResticCommand(log *logrus.Entry, ns str
 	}
 
 	// If there's a caCert on the ObjectStorage, write it to disk so that it can
-	// be passed to restic.
+	// be passed to Restic.
 	if backupLocation.Spec.ObjectStorage != nil &&
 		backupLocation.Spec.ObjectStorage.CACert != nil {
 
@@ -317,24 +316,18 @@ func (r *PodVolumeBackupReconciler) buildResticCommand(log *logrus.Entry, ns str
 
 	details.envs, err = restic.CmdEnv(backupLocation, r.CredsFileStore)
 	if err != nil {
-		return nil, errors.Wrap(err, "setting restic cmd env")
+		return nil, errors.Wrap(err, "setting Restic command environment")
 	}
 	cmd.Env = details.envs
 
-	// If this is a PVC, look for the most recent completed pod volume backup for
-	// it and get its restic snapshot ID to use as the value of the `--parent`
+	// If this is a PVC, look for the most recent completed PodVolumeBackup for
+	// it and get its Restic snapshot ID to use as the value of the `--parent`
 	// flag. Without this, if the pod using the PVC (and therefore the directory
-	// path under /host_pods/) has changed since the PVC's last backup, restic
+	// path under /host_pods/) has changed since the PVC's last backup, Restic
 	// will not be able to identify a suitable parent snapshot to use, and will
 	// have to do a full rescan of the contents of the PVC.
 	if pvcUID, ok := pvb.Labels[velerov1api.PVCUIDLabel]; ok {
-		parentSnapshotID := getParentSnapshot(
-			log,
-			pvcUID,
-			pvb.Spec.BackupStorageLocation,
-			r.PvbLister.PodVolumeBackups(pvb.Namespace),
-		)
-
+		parentSnapshotID := r.getParentSnapshot(ctx, log, pvb.Namespace, pvcUID, pvb.Spec.BackupStorageLocation)
 		if parentSnapshotID == "" {
 			log.Info("No parent snapshot found for PVC, not using --parent flag for this backup")
 		} else {

pkg/controller/pod_volume_backup_controller_test.go

@@ -68,7 +68,7 @@ func bslBuilder() *builder.BackupStorageLocationBuilder {
 		ForBackupStorageLocation(velerov1api.DefaultNamespace, "bsl-loc")
 }
 
-var _ = Describe("Pod Volume Backup Reconciler", func() {
+var _ = Describe("PodVolumeBackup Reconciler", func() {
 	type request struct {
 		pvb *velerov1api.PodVolumeBackup
 		pod *corev1.Pod
@@ -109,7 +109,6 @@ var _ = Describe("Pod Volume Backup Reconciler", func() {
 			Expect(velerov1api.AddToScheme(scheme.Scheme)).To(Succeed())
 			r := PodVolumeBackupReconciler{
 				Client:         fakeClient,
-				Ctx:            ctx,
 				Clock:          clock.NewFakeClock(now),
 				Metrics:        metrics.NewResticServerMetrics(),
 				CredsFileStore: fakeCredsFileStore{},
@@ -119,7 +118,7 @@ var _ = Describe("Pod Volume Backup Reconciler", func() {
 				Log:            velerotest.NewLogger(),
 			}
 
-			actualResult, err := r.Reconcile(r.Ctx, ctrl.Request{
+			actualResult, err := r.Reconcile(ctx, ctrl.Request{
 				NamespacedName: types.NamespacedName{
 					Namespace: velerov1api.DefaultNamespace,
 					Name:      test.pvb.Name,