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

Refactor to switch to snapshotter interface #1906

Merged
merged 11 commits into from
Jan 31, 2024

Conversation

davidcassany
Copy link
Contributor

@davidcassany davidcassany commented Jan 17, 2024

This commit adopts snapshotter interface in install, reset and upgrade commands. The change implies changes to the respective specs, grub configuration and dracut modules.

This commit also changes the behavior of recovery system upgrades. Now recovery upgrades are an optional step of a system upgrade. Recovery image can't be upgraded without upgrading the active system.

Finally build-disk command is also changed to be better aligned with upgrade and install procedures. Expandable disks are an unprivileged build and non expandable ones require privileges as they relay on snapshotter.

Fixes #1874
Fixes #1921
Fixes #1922

@davidcassany davidcassany requested a review from a team as a code owner January 17, 2024 23:10
@davidcassany davidcassany marked this pull request as draft January 17, 2024 23:10
@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch 3 times, most recently from 428ac6a to 00e7851 Compare January 18, 2024 11:33
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2024

Codecov Report

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

Comparison is base (c2d9965) 73.91% compared to head (e0255d5) 72.36%.

Files Patch % Lines
pkg/action/upgrade.go 59.77% 60 Missing and 12 partials ⚠️
pkg/snapshotter/loopdevice.go 57.60% 30 Missing and 9 partials ⚠️
pkg/action/reset.go 55.00% 27 Missing and 9 partials ⚠️
pkg/action/install.go 80.00% 19 Missing and 3 partials ⚠️
pkg/action/build-disk.go 71.01% 14 Missing and 6 partials ⚠️
pkg/types/v1/config.go 69.09% 9 Missing and 8 partials ⚠️
pkg/mocks/utils.go 53.12% 10 Missing and 5 partials ⚠️
pkg/elemental/elemental.go 89.18% 7 Missing and 1 partial ⚠️
pkg/config/config.go 88.52% 5 Missing and 2 partials ⚠️
pkg/types/v1/snapshotter.go 70.00% 4 Missing and 2 partials ⚠️
... and 7 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1906      +/-   ##
==========================================
- Coverage   73.91%   72.36%   -1.55%     
==========================================
  Files          71       72       +1     
  Lines        7762     7947     +185     
==========================================
+ Hits         5737     5751      +14     
- Misses       1600     1724     +124     
- Partials      425      472      +47     

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

@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch 9 times, most recently from 4452069 to 4114b1a Compare January 23, 2024 09:45
@kkaempf
Copy link
Contributor

kkaempf commented Jan 23, 2024

We should do a code walk-through to get this merged 🤔

@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch 2 times, most recently from 833a7e0 to 299a2b4 Compare January 23, 2024 16:29
Copy link
Contributor

@frelon frelon left a comment

Choose a reason for hiding this comment

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

Big PR, so it's hard to get to everything.. I left some comments but generally looks very good!

We could also reconsider setting constants.MaxSnaps = 2 as discussed yesterday.

Good work!

pkg/constants/constants.go Outdated Show resolved Hide resolved
pkg/elemental/elemental.go Show resolved Hide resolved
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.

Skimmed through the changes after yesterday code walk-through, looks good... well, actually great job!
Hope to see this merged soon, so we can start testing/using it 🥳
Nice to see the snapshotter interface getting alive, code looks more readable too.

@davidcassany
Copy link
Contributor Author

We could also reconsider setting constants.MaxSnaps = 2 as discussed yesterday.

Yes I have to apply these little changes yet. As soon as I manage to manually upgrade an elemental cluster from current staging to a Dev including this PR I'll mark this PR ready to be merged and arrange this little details we discussed yesterday.

@davidcassany davidcassany added the all-distros Runs CI for all available example images label Jan 25, 2024
@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch 2 times, most recently from c183404 to 13d07de Compare January 29, 2024 07:32
@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch from fd0c44e to e14ae3e Compare January 29, 2024 22:35
@davidcassany davidcassany marked this pull request as ready for review January 30, 2024 08:41
@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch 2 times, most recently from 1a337bd to e0255d5 Compare January 30, 2024 10:21
This commit adopts snapshotter interface in install,
reset and upgrade commands. The change implies changes
to the respective specs, grub configuration and dracut
modules.

This commit also changes the behavior of recovery system
upgrades. Now recovery upgrades are an optional step
of a system upgrade. Recovery image can't be upgraded
without upgrading the active system.

Finally build-disk command is also changed to be better
aligned with upgrade and install procedures. Expandable
disks are an unprivileged build and non expandable ones
require privileges as they relay on snapshotter.

Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch from e0255d5 to 9b1a0e9 Compare January 30, 2024 12:57
@davidcassany davidcassany removed the all-distros Runs CI for all available example images label Jan 30, 2024
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany force-pushed the refactor_to_snapshotters branch from b651f00 to 9d1655b Compare January 30, 2024 14:43
@davidcassany
Copy link
Contributor Author

davidcassany commented Jan 30, 2024

Finally got green run on the CI here and managed to run a regular UI based upgrade from staging to this current head.

Dev env requires also rancher/elemental#1186 to pass tests

@@ -18,7 +18,7 @@ package common

import "flag"

const upImg = "ghcr.io/rancher/elemental-toolkit/elemental-green:v1.3.0"
const upImg = "ghcr.io/rancher/elemental-toolkit/elemental-green:v0.10.7-g3e4a3c56"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why go back to such an old release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To keep grub compatibility, this is compatible with v1.1.0 but not really with v1.2 or v1.3. In fact I think we should stick with v1.1.0 here, but not sure we ever published it to the registry. This was one of the things I wanted to review after merging this.

The issue is with the ${mode} and ${img} variables in grub, in v1.2 and v1.3 we assume the image is referred by the ${mode} and the dracut module computes the path. This is hard make it compatible with v1.1.0, v1.2.0 and the upcoming v2.0.x as this implies having several paths adjustments.

If we assume the ${img} and is the full path and ${mode} is just to discriminate recovery vs active args this turns to be easier in terms of backwards compatibility. We have to keep in mind in the grub domain further changes are around the corner with the btrfs snapshotter and that our backward compatibility constraint should stick to v1.1.0 version, as being the latest released elemental-toolkit version. Because of that I took a very conservative approach, because we are likely to need revising it in any case.

@davidcassany davidcassany merged commit 4cab6a0 into rancher:main Jan 31, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment