nerdctl icon indicating copy to clipboard operation
nerdctl copied to clipboard

Allow external containerd-logging-plugin for nerdctl

Open phuslu opened this issue 2 years ago • 5 comments

What is the problem you're trying to solve

Memory footprint per container, e.g. https://github.com/containerd/nerdctl/issues/2441 https://github.com/containerd/nerdctl/issues/1419

Describe the solution you'd like

Add an optional containerd-logging-plugin detection and loading, e.g.

diff --git a/pkg/cmd/container/create.go b/pkg/cmd/container/create.go
index c39d036c..47a406fc 100644
--- a/pkg/cmd/container/create.go
+++ b/pkg/cmd/container/create.go
@@ -408,6 +408,10 @@ func GenerateLogURI(dataStore string) (*url.URL, error) {
 	if err != nil {
 		return nil, err
 	}
+	shimExe := selfExe + "-internal-logging"
+	if fi, err := os.Stat(shimExe); err == nil && (fi.Mode().Perm()&0111) != 0 {
+		selfExe = shimExe
+	}
 	args := map[string]string{
 		logging.MagicArgv1: dataStore,
 	}

above naive patch will enable us to putting /usr/local/bin/nerdctl-internal-logging instead of nerdctl image

Additional context

I already have a PoC in https://github.com/phuslu/nerdctl/tree/main/cmd/nerdctl-internal-logging but need more tweaks.

phuslu avatar Dec 06 '23 17:12 phuslu

This would be cool if we can config the binary path when running the container or via the config file.

Would you mind to open a PR?

Zheaoli avatar Dec 06 '23 18:12 Zheaoli

understood. Which mechanism is prefered?

  1. reuse exsiting option, e.g. --log-opt plugin=/usr/local/bin/nerdctl-inerternal-logging
  2. introduce a new option, e.g. --log-plugin /usr/local/bin/nerdctl-inerternal-logging or --log-shim
  3. introduce a config file?

phuslu avatar Dec 07 '23 14:12 phuslu

Although we can use the following command line to call the external log plug-in, but seems that there is still a lack of a log directory mechanism for passing in nerdctl

nerdctl --log-driver=binary:///usr/local/bin/nerdctl-internal-logging

In my implementation https://github.com/phuslu/nerdctl/blob/main/cmd/nerdctl-internal-logging/main.rs#L29 I have to hardcoded "/var/lib/nerdctl/1935db59" in soure code.

Any suggestions here?

phuslu avatar Dec 17 '23 05:12 phuslu

Another issue is that "nerdctl compose" does not support logging.driver option currently. it complains

FATA[0000] validating /home/phuslu/test/compose.yaml: (root) Additional property logging is not allowed

on

version: '3.5'
services:
  jellyfin:
    image: jellyfin/jellyfin
logging:
  driver: "binary:///usr/local/bin/nerdctl-internal-logging"

So base of above, Can we please reconsider the initial patch?

+	shimExe := selfExe + "-internal-logging"
+	if fi, err := os.Stat(shimExe); err == nil && (fi.Mode().Perm()&0111) != 0 {
+		selfExe = shimExe
+	}

phuslu avatar Dec 17 '23 08:12 phuslu

BTW, for those interested about reducing the memory footprint of the nerdctl logging plugin, I recommend give a try https://github.com/phuslu/nerdctl/tree/main/cmd/nerdctl-internal-logging I think it's mature and it successfully reduces the memory usage(both RSS&VIRT) of the nerdctl log plug-in to 1M~2M bytes.

phuslu avatar Dec 17 '23 08:12 phuslu