Ignore failure to ignore thinpool keys #1908
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
|
@edsantiago @Luap99 PTAL |
types/options.go
Outdated
| if len(keys) > 0 { | ||
| logrus.Warningf("Failed to decode the keys %q from %q", keys, configFile) | ||
| for _, key := range keys { | ||
| if key.String() == "storage.options.thinpool" { |
There was a problem hiding this comment.
Seems like a good compromise to me, but can you add a comment why this is added here, i.e.
Do not cause warnings for users with old storage.conf files.
An alternative approach could be adding the struct definition back so toml will not complain in the first place.
There was a problem hiding this comment.
I mean, are the warnings bogus in the first place? The users presumably put the options in there for a reason, and ignoring them is exactly why we have the warning.
If this is intended to specifically silence the case where the default config contains an empty thinpool section, it would be more appropriate to only silence the case where that section really is empty.
There was a problem hiding this comment.
We have an upgrade problem. Older storage.conf and containers/storage supported storage.options.thinpool and newer library does not, so if we have a system that gets the library updated without updating the global storage.conf file, users will see this error, which is what is happening when testing newer podman on existing Fedora systems now. This is likely to happen in the wild, so @edsantiago wanted it shutup before users see it.
There was a problem hiding this comment.
Oh I’m not disagreeing at that high level. It’s the details of the implementation that I think might ignore too much.
There was a problem hiding this comment.
My initial ask was to ignore thinpool iff it comes from /usr/share/conf; this is known at the time the error message is issued. My thinking was that this allows mixed-sequence updating of podman and c-common. Ignoring thinpool if the block is empty is also a great idea, but seems much harder.
Either way, I do think that if a thinpool block is found anywhere outside /usr/share, it should trigger a warning.
I am fully aware that this is asking for a big ugly special case conditional and that we will never find out how much (if at all) it helps anyone in the field.
There was a problem hiding this comment.
Some distros ship storage.conf in /etc, so I think it make sense to just ignore it.
There was a problem hiding this comment.
I think the simple fix is to restore the old type definition for the thinpool table
diff --git a/pkg/config/config.go b/pkg/config/config.go
index dc70531de..7f49d029d 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -115,6 +115,9 @@ type OptionsConfig struct {
// Btrfs container options to be handed to btrfs drivers
Btrfs struct{ BtrfsOptionsConfig } `toml:"btrfs,omitempty"`
+ // Thinpool container options to be handed to thinpool drivers (NOP)
+ Thinpool struct{} `toml:"thinpool,omitempty"`
+
// Overlay container options to be handed to overlay drivers
Overlay struct{ OverlayOptionsConfig } `toml:"overlay,omitempty"`
untested but I would assume that makes toml happy only if the table is empty because all other keys would be still unknown
There was a problem hiding this comment.
On second thought, if the
if key.String() == "storage.options.thinpool" {is an exact match, then this already does what we want (does it?); and I might have been complicating a simple thing.
There was a problem hiding this comment.
In particular, does
[storage.options.thinpool]
blocksize=".." # or some other previously-valid optionstill trigger a warning?
types/options.go
Outdated
| if key.String() == "storage.options.thinpool" { | ||
| continue | ||
| } | ||
| logrus.Warningf("Failed to decode the keys %q from %q", key, configFile) |
There was a problem hiding this comment.
This seems to be a change in behavior, previously it would have logged all errors in one line now it will log each key individually. This is fine for me but you should /s/keys/key in the message then.
rhatdan
force-pushed
the
config
branch
2 times, most recently
from
50c8ff6
to
86e4ef1
Compare
|
@Luap99 PTANL |
|
@edsantiago PTAL |
types/options_test.go
Outdated
| @@ -204,5 +204,5 @@ func TestReloadConfigurationFile(t *testing.T) { | |||
| assert.Equal(t, storageOpts.RunRoot, "/run/containers/test") | |||
| logrus.SetOutput(os.Stderr) | |||
|
|
|||
| assert.Equal(t, strings.Contains(content.String(), "Failed to decode the keys [\\\"foo\\\" \\\"storage.options.graphroot\\\"] from \\\"./storage_broken.conf\\\"\""), true) | |||
| assert.Equal(t, strings.Contains(content.String(), "Failed to decode the key \\\"foo\\\" from \\\"./storage_broken.conf\\\"\""), true) | |||
There was a problem hiding this comment.
I've been struggling with this one line! Is there a way to use assert.ErrorContains(), or something that can give a better error message on failure? And, would it make sense to use the Go backticks to make the string cleaner? And maybe re-add the storage.options.graphroot test as well? I've tried to figure it out but keep going down rabbit holes..
rhatdan
force-pushed
the
config
branch
4 times, most recently
from
5936470
to
219104b
Compare
Fixes: containers/podman#22473 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Fixes: containers/podman#22473