Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add containerd config options #11080

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add containerd config options #11080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cd3f27
chore(containerd): add some config debug options
spnngl May 14, 2024
964fc23
chore(containerd): add CRI config options
spnngl Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions inventory/sample/group_vars/all/containerd.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,21 @@
# containerd_grpc_max_recv_message_size: 16777216
# containerd_grpc_max_send_message_size: 16777216

# Containerd debug socket location: unix or tcp format
# containerd_debug_address: ""

# Containerd log level
# containerd_debug_level: "info"

# Containerd logs format, supported values: text, json
# containerd_debug_format: ""

# Containerd debug socket UID
# containerd_debug_uid: 0

# Containerd debug socket GID
# containerd_debug_gid: 0

# containerd_metrics_address: ""

# containerd_metrics_grpc_histogram: false
Expand Down
10 changes: 10 additions & 0 deletions roles/container-engine/containerd/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ containerd_base_runtime_specs:
containerd_grpc_max_recv_message_size: 16777216
containerd_grpc_max_send_message_size: 16777216

containerd_debug_address: ""
containerd_debug_level: "info"
containerd_debug_format: ""
containerd_debug_uid: 0
containerd_debug_gid: 0

containerd_metrics_address: ""

Expand All @@ -66,6 +70,12 @@ containerd_enable_unprivileged_ports: false
# If enabled it will allow non root users to use icmp sockets
containerd_enable_unprivileged_icmp: false

containerd_enable_selinux: false
containerd_disable_apparmor: false
containerd_tolerate_missing_hugetlb_controller: true
containerd_disable_hugetlb_controller: true
containerd_image_pull_progress_timeout: 5m

containerd_cfg_dir: /etc/containerd

# Extra config to be put in {{ containerd_cfg_dir }}/config.toml literally
Expand Down
9 changes: 9 additions & 0 deletions roles/container-engine/containerd/templates/config.toml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ oom_score = {{ containerd_oom_score }}
max_send_message_size = {{ containerd_grpc_max_send_message_size }}

[debug]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@spnngl spnngl May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed my modifications

address = "{{ containerd_debug_address }}"
level = "{{ containerd_debug_level }}"
format = "{{ containerd_debug_format }}"
uid = {{ containerd_debug_uid }}
gid = {{ containerd_debug_gid }}

Copy link
Member

@yankay yankay Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it in last push

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[metrics]
address = "{{ containerd_metrics_address }}"
Expand All @@ -24,6 +28,11 @@ oom_score = {{ containerd_oom_score }}
max_container_log_line_size = {{ containerd_max_container_log_line_size }}
enable_unprivileged_ports = {{ containerd_enable_unprivileged_ports | lower }}
enable_unprivileged_icmp = {{ containerd_enable_unprivileged_icmp | lower }}
enable_selinux = {{ containerd_enable_selinux | lower }}
disable_apparmor = {{ containerd_disable_apparmor | lower }}
tolerate_missing_hugetlb_controller = {{ containerd_tolerate_missing_hugetlb_controller | lower }}
disable_hugetlb_controller = {{ containerd_disable_hugetlb_controller | lower }}
image_pull_progress_timeout = "{{ containerd_image_pull_progress_timeout }}"
{% if enable_cdi %}
enable_cdi = true
cdi_spec_dirs = ["/etc/cdi", "/var/run/cdi"]
Expand Down