Ensure log level is applied before loading configuration by mdiniz97 · Pull Request #29258 · podman-container-tools/podman

mdiniz97 · 2026-07-23T02:29:09Z

Podman loads its configuration during global initialization, before the --log-level option is processed. As a result, debug and trace messages emitted while reading configuration files are suppressed, making invalid or unknown configuration keys difficult to diagnose.

This change applies the requested log level before configuration initialization, ensuring that configuration-loading diagnostics are visible when commands are executed with --log-level=debug or --log-level=trace.

Fixes: #25362

Checklist

Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all commits (git commit -s).
Referenced issues using Fixes: #00000 in commit message.
Tests have been added/updated.
Documentation has been updated or no documentation changes are needed.
All commits pass make validatepr (format/lint checks).
Release note entered below.

Does this PR introduce a user-facing change?

Configuration parsing diagnostics are now displayed when Podman is started with an appropriate debug or trace log level.

Luap99 · 2026-07-23T11:31:36Z

+func setEarlyLogLevel() {
+	index := parseIndex()
+	if index > 1 {
+		return
+	}
+
+	var logLevel string
+	var debug bool
+	fs := pflag.NewFlagSet("log level", pflag.ContinueOnError)
+	fs.ParseErrorsAllowlist.UnknownFlags = true
+	fs.Usage = func() {}
+	fs.SetInterspersed(false)
+	fs.StringVar(&logLevel, "log-level", "", "")
+	fs.BoolVarP(&debug, "debug", "D", false, "")
+	fs.BoolP("help", "h", false, "") // Need a fake help flag to avoid the `pflag: help requested` error
+	if err := fs.Parse(os.Args[index:]); err != nil {
+		return
+	}
+
+	if debug && logLevel == "" {
+		logrus.SetLevel(logrus.DebugLevel)
+	} else if !debug && logLevel != "" {
+		if level, err := logrus.ParseLevel(logLevel); err == nil {
+			logrus.SetLevel(level)
+		}
+	}
+}


so this adds the fifth argument parser here that has to walk argv. I Do not think this is sustainable/

This, modules, and the remote parsing should be refactored so they only use one flag set once and parse once.

@Luap99 I refactored this as you suggested:

Consolidated the early parsers for logging, modules, and remote into a single entry point (getEarlyCLIOptions() in config.go).

There's now a single pflag.FlagSet, parsed exactly once via sync.Once (earlySync). remote.go no longer has its own parser, it reuses the cached result.

Added tests covering shell completion, environment variables (CONTAINER_HOST/CONTAINER_CONNECTION), podmansh, invalid flags, and the local/remote cases (--connection, --context, --host, --url).

Could you take another look when you have a chance?

packit-as-a-service · 2026-07-23T13:58:07Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

mheon · 2026-07-23T20:33:23Z


+// getEarlyCLIOptions parses flags needed during command initialization.
+// Cobra parses and validates the complete command line later.
+func getEarlyCLIOptions() *earlyCLIOptions {


Nit: feels like this would be better as a sync.OnceValue()

there is not point at all in this if we call it in newPodmanConfig which is already part of a sync.once so it feels like we should go further and just make this structu part of PodmanConfig and parse this one in newPodmanConfig where we have the sync.Once already. No point in using several nested sync.once

I’ll remove the nested synchronization. The early options can be parsed directly in newPodmanConfig(), which is already protected by podmanSync. The parsed state will be incorporated into the Podman configuration initialization.

mheon · 2026-07-23T20:34:51Z

+	if options.parseErr != nil {
+		var flagErr interface{ GetFlag() *pflag.Flag }
+		if !errors.As(options.parseErr, &flagErr) {
+			return options.modules, options.parseErr


Why is this necessary?

(It seems like just bailing if parseErr is not nil is all that's necessary?)

yes this should just return parseErr

I’ll simplify this to return parseErr directly and remove the flag-specific error classification. The related tests for malformed non-module flags will be adjusted accordingly.

Luap99 · 2026-07-24T10:09:04Z


+// getEarlyCLIOptions parses flags needed during command initialization.
+// Cobra parses and validates the complete command line later.
+func getEarlyCLIOptions() *earlyCLIOptions {


there is not point at all in this if we call it in newPodmanConfig which is already part of a sync.once so it feels like we should go further and just make this structu part of PodmanConfig and parse this one in newPodmanConfig where we have the sync.Once already. No point in using several nested sync.once

Luap99 · 2026-07-24T10:10:05Z

+	if options.parseErr != nil {
+		var flagErr interface{ GetFlag() *pflag.Flag }
+		if !errors.As(options.parseErr, &flagErr) {
+			return options.modules, options.parseErr


yes this should just return parseErr

Luap99 · 2026-07-24T10:18:30Z

+func resetEarlyCLIState(t *testing.T, args ...string) {
+	t.Helper()
+	oldArgs := os.Args
+	os.Args = args
+	earlyOptions = earlyCLIOptions{}
+	earlySync = sync.Once{}
+	t.Cleanup(func() {
+		os.Args = oldArgs
+		earlyOptions = earlyCLIOptions{}
+		earlySync = sync.Once{}
+	})
+}


from a unit test POV this is really ugly.

write the function so that you test it without a sync.once and make the os.ARgs an argument to the function then you have a function that can be unit tested which makes a lot more sense that this global state modification.

I’ll separate the parsing logic from the global initialization. The parser will receive the argument slice explicitly, while newPodmanConfig() will pass the relevant portion of os.Args. Unit tests will then invoke the parser directly without resetting os.Args, sync.Once, or shared option state.

Luap99 · 2026-07-24T10:19:22Z

+EOF
+
+    for log_arg in --log-level=debug --log-level=trace --debug; do
+        CONTAINERS_CONF="$conf_tmp" run_podman "$log_arg" version


this should use CONTAINERS_CONF_OVERRIDE instead

I’ll update the regression test to use CONTAINERS_CONF_OVERRIDE for the temporary configuration file.

mheon

LGTM

Honny1

I have some comments.

Honny1 · 2026-08-10T12:25:28Z

+}
+
+// Return the containers.conf modules to load.
+func containersConfModules(options *earlyCLIOptions) ([]string, error) {


Is this function necessary? Maybe the call to this function can be rewritten to this:

options := parseEarlyCLIOptions(os.Args) setEarlyLogLevel(options) if options.parseErr != nil && !options.completion { fmt.Fprintf(os.Stderr, "Error parsing command-line flags: %v\n", options.parseErr) os.Exit(1) } modules := options.modules if options.completion { modules = nil }

Fixed in 22f49ad. Removed containersConfModules and inlined the completion, error, and module handling in newPodmanConfig. The system test now covers incomplete flags and verifies that modules are not loaded during completion.

Honny1 · 2026-08-10T12:28:38Z

 	}

-	podmanOptions = entities.PodmanConfig{ContainersConf: &config.Config{}, ContainersConfDefaultsRO: defaultConfig, EngineMode: mode}
+	podmanOptions = entities.PodmanConfig{ContainersConf: &config.Config{}, ContainersConfDefaultsRO: defaultConfig, EngineMode: mode, Remote: remote}


Is Remote used somewhere?

Removed the initialization in 22f49ad. PodmanConfig.Remote is used as the --remote flag target in rootFlags, but BoolVarP initializes it from registry.IsRemote(), so assigning remote in newPodmanConfig was redundant.

Honny1 · 2026-08-10T12:31:24Z

+	setEarlyLogLevel(options)
+	modules, err := containersConfModules(options)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "Error parsing containers.conf modules: %v\n", err)


I think this is a better error message.

Suggested change

fmt.Fprintf(os.Stderr, "Error parsing containers.conf modules: %v\n", err)

fmt.Fprintf(os.Stderr, "Error parsing command-line flags: %v\n", err)

Fixed in 22f49ad. The message now says "Error parsing command-line flags", which covers every flag handled by the early parser.

Honny1

LGTM, Thanks!

Honny1

Can you please fix linters:

+ /home/ubuntu/golangci-lint-2.12.2-linux-amd64/golangci-lint run --build-tags=apparmor,seccomp,selinux
Error: cmd/podman/registry/config_test.go:94:1: File is not properly formatted (gofumpt)
		name    string
^

Honny1

CI failure seems to be related. Please resolve that and also squash all your commits. Thanks.

mdiniz97 · 2026-08-10T18:01:02Z

Fixed and squashed in 569b49e.

Honny1

LGTM, Thanks!

Non-blocking question: What bout --debug --log-level=<same-as-default-value> edge case? What will happen when both flags are used?

Luap99

First commit looks good to me but I think the second commit is just wrong and should be dropped. I see no point in special casing --debug --log-level=warn, in fact to me it is wrong because that alters the behavior for just that one level? If both options are given explicitly that should always be treated as conflict

Honny1 · 2026-08-13T10:45:34Z

 	"go.podman.io/storage"
 )

+func TestLoggingHookRejectsExplicitDefaultLogLevelWithDebug(t *testing.T) {


I don't think this test is necessary, so I'd drop it. The integration test is enough.

Honny1

Code LGTM. Just one last thing: please squash your commits.

Podman loads containers.conf before Cobra processes global flags, hiding debug and trace diagnostics emitted during configuration parsing. Parse early CLI options once so logging is configured first, then reuse the result for modules and remote-mode selection. Fixes: podman-container-tools#25362 Signed-off-by: Marcos Paulo Diniz <marcosdinizpaulo@gmail.com>

mdiniz97 · 2026-08-13T12:25:55Z

squashed in 18e1154.

Honny1

LGTM

@podman-container-tools/podman-reviewers @podman-container-tools/podman-maintainers Please review and merge.

jankaluza · 2026-08-13T12:58:43Z

I do not have time to check tests right now, but the code looks good otherwise I think. LGTM.

Luap99

LGTM

mdiniz97 marked this pull request as ready for review July 23, 2026 03:16

Luap99 reviewed Jul 23, 2026

View reviewed changes

mdiniz97 force-pushed the fix-25362-config-log-level branch from c7dfab3 to 3c95926 Compare July 23, 2026 13:20

mheon reviewed Jul 23, 2026

View reviewed changes

Luap99 reviewed Jul 24, 2026

View reviewed changes

mdiniz97 requested a review from Luap99 August 1, 2026 18:32

mheon approved these changes Aug 8, 2026

View reviewed changes

Honny1 reviewed Aug 10, 2026

View reviewed changes

mdiniz97 requested a review from Honny1 August 10, 2026 14:15

Honny1 approved these changes Aug 10, 2026

View reviewed changes

Honny1 requested changes Aug 10, 2026

View reviewed changes

mdiniz97 force-pushed the fix-25362-config-log-level branch from 2b40290 to 569b49e Compare August 10, 2026 17:56

mdiniz97 requested a review from Honny1 August 10, 2026 18:37

Honny1 approved these changes Aug 11, 2026

View reviewed changes

Luap99 reviewed Aug 12, 2026

View reviewed changes

mdiniz97 force-pushed the fix-25362-config-log-level branch from 79a220a to 3334d95 Compare August 12, 2026 18:10

Honny1 reviewed Aug 13, 2026

View reviewed changes

Honny1 requested changes Aug 13, 2026

View reviewed changes

mdiniz97 force-pushed the fix-25362-config-log-level branch from 9dab354 to 18e1154 Compare August 13, 2026 12:24

Honny1 approved these changes Aug 13, 2026

View reviewed changes

Luap99 approved these changes Aug 13, 2026

View reviewed changes

Luap99 enabled auto-merge August 13, 2026 13:40

Luap99 merged commit ee19baa into podman-container-tools:main Aug 13, 2026
70 checks passed

	fmt.Fprintf(os.Stderr, "Error parsing containers.conf modules: %v\n", err)
	fmt.Fprintf(os.Stderr, "Error parsing command-line flags: %v\n", err)

Uh oh!

Conversation

mdiniz97 commented Jul 23, 2026

Checklist

Does this PR introduce a user-facing change?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

packit-as-a-service Bot commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon left a comment

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

mdiniz97 commented Aug 10, 2026

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

mdiniz97 commented Aug 13, 2026

Uh oh!