diff --git a/cmd/non-admin/backup/backup_test.go b/cmd/non-admin/backup/backup_test.go index fe1da13..fae8cfb 100644 --- a/cmd/non-admin/backup/backup_test.go +++ b/cmd/non-admin/backup/backup_test.go @@ -60,6 +60,8 @@ func TestNonAdminBackupCommands(t *testing.T) { args: []string{"nonadmin", "backup", "describe", "--help"}, expectContains: []string{ "Describe a non-admin backup", + "--timeout", + "Maximum time to wait for backup details to be available", }, }, { @@ -82,6 +84,8 @@ func TestNonAdminBackupCommands(t *testing.T) { args: []string{"nonadmin", "backup", "logs", "--help"}, expectContains: []string{ "Show logs for a non-admin backup", + "--timeout", + "Maximum time to wait for logs to be available", }, }, { @@ -128,6 +132,7 @@ func TestNonAdminBackupCommands(t *testing.T) { expectContains: []string{ "Describe non-admin resources", "backup", + "--timeout", }, }, { @@ -136,6 +141,7 @@ func TestNonAdminBackupCommands(t *testing.T) { expectContains: []string{ "Get logs for non-admin resources", "backup", + "--timeout", }, }, // Shorthand verb-noun order tests diff --git a/cmd/non-admin/backup/describe.go b/cmd/non-admin/backup/describe.go index 1215cd7..0eaad47 100644 --- a/cmd/non-admin/backup/describe.go +++ b/cmd/non-admin/backup/describe.go @@ -17,12 +17,22 @@ import ( kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + // defaultDescribeTimeout is the default timeout for fetching backup details + defaultDescribeTimeout = 5 * time.Minute +) + func NewDescribeCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Describe a non-admin backup", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + timeout, err := cmd.Flags().GetDuration("timeout") + if err != nil { + return fmt.Errorf("failed to get timeout flag: %w", err) + } + backupName := args[0] // Get the current namespace from kubectl context @@ -51,13 +61,15 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { } // Print in Velero-style format - printNonAdminBackupDetails(cmd, &nab) + printNonAdminBackupDetails(cmd, &nab, kbClient, userNamespace, timeout) return nil }, Example: ` kubectl oadp nonadmin backup describe my-backup`, } + c.Flags().Duration("timeout", defaultDescribeTimeout, "Maximum time to wait for backup details to be available") + output.BindFlags(c.Flags()) output.ClearOutputFlagDefault(c) @@ -65,7 +77,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { } // printNonAdminBackupDetails prints backup details in Velero admin describe format -func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup) { +func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, userNamespace string, timeout time.Duration) { out := cmd.OutOrStdout() // Get Velero backup reference if available @@ -350,8 +362,8 @@ func colorizePhase(phase string) string { // NonAdminDescribeBackup mirrors Velero's output.DescribeBackup functionality // but works within non-admin RBAC boundaries using NonAdminDownloadRequest -func NonAdminDescribeBackup(cmd *cobra.Command, kbClient kbclient.Client, nab *nacv1alpha1.NonAdminBackup, userNamespace string) error { - ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) +func NonAdminDescribeBackup(cmd *cobra.Command, kbClient kbclient.Client, nab *nacv1alpha1.NonAdminBackup, userNamespace string, timeout time.Duration) error { + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() // Print basic backup information diff --git a/cmd/non-admin/backup/logs.go b/cmd/non-admin/backup/logs.go index ee8f555..65de474 100644 --- a/cmd/non-admin/backup/logs.go +++ b/cmd/non-admin/backup/logs.go @@ -30,13 +30,23 @@ import ( kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + // defaultLogsTimeout is the default timeout for waiting for backup logs to be available + defaultLogsTimeout = 5 * time.Minute +) + func NewLogsCommand(f client.Factory, use string) *cobra.Command { - return &cobra.Command{ + c := &cobra.Command{ Use: use + " NAME", Short: "Show logs for a non-admin backup", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + timeout, err := cmd.Flags().GetDuration("timeout") + if err != nil { + return fmt.Errorf("failed to get timeout flag: %w", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() // Get the current namespace from kubectl context @@ -97,20 +107,26 @@ func NewLogsCommand(f client.Factory, use string) *cobra.Command { _ = kbClient.Delete(deleteCtx, req) }() - fmt.Fprintf(cmd.OutOrStdout(), "Waiting for backup logs to be processed...\n") + fmt.Fprintf(cmd.OutOrStdout(), "Waiting for backup logs to be processed (timeout: %v)...\n", timeout) - // Wait for the download request to be processed using shared utility - // Note: We create a custom waiting implementation here to provide user feedback - timeout := time.After(120 * time.Second) - tick := time.Tick(2 * time.Second) + // Wait for the download request to be processed + // Note: We implement custom polling here instead of using shared.ProcessDownloadRequest + // because we want to show progress feedback (dots) to the user + ticker := time.NewTicker(2 * time.Second) + defer ticker.Stop() var signedURL string Loop: for { select { - case <-timeout: - return fmt.Errorf("timed out waiting for NonAdminDownloadRequest to be processed") - case <-tick: + case <-ctx.Done(): + // Check if context was cancelled due to timeout or other reason + if ctx.Err() == context.DeadlineExceeded { + return shared.FormatDownloadRequestTimeoutError(kbClient, req, timeout) + } + // Context cancelled for other reason (e.g., user interruption) + return fmt.Errorf("operation cancelled: %w", ctx.Err()) + case <-ticker.C: fmt.Fprintf(cmd.OutOrStdout(), ".") var updated nacv1alpha1.NonAdminDownloadRequest if err := kbClient.Get(ctx, kbclient.ObjectKey{ @@ -149,4 +165,8 @@ func NewLogsCommand(f client.Factory, use string) *cobra.Command { }, Example: ` kubectl oadp nonadmin backup logs my-backup`, } + + c.Flags().Duration("timeout", defaultLogsTimeout, "Maximum time to wait for logs to be available") + + return c } diff --git a/cmd/shared/download.go b/cmd/shared/download.go index 22a1c8c..cb3e841 100644 --- a/cmd/shared/download.go +++ b/cmd/shared/download.go @@ -31,6 +31,12 @@ import ( kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + // defaultStatusCheckTimeout is the timeout for retrieving status information + // when formatting timeout error messages + defaultStatusCheckTimeout = 5 * time.Second +) + // DownloadRequestOptions holds configuration for creating and processing NonAdminDownloadRequests type DownloadRequestOptions struct { // BackupName is the name of the backup to download data for @@ -103,10 +109,15 @@ func waitForDownloadURL(ctx context.Context, kbClient kbclient.Client, req *nacv for { select { case <-timeoutCtx.Done(): - return "", fmt.Errorf("timed out waiting for NonAdminDownloadRequest to be processed") + // Check if context was cancelled due to timeout or other reason + if timeoutCtx.Err() == context.DeadlineExceeded { + return "", FormatDownloadRequestTimeoutError(kbClient, req, timeout) + } + // Context cancelled for other reason (e.g., user interruption) + return "", fmt.Errorf("operation cancelled: %w", timeoutCtx.Err()) case <-ticker.C: var updated nacv1alpha1.NonAdminDownloadRequest - if err := kbClient.Get(ctx, kbclient.ObjectKey{ + if err := kbClient.Get(timeoutCtx, kbclient.ObjectKey{ Namespace: req.Namespace, Name: req.Name, }, &updated); err != nil { @@ -198,3 +209,35 @@ func StreamDownloadContent(url string, writer io.Writer) error { return nil } + +// FormatDownloadRequestTimeoutError creates an informative timeout error message +// by retrieving the current status of the NonAdminDownloadRequest. +// Note: This function uses context.Background() for the status check because it's typically +// called after the main context has been cancelled (timeout), and we still want to retrieve +// the final status for error reporting. +func FormatDownloadRequestTimeoutError(kbClient kbclient.Client, req *nacv1alpha1.NonAdminDownloadRequest, timeout time.Duration) error { + // Get the latest status to provide helpful error message + // Use a fresh context since the caller's context may already be cancelled + statusCtx, cancel := context.WithTimeout(context.Background(), defaultStatusCheckTimeout) + defer cancel() + + var updated nacv1alpha1.NonAdminDownloadRequest + if err := kbClient.Get(statusCtx, kbclient.ObjectKey{ + Namespace: req.Namespace, + Name: req.Name, + }, &updated); err == nil { + // Show what state the request is in + var statusInfo string + if len(updated.Status.Conditions) > 0 { + var conditions []string + for _, cond := range updated.Status.Conditions { + conditions = append(conditions, fmt.Sprintf("%s=%s", cond.Type, cond.Status)) + } + statusInfo = fmt.Sprintf("NonAdminDownloadRequest conditions: %s", strings.Join(conditions, ", ")) + } else { + statusInfo = "NonAdminDownloadRequest has no status conditions yet" + } + return fmt.Errorf("timed out after %v waiting for NonAdminDownloadRequest %q to be processed. %s", timeout, req.Name, statusInfo) + } + return fmt.Errorf("timed out after %v waiting for NonAdminDownloadRequest %q to be processed", timeout, req.Name) +}