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.

Sign up for GitHub

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

Jump to bottom

fix: logs updated on starting of a stopped container #3896

Open

Shubhranshu153 wants to merge 1 commit into containerd:main from Shubhranshu153:fix-logs-on-attach

+115 −3

Contributor

Shubhranshu153 commented Feb 18, 2025 •

edited

Loading

Issue:

When we start a container that was running and stopped or exited, logs are not updated.

Steps to Reproduce:

step 1: Run and exit container

sudo nerdctl run -it --name test-1 alpine:latest
/ # echo hello
hello
/ # exit

start the same container

 sudo nerdctl start -a test-1
/ # echo bar
bar
/ # exit

Check logs. Bar is missing.

 sudo nerdctl logs test-1
/ # echo hello
hello
/ # exit

fix:

The pr fixes it by adding multiwriters to start of containers.

Shubhranshu153 force-pushed the fix-logs-on-attach branch 6 times, most recently from 49019a2 to ccb179d Compare

February 18, 2025 09:03

Contributor Author

Shubhranshu153 commented Feb 18, 2025

@AkihiroSuda
PTAL when you get a chance.
Thank You.

AkihiroSuda requested a review from fahedouch

February 19, 2025 02:47

AkihiroSuda added this to the v2.x.x (tentative) milestone

AkihiroSuda added the area/logging label

fahedouch reviewed

View reviewed changes

pkg/taskutil/taskutil.go Show resolved Hide resolved


          fix: logs updated on starting of a stopped container

e8fb6c4

Signed-off-by: Shubharanshu Mahapatra <[email protected]>

Shubhranshu153 force-pushed the fix-logs-on-attach branch from ccb179d to e8fb6c4 Compare

February 20, 2025 18:32

Contributor

apostasie commented Feb 21, 2025

Docker failure is #3908

Rootful failure is #3513

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              			),

              			Setup: func(data test.Data, helpers test.Helpers) {

              				cmd := helpers.Command("run", "-it", "--name", data.Identifier(), testutil.CommonImage)

              				cmd.WithWrapper("unbuffer", "-p")

Contributor

apostasie Feb 22, 2025

Hopefully, #3591 should be merged soon and remove the need to use unbuffer and DelayOnceReader.

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              				return &test.Expected{

              					ExitCode: 0,

              					Errors:   []error{},

              					Output: test.All(

Contributor

apostasie Feb 22, 2025

I guess test.All( is unnecessary here as you only have one condition to test.

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {

              				return &test.Expected{

              					ExitCode: 0,

              					Errors:   []error{},

Contributor

apostasie Feb 22, 2025

You can remove this line entirely.

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              			),

              			Setup: func(data test.Data, helpers test.Helpers) {

              				cmd := helpers.Command("run", "-d", "--name", data.Identifier(), testutil.CommonImage, "sh", "-c", "while true; do echo foo; sleep 1; done")

              				cmd.Run(&test.Expected{

Contributor

apostasie Feb 22, 2025 •

edited

Loading

You can simplify to just: helpers.Ensure("run", "-d", "--name", data.Identifier(), testutil.CommonImage, "sh", "-c", "while true; do echo foo; sleep 1; done")
(that does the command and checks it is succesful)

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              				initialFooCount := strings.Count(initialLogs, "foo")

              				data.Set("initialFooCount", strconv.Itoa(initialFooCount))

              				helpers.Anyhow("start", data.Identifier())

              				time.Sleep(5 * time.Second)

Contributor

apostasie Feb 22, 2025

You possibly want to replace helpers.Anyhow("start", data.Identifier()) with helpers.Ensure("start", data.Identifier()) to make sure it is succesful.

Also nerdtest.EnsureContainerStarted should give you a more determistic way to achieve this without relying on an arbitrary wait.

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              				data.Set("initialFooCount", strconv.Itoa(initialFooCount))

              				helpers.Anyhow("start", data.Identifier())

              				time.Sleep(5 * time.Second)

              				cmd := helpers.Command("logs", data.Identifier())

Contributor

apostasie Feb 22, 2025

return helpers.Command("logs", data.Identifier())

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {

              				return &test.Expected{

              					ExitCode: 0,

              					Errors:   []error{},

Contributor

apostasie Feb 22, 2025

Unnecessary.

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              				return &test.Expected{

              					ExitCode: 0,

              					Errors:   []error{},

              					Output: test.All(

Contributor

apostasie Feb 22, 2025

Same as above.

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
              						func(stdout string, info string, t *testing.T) {

              							finalLogsCount := strings.Count(stdout, "foo")

              							initialFooCount, _ := strconv.Atoi(data.Get("initialFooCount"))

              							assert.Assert(t, finalLogsCount > initialFooCount, "Expected 'foo' count to increase after restart")

Contributor

apostasie Feb 22, 2025

assert.Assert(t, finalLogsCount > initialFooCount, "Expected 'foo' count to increase after restart", info)

That will display context if it fails.

apostasie reviewed

View reviewed changes

cmd/nerdctl/container/container_logs_test.go

    
            @@ -255,3 +258,97 @@ func TestTailFollowRotateLogs(t *testing.T) {
          
              	}

              	assert.Equal(t, true, len(tailLogs) > linesPerFile, logRun.Stderr())

              }

              func TestLogsWithStartContainer(t *testing.T) {

              	if runtime.GOOS == "windows" {

Contributor

apostasie Feb 22, 2025

testCase.Require = test.Not(test.Windows)

Contributor

apostasie Feb 22, 2025

Also, if you do it at the test root, no need for it in the subtests.

Contributor

apostasie commented Feb 22, 2025

@Shubhranshu153 I just have a batch of nits on the test.

Btw, feedback on the test framework is highly welcome.

Ideally, if we get #3591 in first, we can get rid of the added DelayOnceReaders here (and unbuffer).

But anyhow, don't sweat it - I can clean that up afterwards if needed.

apostasie approved these changes

View reviewed changes

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

Labels