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

Fix Recovery only upgrades #1988

Merged
merged 12 commits into from
Mar 5, 2024

Conversation

anmazzotti
Copy link
Contributor

@anmazzotti anmazzotti commented Mar 1, 2024

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2024

Codecov Report

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

Project coverage is 72.55%. Comparing base (b424822) to head (5969c0e).

Files Patch % Lines
pkg/elemental/elemental.go 63.63% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1988   +/-   ##
=======================================
  Coverage   72.54%   72.55%           
=======================================
  Files          76       76           
  Lines        8896     8902    +6     
=======================================
+ Hits         6454     6459    +5     
- Misses       1909     1910    +1     
  Partials      533      533           

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

@anmazzotti anmazzotti marked this pull request as ready for review March 1, 2024 14:40
@anmazzotti anmazzotti requested a review from a team as a code owner March 1, 2024 14:40
@anmazzotti anmazzotti marked this pull request as draft March 1, 2024 15:54
@anmazzotti
Copy link
Contributor Author

Back to draft since this seems to be breaking the btrfs install.

pkg/utils/fs.go Show resolved Hide resolved
@@ -294,16 +294,16 @@ func UnmountFileSystemImage(c v1.Config, img *v1.Image) error {
// CreateFileSystemImage creates the image file for the given image. An root tree path
// can be used to determine the image size and the preload flag can be used to create an image
// including the root tree data.
func CreateFileSystemImage(c v1.Config, img *v1.Image, rootDir string, preload bool) error {
c.Logger.Infof("Creating image %s", img.File)
func CreateFileSystemImage(c v1.Config, img *v1.Image, rootDir string, preload bool, excludes ...string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if do we have a need to actually bring this excluding paths up that far. Do we have any call of CreateImageFromTree where we don't set it to the defaults or were this could have an undesired effect? Probably the same applies for CreateFileSystemImage.

I mean probably it is just enough adding the default excludes inside CreateFileSystemImage and on CreateImageFromTree and there is no need to map this change that far up. I think having it ported all way up males sense if we make use of it at action level, otherwise probably we can keep it minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I didn't want to sneak it in because I was afraid it could have unexpected effects.
This is also used by the build-disk, however I agree it should not have any undesired effect.

I think we can hardcode this in CreateImageFromTree, since we need to pass the exclusion list to CreateFileSystemImage and later utils.SyncData.

Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
@anmazzotti
Copy link
Contributor Author

Did a full re-test after latest fixes and rebasing:

  1. Deploy a cluster with 2 nodes (one btrfs, one loopdevice)
  2. Trigger a OS+Recovery upgrade using ManagedOSImage
  3. Trigger a RecoveryOnly upgrade with ManagedOSImage

Seems to all work good now!

@anmazzotti anmazzotti marked this pull request as ready for review March 5, 2024 10:51
Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

Nice, looks good 👍

@@ -696,7 +696,12 @@ var _ = Describe("Utils", Label("utils"), func() {
Expect(err).ShouldNot(HaveOccurred())
Expect(size).To(Equal(int64(3072)))
})
It("Returns the expected size of a test folder", func() {
It("Returns the size of a test folder when skipping subdirectories", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, loving this sort of tests. They are super useful in case someday in the future we change some apparently simple utility.

@anmazzotti anmazzotti merged commit b266360 into rancher:main Mar 5, 2024
13 of 15 checks passed
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.

None yet

3 participants