Merge ~paelzer/ubuntu/+source/qemu:ubuntu-eoan-4.0 into ubuntu/+source/qemu:ubuntu/eoan-devel

Proposed by Christian Ehrhardt  on 2019-06-27
Status: Merged
Approved by: Christian Ehrhardt  on 2019-07-03
Approved revision: 5ba78cb78645cf11f5f06de9bda0d900481462c4
Merge reported by: Christian Ehrhardt 
Merged at revision: 5ba78cb78645cf11f5f06de9bda0d900481462c4
Proposed branch: ~paelzer/ubuntu/+source/qemu:ubuntu-eoan-4.0
Merge into: ubuntu/+source/qemu:ubuntu/eoan-devel
Reviewer Review Type Date Requested Status
Christian Ehrhardt  Approve on 2019-07-03
Rafael David Tinoco (community) 2019-06-27 Needs Fixing on 2019-07-03
Canonical Server Team 2019-06-27 Pending
Review via email: mp+369379@code.launchpad.net
To post a comment you must log in.
Christian Ehrhardt  (paelzer) wrote :

This time I took the chance to switch from the historically grown branches/repos in ubuntu-virt and on salsa to our common scheme based on git Ubuntu.
For the transition we have three branches in case you are interested.

Branches in git+ssh://git.launchpad.net/~ubuntu-virt/ubuntu/+source/qemu
ubuntu-eoan-3.1 - old style based on Debian branch at debian/qemu-3.1+dfsg-2
ubuntu-eoan-3.1-GU - new style identical based on git ubuntu import/1%3.1+dfsg-2
   matches 1:1 pkg/ubuntu/eoan-devel, but now with commit history
   We might need this ubuntu-eoan-3.1-GU until a real rebase from
   Debian to pick up all commits
In my repo we now have:
ubuntu-eoan-4.0 - new MP for qemu 4.0 into Eoan based on pkg/ubuntu/eoan-devel

Until then we can do this (and vice versa)
 $ git rebase -i ubuntu-eoan-3.1-GU --onto import/1%3.1+dfsg-2ubuntu5

--

DFSG with the packaged tools
./debian/get-orig-source.sh 4.0.0
getting upstream version 4.0.0 ...
--2019-06-24 13:45:00-- https://download.qemu.org//qemu-4.0.0.tar.xz
...
extracting source in qemu-4.0.0-tmp and cleaning up ...
...
repacking to qemu_4.0.0.orig.tar.xz

Christian Ehrhardt  (paelzer) wrote :

This LGTM from a commit/code POV and also builds fine on all arches now.
Therefore it is ready for review here and testing in PPA [1].

There are known test TODOs (for me):
* There were quite some changes to machine types, therefore we want to test explicitly:
  - compare machine types reported on all architectures
  - test wily X migration to new Eoan
  - test pre 2.10 migration to new Eoan (use the compat-256k-efi-e1000.rom)
  - todo test a HPB type on horsea to check the bits in the guest
  - ppc64 2.11 sxxm type
* General regresson test suite
  - plus updates in that suite for Eoan types

[1]: https://launchpad.net/~paelzer/+archive/ubuntu/qemu-4.0-eoan-v2

Christian Ehrhardt  (paelzer) wrote :

Almost as expected the wily type in this new code is broken again.

From X->E:
2019-06-27T09:26:56.475122Z qemu-system-x86_64: warning: Unknown firmware file in legacy mode: etc/msr_feature_control
2019-06-27T09:26:56.520684Z qemu-system-x86_64: Configuration section missing
2019-06-27T09:26:56.520714Z qemu-system-x86_64: load of migration failed: Invalid argument

The config section is what we have seen on the 2.4/2.3 mismatch the msr_feature seems new.
A Xenial type works so it isn't that migration to the new qemu might generally fail)
We need to fix this up and retest.

Added to my known TODO list

Christian Ehrhardt  (paelzer) wrote :

x86 has a secondary ubuntu type
ubuntu Ubuntu 19.10 PC (i440FX + PIIX, 1996) (alias of pc-i440fx-eoan)
ubuntu Ubuntu 19.04 PC (i440FX + PIIX, 1996) (alias of pc-i440fx-disco)

Also on todo list now

Christian Ehrhardt  (paelzer) wrote :

Not all tests failed :-)
-hpb is ok

Host:
address sizes : 46 bits physical, 48 bits virtual
Guest
address sizes : 40 bits physical, 48 bits virtual
Guest-HPB
address sizes : 46 bits physical, 48 bits virtual

a180a5c... by Christian Ehrhardt  on 2019-06-27

machine types: fix disco type to no more be default

