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.

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

Support nerdctl logs for the containers running without -d #1657

Closed
ningziwen opened this issue Dec 15, 2022 · 14 comments · Fixed by #2683
Closed

Support nerdctl logs for the containers running without -d #1657

ningziwen opened this issue Dec 15, 2022 · 14 comments · Fixed by #2683

Comments

@ningziwen
Copy link
Contributor

What is the problem you're trying to solve

Support nerdctl logs for the containers running without -d

README.md already stated it is not supported yet. So opening a feature request instead of a bug report.

Reproducd by building source code from main branch.

$ sudo ./_output/nerdctl run -it ubuntu
# open a new terminal

$ sudo ./output/nerdctl ps # get the container id

$ sudo ./_output/nerdctl logs ccc97467def7
FATA[0000] failed to load logging config: failed to read log config file "/var/lib/nerdctl/1935db59/containers/default/ccc97467def7dd95330c6e872917e130180a1de2e4d6c34bdebeb16af434a5a6/log-config.json": open /var/lib/nerdctl/1935db59/containers/default/ccc97467def7dd95330c6e872917e130180a1de2e4d6c34bdebeb16af434a5a6/log-config.json: no such file or directory

Describe the solution you'd like

Add the feature to support logs command with the container running without -d.

Additional context

runfinch/finch#85

@ningziwen
Copy link
Contributor Author

/assign

@ningziwen
Copy link
Contributor Author

ningziwen commented Dec 20, 2022

Trying error log tracing in nerdctl logs command.

nerdctl logs -> log_viewer.InitContainerLogViewer() -> logging.LoadLogConfig()

The error is failed in os.ReadFile(logConfigFilePath) without no such file or directory

The logConfigFilePath is the log-config.json in current container's dataStore dir.

log-config.json's directory of the container running by sudo ./_output/nerdctl run -it ubuntu

$ ls
hostname  oci-hook.createRuntime.log  resolv.conf

log-config.json's directory of the container running by sudo ./_output/nerdctl run -d ubuntu

$ ls
<id>-json.log  hostname  log-config.json  oci-hook.createRuntime.log  resolv.conf

Found the line to write log-config.json.

@ningziwen
Copy link
Contributor Author

Just found this recent merged PR fixed the error. The log-config.json appears in the container ran by nerdctl run -it now.

However, there is no log in the container ran by nerdctl run -it.

Ran while true; do $(echo date); sleep 1; done in terminal of -it container. Then nerdctl logs returns empty.

Ran sudo ./_output/nerdctl -d ubuntu sh -c "while true; do $(echo date); sleep 1; done", then nerdctl logs returns printed date lines.

@yzxiu
Copy link
Contributor

yzxiu commented Dec 22, 2022

When the container is run with -it(or without -d),log data is output to nerdctl process stdout.
If want to get logs through nerdctl logs, it may be necessary to copy the log to the log file(or other log drivers) in another goroutine.

@ningziwen
Copy link
Contributor Author

Currently, -it uses the ioCreator to write stdOut to console.Current().

-d uses the ioCreator to write to uri.String().

@AkihiroSuda
Copy link
Member

@ningziwen Do you still plan to work on this?

@ningziwen
Copy link
Contributor Author

@ningziwen Do you still plan to work on this?

Yes but it will take some time. Feel free to take over if you have urgent need of this feature.

@danishprakash
Copy link
Contributor

@ningziwen I can work on this if it's okay.

@ningziwen
Copy link
Contributor Author

@danishprakash Sure go ahead.

@ningziwen ningziwen removed their assignment Feb 16, 2023
@danishprakash
Copy link
Contributor

I spent some time looking at this and based on what @ningziwen has already mentioned, I tried to create a multiwriter and create a ioCreator out of it but it's messy and didn't work primarily because uri.String() dir tree isn't setup yet or isn't the right path at the point of execution in NewTask.

@yzxiu when you say we need to copy the logs over explicitly, shouldn't that piece go into containerd? I'm new to this project so still trying to understand the demarcations. Sorry If I missed anything.

@fahedouch
Copy link
Member

fahedouch commented Feb 17, 2023

I tried to fix this here but stucked because of this <= containerd-dev slack thread

It looks like containerd io limitation

@danishprakash
Copy link
Contributor

I think this issue can be closed then in lieu of #1946

@AkihiroSuda
Copy link
Member

I think this issue can be closed then in lieu of #1946

No, that one is a different topic

@vsiravar
Copy link
Contributor

vsiravar commented Dec 5, 2023

As per SCOPE.md which states that “Logging can be build on top of containerd because the container’s STDIO will be provided to the clients and they can persist any way they see fit. There is no io copying of container STDIO in containerd.” So this rules out modifying/creating the NewTask(context.Context, cio.Creator, ...NewTaskOpts) (Task, error) interface in containerd.This needs to be handled on the client side(i.e nerdctl) similar to what is done by cri plugin.

A proposal to solve this is for nerdctl to implement it's own cio.Creator which can start the logging binary process as and send stdout and stederr streams to it using pipes, i.e mimic the behavior of shim logging in the cio.Creator implementation.

image

I made draft PR: #2683 and ran some sanity tests

 sudo nerdctl run --name interactive alpine sh -c "echo hello"
hello
$ sudo nerdctl logs interactive
hello



$ sudo nerdctl run -it --name itty alpine 
/ # ls
bin    dev    etc    home   lib    media  mnt    opt    proc   root   run    sbin   srv    sys    tmp    usr    var
/ # pwd
/
/ # exit
$ sudo nerdctl logs itty
/ # ls
bin    dev    etc    home   lib    media  mnt    opt    proc   root   run    sbin   srv    sys    tmp    usr    var
/ # pwd
/
/ # exit

cc @fahedouch @AkihiroSuda Let me know if this approach is okay and I can work on getting my PR merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants