Skip to content

Add exponential backoff for CloudStorage status update failures - #124

Merged
kaovilai merged 2 commits into
CloudStorage-LimitedRetriesfrom
copilot/add-backoff-for-status-updates
Sep 9, 2025
Merged

kaovilai merged 2 commits into
CloudStorage-LimitedRetriesfrom
copilot/add-backoff-for-status-updates

Conversation

Copilot AI commented Sep 9, 2025 •

Copy link
Copy Markdown

Problem

The CloudStorage controller was not applying exponential backoff when status updates failed at the end of the reconciliation process. When b.Client.Status().Update(ctx, &bucket) failed, the error was only logged and the controller returned success (ctrl.Result{}, nil), which prevented controller-runtime's exponential backoff mechanism from being triggered.

This could result in status conditions remaining in incorrect states if status updates consistently failed, as the controller would not retry with increasing delays.

Solution

Modified the final status update error handling in cloudstorage_controller.go to return the error instead of just logging it:

Before:

if err := b.Client.Status().Update(ctx, &bucket); err != nil {
    logger.Error(err, "failed to update CloudStorage status")
}
return ctrl.Result{}, nil

After:

if err := b.Client.Status().Update(ctx, &bucket); err != nil {
    logger.Error(err, "failed to update CloudStorage status")
    // Return error to trigger exponential backoff for status update failures
    return ctrl.Result{}, err
}
return ctrl.Result{}, nil

This change ensures that status update failures trigger controller-runtime's built-in exponential backoff mechanism (5ms initial → 1000s maximum), consistent with the pattern used elsewhere in the controller for bucket operation failures.

Testing

  • Added a documentation test explaining the change and expected behavior
  • Verified all existing CloudStorage controller tests continue to pass
  • Confirmed the change follows the established pattern used throughout the codebase

Related

Addresses feedback from PR openshift#1937 discussion comment: openshift#1937 (comment)

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • openshift-velero-plugin-s3-auto-region-test-1.s3.us-east-1.amazonaws.com
    • Triggering command: /tmp/go-build75824382/b1625/controller.test -test.paniconexit0 -test.gocoverdir=/tmp/go-build75824382/b1625/gocoverdir -test.timeout=10m0s -test.coverprofile=/tmp/go-build75824382/b1625/_cover_.out (dns block)
    • Triggering command: /tmp/go-build75824382/b1646/aws.test -test.paniconexit0 -test.gocoverdir=/tmp/go-build75824382/b1646/gocoverdir -test.timeout=10m0s -test.coverprofile=/tmp/go-build75824382/b1646/_cover_.out (dns block)
  • openshift-velero-plugin-s3-auto-region-test-2.s3.us-east-1.amazonaws.com
    • Triggering command: /tmp/go-build75824382/b1646/aws.test -test.paniconexit0 -test.gocoverdir=/tmp/go-build75824382/b1646/gocoverdir -test.timeout=10m0s -test.coverprofile=/tmp/go-build75824382/b1646/_cover_.out (dns block)
  • openshift-velero-plugin-s3-auto-region-test-3.s3.us-east-1.amazonaws.com
    • Triggering command: /tmp/go-build75824382/b1646/aws.test -test.paniconexit0 -test.gocoverdir=/tmp/go-build75824382/b1646/gocoverdir -test.timeout=10m0s -test.coverprofile=/tmp/go-build75824382/b1646/_cover_.out (dns block)
  • openshift-velero-plugin-s3-auto-region-test-4.s3.us-east-1.amazonaws.com
    • Triggering command: /tmp/go-build75824382/b1646/aws.test -test.paniconexit0 -test.gocoverdir=/tmp/go-build75824382/b1646/gocoverdir -test.timeout=10m0s -test.coverprofile=/tmp/go-build75824382/b1646/_cover_.out (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

- Return error instead of just logging when final status update fails
- Add documentation test explaining the change
- Ensures controller-runtime's exponential backoff is triggered for status update failures

Addresses PR comment openshift#1937 discussion_r2330918689

Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
Copilot AI changed the title [WIP] https://github.com/openshift/oadp-operator/pull/1937#discussion_r2330918689 says should also back off for status update failures Add exponential backoff for CloudStorage status update failures Sep 9, 2025
Copilot AI requested a review from kaovilai September 9, 2025 03:53
@kaovilai
kaovilai marked this pull request as ready for review September 9, 2025 03:53
@coderabbitai

coderabbitai Bot commented Sep 9, 2025

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Comment @coderabbitai help to get the list of available commands and usage tips.

@kaovilai
kaovilai merged commit 68a7dcd into CloudStorage-LimitedRetries Sep 9, 2025
1 check passed
kaovilai added a commit that referenced this pull request Sep 9, 2025
…penshift#1937)

* Exponential Backoff for CloudStorage reconciler

- Add Conditions field to CloudStorageStatus for better observability
- Implement exponential backoff by returning errors on bucket operations
- Controller-runtime automatically handles retries (5ms to 1000s max)
- Add condition constants for type-safe reason strings
- Create mock bucket client for improved testing
- Add comprehensive tests for backoff behavior and conditions

Key improvements:
- Standard Kubernetes pattern using built-in workqueue backoff
- Self-healing: continues retrying with increasing delays
- Better observability through status conditions
- Per-item backoff: each CloudStorage CR gets independent retry timing

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Add exponential backoff for CloudStorage status update failures (#124)

* Initial plan

* Add exponential backoff for status update failures

- Return error instead of just logging when final status update fails
- Add documentation test explaining the change
- Ensures controller-runtime's exponential backoff is triggered for status update failures

Addresses PR comment openshift#1937 discussion_r2330918689

Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
kaovilai added a commit that referenced this pull request Sep 29, 2025
…queueAfter (openshift#1951)

* Exponential Backoff for CloudStorage reconciler

- Add Conditions field to CloudStorageStatus for better observability
- Implement exponential backoff by returning errors on bucket operations
- Controller-runtime automatically handles retries (5ms to 1000s max)
- Add condition constants for type-safe reason strings
- Create mock bucket client for improved testing
- Add comprehensive tests for backoff behavior and conditions

Key improvements:
- Standard Kubernetes pattern using built-in workqueue backoff
- Self-healing: continues retrying with increasing delays
- Better observability through status conditions
- Per-item backoff: each CloudStorage CR gets independent retry timing

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Add exponential backoff for CloudStorage status update failures (#124)

* Initial plan

* Add exponential backoff for status update failures

- Return error instead of just logging when final status update fails
- Add documentation test explaining the change
- Ensures controller-runtime's exponential backoff is triggered for status update failures

Addresses PR comment openshift#1937 discussion_r2330918689

Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>

---------

Co-authored-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
kaovilai added a commit that referenced this pull request Aug 5, 2026
The manager ClusterRole shipped with OADP only granted access to
datauploads/datauploads.status -- there was no datadownloads,
datadownloads/status, or events permission at all. Without this, the
controller would hit RBAC-denied errors reconciling any DataDownload,
regardless of image correctness, once a real velero restore actually
tried to drive it (which migtools/kubevirt-datamover-plugin#41 now
makes possible).

Synced config/kubevirt-datamover-controller_rbac/role.yaml and the
matching block in bundle/manifests/oadp-operator.clusterserviceversion.yaml
(serviceAccountName: oadp-kubevirt-datamover-controller-manager) to
byte-match config/rbac/role.yaml from
migtools/kubevirt-datamover-controller PR #124 (issue #73 phase 3,
commit 825d176), which added these rules on the source side but were
never pulled into OADP's bundled copy -- normally done via
`make update-kubevirt-datamover-manifests KUBEVIRT_DATAMOVER_PATH=...`,
done here by hand since no local checkout of that repo is available in
this environment.

This is a real, pre-existing gap (not introduced by this branch's other
changes) that this branch's e2e work would otherwise have hit blind, so
fixing it here rather than filing it separately.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 5, 2026
The manager ClusterRole shipped with OADP only granted access to
datauploads/datauploads.status -- there was no datadownloads,
datadownloads/status, or events permission at all. Without this, the
controller would hit RBAC-denied errors reconciling any DataDownload,
regardless of image correctness, once a real velero restore actually
tried to drive it (which migtools/kubevirt-datamover-plugin#41 now
makes possible).

Synced config/kubevirt-datamover-controller_rbac/role.yaml and the
matching block in bundle/manifests/oadp-operator.clusterserviceversion.yaml
(serviceAccountName: oadp-kubevirt-datamover-controller-manager) to
byte-match config/rbac/role.yaml from
migtools/kubevirt-datamover-controller PR #124 (issue #73 phase 3,
commit 825d176), which added these rules on the source side but were
never pulled into OADP's bundled copy -- normally done via
`make update-kubevirt-datamover-manifests KUBEVIRT_DATAMOVER_PATH=...`,
done here by hand since no local checkout of that repo is available in
this environment.

This is a real, pre-existing gap (not introduced by this branch's other
changes) that this branch's e2e work would otherwise have hit blind, so
fixing it here rather than filing it separately.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 5, 2026
The manager ClusterRole shipped with OADP only granted access to
datauploads/datauploads.status -- there was no datadownloads,
datadownloads/status, or events permission at all. Without this, the
controller would hit RBAC-denied errors reconciling any DataDownload,
regardless of image correctness, once a real velero restore actually
tried to drive it (which migtools/kubevirt-datamover-plugin#41 now
makes possible).

Synced config/kubevirt-datamover-controller_rbac/role.yaml and the
matching block in bundle/manifests/oadp-operator.clusterserviceversion.yaml
(serviceAccountName: oadp-kubevirt-datamover-controller-manager) to
byte-match config/rbac/role.yaml from
migtools/kubevirt-datamover-controller PR #124 (issue #73 phase 3,
commit 825d176), which added these rules on the source side but were
never pulled into OADP's bundled copy -- normally done via
`make update-kubevirt-datamover-manifests KUBEVIRT_DATAMOVER_PATH=...`,
done here by hand since no local checkout of that repo is available in
this environment.

This is a real, pre-existing gap (not introduced by this branch's other
changes) that this branch's e2e work would otherwise have hit blind, so
fixing it here rather than filing it separately.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 6, 2026
…sk resume gating

Both findings verified against actual code by kdm-controller/kdm-plugin
peer agents rather than guessed:
- VMB is orphaned on genuine Failed (non-canceled) DataUpload; issue #12
  closed but only delivered the success-path half.
- VM RIA resume gating counts only currently-discovered DataDownloads,
  not the VM's full expected volume count; accepted single-disk-only
  scope boundary for #124/#44, dormant since multi-disk restore itself
  is blocked on controller#73 phase4.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 6, 2026
…tnote

Backup/restore design sections (VirtualMachine RIA plugin, DataDownload
reconciler) previously only described pre-#124/#44 behavior. Add the
halt-at-restore/resume-on-siblings-complete mechanism to the actual
design prose, including the multi-disk scope boundary, and trim the
now-duplicated description out of the Implementation status section.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 6, 2026
…ecision

Mechanical: replace tab-indented nested list items with spaces for
consistent rendering (coderabbit minor finding).

Substantive, all verified by peer agents rather than assumed:
- Force-full-backup e2e coverage claim was wrong: e2e only tests the
  automatic max-incremental-backups threshold path, zero coverage for
  the manual force-full-backup annotation. Corrected both the E2E
  coverage bullet and the Open Questions resolution note.
- PVC RIA spec.selector omission: documented why it's very likely safe
  (kubevirt PVCs are always dynamically provisioned, never carry a
  selector) plus the empirical e2e signal, while flagging the one
  remaining unverified step (inspecting an actual backed-up PVC's YAML).
- PVC-sizing bound-PV-capacity fix is NOT on oadp-dev yet - it's part
  of the same unmerged PR124 as everything else in this doc, not
  separately shipped. Currently-shipping manifests record requested
  size, and the restore-side floor doesn't protect against the exact
  backend-bump scenario the fix targets, so pre-#124 backups need a
  migration/compat note before the fix merges. Added a warning next to
  the pvcSizes manifest schema example so readers don't assume a fixed
  meaning without checking which uploader version wrote it.
- Linked the real tracking issue for VMB-orphan-on-Failed
  (kubevirt-datamover-controller#168, filed by kdm-controller, who had
  full context) instead of a vague 'needs an issue' placeholder.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 24, 2026
…oy test

Live e2e validation of the restore-run-state-flip test (unpended in the
prior commit) surfaced two real bugs in the test itself, not in the
underlying openshift#169 fix:

1. The decoy DataDownload used a mismatched velero.io/restore-uid label
   but the SAME velero.io/restore-name as the real restore. The actual
   shipped openshift#169 fix (migtools/kubevirt-datamover-controller#124,
   allSiblingDataDownloadsCompleted) correlates siblings by restore-name,
   not restore-uid -- an earlier draft used restore-uid, but #124's
   restore-name-based version is what shipped. The decoy now uses a
   genuinely different restore-name to correctly simulate a stale sibling
   from a different restore attempt.

2. The decoy's status was marked Failed via Status().Update(), which
   unconditionally 404s: the DataDownload CRD (v2alpha1) has no status
   subresource registered at all (confirmed via
   `oc get crd datadownloads.velero.io -o jsonpath=...subresources`).
   kubevirt_datadownload_controller.go itself only ever uses plain
   r.Update() for this same reason. Switched to plain Update(), wrapped in
   retry.RetryOnConflict since kdm-controller concurrently reconciles the
   decoy (New -> Accepted) the moment it's created, racing a bare Update
   against its bumped resourceVersion.

Cross-validated live against a GCP cluster with peer sessions working the
kubevirt-datamover-controller and kubevirt-datamover-plugin repos; the
controller-side peer confirmed both findings against their own source and
an existing code comment documenting the missing status subresource.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 25, 2026
…oy test

Live e2e validation of the restore-run-state-flip test (unpended in the
prior commit) surfaced two real bugs in the test itself, not in the
underlying openshift#169 fix:

1. The decoy DataDownload used a mismatched velero.io/restore-uid label
   but the SAME velero.io/restore-name as the real restore. The actual
   shipped openshift#169 fix (migtools/kubevirt-datamover-controller#124,
   allSiblingDataDownloadsCompleted) correlates siblings by restore-name,
   not restore-uid -- an earlier draft used restore-uid, but #124's
   restore-name-based version is what shipped. The decoy now uses a
   genuinely different restore-name to correctly simulate a stale sibling
   from a different restore attempt.

2. The decoy's status was marked Failed via Status().Update(), which
   unconditionally 404s: the DataDownload CRD (v2alpha1) has no status
   subresource registered at all (confirmed via
   `oc get crd datadownloads.velero.io -o jsonpath=...subresources`).
   kubevirt_datadownload_controller.go itself only ever uses plain
   r.Update() for this same reason. Switched to plain Update(), wrapped in
   retry.RetryOnConflict since kdm-controller concurrently reconciles the
   decoy (New -> Accepted) the moment it's created, racing a bare Update
   against its bumped resourceVersion.

Cross-validated live against a GCP cluster with peer sessions working the
kubevirt-datamover-controller and kubevirt-datamover-plugin repos; the
controller-side peer confirmed both findings against their own source and
an existing code comment documenting the missing status subresource.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 26, 2026
…oy test

Live e2e validation of the restore-run-state-flip test (unpended in the
prior commit) surfaced two real bugs in the test itself, not in the
underlying openshift#169 fix:

1. The decoy DataDownload used a mismatched velero.io/restore-uid label
   but the SAME velero.io/restore-name as the real restore. The actual
   shipped openshift#169 fix (migtools/kubevirt-datamover-controller#124,
   allSiblingDataDownloadsCompleted) correlates siblings by restore-name,
   not restore-uid -- an earlier draft used restore-uid, but #124's
   restore-name-based version is what shipped. The decoy now uses a
   genuinely different restore-name to correctly simulate a stale sibling
   from a different restore attempt.

2. The decoy's status was marked Failed via Status().Update(), which
   unconditionally 404s: the DataDownload CRD (v2alpha1) has no status
   subresource registered at all (confirmed via
   `oc get crd datadownloads.velero.io -o jsonpath=...subresources`).
   kubevirt_datadownload_controller.go itself only ever uses plain
   r.Update() for this same reason. Switched to plain Update(), wrapped in
   retry.RetryOnConflict since kdm-controller concurrently reconciles the
   decoy (New -> Accepted) the moment it's created, racing a bare Update
   against its bumped resourceVersion.

Cross-validated live against a GCP cluster with peer sessions working the
kubevirt-datamover-controller and kubevirt-datamover-plugin repos; the
controller-side peer confirmed both findings against their own source and
an existing code comment documenting the missing status subresource.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai added a commit that referenced this pull request Aug 27, 2026
…oy test

Live e2e validation of the restore-run-state-flip test (unpended in the
prior commit) surfaced two real bugs in the test itself, not in the
underlying openshift#169 fix:

1. The decoy DataDownload used a mismatched velero.io/restore-uid label
   but the SAME velero.io/restore-name as the real restore. The actual
   shipped openshift#169 fix (migtools/kubevirt-datamover-controller#124,
   allSiblingDataDownloadsCompleted) correlates siblings by restore-name,
   not restore-uid -- an earlier draft used restore-uid, but #124's
   restore-name-based version is what shipped. The decoy now uses a
   genuinely different restore-name to correctly simulate a stale sibling
   from a different restore attempt.

2. The decoy's status was marked Failed via Status().Update(), which
   unconditionally 404s: the DataDownload CRD (v2alpha1) has no status
   subresource registered at all (confirmed via
   `oc get crd datadownloads.velero.io -o jsonpath=...subresources`).
   kubevirt_datadownload_controller.go itself only ever uses plain
   r.Update() for this same reason. Switched to plain Update(), wrapped in
   retry.RetryOnConflict since kdm-controller concurrently reconciles the
   decoy (New -> Accepted) the moment it's created, racing a bare Update
   against its bumped resourceVersion.

Cross-validated live against a GCP cluster with peer sessions working the
kubevirt-datamover-controller and kubevirt-datamover-plugin repos; the
controller-side peer confirmed both findings against their own source and
an existing code comment documenting the missing status subresource.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants