libpod/Container.readFromJournal(): don't skip the first entry #11263

nalind · 2021-08-18T16:19:44Z

When reading log entries from the journal, don't skip past the first matching entry after we've positioned the cursor at it.

Fixes #11253.

vrothberg · 2021-08-18T16:35:33Z

LGTM

I have changed my opinion also since the meeting. I think that #10653 would actually pass when using c/common v0.43.2.

nalind · 2021-08-18T16:36:06Z

Hmm, when I run tests manually in my VM, some of them are still failing. This might require some tweaking yet.

vrothberg · 2021-08-18T16:36:29Z

@nalind, could you pick up https://github.com/containers/podman/pull/10653/files and bump c/common?

nalind · 2021-08-18T16:44:25Z

Hmm, when I run tests manually in my VM, some of them are still failing. This might require some tweaking yet.

Oof, that wasn't with this PR, but it looked better locally once I'd checked out the pull request.

@nalind, could you pick up https://github.com/containers/podman/pull/10653/files and bump c/common?

Okay, I think that's in here now.

vrothberg

LGTM

openshift-ci · 2021-08-18T16:47:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nalind, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nalind · 2021-08-18T17:23:41Z

That extra blank entry that #10653 is adding when each container starts seems to be in conflict here.

nalind · 2021-08-18T18:26:28Z

Let's see what happens when we make that first entry a "history" event.

mheon · 2021-08-18T19:01:41Z

One failure looks like a potential network flake

nalind · 2021-08-18T19:32:54Z

My test system had events_logger = "file" set in /usr/share/containers/containers.conf, which was causing the podman logs - multi journald test to fail.

edsantiago · 2021-08-18T20:07:28Z

This is a real error caused by the new c/common:

$ podman tag 961769676411f082461f9ef46626dd7a2d1e2b2a38e6a44364bcbecf51e66dd4 quay.io/libpod/alpine@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00
Error: quay.io/libpod/alpine@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00: tag by digest not supported

nalind · 2021-08-18T20:25:07Z

This is a real error caused by the new c/common:

$ podman tag 961769676411f082461f9ef46626dd7a2d1e2b2a38e6a44364bcbecf51e66dd4 quay.io/libpod/alpine@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00
Error: quay.io/libpod/alpine@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00: tag by digest not supported

Oh, I don't think we're supposed to be able to "tag" images with names that don't include tags (a.k.a. tagged references). I wrote that test, so I guess it's fitting that I get to fix it.

TomSweeneyRedHat · 2021-08-19T00:00:14Z

LGTM once the flake bit is hunted down.

vrothberg

/lgtm
/hold

Great work, @nalind (and @rhatdan!).

vrothberg · 2021-08-19T08:11:56Z

The failure in the system tests seems consistent but only on sys remote ubuntu-2104 root host.

[+0491s] not ok 149 podman kill - test signal handling in containers

Not sure what to do other than rebasing and trying again?

nalind · 2021-08-19T13:34:41Z

Rebased.

[NO TESTS NEEDED] Since we are just testing the default. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

When reading log entries from the journal, don't skip past the first matching entry after we've positioned the cursor at it. Make the first blank-line entry that we logged so that the container would always have at least one log entry for us to find (until it gets vacuumed out, at least) a fake history entry, so that `logs` doesn't pass it on for display. CI already has tests that exercise journal-based logging, so [NO TESTS NEEDED] Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Update github.com/containers/common from 0.43.0 to 0.43.2. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

In these tests, don't try to tag an image using a canonical ("with digest") image name. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

In libpod/logs.LogLine.Write(), don't write a newline to stdout/stderr when the log message is only part of a line. In libpod.ConmonOCIRuntime.HTTPAttach(), don't send a newline over the HTTP connection when the log message is only part of a line. In pkg/api/handlers/compat.LogsFromContainer(), don't send a newline over the HTTP connection when the log message is only part of a line, and don't make doing so conditional on whether or not the client used the docker or podman endpoint. In pkg/domain/infra/tunnel.ContainerEngine.ContainerLogs(), don't add our own newline to log messages, since they already come through from the server when they need to. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Increase the amount of time we're willing to wait for a log message that a container should be printing to show up in the output of `logs -f`, since on at least one CI configuration we're seeing a turnaround as high as 46s, but it's not something we can directly control, so that's not a hard maximum. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind · 2021-08-24T00:31:37Z

The logs: adjust handling around partial log messages commit changes how the HTTP logs handler works. The previous behavior didn't correctly handle cases where a container was outputting one character at a time (for example if the container was a shell sitting at a shell prompt and a person was typing) - the client would receive each logged character as if it were a different line of output. But I'd appreciate a nod from people who are more familiar with it.

vrothberg

LGTM

@containers/podman-maintainers PTAL on the new changes.

vrothberg · 2021-08-24T09:07:57Z

@Luap99 PTAL

vrothberg · 2021-08-24T09:08:07Z

Note: once merged, we can close #10587

Luap99 · 2021-08-24T09:22:04Z

pkg/api/handlers/compat/containers_logs.go