Signed-off-by: Christian Ehrhardt <email address hidden>

Christian Ehrhardt  (paelzer) wrote :

Disco type no more being default is already fixed, wily will be more interesting (and time consuming)

9ac61b4... by Christian Ehrhardt  on 2019-06-27

machine types: fix wily type which is machine 2.4 and compat 2.3

Signed-off-by: Christian Ehrhardt <email address hidden>

Christian Ehrhardt  (paelzer) wrote :

the wily mismatch is resolved as well and tested

Christian Ehrhardt  (paelzer) wrote :

Used a pseries-bionic-sxxm on Bionic and successfully migrated to Eoan.
like:
 uvt-kvm create --password ubuntu b-sxxm arch=ppc64el release=eoan label=daily
 virsh migrate --unsafe --live b-sxxm qemu+ssh://10.222.144.242/system
and in reverse:
 virsh migrate --unsafe --live b-sxxm qemu+ssh://10.222.144.140/system

Christian Ehrhardt  (paelzer) wrote :

No issues with 256k FW either.

But I found that s390x second level KVM has issues, that is next to debug

Christian Ehrhardt  (paelzer) wrote :

In case you wonder, there will be no LP-diff view because of:
"Launchpad encountered an internal error during the following operation: generating the diff for a merge proposal. It was logged with id OOPS-f7251ec93b8b680609e751f472a9dbe8. Sorry for the inconvenience."

Christian Ehrhardt  (paelzer) wrote :

Issue on s390x understood (nestign was off from older experiements).
ok now, no issue of the merge.

Christian Ehrhardt  (paelzer) wrote :

Regression test spotted:
Disco -> Eoan:
error: operation failed: guest CPU doesn't match specification: missing features: md-clear
echo '><failure type='\''BuildFailure'\''>machine types not unique</failure></testcase>'

But this is what I already fixed and should be gone on a retry.
No other known issues left, restarting the automated regression check.

5ba78cb... by Christian Ehrhardt  on 2019-07-01

d/p/ubuntu/enable-md-clear.patch: fix rebase error to match md-clear feature bit

Signed-off-by: Christian Ehrhardt <email address hidden>

Christian Ehrhardt  (paelzer) wrote :

Tests now look all green (and got extended for a few things we found manually).
I think I'm just waiting on the MP review now - I re-pushed all the latest changes, let me know if anything else is needed.

Rafael David Tinoco (rafaeldtinoco) wrote :

Starting review...

Rafael David Tinoco (rafaeldtinoco) wrote :
Download full text (4.1 KiB)

Alright, I have deviated a bit my review from testing features since I've seen how you're testing generated packages, so I did a basic functional test only. Instead, I have carefully reviewed all historic (and new) patches to make sure things were good, and assumed this would have a better benefit for you.. so here it goes the full version of the file: https://pastebin.ubuntu.com/p/6h3CS9mVg6/ and I'm highlighting bellow only the things that I would like clarification and/or a fix for:

########

1)

changelog: - d/qemu-kvm.service: systemd unit to call qemu-kvm-init

=> this was changed in 1:2.10+dfsg-0ubuntu3 (artful)
=> d/qemu-system-common.qemu-kvm.service: systemd unit to call qemu-kvm-init

2)

changelog: - d/qemu-system-common.install: install systemd unit and helper script

=> d/qemu-system-common.install: install helper script, man pages and docs

=> systemd units are installed by debian/rules using debhelper now:
dh_installsystemd -a -pqemu-system-common --no-restart-on-upgrade --name=qemu-kvm
dh_installsystemd -a -pqemu-guest-agent --no-start --no-enable

3)

changelog: - d/rules: install /etc/default/qemu-kvm

=> d/rules: do not install /etc/default/qemu-kvm anymore (line is commented)

4)

changelog - d/p/ubuntu/lp-1761372-*: provide pseries-bionic-2.11-sxxm type as convenience with all meltdown/spectre workarounds enabled by default. (LP: 1761372).

=> I believe you dropped debian/patches/ubuntu/lp-1761372-* files.

=> d/p/ubuntu/define-ubuntu-machine-types.patch: provide pseries-bionic-2.11-sxxm type as convenience with all meltdown/spectre workarounds enabled by default. (LP: 1761372).

5)

changelog: - arch aware kvm wrappers

=> should this s390x (w/ numa) and qemu-kvm wrapper still be mentioned here ?

6)

changelog: - d/control: update VCS links (updated to match latest Ubuntu)

=> why pointing to salsa vcs or ubuntu-disco-3.1 when you're doing 4.0 from upstream ?

7)

changelog: - d/rules: build s390x-netboot.img with upstream Makefile

