Add ability to rewrite cached paths in the database #27157
Conversation
|
[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
bot
added
the
approved
mheon
force-pushed
the
allow_db_config_rewrite
branch
2 times, most recently
from
5d82351 to
8245598
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
first pass lgtm |
| rm -rf "$RUNROOT_B" | ||
| rm -rf "$GRAPHROOT" | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you do tests for the error conditions as defined in this if?
mheon
force-pushed
the
allow_db_config_rewrite
branch
8 times, most recently
from
9549389 to
0cc16bf
Compare
|
This is ready |
| #### **--rewrite-config** | ||
| When true, cached configuration values in the database will be rewritten. | ||
| Normally, changes to certain configuration values - graphDriver, graphRoot, and runRoot in storage.conf, as well as static_dir, tmp_dir, and volume_path in containers.conf - will be ignored until a `podman system reset`, as old values cached in the database will be used. | ||
| This is done to ensure that configuration changes do not break existing pods, containers, and volumes present in the database. | ||
| This option rewrites the cached values in the database, replacing them with the current configuration. | ||
| This can only be done if no containers, pods, and volumes are present, to prevent the breakage described earlier. | ||
| If any containers, pods, or volumes are present, an error will be returned. |
There was a problem hiding this comment.
What is the point of making this a global flag that applies to every command? That seems design wise not ideal, it means user could just start using that with any command and I fear it leads to users setting this on all commands then just making the scripts and such ugly?
Would something like this not be better done only for system migrate?
Also if it is gated on no containers, volumes, pods (which I agree is an important security detail) then how much would this flag help compared to running system reset? I guess the only benifit is you get to keep the images?
There was a problem hiding this comment.
It keeps networks, machines, images - so there's probably some value in it.
I wouldn't be opposed to adding this to system migrate but I do kind of hate how loaded with unintended side-effects that command has become.
There was a problem hiding this comment.
yeah system migrate may not be the perfect fit either but I find the global option worse?
Thinking about this is there a strong reason to require manual user invention at all? If we just fix it silently when there are no containers, pods, volumes then I guess the users are most happy. Because like that we still require that a user must somehow know about this.
There was a problem hiding this comment.
I think doing it unconditionally is reasonable - the DB query to check if it's safe to rewrite should be very efficient (and can be made more efficient, I think, no need to get an actual count, just check if number of rows is non-zero).
test/system/005-info.bats
Outdated
| GRAPHROOT=$(mktemp -d) | ||
| RUNROOT_A=$(mktemp -d) | ||
| RUNROOT_B=$(mktemp -d) |
There was a problem hiding this comment.
This must use paths below $PODMAN_TMPDIR, and don't use rm -rf at the end since that is done already in teardown(). The way it is written now these paths are always leaked on test errors.
test/system/005-info.bats
Outdated
|
|
||
| # First, verify original runRoot is used with first config | ||
| CONTAINERS_STORAGE_CONF=$STORAGE_CONF1 run_podman info | ||
| echo "$output" | grep -q "runRoot: $RUNROOT_A" |
There was a problem hiding this comment.
That is simply not the way how we write tests and check for output in the bats suite.
Please always use assert and if you want the runroot just call podman info --format "{{.Store.RunRoot}}" instead of the awkward grep, same in all the other places
This is accomplished by a new flag, `--rewrite-config`, which instructs the database to replace cached paths. Before this, the only real way to change paths like tmpdir, runroot, graphroot, etc was to do a `podman system reset` then change the config files before running another Podman command. Now, changing the config files and running any Podman command with `--rewrite-config` should be sufficient to pick up the new paths. Please note that this can only be done with no containers, pods, and volumes present. Otherwise, we risk the breakages that caching paths was supposed to prevent in the first place. This is SQLite only, given the deprecation and impending removal of BoltDB. Signed-off-by: Matt Heon <[email protected]>
mheon
force-pushed
the
allow_db_config_rewrite
branch
from
0cc16bf to
ffac7db
Compare
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This is accomplished by a new flag,
--rewrite-config, which instructs the database to replace cached paths. Before this, the only real way to change paths like tmpdir, runroot, graphroot, etc was to do apodman system resetthen change the config files before running another Podman command. Now, changing the config files and running any Podman command with--rewrite-configshould be sufficient to pick up the new paths.Please note that this can only be done with no containers, pods, and volumes present. Otherwise, we risk the breakages that caching paths was supposed to prevent in the first place.
This is SQLite only, given the deprecation and impending removal of BoltDB.
Does this PR introduce a user-facing change?