Podman 6: Automatic BoltDB to SQLite migration#28569
Podman 6: Automatic BoltDB to SQLite migration#28569mheon wants to merge 3 commits intocontainers:mainfrom
Conversation
|
@mheon tests aren't happy, most notably the FreeBSD cross |
Honny1
left a comment
There was a problem hiding this comment.
I have some comments. Also, I think you'll need to run make vendor.
| // Add and save the container | ||
| if node.container.config.Pod == "" { | ||
| if err := sqliteState.AddContainer(node.container); err != nil { | ||
| if errors.Is(err, define.ErrPodExists) { |
There was a problem hiding this comment.
Should be here?
| if errors.Is(err, define.ErrPodExists) { | |
| if errors.Is(err, define.ErrCtrExists) { |
| // We don't actually *use* the pod in this operation, other than its ID... | ||
| // So fake it | ||
| pod := new(Pod) | ||
| pod.config = new(PodConfig) | ||
| pod.config.ID = node.container.config.Pod | ||
| pod.valid = true |
There was a problem hiding this comment.
Is that correct? This pod isn't used anywhere, not even for ID.
There was a problem hiding this comment.
Excellent point. This was needed in 5.8, but in 6.0 I removed the dedicated AddContainerToPod function in the database and now it's not necessary to fake a pod
| @@ -0,0 +1,598 @@ | |||
| //go:build !remote | |||
There was a problem hiding this comment.
| //go:build !remote | |
| //go:build !remote && (linux || freebsd) |
| @@ -0,0 +1,391 @@ | |||
| //go:build !remote | |||
There was a problem hiding this comment.
| //go:build !remote | |
| //go:build !remote && (linux || freebsd) |
| // So we need a completely new state from disk to see what the user set. | ||
| newCfg, err := config.New(nil) | ||
| if err != nil { | ||
| return fmt.Errorf("reloading configuration to check database backend in use") |
| if err := r.checkCanMigrate(); err != nil { | ||
| switch { | ||
| case errors.Is(err, errCannotMigrateNoBolt): | ||
| fmt.Printf("No migration is necessary: %v", err) |
There was a problem hiding this comment.
| fmt.Printf("No migration is necessary: %v", err) | |
| fmt.Printf("No migration is necessary: %v\n", err) |
|
|
||
| // ocicniPortsToNetTypesPorts convert the old port format to the new one | ||
| // while deduplicating ports into ranges | ||
| func ocicniPortsToNetTypesPorts(ports []types.OCICNIPortMapping) []types.PortMapping { |
There was a problem hiding this comment.
| func ocicniPortsToNetTypesPorts(ports []types.OCICNIPortMapping) []types.PortMapping { | |
| //nolint:staticcheck | |
| func ocicniPortsToNetTypesPorts(ports []types.OCICNIPortMapping) []types.PortMapping { |
| // 2) protocol | ||
| // 3) hostPort | ||
| // 4) container port | ||
| func compareOCICNIPorts(i, j types.OCICNIPortMapping) bool { |
There was a problem hiding this comment.
| func compareOCICNIPorts(i, j types.OCICNIPortMapping) bool { | |
| //nolint:staticcheck | |
| func compareOCICNIPorts(i, j types.OCICNIPortMapping) bool { |
If we're going to maintain migration capability for the full lifespan of 6 - and I think we're going to have to - the only sane options are a separate binary that exclusively performs migrations, or re-adding BoltDB code - in a very minimal way - to allow us to perform migrations within the standard 6 binary. After attempting the separate binary approach, results are not promising - it's impossible to strip enough out to make a truly small binary that still does what we need to perform a migration. That leaves re-adding BoltDB code. This adds a minimal version of the BoltDB code that no longer claims to be a valid State (freeing us from the requirement of continued maintenance - we should never touch these bits again until they get removed in 7) which has just enough to get every container, pod, and volume in the DB, so we can migrate them to SQLite. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
This is gated behind a new option in `podman system migrate`, `--migrate-db`, or by a system restart being performed. BoltDB support was removed in Podman 6, so we are certain that, when we start Podman, a SQLite state is in use. However, if we also detect a valid BoltDB state, we will attempt a migration. Migration is performed by retrieving all volumes, pods, and containers (in that order, to ensure there are no dependency conflicts) from the Bolt database, when adding them to the SQLite database. If there is a conflict - IE, a container exists in both SQLite and Bolt - we skip migration for that object. The old DB is then renamed so we do not try to migrate it again. Our ability to test complex migration scenarios is limited, but this should handle simple migrations easily. This is a heavily adapted version of containers#27660 rebuilt to work with Podman 6.0. This cannot be tested automatically, as the ability to create Bolt databases has been entirely removed with Podman 6. Signed-off-by: Matt Heon <matthew.heon@pm.me>
It makes more sense for the callers. Signed-off-by: Paul Holzinger <pholzing@redhat.com> <MH: Fix cherry-pick conflicts> Signed-off-by: Matthew Heon <matthew.heon@pm.me>
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
Honny1
left a comment
There was a problem hiding this comment.
LGTM, just a non-blocking comment.
CI is super flaky now.
| if ctrError != nil { | ||
| logrus.Errorf("Migrating containers to SQLite: %v", ctrError) | ||
| } | ||
| ctrError = fmt.Errorf("migrating container %s: %w", id, err) |
There was a problem hiding this comment.
Non-blocking: I would use errors.Join to aggregate errors.
@Luap99 I don't think we are vulnerable to the issues we have in 5.8 as there is no possibility of ever getting a BoltDB backend for Podman, but eyes on this would be appreciated.
This is an alternative to https://github.com/chttps://github.com/containers/podman/pull/28553ontainers/podman/pull/28553 and, instead of warning folks to use an external binary or downgrade, adds just enough BoltDB back in to do a migration.
The bad news: I don't think we can actually test this in CI. The ability to make new Bolt databases, and to interact with them in a meaningful way, is entirely gone. Though, now that I think about it, maybe I can ship a Bolt database in the tests directory and do a test migration off it?
Does this PR introduce a user-facing change?