Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hook status to backup/restore CR #7117

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

allenxu404
Copy link
Contributor

@allenxu404 allenxu404 commented Nov 17, 2023

Thank you for contributing to Velero!

Please add a summary of your change

  • Add a hookTracker to backup/restore to track hooks execution status. Hook tracker is extensible to accommodate additional fields as needs develop.
  • Add hook execution status to backup/restore describer
  • Add UTs

Does your change fix a particular issue?

Fixes #6567

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (b276564) 61.66% compared to head (5d1a632) 61.80%.
Report is 15 commits behind head on main.

Files Patch % Lines
pkg/cmd/util/output/backup_describer.go 0.00% 6 Missing ⚠️
pkg/cmd/util/output/restore_describer.go 0.00% 5 Missing ⚠️
pkg/restore/restore.go 68.75% 4 Missing and 1 partial ⚠️
pkg/cmd/util/output/backup_structured_describer.go 0.00% 4 Missing ⚠️
internal/hook/wait_exec_hook_handler.go 66.66% 2 Missing ⚠️
pkg/backup/item_backupper.go 33.33% 1 Missing and 1 partial ⚠️
pkg/backup/backup.go 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7117      +/-   ##
==========================================
+ Coverage   61.66%   61.80%   +0.13%     
==========================================
  Files         258      259       +1     
  Lines       27636    27859     +223     
==========================================
+ Hits        17042    17218     +176     
- Misses       9400     9438      +38     
- Partials     1194     1203       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@allenxu404 allenxu404 marked this pull request as ready for review November 20, 2023 03:03
@reasonerjt
Copy link
Contributor

The error of the hooks should be passed to the controllers in a more explicit way, and reflect in the Status of the CRs

@allenxu404 allenxu404 changed the title Add field hookFailed to backup/restore describer Add hook status to backup/restore CR Nov 24, 2023
config/crd/v1/bases/velero.io_backups.yaml Outdated Show resolved Hide resolved
internal/hook/hook_tracker.go Outdated Show resolved Hide resolved
internal/hook/hook_tracker.go Outdated Show resolved Hide resolved
internal/hook/hook_tracker.go Show resolved Hide resolved
@allenxu404 allenxu404 force-pushed the issue6567 branch 2 times, most recently from 8a3b759 to 11d1857 Compare November 27, 2023 09:40
@allenxu404
Copy link
Contributor Author

Sync with @anshulahuja98, this PR solely adds the "hooksAttempted" and "hooksFailed" fields to backup/restore CR to provide initial hook execution metadata. Tracking additional details like hook error information and skipped hooks can be supported in the future via a separate follow-up effort.

Once this PR is merged, I will create a new issue to track these potential enhancements.

pkg/backup/backup.go Outdated Show resolved Hide resolved
Signed-off-by: allenxu404 <qix2@vmware.com>
@reasonerjt reasonerjt merged commit 85482ae into vmware-tanzu:main Nov 28, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding hook command status in backup/restore status or describe command
3 participants