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

rpm: Update virtualization packages #7277

Merged
merged 10 commits into from
Apr 5, 2022

Conversation

andreabolognani
Copy link
Contributor

What this PR does / why we need it:

Updates virtualization packages. Specifically:

  • libvirt 7.6.0-6 → 8.0.0-2
  • QEMU 6.0.0-33 → 6.2.0-5

The libvirt.org/go/libvirt Go package is also updated to match.

Release note:

This version of KubeVirt includes upgraded virtualization technology based on libvirt 8.0.0 and QEMU 6.2.0.
Each new release of libvirt and QEMU contains numerous improvements and bug fixes.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL labels Feb 24, 2022
@andreabolognani
Copy link
Contributor Author

@alicefr the libguestfs dependencies have changed significantly in the latest package. I have tested the scenarios described in docs/guestfs.md without encountering any issues, but I'm not really confident that covers everything. Can you please take a look?

@andreabolognani
Copy link
Contributor Author

At least some of the failures look legit.

One is caused by netcat being missing from the image - libvirt used to drag this in, but it no longer does. We could add it back manually, but maybe the test case could be reworked so that doing that is not necessary? Perhaps there are other uses of netcat outside of the testsuite.

I'm unable to reproduce one of the failures locally because

S [SKIPPING] [6.113 seconds]
[rfe_id:393][crit:high][vendor:cnv-qe@redhat.com][level:system][sig-compute] VM Live Migration
tests/migration_test.go:97
  Starting a VirtualMachineInstance 
  tests/migration_test.go:360
    live migration cancelation
    tests/migration_test.go:2208
      should be able to cancel a migration
      vendor/github.com/onsi/ginkgo/extensions/table/table.go:92
        [sig-storage][storage-req][test_id:2732] with RWX block disk and virtctl [It]
        vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go:41

        Skip test when Block storage is not present

I'm unclear on what I need to do here, advice would be appreciated :)

I'll look into the whole thing more tomorrow.

@alicefr
Copy link
Member

alicefr commented Feb 25, 2022

@andreabolognani I'll have a try but generally if you succeed with the examples, then it is what we expect to work with the setup.

At least some of the failures look legit.

One is caused by netcat being missing from the image - libvirt used to drag this in, but it no longer does. We could add it back manually, but maybe the test case could be reworked so that doing that is not necessary? Perhaps there are other uses of netcat outside of the testsuite.

I'm unable to reproduce one of the failures locally because

S [SKIPPING] [6.113 seconds]
[rfe_id:393][crit:high][vendor:cnv-qe@redhat.com][level:system][sig-compute] VM Live Migration
tests/migration_test.go:97
  Starting a VirtualMachineInstance 
  tests/migration_test.go:360
    live migration cancelation
    tests/migration_test.go:2208
      should be able to cancel a migration
      vendor/github.com/onsi/ginkgo/extensions/table/table.go:92
        [sig-storage][storage-req][test_id:2732] with RWX block disk and virtctl [It]
        vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go:41

        Skip test when Block storage is not present

I'm unclear on what I need to do here, advice would be appreciated :)

I'll look into the whole thing more tomorrow.

You are missing the storage class that support block storage. In this case ceph, you need to set export KUBEVIRT_STORAGE="rook-ceph-default" before creating the cluster with make cluster-up then it is installed and you can check it with kubectl get sc

@alicefr
Copy link
Member

alicefr commented Feb 25, 2022

Some of the missing packages are:

  • e2fsprogs-*: this should be fine because we use the tool in the fixed appliance, so no need in the container
  • dhcp-*: we don't need any network as we are not operating on VMs but only on disks
  • dnf-*: this is fine because we don't need to install or fetch the dependencies
  • findutlis: I'll readd this
  • gdisk: this I'm not sure
  • genisoimage: this either I'm not sure
  • squashfs-tools: same as for e2fsprogs
  • supermin : this is fine as we use a fixed appliance

I'll try to find out in which cases libguestfs uses gdisk and genisoimage

EDIT: they are listed in the package list for the appliance so I guess they are used there. The new image size has also reduced from 457MB to 392MB :)

@andreabolognani
Copy link
Contributor Author

You are missing the storage class that support block storage. In this case ceph, you need to set export KUBEVIRT_STORAGE="rook-ceph-default" before creating the cluster with make cluster-up then it is installed and you can check it with kubectl get sc

@alicefr thanks, I've tried this and it worked as expected. Did you find this documented anywhere, or is it tribal knowledge? Either way, I couldn't reproduce the failure locally so I'm going to assume it was a fluke until proven otherwise.

findutlis: I'll readd this

I'll try to find out in which cases libguestfs uses gdisk and genisoimage

they are listed in the package list for the appliance so I guess they are used there.

So you think that findutils, gdisk and genisoimage should all be added back to the guestfs image? If they're used by libguestfs at runtime even when working with a pre-generated appliance, isn't it a packaging bug that they're not dragged in explicitly? We should discuss this with Rich.

For the time being, I believe I have fixed one of the test failures. More to come next week :)

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2022
@alicefr
Copy link
Member

alicefr commented Feb 28, 2022

@andreabolognani I think we should only add findutils as standard tool but gdisk and genisoimage no. I don't think we have a good split for using a fixed appliance in centos 8.

About the env variable, yes I don't think that is documented :/. This script uses some of the variables in KubeVirt CI: https://github.com/kubevirt/kubevirt/blob/main/automation/test.sh

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2022
@andreabolognani
Copy link
Contributor Author

I think we should only add findutils as standard tool but gdisk and genisoimage no.

Done.

I don't think we have a good split for using a fixed appliance in centos 8.

AFAICT CentOS Stream 8 and 9 both have the changes we need.

If they're used by libguestfs at runtime even when working with a pre-generated appliance, isn't it a packaging bug that they're not dragged in explicitly? We should discuss this with Rich.

https://bugzilla.redhat.com/show_bug.cgi?id=1989514#c19

@andreabolognani
Copy link
Contributor Author

Shouldn't be an issue since a couple of tests are still failing, but just to be on the safe side

/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2022
@andreabolognani
Copy link
Contributor Author

I'm still investigating the issue with booting the Alpine Linux 3.7 image using the RHEL 8.6 machine type, but in the meantime I've brought all images into the current decade :)

/hold cancel

@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 2, 2022
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2022
@andreabolognani
Copy link
Contributor Author

andreabolognani commented Mar 3, 2022

Updated to fix a couple of tests that were relying on the exact amount of memory used by CirrOS, which has changed when moving to the latest version.

I'm still seeing failures for [sig-network] [rfe_id:694][crit:medium][vendor:cnv-qe@redhat.com][level:component]Networking Multiple virtual machines connectivity using bridge binding interface with a test outbound VMI should be able to reach [test_id:1542]the internet in prow but the test works fine locally, so I'm hoping it might have have been a temporary issue of the testing environment.

@andreabolognani
Copy link
Contributor Author

Most of the failures are probably caused by the ongoing CI cluster upgrade, I assume.

@rmohr @vladikr can you please take a look? I think this should be ready to be merged now :)

@rmohr
Copy link
Member

rmohr commented Mar 4, 2022

Most of the failures are probably caused by the ongoing CI cluster upgrade, I assume.

Probably. It looks weird.

/retest

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2022
Specifically:

  Alpine Linux
    x86_64      3.7.0 -> 3.15.0
    aarch64    3.11.5 -> 3.15.0
  CirrOS
    x86_64      0.4.0 -> 0.5.2
    aarch64     0.5.0 -> 0.5.2

In the case of the Alpine Linux and CirrOS images for x86_64,
we're replacing releases dating back to 2017 with their modern
equivalents. The versions are also consistent across all
architectures now.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Depending on the scenario, SeaBIOS might use escape sequences
or omit them. Make sure we can detect the fact that iPXE has
produced output in both cases.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
@andreabolognani
Copy link
Contributor Author

The ipv6 test is still not working since we don't have DHCP6 so the VM is not familiar with the DNS IP.
The test should be skipped if the cluster doesn't support IPv4.
Sorry for the noise.

@AlonaKaplan does the diff below look reasonable to you? I'm preparing a rebase and I can just squash it in.

diff --git a/tests/network/vmi_networking.go b/tests/network/vmi_networking.go
index ebac17309..1b0f30e25 100644
--- a/tests/network/vmi_networking.go
+++ b/tests/network/vmi_networking.go
@@ -300,12 +300,13 @@ var _ = SIGDescribe("[rfe_id:694][crit:medium][vendor:cnv-qe@redhat.com][level:c
 
        Context("VirtualMachineInstance with default settings", func() {
                It("[test_id:1542]should be able to reach the internet", func() {
+                       libnet.SkipWhenClusterNotSupportIpv4(virtClient)
                        outboundVMI := libvmi.NewCirros(
                                libvmi.WithInterface(libvmi.InterfaceDeviceWithMasqueradeBinding()),
                                libvmi.WithNetwork(v1.DefaultPodNetwork()),
                        )
                        outboundVMI = runVMI(outboundVMI)
-                       Expect(libnet.WithIPv6(console.LoginToCirros)(outboundVMI)).To(Succeed())
+                       tests.WaitUntilVMIReady(outboundVMI, console.LoginToCirros)
 
                        By("checking the VirtualMachineInstance can fetch via HTTP")
                        err := console.SafeExpectBatch(outboundVMI, []expect.Batcher{

@AlonaKaplan
Copy link
Member

The ipv6 test is still not working since we don't have DHCP6 so the VM is not familiar with the DNS IP.
The test should be skipped if the cluster doesn't support IPv4.
Sorry for the noise.

@AlonaKaplan does the diff below look reasonable to you? I'm preparing a rebase and I can just squash it in.

diff --git a/tests/network/vmi_networking.go b/tests/network/vmi_networking.go
index ebac17309..1b0f30e25 100644
--- a/tests/network/vmi_networking.go
+++ b/tests/network/vmi_networking.go
@@ -300,12 +300,13 @@ var _ = SIGDescribe("[rfe_id:694][crit:medium][vendor:cnv-qe@redhat.com][level:c
 
        Context("VirtualMachineInstance with default settings", func() {
                It("[test_id:1542]should be able to reach the internet", func() {
+                       libnet.SkipWhenClusterNotSupportIpv4(virtClient)
                        outboundVMI := libvmi.NewCirros(
                                libvmi.WithInterface(libvmi.InterfaceDeviceWithMasqueradeBinding()),
                                libvmi.WithNetwork(v1.DefaultPodNetwork()),
                        )
                        outboundVMI = runVMI(outboundVMI)
-                       Expect(libnet.WithIPv6(console.LoginToCirros)(outboundVMI)).To(Succeed())
+                       tests.WaitUntilVMIReady(outboundVMI, console.LoginToCirros)
 
                        By("checking the VirtualMachineInstance can fetch via HTTP")
                        err := console.SafeExpectBatch(outboundVMI, []expect.Batcher{

Yes. And the VM should have bridge binding as the original test had.

outboundVMI := libvmi.NewCirros()

Sorry again for my previous misleading request.

The other tests in this group are all concerned with inter-cluster
connectivity, so it makes sense to separate this one. It also
makes the original test function a bit simpler because it no
longer has to accomodate for two different use cases.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
It doesn't make sense to have this in the top-level directory.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The netcat package is not included in the handlerbase images.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Specifically:

        QEMU   6.0.0-33 -> 6.2.0-5
     libvirt    7.6.0-6 -> 8.0.0-2
  libguestfs   1.44.0-3 -> 1.44.0-5
     SeaBIOS   1.14.0-1 -> 1.15.0-1
       EDKII 20210527-1 -> 20220126-2

Since the freshest packages can now be installed directly from
the AppStream repository, we no longer need the to configure and
use the advancedvirt repository.

The latest libguestfs package has dropped a significant number
of dependencies: these are only used when dynamically generating
an appliance at runtime, and they're not needed in our case since
we use a pre-created appliance instead. As a result, the guestfs
container image is going to be smaller.

We have, however, to explicitly include tar (which is used by
the guestfs container's entrypoint.sh script) as well as file
(which is used internally by libguestfs, and whose absence from
its dependencies is a packaging issues that's in the process of
being addressed) now to keep things working.

We also have to include netcat in the launcher image explicitly
because libvirt no longer drags it in but one of the tests uses
it.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
To match the updated RPM package.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 4, 2022
@andreabolognani
Copy link
Contributor Author

Sorry again for my previous misleading request.

No worries, thank you for looking into it and helping out :)

@AlonaKaplan
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2022
@andreabolognani
Copy link
Contributor Author

/retest

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Apr 4, 2022

@andreabolognani: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-e2e-kind-1.19-vgpu 3883bff link true /test pull-kubevirt-e2e-kind-1.19-vgpu

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@EdDev
Copy link
Member

EdDev commented Apr 5, 2022

/test pull-kubevirt-e2e-kind-1.22-sriov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants