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

testiso.go: Add go routine to handle badness #3895

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
118 changes: 72 additions & 46 deletions mantle/cmd/kola/testiso.go
Original file line number Diff line number Diff line change
Expand Up @@ -672,62 +672,88 @@
if err := os.WriteFile(path, []byte(errBuf), 0644); err != nil {
plog.Errorf("Failed to write journal: %v", err)
}
err = platform.ErrInitramfsEmergency

Check failure on line 675 in mantle/cmd/kola/testiso.go

View workflow job for this annotation

GitHub Actions / golangci-lint

ineffectual assignment to err (ineffassign)
}
}
}()
go func() {
err := inst.Wait()
// only one Wait() gets process data, so also manually check for signal
plog.Debugf("qemu exited err=%v", err)
if err == nil && inst.Signaled() {
err = errors.New("process killed")
}
if err != nil {
errchan <- err
errchan <- errors.Wrapf(err, "QEMU unexpectedly exited while awaiting completion")
}
time.Sleep(1 * time.Minute)
errchan <- fmt.Errorf("QEMU exited; timed out waiting for completion")
}()
}
c4rt0 marked this conversation as resolved.
Show resolved Hide resolved
go func() {
err := inst.Wait()
// only one Wait() gets process data, so also manually check for signal
plog.Debugf("qemu exited err=%v", err)
if err == nil && inst.Signaled() {
err = errors.New("process killed")
}
if err != nil {
errchan <- errors.Wrapf(err, "QEMU unexpectedly exited while awaiting completion")
}
time.Sleep(1 * time.Minute)
errchan <- fmt.Errorf("QEMU exited; timed out waiting for completion")
}()
go func() {
r := bufio.NewReader(qchan)
for _, exp := range expected {
l, err := r.ReadString('\n')
if err != nil {
if err == io.EOF {
// this may be from QEMU getting killed or exiting; wait a bit
// to give a chance for .Wait() above to feed the channel with a
// better error
time.Sleep(1 * time.Second)
errchan <- fmt.Errorf("Got EOF from completion channel, %s expected", exp)
} else {
errchan <- errors.Wrapf(err, "reading from completion channel")
go func() {
r := bufio.NewReader(qchan)
for _, exp := range expected {
l, err := r.ReadString('\n')
if err != nil {
if err == io.EOF {
// this may be from QEMU getting killed or exiting; wait a bit
// to give a chance for .Wait() above to feed the channel with a
// better error
time.Sleep(1 * time.Second)
errchan <- fmt.Errorf("Got EOF from completion channel, %s expected", exp)
} else {
errchan <- errors.Wrapf(err, "reading from completion channel")
}
return
}
line := strings.TrimSpace(l)
if line != exp {
errchan <- fmt.Errorf("Unexpected string from completion channel: %s expected: %s", line, exp)
return
}
return
plog.Debugf("Matched expected message %s", exp)
}
line := strings.TrimSpace(l)
if line != exp {
errchan <- fmt.Errorf("Unexpected string from completion channel: %s expected: %s", line, exp)
return
plog.Debugf("Matched all expected messages")
// OK!
errchan <- nil
}()
go func() {
//check for error when switching boot order
if booterrchan != nil {
if err := <-booterrchan; err != nil {
errchan <- err
}
}
plog.Debugf("Matched expected message %s", exp)
}
plog.Debugf("Matched all expected messages")
// OK!
errchan <- nil
}()
go func() {
//check for error when switching boot order
if booterrchan != nil {
if err := <-booterrchan; err != nil {
errchan <- err
}()
go func() {
err := <-errchan
Comment on lines +727 to +728
Copy link
Member

Choose a reason for hiding this comment

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

This really doesn't need to be a goroutine I don't think (see the patch I put in #3895 (comment), it's not a goroutine, just code at the end of the function).

Though maybe there is some reason why it needs to be a goroutine? Convince me :)

if err == nil {
// No error so far, check the console and journal files
files := []string{filepath.Join(outdir, "console.txt"), filepath.Join(outdir, "journal.txt")}
for _, file := range files {
// Read the contents of the file
fileContent, err := inst.ReadFile(file)
if err != nil {
fmt.Printf("error reading file %s: %v\n", file, err)
return
}
// Check for badness with CheckConsole
warnOnly, badlines := kola.CheckConsole([]byte(fileContent), nil)
if len(badlines) > 0 {
err = fmt.Errorf("errors found in console file: %v", badlines)
for _, badline := range badlines {
if err != nil {
fmt.Printf("badness detected in console file: %v\n", badline)
}
}
errchan <- err
} else if warnOnly {
fmt.Println("warnings found in console file")
continue
}
}
}
}
}()
}()
}
err := <-errchan
return time.Since(start), err
}
Expand Down
27 changes: 27 additions & 0 deletions mantle/kola/harness.go
Original file line number Diff line number Diff line change
Expand Up @@ -1917,6 +1917,27 @@ func ScpKolet(machines []platform.Machine) error {
return fmt.Errorf("Unable to locate kolet binary for %s", mArch)
}

// CheckConsoleText checks console output for badness
func CheckConsoleText(consoleOutput []byte) (bool, []string) {
var badlines []string
warnOnly := true
for _, check := range consoleChecks {
match := check.match.FindSubmatch(consoleOutput)
if match != nil {
badline := check.desc
if len(match) > 1 {
// include first subexpression
badline += fmt.Sprintf(" (%s)", match[1])
}
badlines = append(badlines, badline)
if !check.warnOnly {
warnOnly = false
}
}
}
return warnOnly, badlines
}

// CheckConsole checks some console output for badness and returns short
// descriptions of any bad lines it finds along with a boolean
// indicating if the configuration has the bad lines marked as
Expand All @@ -1926,6 +1947,12 @@ func ScpKolet(machines []platform.Machine) error {
func CheckConsole(output []byte, t *register.Test) (bool, []string) {
var badlines []string
warnOnly, allowRerunSuccess := true, true
// Check for badness using CheckConsoleText
addWarnOnly, addBadlines := CheckConsoleText(output)
if !addWarnOnly {
warnOnly = false
}
badlines = append(badlines, addBadlines...)
Comment on lines +1950 to +1955
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the value CheckConsoleText() is adding here?

for _, check := range consoleChecks {
if check.skipFlag != nil && t != nil && t.HasFlag(*check.skipFlag) {
continue
Expand Down
11 changes: 11 additions & 0 deletions mantle/platform/qemu.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,17 @@ func (inst *QemuInstance) WaitIgnitionError(ctx context.Context) (string, error)
return r.String(), nil
}

// Read file, output contents and return error if reading fails
func (inst *QemuInstance) ReadFile(filename string) (string, error) {
data, err := os.ReadFile(filename)
errchan := make(chan error)
if err != nil {
errchan <- err
return "", err
}
return string(data), nil
}
Copy link
Member

Choose a reason for hiding this comment

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

this function isn't needed.. just use os.Readfile() like is done here:

console, err = os.ReadFile(arg)


// WaitAll wraps the process exit as well as WaitIgnitionError,
// returning an error if either fail.
func (inst *QemuInstance) WaitAll(ctx context.Context) error {
Expand Down
Loading