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.

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.

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.

Oh I’m not disagreeing at that high level. It’s the details of the implementation that I think might ignore too much.

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.

Some distros ship storage.conf in /etc, so I think it make sense to just ignore it.

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

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.

In particular, does

[storage.options.thinpool]
blocksize=".." # or some other previously-valid option

still trigger a warning?

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.

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..