Skip to content

Conversation

@fuweid
Copy link
Contributor

@fuweid fuweid commented Oct 8, 2018

Signed-off-by: Wei Fu [email protected]

Ⅰ. Describe what this PR did

source in jsonfile log should be marshaled to stream

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

no need

Ⅳ. Describe how to verify it

The stream will replace the source in the json.

# vagrant @ ubuntu-xenial in ~/go/src/github.com/alibaba/pouch on git:bugfix_jsonfile_marshal_field o [16:35:48]
$ sudo pouch run -it registry.hub.docker.com/library/busybox:latest echo hello
hello

# vagrant @ ubuntu-xenial in ~/go/src/github.com/alibaba/pouch on git:bugfix_jsonfile_marshal_field o [16:36:33]
$ sudo cat /var/lib/pouch/containers/9a420911f1371f9abce896fa3d21b1c4f879e151b1d96f01a64a35232579804c/json.log
{"stream":"stdout","log":"hello\n","time":"2018-10-08T13:15:29.603009864Z"}

Ⅴ. Special notes for reviews

For the existing container, the jsonfile will contain two types json formats, like:

# vagrant @ ubuntu-xenial in ~/go/src/github.com/alibaba/pouch on git:master x [16:46:56]
$ sudo cat /var/lib/pouch/containers/9a420911f1371f9abce896fa3d21b1c4f879e151b1d96f01a64a35232579804c/json.log
{"stream":"stdout","log":"hello\n","time":"2018-10-08T13:15:29.603009864Z"}
{"source":"stdout","line":"hello\n","timestamp":"2018-10-08T08:46:06.605697918Z"}

it will impact the pouch log command and pouch log api to fetch the logs. The log API will not parse the existing data so that it will show empty to the user.

@codecov
Copy link

codecov bot commented Oct 8, 2018

Codecov Report

Merging #2297 into master will increase coverage by 43.37%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2297       +/-   ##
===========================================
+ Coverage   23.47%   66.84%   +43.37%     
===========================================
  Files         180      211       +31     
  Lines       15860    17195     +1335     
===========================================
+ Hits         3723    11494     +7771     
+ Misses      11860     4310     -7550     
- Partials      277     1391     +1114
Flag Coverage Δ
#criv1alpha1test 32.01% <60%> (?)
#criv1alpha2test 35.67% <60%> (?)
#integrationtest 39.74% <100%> (?)
#nodee2etest 33.12% <60%> (?)
#unittest 23.47% <100%> (ø) ⬆️
Impacted Files Coverage Δ
daemon/logger/jsonfile/encode.go 80.45% <100%> (+25.28%) ⬆️
cri/stream/httpstream/spdy/upgrade.go 54.28% <0%> (ø)
cri/stream/remotecommand/httpstream.go 46.63% <0%> (ø)
cri/criservice.go 55.95% <0%> (ø)
lxcfs/lxcfs.go 55.55% <0%> (ø)
cri/v1alpha2/service/cri.go 85.71% <0%> (ø)
cri/stream/remotecommand/websocket.go 81.53% <0%> (ø)
cri/stream/remotecommand/attach.go 66.66% <0%> (ø)
cri/stream/portforward/portforward.go 50% <0%> (ø)
pkg/jsonstream/image_progress.go 0% <0%> (ø)
... and 151 more

@pouchrobot pouchrobot added areas/log kind/bug This is bug report for project size/XS size/S and removed size/XS labels Oct 8, 2018
@allencloud
Copy link
Collaborator

I found that in Moby's API, the log is like the following

{"log":"alecthomas/gometalinter info found version: 2.0.11 for v2.0.11/linux/amd64\r\n","stream":"stdout","time":"2018-09-29T09:44:06.296939861Z"}

The struct has a field of log, stream and time.

While in your commit, there are line, timestamp and stream.

Incompatibility happens. @fuweid

@Ace-Tang
Copy link
Contributor

Ace-Tang commented Oct 8, 2018

Checkd in docker 1.12.6,

// JSONLog represents a log message, typically a single entry from a given log stream.
// JSONLogs can be easily serialized to and from JSON and support custom formatting.
type JSONLog struct {
    // Log is the log message
    Log string `json:"log,omitempty"`
    // Stream is the log source
    Stream string `json:"stream,omitempty"`
    // Created is the created timestamp of log
    Created time.Time `json:"time"`
    // Attrs is the list of extra attributes provided by the user
    Attrs map[string]string `json:"attrs,omitempty"`
}

source in jsonfile log should be marshaled to stream.

Signed-off-by: Wei Fu <[email protected]>
@Ace-Tang Ace-Tang merged commit 83ef7e3 into AliyunContainerService:master Oct 8, 2018
CodeJuan pushed a commit to CodeJuan/pouch that referenced this pull request Oct 15, 2018
bugfix: jsonfile log should be compatible with moby

Signed-off-by: Wei Fu <[email protected]>

more detail is here: AliyunContainerService#2297

See merge request !124907
@fuweid fuweid deleted the bugfix_jsonfile_marshal_field branch February 21, 2019 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

areas/log kind/bug This is bug report for project size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants