| Message ID | 20211221092130.444225-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [kvm-unit-tests] scripts/arch-run: Mark migration tests as SKIP if ncat is not available | expand |
On 12/21/21 10:21, Thomas Huth wrote: > Instead of failing the tests, we should rather skip them if ncat is > not available. > While we're at it, also mention ncat in the README.md file as a > requirement for the migration tests. > > Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 > Signed-off-by: Thomas Huth <thuth@redhat.com> I would rather remove the migration tests. There's really no reason for them, the KVM selftests in the Linux tree are much better: they can find migration bugs deterministically and they are really really easy to debug. The only disadvantage is that they are harder to write. Paolo > --- > README.md | 4 ++++ > scripts/arch-run.bash | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/README.md b/README.md > index 6e6a9d0..a82da56 100644 > --- a/README.md > +++ b/README.md > @@ -54,6 +54,10 @@ ACCEL=name environment variable: > > ACCEL=kvm ./x86-run ./x86/msr.flat > > +For running tests that involve migration from one QEMU instance to another > +you also need to have the "ncat" binary (from the nmap.org project) installed, > +otherwise the related tests will be skipped. > + > # Tests configuration file > > The test case may need specific runtime configurations, for > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 43da998..cd92ed9 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -108,7 +108,7 @@ run_migration () > { > if ! command -v ncat >/dev/null 2>&1; then > echo "${FUNCNAME[0]} needs ncat (netcat)" >&2 > - return 2 > + return 77 > fi > > migsock=$(mktemp -u -t mig-helper-socket.XXXXXXXXXX) >
On 21/12/2021 10.58, Paolo Bonzini wrote: > On 12/21/21 10:21, Thomas Huth wrote: >> Instead of failing the tests, we should rather skip them if ncat is >> not available. >> While we're at it, also mention ncat in the README.md file as a >> requirement for the migration tests. >> >> Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > I would rather remove the migration tests. There's really no reason for > them, the KVM selftests in the Linux tree are much better: they can find > migration bugs deterministically and they are really really easy to debug. > The only disadvantage is that they are harder to write. I disagree: We're testing migration with QEMU here - the KVM selftests don't include QEMU, so we'd lose some test coverage here. And at least the powerpc/sprs.c test helped to find some nasty bugs in the past already. Thomas
Hi, On 12/21/21 11:12 AM, Thomas Huth wrote: > On 21/12/2021 10.58, Paolo Bonzini wrote: >> On 12/21/21 10:21, Thomas Huth wrote: >>> Instead of failing the tests, we should rather skip them if ncat is >>> not available. >>> While we're at it, also mention ncat in the README.md file as a >>> requirement for the migration tests. >>> >>> Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >> >> I would rather remove the migration tests. There's really no reason >> for them, the KVM selftests in the Linux tree are much better: they >> can find migration bugs deterministically and they are really really >> easy to debug. The only disadvantage is that they are harder to write. > > I disagree: We're testing migration with QEMU here - the KVM selftests > don't include QEMU, so we'd lose some test coverage here. > And at least the powerpc/sprs.c test helped to find some nasty bugs in > the past already. I do agree. The ITS migration tests were good reproducer for upstream bugs. Thanks Eric > > Thomas >
On 12/21/21 11:12, Thomas Huth wrote: > On 21/12/2021 10.58, Paolo Bonzini wrote: >> On 12/21/21 10:21, Thomas Huth wrote: >>> Instead of failing the tests, we should rather skip them if ncat is >>> not available. >>> While we're at it, also mention ncat in the README.md file as a >>> requirement for the migration tests. >>> >>> Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >> >> I would rather remove the migration tests. There's really no reason >> for them, the KVM selftests in the Linux tree are much better: they >> can find migration bugs deterministically and they are really really >> easy to debug. The only disadvantage is that they are harder to write. > > I disagree: We're testing migration with QEMU here - the KVM selftests > don't include QEMU, so we'd lose some test coverage here. > And at least the powerpc/sprs.c test helped to find some nasty bugs in > the past already. I understand that this is testing QEMU, but I'm not sure that testing QEMU should be part of kvm-unit-tests. Migrating an instance of QEMU that runs kvm-unit-tests would be done more easily in avocado-vt or avocado-qemu. Paolo
On Tue, Dec 21, 2021 at 06:25:30PM +0100, Paolo Bonzini wrote: > On 12/21/21 11:12, Thomas Huth wrote: > > On 21/12/2021 10.58, Paolo Bonzini wrote: > > > On 12/21/21 10:21, Thomas Huth wrote: > > > > Instead of failing the tests, we should rather skip them if ncat is > > > > not available. > > > > While we're at it, also mention ncat in the README.md file as a > > > > requirement for the migration tests. > > > > > > > > Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > > > I would rather remove the migration tests. There's really no reason > > > for them, the KVM selftests in the Linux tree are much better: they > > > can find migration bugs deterministically and they are really really > > > easy to debug. The only disadvantage is that they are harder to > > > write. > > > > I disagree: We're testing migration with QEMU here - the KVM selftests > > don't include QEMU, so we'd lose some test coverage here. > > And at least the powerpc/sprs.c test helped to find some nasty bugs in > > the past already. > > I understand that this is testing QEMU, but I'm not sure that testing QEMU > should be part of kvm-unit-tests. Migrating an instance of QEMU that runs > kvm-unit-tests would be done more easily in avocado-vt or avocado-qemu. > Migrating is easier with avocado*, but if we want to migrate kut unit tests, and the unit tests want to ensure the guest is in a specific state at the time of the migration, then we'll still need the getchar() stuff. And, if we need the getchar() stuff, then I think we also need a lightweight way to test migration, which is currently the ncat-based run_migration bash function. IOW, I vote we keep the code we have, but I'm also in favor of people building new test harnesses for the kut *.flat files which can better exercise QEMU or whatever. Thanks, drew
On Tue, Dec 21, 2021 at 10:21:30AM +0100, Thomas Huth wrote: > Instead of failing the tests, we should rather skip them if ncat is > not available. > While we're at it, also mention ncat in the README.md file as a > requirement for the migration tests. > > Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > README.md | 4 ++++ > scripts/arch-run.bash | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/README.md b/README.md > index 6e6a9d0..a82da56 100644 > --- a/README.md > +++ b/README.md > @@ -54,6 +54,10 @@ ACCEL=name environment variable: > > ACCEL=kvm ./x86-run ./x86/msr.flat > > +For running tests that involve migration from one QEMU instance to another > +you also need to have the "ncat" binary (from the nmap.org project) installed, > +otherwise the related tests will be skipped. > + > # Tests configuration file > > The test case may need specific runtime configurations, for > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 43da998..cd92ed9 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -108,7 +108,7 @@ run_migration () > { > if ! command -v ncat >/dev/null 2>&1; then > echo "${FUNCNAME[0]} needs ncat (netcat)" >&2 > - return 2 > + return 77 > fi > > migsock=$(mktemp -u -t mig-helper-socket.XXXXXXXXXX) > -- > 2.27.0 > Reviewed-by: Andrew Jones <drjones@redhat.com>
On 21/12/2021 18.25, Paolo Bonzini wrote: > On 12/21/21 11:12, Thomas Huth wrote: >> On 21/12/2021 10.58, Paolo Bonzini wrote: >>> On 12/21/21 10:21, Thomas Huth wrote: >>>> Instead of failing the tests, we should rather skip them if ncat is >>>> not available. >>>> While we're at it, also mention ncat in the README.md file as a >>>> requirement for the migration tests. >>>> >>>> Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> >>> I would rather remove the migration tests. There's really no reason for >>> them, the KVM selftests in the Linux tree are much better: they can find >>> migration bugs deterministically and they are really really easy to >>> debug. The only disadvantage is that they are harder to write. >> >> I disagree: We're testing migration with QEMU here - the KVM selftests >> don't include QEMU, so we'd lose some test coverage here. >> And at least the powerpc/sprs.c test helped to find some nasty bugs in the >> past already. > > I understand that this is testing QEMU, but I'm not sure that testing QEMU > should be part of kvm-unit-tests. It's the combination of QEMU (migration handling) + KVM in the kernel (register saving and restoring) that we are testing here. If you say that QEMU should not be involved at all, we could also say that all kvm-unit-tests should be converted to KVM selftests instead... > Migrating an instance of QEMU that runs > kvm-unit-tests would be done more easily in avocado-vt or avocado-qemu. But we don't have the environment for compiling small, Linux-independent test kernels there, do we? So unless there is a way to write and compile small test kernels in that framework, I think kvm-unit-tests is the better fit for these kind of tests. Thomas
On 12/21/21 10:21, Thomas Huth wrote: > Instead of failing the tests, we should rather skip them if ncat is > not available. > While we're at it, also mention ncat in the README.md file as a > requirement for the migration tests. > > Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > README.md | 4 ++++ > scripts/arch-run.bash | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/README.md b/README.md > index 6e6a9d0..a82da56 100644 > --- a/README.md > +++ b/README.md > @@ -54,6 +54,10 @@ ACCEL=name environment variable: > > ACCEL=kvm ./x86-run ./x86/msr.flat > > +For running tests that involve migration from one QEMU instance to another > +you also need to have the "ncat" binary (from the nmap.org project) installed, > +otherwise the related tests will be skipped. > + > # Tests configuration file > > The test case may need specific runtime configurations, for > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 43da998..cd92ed9 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -108,7 +108,7 @@ run_migration () > { > if ! command -v ncat >/dev/null 2>&1; then > echo "${FUNCNAME[0]} needs ncat (netcat)" >&2 > - return 2 > + return 77 > fi > > migsock=$(mktemp -u -t mig-helper-socket.XXXXXXXXXX) Queued, thanks. Paolo
diff --git a/README.md b/README.md index 6e6a9d0..a82da56 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,10 @@ ACCEL=name environment variable: ACCEL=kvm ./x86-run ./x86/msr.flat +For running tests that involve migration from one QEMU instance to another +you also need to have the "ncat" binary (from the nmap.org project) installed, +otherwise the related tests will be skipped. + # Tests configuration file The test case may need specific runtime configurations, for diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 43da998..cd92ed9 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -108,7 +108,7 @@ run_migration () { if ! command -v ncat >/dev/null 2>&1; then echo "${FUNCNAME[0]} needs ncat (netcat)" >&2 - return 2 + return 77 fi migsock=$(mktemp -u -t mig-helper-socket.XXXXXXXXXX)
Instead of failing the tests, we should rather skip them if ncat is not available. While we're at it, also mention ncat in the README.md file as a requirement for the migration tests. Resolves: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/issues/4 Signed-off-by: Thomas Huth <thuth@redhat.com> --- README.md | 4 ++++ scripts/arch-run.bash | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)