HI @spnngl

When using containerd config default, it outputs:

[debug]
  address = ""
  format = ""
  gid = 0
  level = ""
  uid = 0

So, the default config should not be changed :-)

If we want to customize the config, we can use the code:

{% if containerd_debug_address %}
  address = "{{ containerd_debug_address }}"
{% else %}

And the code in roles/container-engine/containerd/defaults/main.yml

# some comments...
# containerd_debug_address: "/var/run/containerd/debug.sock"

The same as the other variables.
Thanks

Changed it in last push

Hum, I really disagree with that. It's of little use to match the output of containerd config default, because it's not like we can use that directly in our jinja template, so we have to update mostly manually anyway if containerd defaults change.

I'd rather have explicit defaults and a more straightforward template, with as little branching as possible.

I put containerd_debug_level back to its previous value and the others will be used only if defined by the user

What I meant is that we should not have to use is defined everywhere, and keeping the same structure that the output of containerd config default buy us nothing, so we should just have something like:

{% if containerd_debug_enabled %}
[debug]
  setting_1 = {{ containerd_default_value_for_setting_1 }}
  ...
{% endif %}

Unless I'm mistaken, the debug socket is not enabled if this section is not present.
IMO, this whole section should be guarded with something like containerd_enabled_debug_socket, defaulting to false.
Production setup should presumably not have a debug socket lying around.

This section is already present in current kubespray config to customize log level

See: https://github.com/kubernetes-sigs/kubespray/blob/master/roles/container-engine/containerd/templates/config.toml.j2#L10

The debug log level is not the same thing that the normal log level, is it not ?

I know that section is already present. What I mean is that it should not be enabled by default, as kubespray by default target production use cases, and having a debug socket enabled in production is not optimal.

It set the log level for everything, not just debug ones.
Same for the log format inside it.

Do you still want to disable this section entirely with a containerd_enabled_debug bool ? And no if defined inside it ?

See: https://github.com/containerd/containerd/blob/83031836b2cf55637d7abf847b17134c51b38e53/cmd/containerd/command/main.go#L348

It takes the config.Debug.Level if no cli params is used, cli flags have higher priorities but we do not use them in kubespray.

func setLogLevel(context *cli.Context, config *srvconfig.Config) error {
	l := context.GlobalString("log-level")
	if l == "" {
		l = config.Debug.Level
	}
	if l != "" {
		return log.SetLevel(l)
	}
	return nil
}

Config struct is found here and the debug section looks like this:

type Debug struct {
	Address string `toml:"address"`
	UID     int    `toml:"uid"`
	GID     int    `toml:"gid"`
	Level   string `toml:"level"`
	// Format represents the logging format. Supported values are 'text' and 'json'.
	Format string `toml:"format"`
}

Wow, this is so weird (or I totally misunderstood something). Does that mean there is no way to change the log level from the config without enabling the debug socket ?
I mean, I assumed that the "normal" log stream (which I presume goes to standard output) and the "debug" log stream (to the debug socket) where different, but are they in fact the same ?

I've tried to check containerd documentation but couldn't find explanation about this.

That said, I still thinks we should not enable the debug socket in production, (unless the debug socket is an unfortunate naming and it's in fact required ?). Since we handle the generation of the systemd unit for containerd we could inject the log level as flags... 🤔

That does seems a bit weird from containerd though, I feel like I'm missing something.

I'd rather have some consistency here : either put the default for all settings, or for none.
And I'd lean towards documenting the defaults explicitely.

I put it to the containerd config default as requested previously, I changed it back to what it was for containerd_debug_level and add the others only if they are defined by the user

Hum, I really disagree with that. It's of little use to match the output of containerd config default, because it's not like we can use that directly in our jinja template, so we have to update mostly manually anyway if containerd defaults change.

I'd rather have explicit defaults and a more straightforward template, with as little branching as possible.