From e03ff894ffb8179f8e0e95041800a333b97ec420 Mon Sep 17 00:00:00 2001 From: chlins Date: Wed, 2 Sep 2026 15:24:51 +0800 Subject: [PATCH] Add operation context to user-facing error messages Prefix raw err.Error() strings surfaced in CR statuses and CLI stderr with the failed operation. Signed-off-by: chlins --- changelogs/unreleased/10464-chlins | 1 + pkg/cmd/errors.go | 2 +- pkg/controller/backup_controller.go | 4 ++-- pkg/controller/backup_controller_test.go | 4 ++-- pkg/controller/backup_deletion_controller.go | 14 +++++++------- .../backup_deletion_controller_test.go | 16 ++++++++-------- pkg/controller/pod_volume_backup_controller.go | 5 ++--- pkg/controller/restore_controller.go | 8 ++++---- 8 files changed, 27 insertions(+), 27 deletions(-) create mode 100644 changelogs/unreleased/10464-chlins diff --git a/changelogs/unreleased/10464-chlins b/changelogs/unreleased/10464-chlins new file mode 100644 index 000000000..0a89da583 --- /dev/null +++ b/changelogs/unreleased/10464-chlins @@ -0,0 +1 @@ +Add operation context to user-facing error messages in CR statuses and CLI output diff --git a/pkg/cmd/errors.go b/pkg/cmd/errors.go index 4f374c8af..50c0a8e35 100644 --- a/pkg/cmd/errors.go +++ b/pkg/cmd/errors.go @@ -27,7 +27,7 @@ import ( func CheckError(err error) { if err != nil { if err != context.Canceled { - fmt.Fprintf(os.Stderr, "An error occurred: %v\n", err) + fmt.Fprintf(os.Stderr, "velero: %v\n", err) } os.Exit(1) } diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 569ff18d1..f3abcc6f9 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -362,7 +362,7 @@ func (b *backupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // result in the backup being Failed. log.WithError(err).Error("backup failed") request.Status.Phase = velerov1api.BackupPhaseFailed - request.Status.FailureReason = err.Error() + request.Status.FailureReason = fmt.Sprintf("backup execution failed: %v", err) } switch request.Status.Phase { @@ -619,7 +619,7 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackupWithGlobal( *request.Backup, b.kbClient, b.globalVolumePoliciesConfigMap, request.Namespace, logger) if err != nil { - request.Status.ValidationErrors = append(request.Status.ValidationErrors, err.Error()) + request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("invalid resource policies: %v", err)) } else if b.globalVolumePoliciesConfigMap != "" { // Record the contributing global volume policies ConfigMap so `velero backup describe` can surface it. request.Annotations[velerov1api.GlobalBackupVolumePolicyConfigMapAnnotation] = b.globalVolumePoliciesConfigMap diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 13bac2e4c..eb7786f63 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -1205,7 +1205,7 @@ func TestProcessBackupCompletions(t *testing.T) { }, Status: velerov1api.BackupStatus{ Phase: velerov1api.BackupPhaseFailed, - FailureReason: "backup already exists in object storage", + FailureReason: "backup execution failed: backup already exists in object storage", Version: 1, FormatVersion: "1.1.0", StartTimestamp: ×tamp, @@ -1250,7 +1250,7 @@ func TestProcessBackupCompletions(t *testing.T) { }, Status: velerov1api.BackupStatus{ Phase: velerov1api.BackupPhaseFailed, - FailureReason: "error checking if backup already exists in object storage: Backup already exists in object storage", + FailureReason: "backup execution failed: error checking if backup already exists in object storage: Backup already exists in object storage", Version: 1, FormatVersion: "1.1.0", StartTimestamp: ×tamp, diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index 416c28cd4..18a545979 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -315,7 +315,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque volumeSnapshotter, ok := volumeSnapshotters[snapshot.Spec.Location] if !ok { if volumeSnapshotter, err = r.volumeSnapshottersForVSL(ctx, backup.Namespace, snapshot.Spec.Location, pluginManager); err != nil { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrapf(err, "error getting volume snapshotter for location %q", snapshot.Spec.Location).Error()) continue } volumeSnapshotters[snapshot.Spec.Location] = volumeSnapshotter @@ -334,7 +334,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque log.Info("Removing pod volume snapshots") if deleteErrs := r.deletePodVolumeSnapshots(ctx, backup); len(deleteErrs) > 0 { for _, err := range deleteErrs { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error deleting pod volume snapshots").Error()) } } @@ -342,7 +342,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque log.Info("Removing snapshot data by data mover") if deleteErrs := r.deleteMovedSnapshots(ctx, backup); len(deleteErrs) > 0 { for _, err := range deleteErrs { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error deleting moved snapshot data").Error()) } } duList := &velerov2alpha1.DataUploadList{} @@ -354,12 +354,12 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque }), }); err != nil { log.WithError(err).Error("Error listing datauploads") - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error listing datauploads for backup").Error()) } else { for i := range duList.Items { du := duList.Items[i] if err := r.Delete(ctx, &du); err != nil { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrapf(err, "error deleting dataupload %q", du.Name).Error()) } } } @@ -368,7 +368,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque if backupStore != nil && len(errs) == 0 { log.Info("Removing backup from backup storage") if err := backupStore.DeleteBackup(backup.Name); err != nil { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error removing backup from backup storage").Error()) } } else if len(errs) > 0 { log.Info("Skipping removal of backup from backup storage due to previous errors") @@ -631,7 +631,7 @@ func (r *backupDeletionReconciler) patchDeleteBackupRequest(ctx context.Context, func (r *backupDeletionReconciler) patchDeleteBackupRequestWithError(ctx context.Context, req *velerov1api.DeleteBackupRequest, err error) error { _, err = r.patchDeleteBackupRequest(ctx, req, func(r *velerov1api.DeleteBackupRequest) { r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed - r.Status.Errors = []string{err.Error()} + r.Status.Errors = []string{errors.WithMessage(err, "backup deletion failed").Error()} }) return err } diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index d358fbe5e..3a0bc2fe5 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -137,7 +137,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { td.fakeClient.Get(ctx, td.req.NamespacedName, res) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.True(t, strings.HasPrefix(res.Status.Errors[0], "error getting the backup store")) + assert.True(t, strings.HasPrefix(res.Status.Errors[0], "backup deletion failed: error getting the backup store")) }) t.Run("missing spec.backupName", func(t *testing.T) { @@ -153,7 +153,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "spec.backupName is required", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: spec.backupName is required", res.Status.Errors[0]) }) t.Run("existing deletion requests for the backup are deleted", func(t *testing.T) { @@ -221,7 +221,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup is still in progress", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup is still in progress", res.Status.Errors[0]) }) t.Run("unable to find backup", func(t *testing.T) { @@ -235,7 +235,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup not found", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup not found", res.Status.Errors[0]) }) t.Run("unable to find backup storage location", func(t *testing.T) { backup := builder.ForBackup(velerov1api.DefaultNamespace, "foo").StorageLocation("default").Result() @@ -250,7 +250,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup storage location default not found", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup storage location default not found", res.Status.Errors[0]) }) t.Run("backup storage location is in read-only mode", func(t *testing.T) { @@ -267,7 +267,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "cannot delete backup because backup storage location default is currently in read-only mode", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: cannot delete backup because backup storage location default is currently in read-only mode", res.Status.Errors[0]) }) t.Run("backup storage location is in unavailable state", func(t *testing.T) { @@ -284,7 +284,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "cannot delete backup because backup storage location default is currently in Unavailable state", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: cannot delete backup because backup storage location default is currently in Unavailable state", res.Status.Errors[0]) }) t.Run("full delete, no errors", func(t *testing.T) { @@ -877,7 +877,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup not found", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup not found", res.Status.Errors[0]) }) } diff --git a/pkg/controller/pod_volume_backup_controller.go b/pkg/controller/pod_volume_backup_controller.go index 13dbd5d79..c4e68ce33 100644 --- a/pkg/controller/pod_volume_backup_controller.go +++ b/pkg/controller/pod_volume_backup_controller.go @@ -784,10 +784,9 @@ func UpdatePVBStatusToFailed(ctx context.Context, c client.Client, pvb *velerov1 pvb.Status.SnapshotID = dataPathError.GetSnapshotID() } if len(strings.TrimSpace(msg)) == 0 { - pvb.Status.Message = errOut.Error() - } else { - pvb.Status.Message = errors.WithMessage(errOut, msg).Error() + msg = "pod volume backup failed" } + pvb.Status.Message = errors.WithMessage(errOut, msg).Error() if pvb.Status.StartTimestamp.IsZero() { pvb.Status.StartTimestamp = &metav1.Time{Time: time} } diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 69f8636b7..8248ee538 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -275,7 +275,7 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if err := r.runValidatedRestore(restore, info, resourceModifiers, restoreResPolicies); err != nil { log.WithError(err).Debug("Restore failed") restore.Status.Phase = api.RestorePhaseFailed - restore.Status.FailureReason = err.Error() + restore.Status.FailureReason = fmt.Sprintf("restore execution failed: %v", err) r.metrics.RegisterRestoreFailed(backupScheduleName) } @@ -349,7 +349,7 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap // validate Restore Init Hook's InitContainers restoreHooks, err := hook.GetRestoreHooksFromSpec(&restore.Spec.Hooks) if err != nil { - restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, err.Error()) + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("invalid restore hooks: %v", err)) } for _, resource := range restoreHooks { for _, h := range resource.RestoreHooks { @@ -357,7 +357,7 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap for _, container := range h.Init.InitContainers { err = hook.ValidateContainer(container.Raw) if err != nil { - restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, err.Error()) + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("invalid init container in restore hook %q: %v", resource.Name, err)) } } } @@ -433,7 +433,7 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap ) if err != nil { restore.Status.ValidationErrors = append( - restore.Status.ValidationErrors, err.Error(), + restore.Status.ValidationErrors, fmt.Sprintf("invalid restore resource policies: %v", err), ) return backupInfo{}, nil, nil }