Skip to content
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.

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

accept Ctrl-C in cri log viewer #1972

Merged
merged 1 commit into from
Feb 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 49 additions & 44 deletions pkg/logging/cri_logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ package logging
import (
"bufio"
"bytes"
"context"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -74,13 +73,13 @@ func viewLogsCRI(lvopts LogViewOptions, stdout, stderr io.Writer, stopChannel ch
return fmt.Errorf("logpath is nil ")
}

return ReadLogs(context.Background(), &lvopts, stdout, stderr)
return ReadLogs(&lvopts, stdout, stderr, stopChannel)
}

// ReadLogs read the container log and redirect into stdout and stderr.
// Note that containerID is only needed when following the log, or else
// just pass in empty string "".
func ReadLogs(ctx context.Context, opts *LogViewOptions, stdout, stderr io.Writer) error {
func ReadLogs(opts *LogViewOptions, stdout, stderr io.Writer, stopChannel chan os.Signal) error {
var logPath = opts.LogPath
evaluated, err := filepath.EvalSymlinks(logPath)
if err != nil {
Expand Down Expand Up @@ -113,56 +112,62 @@ func ReadLogs(ctx context.Context, opts *LogViewOptions, stdout, stderr io.Write
writer := newLogWriter(stdout, stderr, opts)
msg := &logMessage{}
for {
if stop || (limitedMode && limitedNum == 0) {
logrus.Debugf("Finished parsing log file, path; %s", logPath)
select {
case <-stopChannel:
logrus.Debugf("received stop signal while reading cri logfile, returning")
return nil
}
l, err := r.ReadBytes(eol[0])
if err != nil {
if err != io.EOF { // This is an real error
return fmt.Errorf("failed to read log file %q: %v", logPath, err)
default:
if stop || (limitedMode && limitedNum == 0) {
logrus.Debugf("finished parsing log file, path: %s", logPath)
return nil
Copy link
Member

Choose a reason for hiding this comment

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

nit: Finished -> finished to be consistent; and typo ; -> :.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to common custom,maybe it's a good idea to let log print line ( such as logrus.Debugf(“Example”) logrus.Warnf(“Example”)) start with uppercase, and error string returned (such asfmt.Errorf("example")) start with lowercase

And It seems that there is no uniformity about uppercase and lowercase
uppercase : https://github.com/containerd/nerdctl/blob/main/pkg/composer/create.go#L169

lowercase: https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/container_cp_linux.go#L259

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference only: https://www.postgres-xl.org/documentation/error-style-guide.html

I think it is also necessary to unify it in our project.

Copy link
Contributor Author

@zhaojizhuang zhaojizhuang Feb 6, 2023

Choose a reason for hiding this comment

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

+1 Agree, maybe we should open a issue to track the uniformity in our registry,cc @djdongjin

Copy link
Member

@djdongjin djdongjin Feb 6, 2023

Choose a reason for hiding this comment

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

This SGTM, but personally I feel this uniformity is more a best-effort but difficult to achieve (or maintain after achieved), part because it's quite detailed and there is a lot of error/log messages, and also because there are new contributors (and reviewers) who're not aware of these.

Speaking of this PR, I'd suggest to lowercase all error/log messages :) since it has both uppercase (line 122, 146, 152, ...) and lowercase (118, 128, 135, ...). Thanks :)

Copy link
Contributor

@suyanhanx suyanhanx Feb 6, 2023

Choose a reason for hiding this comment

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

This SGTM, but personally I feel this uniformity is more a best-effort but difficult to achieve (or maintain after achieved), part because it's quite detailed and there is a lot of error/log messages, and also because there are new contributors (and reviewers) who're not aware of these.

You're right. However, we can add some related guides in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me lowercase error/log message in this PR

}
if opts.Follow {
l, err := r.ReadBytes(eol[0])
if err != nil {
if err != io.EOF { // This is an real error
return fmt.Errorf("failed to read log file %q: %v", logPath, err)
}
if opts.Follow {

// Reset seek so that if this is an incomplete line,
// it will be read again.
if _, err := f.Seek(-int64(len(l)), io.SeekCurrent); err != nil {
return fmt.Errorf("failed to reset seek in log file %q: %v", logPath, err)
}

// Reset seek so that if this is an incomplete line,
// it will be read again.
if _, err := f.Seek(-int64(len(l)), io.SeekCurrent); err != nil {
return fmt.Errorf("failed to reset seek in log file %q: %v", logPath, err)
// If the container exited consume data until the next EOF
continue
}
// Should stop after writing the remaining content.
stop = true
if len(l) == 0 {
continue
}
logrus.Debugf("incomplete line in log file, path: %s, line: %s", logPath, l)
}

// If the container exited consume data until the next EOF
// Parse the log line.
msg.reset()
if err := ParseCRILog(l, msg); err != nil {
logrus.WithError(err).Errorf("failed when parsing line in log file, path: %s, line: %s", logPath, l)
continue
}
// Should stop after writing the remaining content.
stop = true
if len(l) == 0 {
continue
// Write the log line into the stream.
if err := writer.write(msg, isNewLine); err != nil {
if err == errMaximumWrite {
logrus.Debugf("finished parsing log file, hit bytes limit path: %s", logPath)
return nil
}
logrus.WithError(err).Errorf("failed when writing line to log file, path: %s, line: %s", logPath, l)
return err
}
logrus.Debugf("Incomplete line in log file, path: %s line: %s", logPath, l)
}

// Parse the log line.
msg.reset()
if err := ParseCRILog(l, msg); err != nil {
logrus.WithError(err).Errorf("Failed when parsing line in log file, path: %s line: %s", logPath, l)
continue
}
// Write the log line into the stream.
if err := writer.write(msg, isNewLine); err != nil {
if err == errMaximumWrite {
logrus.Debugf("Finished parsing log file, hit bytes limit path: %s", logPath)
return nil
if limitedMode {
limitedNum--
}
if len(msg.log) > 0 {
isNewLine = msg.log[len(msg.log)-1] == eol[0]
} else {
isNewLine = true
}
logrus.WithError(err).Errorf("Failed when writing line to log file, path: %s line: %s", logPath, l)
return err
}
if limitedMode {
limitedNum--
}
if len(msg.log) > 0 {
isNewLine = msg.log[len(msg.log)-1] == eol[0]
} else {
isNewLine = true
}
}
}
Expand Down
9 changes: 6 additions & 3 deletions pkg/logging/cri_logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ package logging
import (
"bufio"
"bytes"
"context"
"fmt"
"io"
"os"
Expand All @@ -43,6 +42,8 @@ func TestReadLogs(t *testing.T) {
file.WriteString(`2016-10-06T00:17:09.669794202Z stdout F line1` + "\n")
file.WriteString(`2016-10-06T00:17:10.669794202Z stdout F line2` + "\n")
file.WriteString(`2016-10-06T00:17:11.669794202Z stdout F line3` + "\n")

stopChan := make(chan os.Signal)
testCases := []struct {
name string
logViewOptions LogViewOptions
Expand Down Expand Up @@ -85,7 +86,7 @@ func TestReadLogs(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
stdoutBuf := bytes.NewBuffer(nil)
stderrBuf := bytes.NewBuffer(nil)
err = ReadLogs(context.TODO(), &tc.logViewOptions, stdoutBuf, stderrBuf)
err = ReadLogs(&tc.logViewOptions, stdoutBuf, stderrBuf, stopChan)

if err != nil {
t.Fatalf(err.Error())
Expand Down Expand Up @@ -177,6 +178,8 @@ func TestReadLogsLimitsWithTimestamps(t *testing.T) {
t.Fatalf("unable to create temp file")
}

stopChan := make(chan os.Signal)

count := 10000

for i := 0; i < count; i++ {
Expand All @@ -198,7 +201,7 @@ func TestReadLogsLimitsWithTimestamps(t *testing.T) {
var buf bytes.Buffer
w := io.MultiWriter(&buf)

err = ReadLogs(context.Background(), &LogViewOptions{LogPath: tmpfile.Name(), Tail: 0, Timestamps: true}, w, w)
err = ReadLogs(&LogViewOptions{LogPath: tmpfile.Name(), Tail: 0, Timestamps: true}, w, w, stopChan)
if err != nil {
t.Errorf("ReadLogs file %s failed %s", tmpfile.Name(), err.Error())
}
Expand Down