coreos / coreos-assembler

Tooling container to assemble CoreOS-like systems
https://coreos.github.io/coreos-assembler/
Apache License 2.0
347 stars 168 forks source link

testiso.go: Add check for badness #3895

Closed c4rt0 closed 2 weeks ago

c4rt0 commented 2 months ago

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

(use checkConsole to verify console.txt and journal.txt for badness)

openshift-ci[bot] commented 2 months ago

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

dustymabe commented 1 month ago

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.

https://github.com/coreos/coreos-assembler/blob/7550a58bb623f9cf2c754dc130516ca394a4eec5/mantle/cmd/kola/console.go#L77-L81

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Maybe can try newer go version like 1.22, see https://github.com/coreos/repo-templates/pull/248 and https://github.com/coreos/repo-templates/pull/249 (as 1.21 is EOL)

c4rt0 commented 1 month ago

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())
                                         ^
dustymabe commented 3 weeks ago

In the interest of time I'll push up a commit that is working in local testing.

c4rt0 commented 3 weeks ago

Thank you @dustymabe for the last update.