-		// Log lines in the compat layer require adding EOL
-		// https://github.com/containers/podman/issues/8058
-		if !utils.IsLibpodRequest(r) {
+		if !line.Partial() {


Does this match docker?

We use the same handler for both types of clients, so this makes both behave the same. Anecdotally, when I pointed a docker client at the podman socket, I get what looks like the correct behavior on the client end.

Luap99 · 2021-08-24T09:29:56Z

This is a breaking change for API users. Therefore this should not be backported to any 3.X branches. I am fine to merge this since we are on 4.0 on main but this should be documented somewhere.

vrothberg · 2021-08-24T09:41:14Z

This is a breaking change for API users. Therefore this should not be backported to any 3.X branches. I am fine to merge this since we are on 4.0 on main but this should be documented somewhere.

Unless it's a bug fix :) Let's explore what Docker does.

Luap99 · 2021-08-24T09:49:21Z

This is a breaking change for API users. Therefore this should not be backported to any 3.X branches. I am fine to merge this since we are on 4.0 on main but this should be documented somewhere.

Unless it's a bug fix :) Let's explore what Docker does.

The log change is not really a bug fix. The new lines are now added on the server side and not on the client side for the libpod endpoint. Therefore this is breaking libpod api users.

vrothberg · 2021-08-24T12:24:06Z

/hold cancel

Great work, @nalind and @rhatdan!

ashley-cui

/lgtm

nalind force-pushed the journal-read branch from 5737dc3 to c7fcc8a Compare August 18, 2021 16:43

vrothberg approved these changes Aug 18, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2021

nalind force-pushed the journal-read branch from c7fcc8a to 427b01d Compare August 18, 2021 16:49

nalind force-pushed the journal-read branch 2 times, most recently from 1c718f6 to 0a04558 Compare August 18, 2021 18:26

nalind force-pushed the journal-read branch from 0a04558 to 0fead70 Compare August 18, 2021 18:35

nalind force-pushed the journal-read branch from 0fead70 to 8418b0d Compare August 18, 2021 20:20

vrothberg reviewed Aug 19, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2021

openshift-ci bot assigned vrothberg Aug 19, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2021

This was referenced Aug 19, 2021

Switch eventlogger to journald by default #10653

Closed

Switch default logdriver and eventslogger to journald, if root containers/common#621

Merged

nalind force-pushed the journal-read branch from 8418b0d to bacdcb0 Compare August 19, 2021 13:34

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2021

rhatdan and others added 5 commits August 23, 2021 17:59

Switch eventlogger to journald by default

6b06e9b

[NO TESTS NEEDED] Since we are just testing the default. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

bump github.com/containers/common

fc6a020

Update github.com/containers/common from 0.43.0 to 0.43.2. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

330-corrupt-images: don't try to tag with a canonical name

b20a547

In these tests, don't try to tag an image using a canonical ("with digest") image name. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind force-pushed the journal-read branch from 510c14d to 7b19712 Compare August 23, 2021 22:02

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2021

nalind force-pushed the journal-read branch from 7b19712 to 3007bd4 Compare August 23, 2021 22:10

nalind mentioned this pull request Aug 24, 2021

utils.RunUnderSystemdScope(): always close Conn #11313

Merged

vrothberg reviewed Aug 24, 2021

View reviewed changes

Luap99 reviewed Aug 24, 2021

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2021

vrothberg mentioned this pull request Aug 24, 2021

default log driver: switch to journald #10587

Closed

ashley-cui reviewed Aug 24, 2021

View reviewed changes

openshift-ci bot assigned ashley-cui Aug 24, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2021

openshift-merge-robot merged commit 23f9565 into containers:main Aug 24, 2021

nalind deleted the journal-read branch August 24, 2021 18:16

Luap99 mentioned this pull request Sep 14, 2021

[v3.4] Revert "logs: adjust handling around partial log messages" #11562

Merged

riyad mentioned this pull request Jan 18, 2022

API server crashes with race condition in logging code #12904

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

libpod/Container.readFromJournal(): don't skip the first entry #11263

libpod/Container.readFromJournal(): don't skip the first entry #11263

Uh oh!

Conversation

nalind commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrothberg commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nalind commented Aug 18, 2021

Uh oh!

vrothberg commented Aug 18, 2021

Uh oh!

nalind commented Aug 18, 2021

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 18, 2021

Uh oh!

nalind commented Aug 18, 2021

Uh oh!

nalind commented Aug 18, 2021

Uh oh!

mheon commented Aug 18, 2021

Uh oh!

nalind commented Aug 18, 2021

Uh oh!

edsantiago commented Aug 18, 2021

Uh oh!

nalind commented Aug 18, 2021

Uh oh!

TomSweeneyRedHat commented Aug 19, 2021

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg commented Aug 19, 2021

Uh oh!

nalind commented Aug 19, 2021

Uh oh!

nalind commented Aug 24, 2021

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg commented Aug 24, 2021

Uh oh!

vrothberg commented Aug 24, 2021

Uh oh!

Luap99 Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

nalind Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

Luap99 commented Aug 24, 2021

Uh oh!

vrothberg commented Aug 24, 2021

Uh oh!

Luap99 commented Aug 24, 2021

Uh oh!

vrothberg commented Aug 24, 2021

Uh oh!

ashley-cui left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nalind commented Aug 18, 2021 •

edited

Loading

vrothberg commented Aug 18, 2021 •

edited

Loading