=> extra "x" after s390
=> d/rules: build s390-netboot.img with upstream Makefile

8)

changelog: - remove /dev/kvm permission handling (moved to systemd 239-6) (#892945)

=> should this still be mentioned ?
=> we're relying on systemd udev /dev/kvm rule for some time now.

9)

changelog: - pc_compat_2_0 got a _fn suffix and slight changes

=> can we refer d/p/u/define-ubuntu-machine-types.patch being changed here ?

=> its clear for you, i took me couple of minutes to realize what/how you've done

=> i confess i'm not a big fan of this big patch, even if they're all related to mach types (new ones, or old ones adapted like -hpb types and GlobalProperties). maybe in the next merge (release/etc) this could be split into d/p/u/mach/* or something else ? to make it easier to correlate upstream cases like the irqchip split being default, to ubuntu bugs being addressed, or even machine types being created because of CVEs only.

10)

changelog: - d/p/ubuntu/lp-1790901-partial-SLOF-for-s390x-netboot.patch: update to SLOF of qemu 4.0

=> already mentioned before (not with 4.0 version)

=> by doing like this, how to correlate between qemu upstream versions and SLOF versions ? this is a ...

Read more...

review: Needs Fixing
Christian Ehrhardt  (paelzer) wrote :
Download full text (3.5 KiB)

Yes reviewing that was exactly what I needed to clean up all the little mistakes that creeped in over the last years.

#1 yes - done

#2 the helper script is still installed by this, but the rest is doen via debhelper automatically - done

#3 that is not a commented line, it really is a comment what the next line will do. Never the less I adapted the cahngelog - done

#4 yes the infra is not upstream (dropped) and the type is all that is left is merged - done
   Also fixed a double mentionign of bug nummbers.

#5 no need to mention details here - I think it is fine as-is

#6 was an old ref, since I want to not update it all the time.
   Since we switch to git ubuntu lets use these and no more change it ever again

#7 in fact s390x = 64bit and s390 = 31 bit, so s390x feels correct
   But then boot code is 31bit (or even 24) and the file name is therefore without x
   fixed changelog

#8 yes it needs to be mentioned, while we rely on systemd/udev a long time (ack) the code was
   still there all the time
   On the next merge it will be in Debian and it will one last time be mentioned as "Dropped"

#9 I had those patches split and maintaining it split was far more error prone (search replaces hit only a few of them) and caused extra work (changing one made the other misapply). So I made them one patch intentionally and plan to keep it that way.
This is a lesson I had to learn the hard way, this particular Delta really is better a single patch :-/

The added changes that you were missing are all mentioned under "Added Changes".
Quote:
"
 100 - added eoan types
 101 - fixed s390x issue of upstream types having a "v" prefix
 102 - add back dropped machine types to avoid more issues like LP: 1802944
 103 - fix kvm split irqchip default in ubuntu q35 machine type
 104 - drop no more needed spapr_machine_2_11_sxxm_instance_options and
 105 adapt updated CamelCase
 106 - -hpb types now need to use GlobalProperties
 107 - pc_compat_2_0 got a _fn suffix and slight changes
"

#10 - yeah the first is the change that remains (slof plus all the reasons and details) and in the "Added changes" section it is only referred to as "updated to 4.0".
Like we usually mention there "updated patch foo for context change"

For your qeustions - about SLOF.
Slof itself is an extra source package "slof" and maintained there.
For a IBM feature request (and since Debian can#t agree where to maintain the s390x bits) we need to add some (only some) of the code back (which is updated every time qemu changes). Out of that we build the s390x roms until Debian finally adds them.

We have regresson tests for slof itself (the package) and for the ccw image (used on normal guests). The only one missing is the s390x netboot which is a known todo at https://trello.com/c/7clwfTYC

#11 again some changes are mentioned as kept (for the function) and as added (for the patch update that was required to match the new version)

#12 done

#13 no really i386 b...

Read more...

Christian Ehrhardt  (paelzer) wrote :

Tests are now all completed as well and all green.
And I have addressed all that Rafael has found.

+1 the MP and prep the eoan upload.

Note: I'll tag and push git-ubuntu tags but I expect some remaining trouble since we have currently a structure like this:

#1 import-old-debian
#2 import-old-ubuntu
#3 new changes broken into commits

With this upload we provide individual commits for #1-#3 but I'm expecting that the importer will only pick up #2-#3 as #2 is already imported. On the next merge from Debian we will tag and upload again with all commits separated and then I hope that git ubuntu will have all of them resolved properly.

review: Approve

Diff calculation failed

Calculating the branch diff failed. You can manually schedule an update if required.

Subscribers

People subscribed via source and target branches