Docker reports error on parsing crun version

[carlosedp]

Docker is reporting error on parsing crun version:

Aug 28 09:24:08 fedora-unleashed dockerd[2639]: time="2019-08-28T09:24:08.085535495-03:00" level=warning msg="failed to parse /usr/local/bin/crun version: unknown output format: crun 0.7\nspec: 1.0.0\n+SYSTEMD +SELINUX +CAP +SECCOMP +EBPF +YAJL\n"

Docker is configured like this:

{
  "default-runtime": "crun",
  "debug": false,
  "max-concurrent-uploads": 1,
  "runtimes": {
    "crun": {
        "path": "/usr/local/bin/crun",
        "runtimeArgs": [
              "--debug"
      ]
    }
  }
}

[rhatdan]

What is the version of docker you are testing with?
BTW Works well with Podman. :^)

[carlosedp]

Built from tree in august, 12.

❯ docker version                                                                                                                                                                           ─╯
Client:
 Version:           unknown-version
 API version:       1.40
 Go version:        devel +0c72bd386b Fri Aug 9 17:52:39 2019 +1000
 Git commit:        f807b5ef
 Built:             Mon Aug 12 15:53:00 2019
 OS/Arch:           linux/riscv64
 Experimental:      true

Server:
 Engine:
  Version:          library-import
  API version:      1.41 (minimum version 1.12)
  Go version:       devel +0c72bd386b Fri Aug 9 17:52:39 2019 +1000
  Git commit:       library-import
  Built:            library-import
  OS/Arch:          linux/riscv64
  Experimental:     false
 containerd:
  Version:          1.2.0+unknown
  GitCommit:
 docker-init:
  Version:          0.18.0
  GitCommit:        3c53686

I know... have been using podman for a while too :)

[rhatdan]

Must be something about the output that crun is generating that Docker does not like.

[rhatdan]

It would be nice to get it working though, since F31 is going to cgroups v2 and runc is not be ready for it. So we are moving default to crun for the time being.

[carlosedp]

That's great! This is the function on Docker that parses the runtime version:

// parseRuncVersion parses the output of `runc --version` and extracts the
// "version" and "git commit" from the output.
//
// Output example from `runc --version`:
//
//   runc version 1.0.0-rc5+dev
//   commit: 69663f0bd4b60df09991c08812a60108003fa340
//   spec: 1.0.0
func parseRuncVersion(v string) (version string, commit string, err error) {
	lines := strings.Split(strings.TrimSpace(v), "\n")
	for _, line := range lines {
		if strings.HasPrefix(line, "runc version") {
			version = strings.TrimSpace(strings.TrimPrefix(line, "runc version"))
			continue
		}
		if strings.HasPrefix(line, "commit:") {
			commit = strings.TrimSpace(strings.TrimPrefix(line, "commit:"))
			continue
		}
	}
	if version == "" && commit == "" {
		err = errors.Errorf("unknown output format: %s", v)
	}
	return version, commit, err
}

I think Docker shouldn't hardcore "runc version" on parsing.

[giuseppe]

Could you fill an issue with Moby? We could easily work around it but I don't think there should be any assumption on the runtime version output if multiple runtime are supported, and should not be crun to lie about it.

[carlosedp]

I will. Agree that it's not a thing to be fixed on crun side.
I also found other weirdnesses on Moby like if I don't have a runc binary or link, it does not start, even defining that crun is my default-runtime.
Looking for more hard-coded stuff on their side.

[giuseppe]

Can you please tag me on the issue once you open it?

[carlosedp]

Sure, one thing tho... I was looking at the code and is it possible to have something like "crun version 0.7" printed instead of "crun 0.7". This would ease parsing since it's the way runc does.
Do you think it's worth? I haven't looked into other runtimes to see how they print the version.

[ghost]

That smells sloppy. I think we should not blindly copy what runc or other runtimes does.

[carlosedp]

What do you think @giuseppe and @rhatdan ? Should we have something similar to how Docker parses the runtime to avoid swamping the log?

[rhatdan]

I think having version on the line would make it easier to patch. I would just split the line via spaces and take the last value.

Why does Docker wants these values in the first place? Just for docker info?

[carlosedp]

I think it’s the easier and faster option. I’ll submit a PR to address this. I believe it uses only for information.

[carlosedp]

Opened moby/moby#39940. I think that with this, changing Crun won't be necessary.

[carlosedp]

I also opened the PR #93 to address this on crun.