diff --git a/go.mod b/go.mod index b55a85266e..9791b6aca1 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 5084b6c5b0..04458f9f96 100644 --- a/go.sum +++ b/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= diff --git a/pkg/apis/velero/v1/pod_volume_backup_types.go b/pkg/apis/velero/v1/pod_volume_backup_types.go index a291c1d184..64f18f7316 100644 --- a/pkg/apis/velero/v1/pod_volume_backup_types.go +++ b/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 { diff --git a/pkg/cmd/cli/restic/server.go b/pkg/cmd/cli/restic/server.go index c687861092..159f48689e 100644 --- a/pkg/cmd/cli/restic/server.go +++ b/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) diff --git a/pkg/controller/pod_volume_backup_controller.go b/pkg/controller/pod_volume_backup_controller.go index 067064f077..5cab2b2ec8 100644 --- a/pkg/controller/pod_volume_backup_controller.go +++ b/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,15 +108,20 @@ 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)) @@ -143,7 +129,7 @@ func (r *PodVolumeBackupReconciler) Reconcile(ctx context.Context, req ctrl.Requ 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 { diff --git a/pkg/controller/pod_volume_backup_controller_test.go b/pkg/controller/pod_volume_backup_controller_test.go index 02c54aea31..ffc5f662cb 100644 --- a/pkg/controller/pod_volume_backup_controller_test.go +++ b/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,