Skip to content

fluentd log driver. failed parse last partial message in fluentd #38951#38952

Merged
thaJeztah merged 1 commit intomoby:masterfrom
alexei38:master
Apr 15, 2019
Merged

fluentd log driver. failed parse last partial message in fluentd #38951#38952
thaJeztah merged 1 commit intomoby:masterfrom
alexei38:master

Conversation

@alexei38
Copy link

@alexei38 alexei38 commented Mar 27, 2019

I use fluentd logging driver
In my containers the log it is more than 16 kb
Such the log are marked with partial_message flag

fluent-plugin-concat waited for the message which came without partial_message from stopped to stick together the message for further transfer

Signed-off-by: Alexei Margasov [email protected]

@thaJeztah
Copy link
Member

thaJeztah commented Mar 27, 2019

ping @anusha-ragunathan ptal

@anusha-ragunathan
Copy link
Contributor

anusha-ragunathan commented Mar 27, 2019 •
edited
Loading

FYI, you can also check for msg.PLogMetaData.Last to be true to start assembling the partial messages together.

The idea of this PR LGTM. The last message in a series of partial messages should not be flagged as Partial

anusha-ragunathan
anusha-ragunathan reviewed Mar 27, 2019
View reviewed changes
daemon/logger/fluentd/fluentd.go Outdated
Copy link
Contributor

@anusha-ragunathan anusha-ragunathan Mar 27, 2019

Choose a reason for hiding this comment

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

While we are here, lets also add the missing fields in the data map.

data["partial_ordinal"] = strconv.Itoa(msg.PLogMetaData.Ordinal)
data["partial_last"] = strconv.FormatBool(msg.PLogMetaData.Last)```

Copy link
Contributor

@anusha-ragunathan anusha-ragunathan Mar 27, 2019

Choose a reason for hiding this comment

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

This is already taken care of by https://github.com/moby/moby/pull/38065/files, but CI is broke on that. You can carry over this specific change from that PR.

Copy link
Author

@alexei38 alexei38 Mar 28, 2019

Choose a reason for hiding this comment

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

Yes, well, this approach is much better

@anusha-ragunathan
Copy link
Contributor

anusha-ragunathan commented Mar 27, 2019

experimental failed with unrelated test failure. Is this a flaky test @thaJeztah ?

03:57:51 --- FAIL: TestCheckpoint (1.40s)
03:57:51     checkpoint_test.go:40: Error (criu/util.c:816): exited, status=3
03:57:51         Warn  (criu/net.c:2840): Unable to get socket network namespace
03:57:51         Warn  (criu/net.c:2840): Unable to get tun network namespace
03:57:51         Warn  (criu/sk-unix.c:229): unix: Unable to open a socket file: Bad address
03:57:51         Warn  (criu/net.c:2840): Unable to get socket network namespace
03:57:51         Warn  (criu/kerndat.c:881): Can't keep kdat cache on non-tempfs
03:57:51         Looks good.
03:57:51     checkpoint_test.go:51: Start a container
03:57:51     checkpoint_test.go:69: ++ type -P true
03:57:51         ++ type -P ip6tables-restore
03:57:51         + mount --bind /bin/true /sbin/ip6tables-restore
03:57:51         ++ type -P true
03:57:51         ++ type -P ip6tables-save
03:57:51         + mount --bind /bin/true /sbin/ip6tables-save
03:57:51     checkpoint_test.go:81: Do a checkpoint and leave the container running
03:57:51     checkpoint_test.go:24: Exec: [touch /tmp/test-file]
03:57:51     checkpoint_test.go:28: 
03:57:51     checkpoint_test.go:115: Do a checkpoint and stop the container
03:57:51     checkpoint_test.go:117: assertion failed: error is not nil: Error response from daemon: Cannot checkpoint container 0ab3c1493bdd6aacbadd15ce20b338357cfdf98cfc3cfff325ef0fe065050491: cannot checkpoint a stopped container: unknown
03:57:51     checkpoint_test.go:77: ++ type -P ip6tables-restore
03:57:51         + umount -c -i -l /sbin/ip6tables-restore
03:57:51         ++ type -P ip6tables-save
03:57:51         + umount -c -i -l /sbin/ip6tables-save
03:57:51     main_test.go:32: assertion failed: error is not nil: Error response from daemon: Container 0ab3c1493bdd6aacbadd15ce20b338357cfdf98cfc3cfff325ef0fe065050491 is not paused: failed to unpause container 0ab3c1493bdd6aacbadd15ce20b338357cfdf98cfc3cfff325ef0fe065050491

@thaJeztah
Copy link
Member

thaJeztah commented Mar 27, 2019

doesn't look related no; retriggered ci

@codecov
Copy link

codecov bot commented Mar 28, 2019 •
edited
Loading

Codecov Report

Merging #38952 into master will increase coverage by 0.42%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #38952      +/-   ##
==========================================
+ Coverage   36.89%   37.32%   +0.42%     
==========================================
  Files         612      612              
  Lines       45332    45846     +514     
==========================================
+ Hits        16727    17113     +386     
- Misses      26319    26393      +74     
- Partials     2286     2340      +54

@thaJeztah thaJeztah mentioned this pull request Mar 28, 2019
Open
@alexei38
Copy link
Author

alexei38 commented Apr 10, 2019

My commit is not related to build issues.
How can I restart the build?

@alexei38
Copy link
Author

alexei38 commented Apr 14, 2019

ping @cpuguy83 @anusha-ragunathan ptal

thaJeztah
thaJeztah approved these changes Apr 15, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

ping @anusha-ragunathan ptal

anusha-ragunathan
anusha-ragunathan approved these changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@anusha-ragunathan anusha-ragunathan left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit c524e15 into moby:master Apr 15, 2019
@alexei38 alexei38 mentioned this pull request Apr 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@anusha-ragunathan anusha-ragunathan anusha-ragunathan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cpuguy83 cpuguy83

Labels

area/logging status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alexei38 @thaJeztah @anusha-ragunathan @cpuguy83 @GordonTheTurtle