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

Conversation

c4rt0
Copy link
Member

@c4rt0 c4rt0 commented Oct 1, 2024

This PR aims to address the first step of a suggestion in the #3788 issue.

CheckConsoleForBadness reads output from a journal pipe and checks each line for "badness". It then calls CheckConsoleText to analyze lines of Console output.

Copy link

openshift-ci bot commented Oct 1, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@c4rt0 c4rt0 requested review from jbtrystram and removed request for jbtrystram October 1, 2024 19:41
// failed inside the CheckConsole. The resulting string will
// be a newline-delimited stream of JSON strings, as returned
// by `journalctl -o json`.
func (inst *QemuInstance) CheckConsoleForBadness(ctx context.Context) (string, error) {
Copy link
Member

@cverna cverna Oct 2, 2024

Choose a reason for hiding this comment

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

This looks like almost the same code as the WaitIgnitionError function. We don't want to duplicate code, we should probably have a function that deals with the journal processing that can be used in WaitIgnitionError and CheckConsoleForBadness.

Copy link
Member

Choose a reason for hiding this comment

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

Also not 💯 % sure about this, but do we want to check the content of the journal or the serial console? AFAIU these are different.

Copy link
Member Author

@c4rt0 c4rt0 Oct 2, 2024

Choose a reason for hiding this comment

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

That's correct. For now I reverted changes to go back to the initial stages of the first PR related to the same issue. Once work in this PR compiles and works as expected I will request another review if you don't mind :)

@dustymabe
Copy link
Member

ok I think the strategy here should probably be to just call kola.CheckConsole directly from within awaitCompletion in testiso.go AFTER the errchan has been populated. i.e. not as a separate parallel running gofunc, but just at the end after the error channel has been filled.

There is precedent for calling CheckConsole() like this without a t *register.Test in kola check-console which I admit I didn't even know existed.

_, badlines := kola.CheckConsole(console, nil)
for _, badline := range badlines {
fmt.Printf("%v: %v\n", sourceName, badline)
errorcount++
}

So we should be able to do something like:

diff --git a/mantle/cmd/kola/testiso.go b/mantle/cmd/kola/testiso.go
index a925d3d25..e2cd761c1 100644
--- a/mantle/cmd/kola/testiso.go
+++ b/mantle/cmd/kola/testiso.go
@@ -729,7 +729,18 @@ func awaitCompletion(ctx context.Context, inst *platform.QemuInstance, outdir st
                }
        }()
        err := <-errchan
-       return time.Since(start), err
+       // The test is done so let's record the amount of elapsed time
+       elapsed = time.Since(start)
+       if err == nil; {
+               // No error so far; let's check the console/journal
+               for each of inst.builder.consoleFile and filepath.Join(outdir, "journal.txt") we need to {
+
+                       warnOnly, badlines := kola.CheckConsole(readfile, nil)
+                       for range badlines
+                               do something and set err to !nil if there was badness
+               }
+       }
+       return elapsed, err
 }

 func printResult(test string, duration time.Duration, err error) bool {

@c4rt0 c4rt0 force-pushed the PR/badness_1 branch 2 times, most recently from 85148a6 to dd184f1 Compare October 15, 2024 12:41
After the tests are complete this go routine verifies contents
of console.txt and journal.txt through the use of CheckConsole.

Ref: coreos#3788
@c4rt0 c4rt0 changed the title qemu.go, harness.go: Add CheckConsoleForBadness & CheckConsoleText testiso.go: Add go routine to handle badness Oct 15, 2024
@c4rt0
Copy link
Member Author

c4rt0 commented Oct 15, 2024

I ran the code through the gofmt, yet it still fails the linter check. Can I get any pointers on this, please?
The error description does not seem to be precise enough for me to decypher it:

  Error: ineffectual assignment to err (ineffassign)

🤔

@jbtrystram
Copy link
Contributor

I ran the code through the gofmt, yet it still fails the linter check. Can I get any pointers on this, please? The error description does not seem to be precise enough for me to decypher it:

  Error: ineffectual assignment to err (ineffassign)

🤔

the CI runs gofmt and golangci-lint, which are two different things : https://golangci-lint.run/

@c4rt0
Copy link
Member Author

c4rt0 commented Oct 15, 2024

I installed the version of golangci-lint as per the pipeline. The output seems to have nothing to do with code I modified. Here's the result of golangci-lint:

adamsky@fedorapc Work/coreos-assembler (PR/badness_1) » golangci-lint --version                                               
golangci-lint has version 1.55.1 built with go1.21.3 from 9b20d49d on 2023-10-25T10:03:43Z

adamsky@fedorapc Work/coreos-assembler (PR/badness_1) » golangci-lint run mantle/cmd/kola/testiso.go

mantle/cmd/kola/testiso.go:50:12: undefined: preRun (typecheck)
                PreRunE: preRun,
                         ^
mantle/cmd/kola/testiso.go:501:3: undefined: plog (typecheck)
                plog.Fatal(err)
                ^
mantle/cmd/kola/testiso.go:519:39: undefined: outputDir (typecheck)
        outputDir, err = kola.SetupOutputDir(outputDir, "testiso")
                                             ^
mantle/cmd/kola/testiso.go:525:29: undefined: outputDir (typecheck)
        reportDir := filepath.Join(outputDir, "reports")
                                   ^
mantle/cmd/kola/testiso.go:608:53: undefined: outputDir (typecheck)
                        duration, err = testPXE(ctx, inst, filepath.Join(outputDir, test))
                                                                         ^
mantle/cmd/kola/testiso.go:629:5: undefined: plog (typecheck)
                                plog.Fatalf("Unknown test name:%s", test)
                                ^
mantle/cmd/kola/testiso.go:633:4: undefined: plog (typecheck)
                        plog.Fatalf("Unknown test name:%s", test)
                        ^
mantle/cmd/kola/testiso.go:347:2: undefined: root (typecheck)
        root.AddCommand(cmdTestIso)

All golangci-lint errors I get for the harness.go, testiso.go and qemu.go seem to be unrelated to my modifications.

Is this common, or am I missing something?

@HuijingHei
Copy link
Member

HuijingHei commented Oct 16, 2024

Maybe can try newer go version like 1.22, see coreos/repo-templates#248 and coreos/repo-templates#249 (as 1.21 is EOL)

@c4rt0
Copy link
Member Author

c4rt0 commented Oct 17, 2024

Maybe can try newer go version like 1.22, see coreos/repo-templates#248 and coreos/repo-templates#249 (as 1.21 is EOL)

I tried it and it didn't help:

adamsky@fedorapc Work/coreos-assembler (PR/badness_1) » golangci-lint --version                                                                                         1 ↵
golangci-lint has version 1.61.0 built with go1.23.1 from a1d6c560 on 2024-09-09T17:44:42Z
adamsky@fedorapc Work/coreos-assembler (PR/badness_1) » golangci-lint run mantle/cmd/kola/testiso.go                                                                    1 ↵
mantle/cmd/kola/testiso.go:50:12: undefined: preRun (typecheck)
                PreRunE: preRun,
                         ^
mantle/cmd/kola/testiso.go:501:3: undefined: plog (typecheck)
                plog.Fatal(err)
                ^
mantle/cmd/kola/testiso.go:519:39: undefined: outputDir (typecheck)
        outputDir, err = kola.SetupOutputDir(outputDir, "testiso")
                                             ^
mantle/cmd/kola/testiso.go:525:29: undefined: outputDir (typecheck)
        reportDir := filepath.Join(outputDir, "reports")
                                   ^
mantle/cmd/kola/testiso.go:608:53: undefined: outputDir (typecheck)
                        duration, err = testPXE(ctx, inst, filepath.Join(outputDir, test))
                                                                         ^
mantle/cmd/kola/testiso.go:629:5: undefined: plog (typecheck)
                                plog.Fatalf("Unknown test name:%s", test)
                                ^
mantle/cmd/kola/testiso.go:633:4: undefined: plog (typecheck)
                        plog.Fatalf("Unknown test name:%s", test)
                        ^
mantle/cmd/kola/testiso.go:347:2: undefined: root (typecheck)
        root.AddCommand(cmdTestIso)
        ^
adamsky@fedorapc Work/coreos-assembler (PR/badness_1) » golangci-lint run mantle/kola/harness.go    
mantle/kola/harness.go:1265:14: printf: non-constant format string in call to (*github.com/coreos/coreos-assembler/mantle/harness.H).Fatalf (govet)
                                c.Fatalf(errors.Wrapf(err, "kolet failed: %s", stderr).Error())
                                         ^

mantle/cmd/kola/testiso.go Show resolved Hide resolved
Comment on lines +727 to +728
go func() {
err := <-errchan
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 :)

Comment on lines +1950 to +1955
// Check for badness using CheckConsoleText
addWarnOnly, addBadlines := CheckConsoleText(output)
if !addWarnOnly {
warnOnly = false
}
badlines = append(badlines, addBadlines...)
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?

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants