[Regression in v1.2.0] nerdctl logs exits silently for nerdctl run w/o -d

[AkihiroSuda]

nerdctl run --name=foo alpine echo foo
nerdctl logs foo

v1.1.0 fails as expected (because -d is not specified and the logging driver is not enabled)

FATA[0000] open /home/suda/.local/share/nerdctl/1935db59/containers/default/ed96e51c4c5f9b3d64cae624cba6b33ff3b9db3ed3b0224cc4bb02d1cbfc1a5c/log-config.json: no such file or directory

The main branch exits silently, with exit code 0.

Regression in c67b102

[fahedouch]

Because of this c67b102#diff-464b1e1a904b2f79ca04e2202190403c19f87a135b6a9e1ef2e65fa52772a221R85

Condition should be replaced by if logURI != ""

[AkihiroSuda]

Because of this c67b102#diff-464b1e1a904b2f79ca04e2202190403c19f87a135b6a9e1ef2e65fa52772a221R85

Condition should be replaced by if logURI != ""

Thanks, would you like to open a PR?

[fahedouch]

Because of this c67b102#diff-464b1e1a904b2f79ca04e2202190403c19f87a135b6a9e1ef2e65fa52772a221R85
Condition should be replaced by if logURI != ""

Thanks, would you like to open a PR?

Sure

[fahedouch]

@AkihiroSuda We have an other issue here
nerdctl run --name=foo alpine echo foo is not printing foo to stdout. Looks like nerdctl logs is not triggered behind the scene. Is it normal ?

[AkihiroSuda]

nerdctl run --name=foo alpine echo foo is not printing foo to stdout.

Can't repro on my side

[fahedouch]

nerdctl run --name=foo alpine echo foo is not printing foo to stdout.

Can't repro on my side

@AkihiroSuda Yes sorry, it is fine until this fix

[AkihiroSuda]

The regression isn't critical, so we can defer fixing this to v1.3 or later.
Maybe even no need to fix it, as long as we mention this behavior in nerdctl logs --help

[austinvazquez]

Hi @fahedouch, I had a look at the purposed fix in the draft PR above and it looked like you are attempting to build a feature similar to Docker's dual logging. For the purpose of this issue, if the goal is to restore previous behavior, then the change would be to restore the line here: c67b102#diff-21f959b0a9d34cee85c5c25014fe6cef81d7c30c93c5294231cf6ea80f0f65c9L495

Currently an empty file is being created for containers run without -d which causes the os.Stat to succeed on nerdctl logs walk.

Dual logging is a nice to have but is likely much more involved and can be covered by #1657.

[fahedouch]

hi @austinvazquez,

For the purpose of this issue, if the goal is to restore previous behavior, then the change would be to restore the line here: c67b102#diff-21f959b0a9d34cee85c5c25014fe6cef81d7c30c93c5294231cf6ea80f0f65c9L495

restore the the above line means no log persistance for nerdctl run w/o -d, that means we lost logs when restart container. I believe that we can accept this regression as is not critical and focus on how to complete the log flow by attempting to make a dual logging for nerdctl run w/o -d.

[austinvazquez]

restore the the above line means no log persistance for nerdctl run w/o -d, that means we lost logs when restart container

Not quite, log persistence is not supported for containers ran without -d. See here. Also restart container uses detached so that case is covered. Reference here.

[fahedouch]

@austinvazquez

Not quite, log persistence is not supported for containers ran without -d.

You are right. it is a non complete fix introduced by this commit. the log persistence need this change to be ok, but this change needs the dual logging to satisfy CI tests. So we have two option:

• rollback commit and work on it as a new feature
• consider this bug as non critical and continue working on the dual logging setup to unblock the CI ( we may defer fixing this to v1.3 or later)

cc @AkihiroSuda WDYT ?

[AkihiroSuda]

No need to rollback the commit, this is a non-critical.

[austinvazquez]

Sounds good thanks folks @fahedouch @AkihiroSuda. Will focus efforts on dual logging design.