Use paths written in DB instead if they differ from our defaults #1918
Conversation
When we create a Libpod database, we store a number of runtime configuration fields in it. If we can retrieve those, we can use them to configure the runtime to match the DB instead of inbuilt defaults, helping to ensure that we don't error in cases where our compiled-in defaults changed. Signed-off-by: Matthew Heon <mheon@redhat.com>
When we configure a runtime, we now will need to hit the DB early on, so we can verify the paths we're going to use for c/storage are correct. Signed-off-by: Matthew Heon <mheon@redhat.com>
Previously, we implicitly validated runtime configuration against what was stored in the database as part of database init. Make this an explicit step, so we can call it after the database has been initialized. This will allow us to retrieve paths from the database and use them to overwrite our defaults if they differ. Signed-off-by: Matthew Heon <mheon@redhat.com>
To configure runtime fields from the database, we need to know whether they were explicitly overwritten by the user (we don't want to overwrite anything that was explicitly set). Store a struct containing whether the variables we'll grab from the DB were explicitly set by the user so we know what we can and can't overwrite. This determines whether libpod runtime and static dirs were set via config file in a horribly hackish way (double TOML decode), but I can't think of a better way, and it shouldn't be that expensive as the libpod config is tiny. Signed-off-by: Matthew Heon <mheon@redhat.com>
If the DB contains default paths, and the user has not explicitly overridden them, use the paths in the DB over our own defaults. The DB validates these paths, so it would error and prevent operation if they did not match. As such, instead of erroring, we can use the DB's paths instead of our own. Signed-off-by: Matthew Heon <mheon@redhat.com>
Previous commits ensured that we would use database-configured paths if not explicitly overridden. However, our runtime generation did unconditionally override storage config, which made this useless. Move rootless storage configuration setup to libpod, and change storage setup so we only override if a setting is explicitly set, so we can still override what we want. Signed-off-by: Matthew Heon <mheon@redhat.com>
We already create the locks directory as part of the libpod runtime's init - no need to do it again as part of BoltDB's init. Signed-off-by: Matthew Heon <mheon@redhat.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
Tested this locally, and it seems to resolve the breaking change we saw in 0.11.1 |
Ensure we don't break the unit tests by creating a locks directory (which, prior to the last commit, would be created by BoltDB state init). Signed-off-by: Matthew Heon <mheon@redhat.com>
Signed-off-by: Matthew Heon <mheon@redhat.com>
pkg/util/utils.go
Outdated
| @@ -313,33 +313,31 @@ func getTomlStorage(storeOptions *storage.StoreOptions) *tomlConfig { | |||
| return config | |||
| } | |||
|
|
|||
| // GetDefaultStoreOptions returns the storage ops for containers | |||
| func GetDefaultStoreOptions() (storage.StoreOptions, error) { | |||
There was a problem hiding this comment.
I think the reason for having this function here is that pkg/util is vendored inside buildah and GetDefaultStoreOptions is used there as well. Do you think it makes sense if we leave it?
Anyway, should not be too difficult to reimplement in Buildah if it is dropped
There was a problem hiding this comment.
+1 to a Buildah dependency on this. Great catch @giuseppe
There was a problem hiding this comment.
Sure - I saw no more internal users, so I modified it to suit our uses, but if Buildah is vendoring it I can revert the changes
libpod/state.go
Outdated
| // validate runtime configuration. | ||
| GetDBConfig() (*DBConfig, error) | ||
|
|
||
| // ValidateDBConfig ralidates the config in the given Runtime struct |
libpod/boltdb_state_internal.go
Outdated
| return err | ||
| } | ||
|
|
||
| if err := validateDBAgainstConfig(configBkt, "run root", | ||
| rt.config.StorageConfig.RunRoot, runRoot, | ||
| if err := validateDBAgainstConfig(configBkt, "storage temporary directory", |
There was a problem hiding this comment.
Is this an appropriate description?
libpod/boltdb_state_internal.go
Outdated
| @@ -74,31 +79,31 @@ func checkRuntimeConfig(db *bolt.DB, rt *Runtime) error { | |||
| return err | |||
| } | |||
|
|
|||
| if err := validateDBAgainstConfig(configBkt, "static dir", | |||
| rt.config.StaticDir, staticDir, ""); err != nil { | |||
| if err := validateDBAgainstConfig(configBkt, "libpod root directory", | |||
There was a problem hiding this comment.
I think it would make sense to have static in this description somehow still.
When validating fields against the DB, report more verbosely the name of the field being validated if it fails. Specifically, add the name used in config files, so people will actually know what to change it errors happen. Signed-off-by: Matthew Heon <mheon@redhat.com>
Ensure that the directory where we will create the Podman db exists prior to creating the database - otherwise creating the DB will fail. Signed-off-by: Matthew Heon <mheon@redhat.com>
|
/retest |
|
/retry |
|
Alright. Not sure what's going on with the tests here, but they're passing locally, and Cirrus seems to be working after a retest... |
|
I see |
|
/retest |
|
Hm. It looks like the user-set tmp dir is not overriding in the rootless case... |
When graphroot is set by the user, we should set libpod's static directory to a subdirectory of that by default, to duplicate previous behavior. Signed-off-by: Matthew Heon <mheon@redhat.com>
We don't need this for anything more than rootless work in Libpod now, but Buildah still uses it as it was originally written, so leave it intact as part of our API. Signed-off-by: Matthew Heon <mheon@redhat.com>
Instead of storing the runtime's file lock dir in the BoltDB state, refer to the runtime inside the Bolt state instead, and use the path stored in the runtime. This is necessary since we moved DB initialization very far up in runtime init, before the locks dir is properly initialized (and it must happen before the locks dir can be created, as we use the DB to retrieve the proper path for the locks dir now). Signed-off-by: Matthew Heon <mheon@redhat.com>
|
bot, retest this please |
|
@rhatdan @baude @vrothberg @TomSweeneyRedHat @umohnani8 I think the tests are finally green - mind reviewing? |
|
LGTM but as I'm not very familiar with the DB code, I prefer someone else to merge :^) |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Finally have this in satisfying shape.
We encountered a regression a little over a week ago in Podman 0.11.x releases, where some directories' defaults changed for rootless. As we validate critical paths against the database, this caused a breaking change, requiring manual config file creation and editing (or deleting/recreating rootless container storage) to fix.
This resolves the breaking change by using the paths we validate against in the database if libpod would otherwise use default values (IE, the user has not explicitly overridden a setting). This ensures that changes to default paths will be respected for new installs, but existing installs will still continue working as expected. This also applies to the storage graph driver.
This cannot, however, fix situations where the libpod static directory's path changes - in which case we don't find the old DB, and cannot find the old paths. Libpod/Podman will function properly, but all containers/images from the old paths will be lost.