Add containerd config options

[spnngl]

What type of PR is this?
/kind feature

What this PR does / why we need it:

Get more containerd configuration options, notably SElinux and debug socket.

Which issue(s) this PR fixes:

Fixes #11081

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[containerd] added debug config variables `containerd_debug_address`, `containerd_debug_level`, `containerd_debug_format` `containerd_debug_uid`, `containerd_debug_gid`; it is disabled by default.
[containerd] added "io.containerd.grpc.v1.cri" config variables `containerd_enable_selinux`, `containerd_disable_apparmor`, `containerd_tolerate_missing_hugetlb_controller`, `containerd_disable_hugetlb_controller`, `containerd_image_pull_progress_timeout`

[k8s-ci-robot]

Hi @spnngl. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[yankay]

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

[spnngl]

Changed it in last push

[VannTen]

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.

[spnngl]

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

[VannTen]

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 %}

[yankay]

/ok-to-test

[mzaian]

@spnngl please add a proper release note.

[spnngl]

I just added it to the release-note section of this PR

[mzaian]

@VannTen @yankay Could you please do a final review here.

[VannTen]

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.

[spnngl]

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

[VannTen]

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.

[spnngl]

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"`
}

[VannTen]

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.

[spnngl]

Having the [debug] section in the toml config does not mean we activate the debug socket.
It is only activated if config.Debug.Address != "", you can see the code here: https://github.com/containerd/containerd/blob/v1.7.16/cmd/containerd/command/main.go#L232

By default debug socket is always disabled.

		if config.Debug.Address != "" {
			var l net.Listener
			if isLocalAddress(config.Debug.Address) {
				if l, err = sys.GetLocalListener(config.Debug.Address, config.Debug.UID, config.Debug.GID); err != nil {
					return fmt.Errorf("failed to get listener for debug endpoint: %w", err)
				}
			} else {
				if l, err = net.Listen("tcp", config.Debug.Address); err != nil {
					return fmt.Errorf("failed to get listener for debug endpoint: %w", err)
				}
			}
			serve(ctx, l, server.ServeDebug)
		}

https://github.com/containerd/containerd/blob/v1.7.16/cmd/containerd/command/main_unix.go#L76

func isLocalAddress(path string) bool {
	return filepath.IsAbs(path)
}

It is exactly the same for the metrics one: https://github.com/containerd/containerd/blob/v1.7.16/cmd/containerd/command/main.go#L245

		if config.Metrics.Address != "" {
			l, err := net.Listen("tcp", config.Metrics.Address)
			if err != nil {
				return fmt.Errorf("failed to get listener for metrics endpoint: %w", err)
			}
			serve(ctx, l, server.ServeMetrics)
		}

[VannTen]

Ok, I think I got it, thanks for bearing with me (https://github.com/containerd/containerd/blob/main/docs/man/containerd-config.toml.5.md is not super helpful here !).

So, defining the address is what activate the debug socket, got it.
But according to the link above, it has a default value of /run/containerd/debug.sock, so it would be activated by default, is that correct ?

What I would do in that case is have in our defaults/main.yml something like that.

containerd_debug_enable: false
contaienrd_debug_address: '/run/containerd/debug.sock
# the rest of the value with default matching containerd 

And in the template something likg

[debug]
     address = "{{ containerd_debug_enable | ternary(containerd_debug_address, '') }}"
     # the rest of the values

wdyt ?

[spnngl]

The doc is misleading, it says it is enabled by default but it is not.

Here the config from a newly created node in my cluster

version = 2
root = "/var/lib/containerd"
state = "/run/containerd"
oom_score = 0

[grpc]
  max_recv_message_size = 16777216
  max_send_message_size = 16777216

[debug]
  level = "info"

[metrics]
  address = ""
  grpc_histogram = false

[plugins]
  [plugins."io.containerd.grpc.v1.cri"]
    sandbox_image = "registry.k8s.io/pause:3.9"
    max_container_log_line_size = -1
    enable_unprivileged_ports = false
    enable_unprivileged_icmp = false
  [plugins."io.containerd.grpc.v1.cri".containerd]
    default_runtime_name = "runc"
    snapshotter = "overlayfs"
  
    [plugins."io.containerd.grpc.v1.cri".containerd.runtimes]
      [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
        runtime_type = "io.containerd.runc.v2"
        runtime_engine = ""
        runtime_root = ""
        base_runtime_spec = "/etc/containerd/cri-base.json"
    
          [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options]
            systemdCgroup = true
            binaryName = "/opt/bin/runc"

No debug.sock is setup nor present on the node, it seems to default to "", we can check with /opt/bin/containerd config dump which gives us

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

We can setup it explicitely like you said, it may be less confusing for us and users.

[VannTen]

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

I think this would be an okay output when running with kubespray defaults, so yeah, let's go the explicit way (minus the enable which is not needed if we just default to "" for the address.

[spnngl]

I pushed my modifications

[VannTen]

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.

[spnngl]

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

[VannTen]

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.

[VannTen]

Also, could you make the release note a bit more concise ? It's supposed to be a single line entry in a Releases Notes (example

[spnngl]

@VannTen Hi, anything to add before merging ?

[yankay]

Thanks @spnngl

A great PR. /approve

[yankay]

HI @VannTen @mzaian
Would you please help to review it :-)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mzaian, spnngl, yankay

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:

• OWNERS [mzaian,yankay]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tico88612]

/lgtm