feat: implment persistent logging for containers running in foreground #2683
Conversation
go.mod
Outdated
| @@ -33,6 +33,7 @@ require ( | |||
| github.com/fahedouch/go-logrotate v0.2.0 | |||
| github.com/fatih/color v1.16.0 | |||
| github.com/fluent/fluent-logger-golang v1.9.0 | |||
| github.com/hashicorp/go-multierror v1.1.1 | |||
There was a problem hiding this comment.
You can use errors.Join
https://github.com/search?q=repo%3Acontainerd%2Fnerdctl%20errors.Join&type=code
pkg/cmd/container/run_runtime.go
Outdated
| @@ -48,7 +48,6 @@ func generateRuntimeCOpts(cgroupManager, runtimeStr string) ([]containerd.NewCon | |||
| runtimeOpts = nil | |||
| } | |||
| } else { | |||
| // runtimeStr is a runc binary | |||
There was a problem hiding this comment.
Was this line removed by an accident?
There was a problem hiding this comment.
Yes, fixed it in the last commit.
pkg/ioutil/container_io.go
Outdated
| func NewContainerIO(namespace string, logURI string, tty bool, stdin io.Reader, stdout, stderr io.Writer) cio.Creator { | ||
| return func(id string) (cio.IO, error) { | ||
|
|
||
| // is from https://github.com/containerd/containerd/blob/main/cmd/containerd-shim-runc-v2/process/io.go#L247 |
There was a problem hiding this comment.
Please use a permanent link that contains a tag or a commit hash
pkg/ioutil/container_io.go
Outdated
| cmd.ExtraFiles = append(cmd.ExtraFiles, sr, serrr, w) | ||
|
|
||
| if err := cmd.Start(); err != nil { | ||
| return nil, fmt.Errorf("failed to start binary process: %w", err) |
There was a problem hiding this comment.
The error string should contain cmd.Args
pkg/ioutil/container_io.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package ioutil |
There was a problem hiding this comment.
The package name can be confused with the deprecated io/ioutil
There was a problem hiding this comment.
Yes makes sense, I missed the io/ioutil package. Changed it to cioutil in last commit.
pkg/ioutil/container_io.go
Outdated
| cmd := process.NewBinaryCmd(uri, id, namespace) | ||
| cmd.ExtraFiles = append(cmd.ExtraFiles, sr, serrr, w) | ||
|
|
||
| if err := cmd.Start(); err != nil { |
There was a problem hiding this comment.
The pipes should be cleaned up on an error
There was a problem hiding this comment.
Updated with closer to close pipes and stop logger process on errors.
vsiravar
changed the title
vsiravar
force-pushed
the
vsiravar/update-container-io
branch
7 times, most recently
from
3aa3276
to
76bf40d
Compare
Signed-off-by: Vishwas Siravara <siravara@amazon.com>
vsiravar
force-pushed
the
vsiravar/update-container-io
branch
from
3bf4c8d
to
990d7ec
Compare
vsiravar
changed the title
vsiravar
changed the title
|
Thanks for reviewing the PR. I have addressed the initial feedback, refactored |
Fixes: #1657
cio.Creatorinterface to start logging binary as per https://github.com/containerd/containerd/blob/main/cmd/containerd-shim-runc-v2/process/io.go#L247 which creates 2 pipes to forward container output to the logging binary and one pipe to check if the logging binary is ready.TODO:
- Refactoringtaskutilandcontainer_ioto use customcio.Creatorfor all flags(-d,-t,-i,-a). This would unblock attach for -d.[X] Changes for windows.
[X] Adding integration tests