search

kubernetes-retired / rktlet

[EOL] The rkt implementation of the Kubernetes Container Runtime Interface
Apache License 2.0
137 stars 43 forks source link

Replace journal2cri with solution based on stdout/stderr stream #147

Closed nhlfr closed 7 years ago

nhlfr commented 7 years ago

rktlet was using journal2cri binary for converting journald logs to CRI format. Container with that binary was injected into each pod. Unfortunately, it couldn't disginguish stdout and stderr output. It also wasn't very stable and reliable.

Currently rkt is able to store logs in CRI format in provided directory[1]. Therefore we can remove journal2cri completely and just point rkt to the directory which CRI provides.

Fixes #107

[1] https://github.com/rkt/rkt/pull/3798

nhlfr commented 7 years ago

@dongsupark @iaguis PTAL

iaguis commented 7 years ago

Can we also remove the sdjournal dependency? I tried github.com/coreos/go-systemd and it seems to remove a lot of stuff not related with go-systemd. This is not a big deal though.

FWIW, it seems glide rm github.com/coreos/go-systemd && make glide works.

nhlfr commented 7 years ago

One question: what happens if there's no LogPath given in the ContainerCreate request?

Then we are in trouble, logging will not work and that would be a bug in upstream Kubernetes to fix. :)

nhlfr commented 7 years ago

But yeah, we can assume some default dir - /var/log/pods/. Will push a change soon.

dongsupark commented 7 years ago

Really? Where? The only place that annotation is set is here https://github.com/kubernetes-incubator/rktlet/pull/147/files#diff-cc47267a78cd35bd77be1ffb2771d4abR236 - and the value is not empty. ;)

@nhlfr AFAICS the LogPath is sometimes set to an empty string by the caller, cri-tools, for example, https://github.com/kubernetes-incubator/cri-tools/blob/v0.2/pkg/validate/container.go#L364_L376. OTOH some tests set LogPath correctly, for example, https://github.com/kubernetes-incubator/cri-tools/blob/v0.2/pkg/validate/container.go#L502..L513, In that case the annotation is set correctly in rktlet.

nhlfr commented 7 years ago

@dongsupark done

nhlfr commented 7 years ago

@iaguis done