Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Commit 71909a1

Browse files
Random-LiuRandom-Liu
authored
Merge pull request #1031 from Random-Liu/cherrypick-#1027-release-1.0
Cherrypick #1027 release 1.0
2 parents 562eefa + dd55db0 commit 71909a1

5 files changed

Lines changed: 151 additions & 20 deletions

File tree

‎integration/container_log_test.go‎

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,66 @@ import (
3030
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
3131
)
3232

33+
func TestContainerLogWithoutTailingNewLine(t *testing.T) {
34+
testPodLogDir, err := ioutil.TempDir("/tmp", "container-log-without-tailing-newline")
35+
require.NoError(t, err)
36+
defer os.RemoveAll(testPodLogDir)
37+
38+
t.Log("Create a sandbox with log directory")
39+
sbConfig := PodSandboxConfig("sandbox", "container-log-without-tailing-newline",
40+
WithPodLogDirectory(testPodLogDir),
41+
)
42+
sb, err := runtimeService.RunPodSandbox(sbConfig)
43+
require.NoError(t, err)
44+
defer func() {
45+
assert.NoError(t, runtimeService.StopPodSandbox(sb))
46+
assert.NoError(t, runtimeService.RemovePodSandbox(sb))
47+
}()
48+
49+
const (
50+
testImage = "busybox"
51+
containerName = "test-container"
52+
)
53+
t.Logf("Pull test image %q", testImage)
54+
img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil)
55+
require.NoError(t, err)
56+
defer func() {
57+
assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img}))
58+
}()
59+
60+
t.Log("Create a container with log path")
61+
cnConfig := ContainerConfig(
62+
containerName,
63+
testImage,
64+
WithCommand("sh", "-c", "printf abcd"),
65+
WithLogPath(containerName),
66+
)
67+
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
68+
require.NoError(t, err)
69+
70+
t.Log("Start the container")
71+
require.NoError(t, runtimeService.StartContainer(cn))
72+
73+
t.Log("Wait for container to finish running")
74+
require.NoError(t, Eventually(func() (bool, error) {
75+
s, err := runtimeService.ContainerStatus(cn)
76+
if err != nil {
77+
return false, err
78+
}
79+
if s.GetState() == runtime.ContainerState_CONTAINER_EXITED {
80+
return true, nil
81+
}
82+
return false, nil
83+
}, time.Second, 30*time.Second))
84+
85+
t.Log("Check container log")
86+
content, err := ioutil.ReadFile(filepath.Join(testPodLogDir, containerName))
87+
assert.NoError(t, err)
88+
checkContainerLog(t, string(content), []string{
89+
fmt.Sprintf("%s %s %s", runtime.Stdout, runtime.LogTagPartial, "abcd"),
90+
})
91+
}
92+
3393
func TestLongContainerLog(t *testing.T) {
3494
testPodLogDir, err := ioutil.TempDir("/tmp", "long-container-log")
3595
require.NoError(t, err)
@@ -66,9 +126,9 @@ func TestLongContainerLog(t *testing.T) {
66126
longLineCmd := fmt.Sprintf("i=0; while [ $i -lt %d ]; do printf %s; i=$((i+1)); done", maxSize+1, "c")
67127
cnConfig := ContainerConfig(
68128
containerName,
69-
"busybox",
129+
testImage,
70130
WithCommand("sh", "-c",
71-
fmt.Sprintf("%s; echo; %s; echo; %s", shortLineCmd, maxLenLineCmd, longLineCmd)),
131+
fmt.Sprintf("%s; echo; %s; echo; %s; echo", shortLineCmd, maxLenLineCmd, longLineCmd)),
72132
WithLogPath(containerName),
73133
)
74134
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)

‎pkg/server/events.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,11 @@ func convertEvent(e *gogotypes.Any) (string, interface{}, error) {
106106
return "", nil, errors.Wrap(err, "failed to unmarshalany")
107107
}
108108

109-
switch evt.(type) {
109+
switch e := evt.(type) {
110110
case *eventtypes.TaskExit:
111-
containerID = evt.(*eventtypes.TaskExit).ContainerID
111+
containerID = e.ContainerID
112112
case *eventtypes.TaskOOM:
113-
containerID = evt.(*eventtypes.TaskOOM).ContainerID
113+
containerID = e.ContainerID
114114
default:
115115
return "", nil, errors.New("unsupported event")
116116
}
@@ -185,13 +185,12 @@ func (em *eventMonitor) stop() {
185185
// handleEvent handles a containerd event.
186186
func (em *eventMonitor) handleEvent(any interface{}) error {
187187
ctx := ctrdutil.NamespacedContext()
188-
switch any.(type) {
188+
switch e := any.(type) {
189189
// If containerd-shim exits unexpectedly, there will be no corresponding event.
190190
// However, containerd could not retrieve container state in that case, so it's
191191
// fine to leave out that case for now.
192192
// TODO(random-liu): [P2] Handle containerd-shim exit.
193193
case *eventtypes.TaskExit:
194-
e := any.(*eventtypes.TaskExit)
195194
cntr, err := em.containerStore.Get(e.ContainerID)
196195
if err == nil {
197196
if err := handleContainerExit(ctx, e, cntr); err != nil {
@@ -213,7 +212,6 @@ func (em *eventMonitor) handleEvent(any interface{}) error {
213212
}
214213
return nil
215214
case *eventtypes.TaskOOM:
216-
e := any.(*eventtypes.TaskOOM)
217215
logrus.Infof("TaskOOM event %+v", e)
218216
cntr, err := em.containerStore.Get(e.ContainerID)
219217
if err != nil {

‎pkg/server/io/logger.go‎

Lines changed: 69 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package io
1919
import (
2020
"bufio"
2121
"bytes"
22+
"fmt"
2223
"io"
2324
"io/ioutil"
2425
"time"
@@ -61,6 +62,56 @@ func NewCRILogger(path string, w io.Writer, stream StreamType, maxLen int) (io.W
6162
return pwc, stop
6263
}
6364

65+
// bufio.ReadLine in golang eats both read errors and tailing newlines
66+
// (See https://golang.org/pkg/bufio/#Reader.ReadLine). When reading
67+
// to io.EOF, it is impossible for the caller to figure out whether
68+
// there is a newline at the end, for example:
69+
// 1) When reading "CONTENT\n", it returns "CONTENT" without error;
70+
// 2) When reading "CONTENT", it also returns "CONTENT" without error.
71+
//
72+
// To differentiate these 2 cases, we need to write a readLine function
73+
// ourselves to not ignore the error.
74+
//
75+
// The code is similar with https://golang.org/src/bufio/bufio.go?s=9537:9604#L359.
76+
// The only difference is that it returns all errors from `ReadSlice`.
77+
//
78+
// readLine returns err != nil if and only if line does not end with a new line.
79+
func readLine(b *bufio.Reader) (line []byte, isPrefix bool, err error) {
80+
line, err = b.ReadSlice('\n')
81+
if err == bufio.ErrBufferFull {
82+
// Handle the case where "\r\n" straddles the buffer.
83+
if len(line) > 0 && line[len(line)-1] == '\r' {
84+
// Unread the last '\r'
85+
if err := b.UnreadByte(); err != nil {
86+
panic(fmt.Sprintf("invalid unread %v", err))
87+
}
88+
line = line[:len(line)-1]
89+
}
90+
return line, true, nil
91+
}
92+
93+
if len(line) == 0 {
94+
if err != nil {
95+
line = nil
96+
}
97+
return
98+
}
99+
100+
if line[len(line)-1] == '\n' {
101+
// "ReadSlice returns err != nil if and only if line does not end in delim"
102+
// (See https://golang.org/pkg/bufio/#Reader.ReadSlice).
103+
if err != nil {
104+
panic(fmt.Sprintf("full read with unexpected error %v", err))
105+
}
106+
drop := 1
107+
if len(line) > 1 && line[len(line)-2] == '\r' {
108+
drop = 2
109+
}
110+
line = line[:len(line)-drop]
111+
}
112+
return
113+
}
114+
64115
func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxLen int) {
65116
defer rc.Close()
66117
var (
@@ -88,7 +139,16 @@ func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxL
88139
}
89140
for {
90141
var stop bool
91-
newLine, isPrefix, err := r.ReadLine()
142+
newLine, isPrefix, err := readLine(r)
143+
// NOTE(random-liu): readLine can return actual content even if there is an error.
144+
if len(newLine) > 0 {
145+
// Buffer returned by ReadLine will change after
146+
// next read, copy it.
147+
l := make([]byte, len(newLine))
148+
copy(l, newLine)
149+
buf = append(buf, l)
150+
length += len(l)
151+
}
92152
if err != nil {
93153
if err == io.EOF {
94154
logrus.Debugf("Getting EOF from stream %q while redirecting to log file %q", s, path)
@@ -101,13 +161,6 @@ func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxL
101161
}
102162
// Stop after writing the content left in buffer.
103163
stop = true
104-
} else {
105-
// Buffer returned by ReadLine will change after
106-
// next read, copy it.
107-
l := make([]byte, len(newLine))
108-
copy(l, newLine)
109-
buf = append(buf, l)
110-
length += len(l)
111164
}
112165
if maxLen > 0 && length > maxLen {
113166
exceedLen := length - maxLen
@@ -125,7 +178,14 @@ func redirectLogs(path string, rc io.ReadCloser, w io.Writer, s StreamType, maxL
125178
if isPrefix {
126179
continue
127180
}
128-
writeLine(full, bytes.Join(buf, nil))
181+
if stop {
182+
// readLine only returns error when the message doesn't
183+
// end with a newline, in that case it should be treated
184+
// as a partial line.
185+
writeLine(partial, bytes.Join(buf, nil))
186+
} else {
187+
writeLine(full, bytes.Join(buf, nil))
188+
}
129189
buf = nil
130190
length = 0
131191
if stop {

‎pkg/server/io/logger_test.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func TestRedirectLogs(t *testing.T) {
7272
maxLen: maxLen,
7373
tag: []runtime.LogTag{
7474
runtime.LogTagFull,
75-
runtime.LogTagFull,
75+
runtime.LogTagPartial,
7676
},
7777
content: []string{
7878
"test stderr log 1",
@@ -222,6 +222,19 @@ func TestRedirectLogs(t *testing.T) {
222222
strings.Repeat("a", defaultBufSize*10+20),
223223
},
224224
},
225+
"log length longer than buffer size with tailing \\r\\n": {
226+
input: strings.Repeat("a", defaultBufSize-1) + "\r\n" + strings.Repeat("a", defaultBufSize-1) + "\r\n",
227+
stream: Stdout,
228+
maxLen: -1,
229+
tag: []runtime.LogTag{
230+
runtime.LogTagFull,
231+
runtime.LogTagFull,
232+
},
233+
content: []string{
234+
strings.Repeat("a", defaultBufSize-1),
235+
strings.Repeat("a", defaultBufSize-1),
236+
},
237+
},
225238
} {
226239
t.Logf("TestCase %q", desc)
227240
rc := ioutil.NopCloser(strings.NewReader(test.input))

‎pkg/util/strings.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import "strings"
2222
// Comparison is case insensitive.
2323
func InStringSlice(ss []string, str string) bool {
2424
for _, s := range ss {
25-
if strings.ToLower(s) == strings.ToLower(str) {
25+
if strings.EqualFold(s, str) {
2626
return true
2727
}
2828
}
@@ -34,7 +34,7 @@ func InStringSlice(ss []string, str string) bool {
3434
func SubtractStringSlice(ss []string, str string) []string {
3535
var res []string
3636
for _, s := range ss {
37-
if strings.ToLower(s) == strings.ToLower(str) {
37+
if strings.EqualFold(s, str) {
3838
continue
3939
}
4040
res = append(res, s)

0 commit comments

Comments
 (0)