toml configuration file does not work

[sctb512]

It seems that the toml configuration file does not work. Because it is overwritten even the flags.Args is null in the ParseParameters function.
Here is a test:

diff --git a/config/config_test.go b/config/config_test.go
index ad47909..d123f45 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -10,6 +10,7 @@ import (
        "testing"
        "time"
 
+       "github.com/containerd/nydus-snapshotter/cmd/containerd-nydus-grpc/pkg/command"
        "github.com/stretchr/testify/assert"
 )
 
@@ -81,6 +82,13 @@ func TestLoadSnapshotterTOMLConfig(t *testing.T) {
 
        A.EqualValues(cfg, &exampleConfig)
 
+       var args = command.Args{}
+       args.RootDir = "/var/lib/containerd/nydus"
+       exampleConfig.Root = "/var/lib/containerd/nydus"
+       err = ParseParameters(&args, cfg)
+       A.NoError(err)
+       A.EqualValues(cfg, &exampleConfig)
+
        err = ProcessConfigurations(cfg)
        A.NoError(err)

--- FAIL: TestLoadSnapshotterTOMLConfig (0.00s)
    /data00/home/tangbin.bin/test_code/nydus-snapshotter/config/config_test.go:90: 
        	Error Trace:	/data00/home/tangbin.bin/test_code/nydus-snapshotter/config/config_test.go:90
        	Error:      	Not equal: 
        	            	expected: &config.SnapshotterConfig{Version:1, Root:"/var/lib/containerd/nydus", Address:"", DaemonMode:"", CleanupOnClose:false, SystemControllerConfig:config.SystemControllerConfig{Enable:true, Address:"/var/run/containerd-nydus/system.sock", DebugConfig:config.DebugConfig{ProfileDuration:5, PprofAddress:""}}, MetricsConfig:config.MetricsConfig{Address:":9110"}, DaemonConfig:config.DaemonConfig{NydusdPath:"/usr/local/bin/nydusd", NydusImagePath:"/usr/local/bin/nydus-image", NydusdConfigPath:"", RecoverPolicy:"restart", FsDriver:"", ThreadsNumber:4}, SnapshotsConfig:config.SnapshotConfig{EnableNydusOverlayFS:false, SyncRemove:false}, RemoteConfig:config.RemoteConfig{AuthConfig:config.AuthConfig{EnableKubeconfigKeychain:false, KubeconfigPath:"", EnableCRIKeychain:false, ImageServiceAddress:""}, ConvertVpcRegistry:false, MirrorsConfig:config.MirrorsConfig{Dir:"/etc/nydus/certs.d"}}, ImageConfig:config.ImageConfig{PublicKeyFile:"", ValidateSignature:false}, CacheManagerConfig:config.CacheManagerConfig{Disable:false, GCPeriod:"24h", CacheDir:""}, LoggingConfig:config.LoggingConfig{LogToStdout:false, LogLevel:"", LogDir:"", RotateLogMaxSize:1, RotateLogMaxBackups:5, RotateLogMaxAge:7, RotateLogLocalTime:true, RotateLogCompress:true}, Experimental:config.Experimental{EnableStargz:false}}
        	            	actual  : &config.SnapshotterConfig{Version:1, Root:"/var/lib/containerd/nydus", Address:"/run/containerd-nydus/containerd-nydus-grpc.sock", DaemonMode:"multiple", CleanupOnClose:false, SystemControllerConfig:config.SystemControllerConfig{Enable:true, Address:"/var/run/containerd-nydus/system.sock", DebugConfig:config.DebugConfig{ProfileDuration:5, PprofAddress:""}}, MetricsConfig:config.MetricsConfig{Address:":9110"}, DaemonConfig:config.DaemonConfig{NydusdPath:"/usr/local/bin/nydusd", NydusImagePath:"/usr/local/bin/nydus-image", NydusdConfigPath:"/etc/nydus/nydusd-config.fusedev.json", RecoverPolicy:"restart", FsDriver:"fusedev", ThreadsNumber:4}, SnapshotsConfig:config.SnapshotConfig{EnableNydusOverlayFS:false, SyncRemove:false}, RemoteConfig:config.RemoteConfig{AuthConfig:config.AuthConfig{EnableKubeconfigKeychain:false, KubeconfigPath:"", EnableCRIKeychain:false, ImageServiceAddress:""}, ConvertVpcRegistry:false, MirrorsConfig:config.MirrorsConfig{Dir:"/etc/nydus/certs.d"}}, ImageConfig:config.ImageConfig{PublicKeyFile:"", ValidateSignature:false}, CacheManagerConfig:config.CacheManagerConfig{Disable:false, GCPeriod:"24h", CacheDir:""}, LoggingConfig:config.LoggingConfig{LogToStdout:false, LogLevel:"info", LogDir:"", RotateLogMaxSize:1, RotateLogMaxBackups:5, RotateLogMaxAge:7, RotateLogLocalTime:true, RotateLogCompress:true}, Experimental:config.Experimental{EnableStargz:false}}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -3,4 +3,4 @@
        	            	  Root: (string) (len=25) "/var/lib/containerd/nydus",
        	            	- Address: (string) "",
        	            	- DaemonMode: (string) "",
        	            	+ Address: (string) (len=48) "/run/containerd-nydus/containerd-nydus-grpc.sock",
        	            	+ DaemonMode: (string) (len=8) "multiple",
        	            	  CleanupOnClose: (bool) false,
        	            	@@ -20,5 +20,5 @@
        	            	   NydusImagePath: (string) (len=26) "/usr/local/bin/nydus-image",
        	            	-  NydusdConfigPath: (string) "",
        	            	+  NydusdConfigPath: (string) (len=37) "/etc/nydus/nydusd-config.fusedev.json",
        	            	   RecoverPolicy: (string) (len=7) "restart",
        	            	-  FsDriver: (string) "",
        	            	+  FsDriver: (string) (len=7) "fusedev",
        	            	   ThreadsNumber: (int) 4
        	            	@@ -52,3 +52,3 @@
        	            	   LogToStdout: (bool) false,
        	            	-  LogLevel: (string) "",
        	            	+  LogLevel: (string) (len=4) "info",
        	            	   LogDir: (string) "",
        	Test:       	TestLoadSnapshotterTOMLConfig
    /data00/home/tangbin.bin/test_code/nydus-snapshotter/config/config_test.go:93: 
        	Error Trace:	/data00/home/tangbin.bin/test_code/nydus-snapshotter/config/config_test.go:93
        	Error:      	Received unexpected error:
        	            	invalid argument
        	            	github.com/containerd/nydus-snapshotter/pkg/errdefs.init
        	            		/data00/home/tangbin.bin/test_code/nydus-snapshotter/pkg/errdefs/errors.go:20
        	            	runtime.doInit
        	            		/usr/local/go/src/runtime/proc.go:6222
        	            	runtime.doInit
        	            		/usr/local/go/src/runtime/proc.go:6199
        	            	runtime.doInit
        	            		/usr/local/go/src/runtime/proc.go:6199
        	            	runtime.main
        	            		/usr/local/go/src/runtime/proc.go:233
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1571
        	Test:       	TestLoadSnapshotterTOMLConfig
FAIL
FAIL	github.com/containerd/nydus-snapshotter/config	0.011s
FAIL

[PKizzle]

With the suggested fix #380 it is no longer possible to enable log_to_stdout via the toml file. It is completely ignored. logConfig.LogToStdout shall only be overridden by the command line argument if it was specified. The following works for me:

if args.LogToStdout {
	logConfig.LogToStdout = args.LogToStdout
}

[sctb512]

With the suggested fix #380 it is no longer possible to enable log_to_stdout via the toml file. It is completely ignored. logConfig.LogToStdout shall only be overridden by the command line argument if it was specified. The following works for me:

if args.LogToStdout {
	logConfig.LogToStdout = args.LogToStdout
}

Hi @PKizzle, thanks for your comment.

Actually, I also considered this solution. But I think it might not work because the args.LogToStdout will always be assigned the value of True or False. So the value loaded from the toml file will be overwritten.

Here is a test for this:

func TestSnapshotterConfig(t *testing.T) {
	A := assert.New(t)

	var cfg SnapshotterConfig
	var args command.Args
	cfg.LoggingConfig.LogToStdout = true
	err := ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)
}

result:

--- FAIL: TestSnapshotterLConfig (0.00s)
    /Users/bintang/go/src/nydus-snapshotter/config/config_test.go:110: 
        	Error Trace:	/Users/bintang/go/src/nydus-snapshotter/config/config_test.go:110
        	Error:      	Not equal: 
        	            	expected: false
        	            	actual  : true
        	Test:       	TestSnapshotterLConfig
FAIL
FAIL	github.com/containerd/nydus-snapshotter/config	0.608s
FAIL

In my opinion the LogToStdout is needed, but it is possible to run nydus-snapshotter without the toml file. So it makes more sense to keep the --log-to-stdout command argument.

[PKizzle]

If I execute the exact same test it passes for me. Yes args.LogToStdout will always be assigned a value, but only if the command line option is present it well be set to true. Hence the command line option shall only override the toml configuration if it is true, which I ensured with the source code above. Did you also change back this line?
I adjusted your test for the command line override I mentioned above:

func TestSnapshotterConfig(t *testing.T) {
	A := assert.New(t)

	var cfg SnapshotterConfig
	var args command.Args
	args.LogToStdout = true
	cfg.LoggingConfig.LogToStdout = false
	err := ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)
}

[sctb512]

Did you also change back this line?

Yes, I have changed back that line. But I think this has nothing to do with our test because we are assigning cfg.LoggingConfig.LogToStdout to simulate loading the value from the toml file.

func TestSnapshotterConfig(t *testing.T) {
	A := assert.New(t)

	var cfg SnapshotterConfig
	var args command.Args
	args.LogToStdout = true
	cfg.LoggingConfig.LogToStdout = false
	err := ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)
}

This test works for me too. If I understand correctly, there are four cases we should consider. If we use the toml file to set LogToStdout without the command flag --log-to-stdout, it will not work.

func TestSnapshotterConfig(t *testing.T) {
	A := assert.New(t)

	var cfg SnapshotterConfig
	var args command.Args

	// The log_to_stdout is false in toml file without --log-to-stdout flag.
	// Expected false.
	cfg.LoggingConfig.LogToStdout = false
	err := ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, false)

	// The log_to_stdout is true in toml file without --log-to-stdout flag.
	// Expected true.
	// This case is failed.
	cfg.LoggingConfig.LogToStdout = true
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)

	// The log_to_stdout is true in toml file but --log-to-stdout flag is false.
	// Expected true (command flag has higher priority).
	args.LogToStdout = true
	cfg.LoggingConfig.LogToStdout = false
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)

	// The log_to_stdout is false in toml file but --log-to-stdout flag is true.
	// Expected false (command flag has higher priority).
	args.LogToStdout = false
	cfg.LoggingConfig.LogToStdout = true
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, false)
}

Could you please help me check this test? Thank you. 😄

[PKizzle]

I think there was something mixed up in the test source code. I changed it to the following and it passes successfully:

func TestSnapshotterConfig(t *testing.T) {
	A := assert.New(t)

	var cfg SnapshotterConfig
	var args command.Args

	// The log_to_stdout is false in toml file without --log-to-stdout flag.
	// Expected false.
	cfg.LoggingConfig.LogToStdout = false
	err := ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, false)

	// The log_to_stdout is true in toml file without --log-to-stdout flag.
	// Expected true.
	// This case is failed.
	cfg.LoggingConfig.LogToStdout = true
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)

	// The log_to_stdout is false in toml file with --log-to-stdout flag.
	// Expected true (command flag has higher priority).
	args.LogToStdout = true
	cfg.LoggingConfig.LogToStdout = false
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)

	// The log_to_stdout is true in toml file with --log-to-stdout flag.
	// Expected true (command flag has higher priority).
	args.LogToStdout = true
	cfg.LoggingConfig.LogToStdout = true
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)
}

[PKizzle]

Currently --log-to-stdout=false is not possible. I am working on a solution.

[PKizzle]

I fixed it by updating https://github.com/urfave/cli to v2.24.4 which introduces a Count property to BoolFlag which can be used to detect whether it has been set or not. Still the best solution would be to not manually read the toml file but instead use https://github.com/urfave/cli-altsrc which was created exactly for this purpose.

flags.go

type Args struct {
	Address               string
	LogLevel              string
	NydusdConfigPath      string
	SnapshotterConfigPath string
	RootDir               string
	NydusdPath            string
	NydusImagePath        string
	DaemonMode            string
	FsDriver              string
	LogToStdout           bool
	LogToStdoutCount      int
	PrintVersion          bool
}
...
		&cli.BoolFlag{
			Name:        "log-to-stdout",
			Usage:       "print log messages to STDOUT",
			Destination: &args.LogToStdout,
			Count:       &args.LogToStdoutCount,
		},

config.go

	if args.LogToStdoutCount > 0 {
		logConfig.LogToStdout = args.LogToStdout
	}

config_test.go

func TestSnapshotterConfig(t *testing.T) {
	A := assert.New(t)

	var cfg SnapshotterConfig
	var args command.Args

	// The log_to_stdout is false in toml file without --log-to-stdout flag.
	// Expected false.
	cfg.LoggingConfig.LogToStdout = false
	args.LogToStdoutCount = 0
	err := ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, false)

	// The log_to_stdout is true in toml file without --log-to-stdout flag.
	// Expected true.
	// This case is failed.
	cfg.LoggingConfig.LogToStdout = true
	args.LogToStdoutCount = 0
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)

	// The log_to_stdout is false in toml file with --log-to-stdout=true.
	// Expected true (command flag has higher priority).
	args.LogToStdout = true
	args.LogToStdoutCount = 1
	cfg.LoggingConfig.LogToStdout = false
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)

	// The log_to_stdout is true in toml file with --log-to-stdout=true.
	// Expected true (command flag has higher priority).
	args.LogToStdout = true
	args.LogToStdoutCount = 1
	cfg.LoggingConfig.LogToStdout = true
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, true)

	// The log_to_stdout is false in toml file with --log-to-stdout=false.
	// Expected false (command flag has higher priority).
	args.LogToStdout = false
	args.LogToStdoutCount = 1
	cfg.LoggingConfig.LogToStdout = false
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, false)

	// The log_to_stdout is true in toml file with --log-to-stdout=false.
	// Expected false (command flag has higher priority).
	args.LogToStdout = false
	args.LogToStdoutCount = 1
	cfg.LoggingConfig.LogToStdout = true
	err = ParseParameters(&args, &cfg)
	A.NoError(err)
	A.EqualValues(cfg.LoggingConfig.LogToStdout, false)
}

[sctb512]

I fixed it by updating https://github.com/urfave/cli to v2.24.4 which introduces a Count property to BoolFlag which can be used to detect whether it has been set or not.

I think this is the perfect solution to our problem.
The Count for BoolFlag is available since v2.15.0. Here is the corresponding PR urfave/cli#1257. cc @changweige
Could you please send a PR for this issue? @PKizzle

[PKizzle]

Do you specifically want version v2.15.0 or can I go for the latest version?

[PKizzle]

This is my PR with the fix: #391

[sctb512]

Do you specifically want version v2.15.0 or can I go for the latest version?

I think v2.24.4 is fine.