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

Keep previous behavior for persistent binded paths #1984

Conversation

davidcassany
Copy link
Contributor

@davidcassany davidcassany commented Feb 29, 2024

With this patch the current (stable release) behavior of binded persistent paths is kept.

This commit adds a new rsync wrapper method (MirrorData) which includes the --delete flag. Named mirror because it actually causes the target and source to have the same exact data causing the deletion of any file already present in target that is not present in source. This is the behavior we want when dumping the image root-tree to a new snapshot.

Current behavior updates at early boot all files in a given path from the image to the persisted path. This includes a couple of non obvious behaviors:

  • Reverts changes done in files that were originally part of the image on every boot.
  • Files included in path configured as persistent that are present in image A but not present in image B and kept in the persistent storage when upgrading from A to B.

Fixes #1982

@davidcassany davidcassany requested a review from a team as a code owner February 29, 2024 14:41
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 73.05%. Comparing base (9538960) to head (995c623).

Files Patch % Lines
pkg/action/mount.go 42.85% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1984      +/-   ##
==========================================
+ Coverage   73.04%   73.05%   +0.01%     
==========================================
  Files          74       74              
  Lines        8713     8719       +6     
==========================================
+ Hits         6364     6370       +6     
  Misses       1831     1831              
  Partials      518      518              

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

Copy link
Member

@fgiudici fgiudici left a comment

Choose a reason for hiding this comment

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

Wondering if could be worth to add a unit test in utils_test.go to cover MirrorData... but maybe not after all, as is just the old SyncData (and the same as the actual SyncData but with the --delete 🤷🏼 ).
LGTM!

Copy link
Contributor

@ldevulder ldevulder left a comment

Choose a reason for hiding this comment

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

Let's go to merge and test!

@davidcassany
Copy link
Contributor Author

Wondering if could be worth to add a unit test in utils_test.go to cover MirrorData... but maybe not after all, as is just the old SyncData (and the same as the actual SyncData but with the --delete 🤷🏼 ). LGTM!

Yes I also wondered about it, then I realized that all lines of this method are actually covered by unit tests (kudos to codecov report), the only four lines that are not covered, are the error exits for mount command and they are already not covered...

@davidcassany davidcassany merged commit e56244d into rancher:main Feb 29, 2024
17 of 19 checks passed
@davidcassany davidcassany deleted the keep_previous_behavior_for_persistent_storage branch February 29, 2024 17:07
@ldevulder
Copy link
Contributor

Yes I also wondered about it, then I realized that all lines of this method are actually covered by unit tests

And I also think that in case of issue the Elemental CI should trap it quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Root password lost after an OS upgrade
4 participants