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.

Sign up for GitHub

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

Jump to bottom

[RHELC-1154] Try to rollback all changes even if one of them fails #912

Merged
merged 1 commit into from
Nov 2, 2023
Merged

[RHELC-1154] Try to rollback all changes even if one of them fails #912

merged 1 commit into from
Nov 2, 2023

Conversation

abadger
Copy link
Member

@abadger abadger commented Sep 13, 2023 •
edited by bocekm
Loading

Previously, if a RestorableChange managed by BackupController raised an exception on restore, then we would stop rollback. This change makes it so we try to restore every change even if previous ones have failed.

Jira Issues: RHELC-1154

Checklist

  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • GitHub label has been added to help with Release notes
  • PR title explains the change from the user's point of view
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending if relevant

@abadger abadger added the tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. label Sep 13, 2023
@has-bot
Copy link
Member

has-bot commented Sep 13, 2023

/packit test --labels tier0

@oamg/conversions-qe please review results and provide ack.

@codecov
Copy link

codecov bot commented Sep 13, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (dfbb8ee) 93.78% compared to head (1ad3697) 94.24%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #912      +/-   ##
==========================================
+ Coverage   93.78%   94.24%   +0.45%     
==========================================
  Files          47       47              
  Lines        4345     4341       -4     
  Branches      769      768       -1     
==========================================
+ Hits         4075     4091      +16     
+ Misses        192      174      -18     
+ Partials       78       76       -2
Flag Coverage Δ
centos-linux-7 89.16% <89.47%> (+0.45%) ⬆️
centos-linux-8 90.23% <89.47%> (+0.45%) ⬆️
centos-linux-9 90.29% <88.88%> (+0.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
convert2rhel/backup.py 95.65% <89.47%> (+5.05%) ⬆️

... and 1 file with indirect coverage changes

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

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 13, 2023
View reviewed changes
convert2rhel/backup.py Fixed Show fixed Hide fixed
@abadger abadger added the kind/bug-fix A bug has been fixed label Sep 13, 2023
@abadger
Copy link
Member Author

abadger commented Sep 13, 2023

/packit test --labels tier0

@bocekm bocekm changed the title [RHELC-1121] Try to rollback all changes even if one of them fails. [RHELC-1154] Try to rollback all changes even if one of them fails. Sep 13, 2023
@abadger
Copy link
Member Author

abadger commented Sep 13, 2023 •
edited
Loading

Ready for review. I believe codecov's warning about coverage of the patch is due to an old version of coverage. When run locally, there is only one line (not in the patch) which is uncovered in pkghandler.py whereas the output of the github Action shows multiple uncovered lines.

Coverage report for backup.py locally:
convert2rhel/backup.py 225 1 97 1 99% 380

pr-watson
pr-watson reviewed Sep 14, 2023
View reviewed changes
convert2rhel/backup.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Sep 14, 2023
View reviewed changes
convert2rhel/backup.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@r0x0d r0x0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Haven't tested that in a VM yet, but made one comment about what we want to log in case of error.

@abadger
Copy link
Member Author

abadger commented Sep 14, 2023

/packit test --labels tier0

Venefilyn
Venefilyn reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just needs unit tests rebased

r0x0d
r0x0d reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@r0x0d r0x0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@abadger abadger force-pushed the ignore-failures-on-restore branch 2 times, most recently from b68c091 to 0f6e925 Compare September 28, 2023 22:49
@abadger
Copy link
Member Author

abadger commented Sep 29, 2023

Rebased.

@abadger
Copy link
Member Author

abadger commented Sep 29, 2023

/packit build --labels tier0

@abadger
Copy link
Member Author

abadger commented Sep 29, 2023

/packit test --labels tier0

@abadger
Copy link
Member Author

abadger commented Sep 30, 2023

/packit retest-failed

1 similar comment
@abadger
Copy link
Member Author

abadger commented Oct 1, 2023

/packit retest-failed

@r0x0d
Copy link
Member

r0x0d commented Oct 2, 2023 •
edited
Loading

@abadger, the test failures here are not related to the code changes you did? Haven't gone through them yet, just asking here first to see if you had the chance to take a look at it first.

@abadger @pr-watson
Change the way Restores work on error.
1ad3697 
* Previously, if a RestorableChange managed by BackupController raised an exception on restore, then
  we would stop rollback.  This change makes it so we try to restore every change even if previous
  ones have failed.
* Add the name of the RestorableChange which failed. We can retrieve the class name from the Change
  instance to display in the error message.  This isn't perfect but it identifies where the error
  occurred a little better than just the exception message.
  Thanks to Rodolfo for suggesting this.

Co-authored-by: Preston Watson <prwatson@redhat.com>
@abadger
Copy link
Member Author

abadger commented Oct 31, 2023

Rebased. (Note: I haven't merged this myself because I don't know how it interacts with the release schedule. Feel free to merge when the time is right).

@bocekm
Copy link
Member

bocekm commented Nov 2, 2023

Thanks, @abadger. Merging.

@bocekm bocekm merged commit 0c8decd into oamg:main Nov 2, 2023
16 of 49 checks passed
@abadger abadger deleted the ignore-failures-on-restore branch November 6, 2023 08:38
@Venefilyn Venefilyn changed the title [RHELC-1154] Try to rollback all changes even if one of them fails. [RHELC-1154] Try to rollback all changes even if one of them fails Nov 14, 2023
@pr-watson pr-watson mentioned this pull request Dec 8, 2023
Merged
r0x0d added a commit to r0x0d/convert2rhel that referenced this pull request Feb 12, 2024
@r0x0d
Remove backup controller partition
e38cfe5 
The workaround was introduced in oamg#912 and oamg#869, but it is no longer
necessary as all legacy rollback items are now ported to be an instance
tracked by the backup controller.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
@r0x0d r0x0d mentioned this pull request Feb 12, 2024
Merged
10 tasks
r0x0d added a commit to r0x0d/convert2rhel that referenced this pull request Feb 22, 2024
@r0x0d
Remove backup controller partition
50649b4 
The workaround was introduced in oamg#912 and oamg#869, but it is no longer
necessary as all legacy rollback items are now ported to be an instance
tracked by the backup controller.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
r0x0d added a commit to r0x0d/convert2rhel that referenced this pull request Feb 28, 2024
@r0x0d
Remove backup controller partition
b3db692 
The workaround was introduced in oamg#912 and oamg#869, but it is no longer
necessary as all legacy rollback items are now ported to be an instance
tracked by the backup controller.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
r0x0d added a commit to r0x0d/convert2rhel that referenced this pull request Mar 6, 2024
@r0x0d
Remove backup controller partition
fee331a 
The workaround was introduced in oamg#912 and oamg#869, but it is no longer
necessary as all legacy rollback items are now ported to be an instance
tracked by the backup controller.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Venefilyn pushed a commit that referenced this pull request Mar 7, 2024
@r0x0d
[RHELC-1376] Remove backup controller partition (#1096)
3db00f5 
The workaround was introduced in #912 and #869, but it is no longer
necessary as all legacy rollback items are now ported to be an instance
tracked by the backup controller.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
jochapma pushed a commit to jochapma/convert2rhel that referenced this pull request Mar 11, 2024
@r0x0d @jochapma
[RHELC-1376] Remove backup controller partition (oamg#1096)
a2f7160 
The workaround was introduced in oamg#912 and oamg#869, but it is no longer
necessary as all legacy rollback items are now ported to be an instance
tracked by the backup controller.

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

@pr-watson pr-watson pr-watson left review comments

@bocekm bocekm bocekm left review comments

@Venefilyn Venefilyn Venefilyn left review comments

@r0x0d r0x0d r0x0d approved these changes

Assignees
No one assigned
Labels
kind/bug-fix A bug has been fixed tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@abadger @has-bot @r0x0d @bocekm @Venefilyn @pr-watson