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

[Carry 2337] shutdown logger after exit #2621

Closed

Conversation

fahedouch
Copy link
Member

carry #2337 ( originally created by @mharwani)

@fahedouch fahedouch changed the title Carry shutdown logger after exit [Carry 2337] shutdown logger after exit Nov 1, 2023
@fahedouch fahedouch force-pushed the carry-shutdown-logger-after-exit branch 2 times, most recently from 46fe32b to 10676af Compare November 1, 2023 22:10
@AkihiroSuda
Copy link
Member

rebase changes

This shouldn't be a separate commit

@fahedouch fahedouch force-pushed the carry-shutdown-logger-after-exit branch 2 times, most recently from f351d2e to f2a16e4 Compare November 6, 2023 16:47
@fahedouch
Copy link
Member Author

fahedouch commented Nov 6, 2023

rebase changes

This shouldn't be a separate commit

@AkihiroSuda is it better now ?

@AkihiroSuda
Copy link
Member

The commit message isn't still really descriptive

@@ -207,7 +208,7 @@ func loggingProcessAdapter(ctx context.Context, driver Driver, dataStore, hostAd
buf := make([]byte, 32<<10)
_, err := io.CopyBuffer(writer, reader, buf)
if err != nil {
logrus.Errorf("failed to copy stream: %s", err)
log.L.Errorf("failed to copy stream: %v", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should meld into the carried commit so that the carried commit is still compilable

@fahedouch fahedouch force-pushed the carry-shutdown-logger-after-exit branch 3 times, most recently from d6055c3 to 193c92b Compare November 6, 2023 20:06
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still contains uncompilable commits

$ git checkout a234c8eab60db7b5a0d2933729c9035ae66fa0e4

$ make
GO111MODULE=on CGO_ENABLED=0 GOOS=linux go build -ldflags "-s -w -X github.com/containerd/nerdctl/pkg/version.Version=v1.7.0-10-ga234c8ea -X github.com/containerd/nerdctl/pkg/version.Revision=a234c8eab60db7b5a0d2933729c9035ae66fa0e4"   -o /home/suda/gopath/src/github.com/containerd/nerdctl/_output/nerdctl github.com/containerd/nerdctl/cmd/nerdctl
# github.com/containerd/nerdctl/pkg/logging
pkg/logging/logging.go:179:4: undefined: logrus
make: *** [Makefile:57: nerdctl] Error 1

// the logger will obtain an exclusive lock on a file until the container is
// stopped and the driver has finished processing all output,
// so that waiting log viewers can be signalled when the process is complete.
return lockutil.WithDirLock(loggerLock, func() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"DirLock" is now misnomer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ? could you be more explicit please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loggerLock doesn't seem to be a dir

@fahedouch fahedouch force-pushed the carry-shutdown-logger-after-exit branch 11 times, most recently from 41e8522 to c322829 Compare December 9, 2023 22:19
Signed-off-by: Mrudul Harwani <[email protected]>

fixes to squash

Signed-off-by: fahed dorgaa <[email protected]>

fixes to squash

Signed-off-by: fahed dorgaa <[email protected]>
@fahedouch fahedouch force-pushed the carry-shutdown-logger-after-exit branch from 10132ee to 21730db Compare December 10, 2023 11:03
@fahedouch fahedouch closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants