kola/switch-kernel: add a test for switching between rt and default kernel#1218
kola/switch-kernel: add a test for switching between rt and default kernel#1218
Conversation
a3b8e49 to
8fa71b0
Compare
|
cc @arithx EDIT: *if it's reasonable |
cgwalters
left a comment
There was a problem hiding this comment.
Overall seems sane! That said, I am hoping to push #1215 over the line soon into supporting exactly this type of thing. Basically it needs:
- Support for injecting a directory of files too
And I didn't yet test handling reboots, but the idea is to do so.
mantle/cmd/kola/switchkernel.go
Outdated
|
|
||
| err := dropRpmFilesAll(m, rtKernelRpmDir) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed dropping Kernel RT RPM files: %v", err) |
There was a problem hiding this comment.
In new code let's use errors.Wrapf().
sample run & output |
mantle/cmd/kola/switchkernel.go
Outdated
| } | ||
| }`, base64.StdEncoding.EncodeToString([]byte(switchKernelScript)))) | ||
|
|
||
| flight, err := kola.NewFlight("qemu-unpriv") |
There was a problem hiding this comment.
There's nothing intrinsically specific to qemu about this, right? I think you can use kola.NewFlight(kolaPlatform) and drop the kolaPlatform check above.
|
Seems reasonable to me; I agree with @cgwalters' comments but after those are fixed should be fine. |
75c1a41 to
b5750ca
Compare
|
Switched to Rebased and squashed into one commit. |
mantle/cmd/kola/switchkernel.go
Outdated
|
|
||
| flight, err := kola.NewFlight(kolaPlatform) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "Flight failed: %v", err) |
There was a problem hiding this comment.
The nice thing about errors.Wrapf is you don't need to do the : %v", err); it does that automatically. And it also preserves the underlying error so it's not turned into a string.
There was a problem hiding this comment.
(Search for other places in the code that use it)
b5750ca to
9700e4a
Compare
|
Fixed |
mantle/cmd/kola/switchkernel.go
Outdated
| fmt.Println("Checking kernel type...") | ||
| cmd = "uname -v | grep -q 'PREEMPT RT'" | ||
| _, _, err = m.SSH(cmd) | ||
| if err == nil { |
There was a problem hiding this comment.
This should be err != nil right?
Hmm...I think this test is passing because you're missing a _ in PREEMPT_RT so both tests are inverted?
There was a problem hiding this comment.
Double checked inside a rhcos machine..
[core@ibm-p8-kvm-03-guest-02 ~]$ uname -v
#1 SMP PREEMPT RT Tue Jan 14 16:03:46 UTC 2020
There was a problem hiding this comment.
Ah OK. But then I think we should invert these checks, because if err == nil is a HUGE trap in Go, it just looks like a typo.
And specifically here the error.Wrapf() is wrong for this one because err is nil, so it will return nil, and the rest of the code will think no error occurred.
There was a problem hiding this comment.
The make check ran by openshift-ci-rebot didn't run switchkernel.go at all, it only ran kola --help for sanity check.
Yes, it should be if err != nil, sorry should've mentioned earlier.. and I confirmed on RHCOS the original test did not produce expected result (because error.Wrapf() was hiding the nil error)
mantle/cmd/kola/switchkernel.go
Outdated
|
|
||
| // check if the kernel has switched back to default kernel | ||
| fmt.Println("Checking kernel type...") | ||
| cmd = "! $(uname -v | grep -q 'PREEMPT RT')" |
There was a problem hiding this comment.
I think it's better to use grep -v then ! grep - it more clearly expresses intent (and ensures that you still do get an error if e.g. an input file doesn't exist or grep got OOM killed or whatever).
And see above too about the _.
There was a problem hiding this comment.
Updated with grep -v and tested on a RHCOS machine, worked without issue
mantle/cmd/kola/switchkernel.go
Outdated
| cmd := "sudo " + homeDir + "/switch-kernel.sh rt-kernel default" | ||
| stdout, stderr, err := m.SSH(cmd) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to run %", cmd) |
There was a problem hiding this comment.
cmd/kola/switchkernel.go:206:10: Wrapf format % is missing verb at end of string
There was a problem hiding this comment.
You're missing the s of the %s in the format string.
There was a problem hiding this comment.
Yep I wanted to make a note I missed that
Supports --rpm-dir option for uploading the rt kernel rpms into the cluster machine and adds a test that switches to target kernel with rpm-ostree, using logic defined in https://github.com/openshift/machine-config-operator/blob/f363c7be6d2d506d900e196fa2e2d05ca08b93b6/pkg/daemon/update.go#L651. Closes: https://issues.redhat.com/browse/GRPA-1439 Signed-off-by: Allen Bai <abai@redhat.com> kola: add a switch-kernel sub-command Instead of adding a direct test under kola/tests, adds a sub-command that uploads the specified kernel rt rpms and switch to RT Kernel and switch back with rpm-ostree. Closes: https://issues.redhat.com/browse/GRPA-1439 Signed-off-by: Allen Bai <abai@redhat.com> kola/switchkernel: use wrapf and drop hardcoded platform id Drops hardcoded id "qemu-unpriv" and sets to global kolaPlatform. Also switches to "errors.Wrapf" for better error context and stack trace info. Signed-off-by: Allen Bai <abai@redhat.com> kola/switchkernel: remove use of '%v, err' Signed-off-by: Allen Bai <abai@redhat.com>
9700e4a to
3a48a44
Compare
|
Updated and tested locally Output |
|
/lgtm |
|
Thank you @cgwalters, merging |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, zonggen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Currently this repo uses Prow, so the bot will merge. |
Adds a sub-command that uploads the RT Kernel RPMs to a machine and switch between kernels using
rpm-ostree. The logic used to switch between Default Kernel and RT Kernel is defined in https://github.com/openshift/machine-config-operator/blob/f363c7be6d2d506d900e196fa2e2d05ca08b93b6/pkg/daemon/update.go#L651.Closes: https://issues.redhat.com/browse/GRPA-1439
Signed-off-by: Allen Bai abai@redhat.com