machines: add support for existing disk as installation mode for new VMs #11206

KKoukiou · 2019-02-19T18:53:45Z

and the existing Filesystem select Entry was renamed to Local Install media to avoid confusion.

martinpitt

Many thanks for working on this! Such an useful feature.

test/verify/machineslib.py

pkg/machines/components/create-vm-dialog/createVmDialog.jsx

pkg/machines/scripts/create_machine.sh

test/verify/machineslib.py

martinpitt

Nice, LGTM!

KKoukiou · 2019-02-20T12:21:59Z

testCreate is failing on previous test, need to check it.
https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190220-092222-9995b98c-verify-debian-stable/log.html#70

KKoukiou · 2019-02-21T10:30:53Z

This needs more thought, because importing existing disk means that the VMs will not have install phase. Aka the user should never see the install button, which is what is happening now.

KKoukiou · 2019-02-21T11:59:44Z

@martinpitt I did some changes so that the install button will never appear when importing existing disks.
Also I added a test for the case where the VM is set to start automatically just to make sure that the domain running. Please review once more.

KKoukiou · 2019-02-21T14:50:53Z

looks like sometime it fails on lack of memory, decreasing it a bit.
https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190221-135757-d510cece-verify-debian-testing/TestMachinesDBus-testCreate-debian-testing-127.0.0.2-2801-FAIL.png

KKoukiou · 2019-02-23T11:18:36Z

Looks like the new test is a bit flaky on debian stable,
https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190221-145151-19f84b50-verify-debian-stable/TestMachinesVirsh-testCreate-debian-stable-127.0.0.2-2401-FAIL.png
And related to this bug https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1722702
But I 'll need to investigate more.

And on rhel-8 is failed with timeout to start VM. https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190221-145151-19f84b50-verify-rhel-8-0/TestMachinesDBus-testCreate-rhel-8-0-127.0.0.2-2801-FAIL.png

Thinking how to robustify it.

KKoukiou · 2019-02-25T22:06:12Z

Looks like the new test fails undeterministically with VM having state == 'paused' instead of running.
Managed to reproduce locally (after a lot of runs) and when I sshed to the test VM I noticed errors in the libvirt logs from the VM that ended up in paused state.

root@ibm-p8-kvm-03-guest-02:~# cat /var/log/libvirt/qemu/subVmTestCreate16.log 
2019-02-25 22:03:10.480+0000: starting up libvirt version: 4.6.0, package: 2ubuntu3.2 (Christian Ehrhardt <[email protected]> Thu, 06 Dec 2018 09:16:13 +0100), qemu version: 2.12.0Debian 1:2.12+dfsg-3ubuntu8.3, kernel: 4.18.0-15-generic, hostname: ibm-p8-kvm-03-guest-02.virt.pnr.lab.eng.rdu2.redhat.com
LC_ALL=C PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin QEMU_AUDIO_DRV=none /usr/bin/kvm-spice -name guest=subVmTestCreate16,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-2-subVmTestCreate16/master-key.aes -machine pc-i440fx-cosmic,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu Skylake-Client-IBRS -m 256 -realtime mlock=off -smp 1,sockets=1,cores=1,threads=1 -uuid 39da5890-9a44-4e4f-92f3-905add6607a8 -no-user-config -nodefaults -chardev socket,id=charmonitor,fd=29,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x5.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5 -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x5.0x1 -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x5.0x2 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 -drive file=/var/lib/libvirt/images/example.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -netdev tap,fd=31,id=hostnet0,vhost=on,vhostfd=32 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:00:17:6e,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -chardev spicevmc,id=charchannel0,name=vdagent -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 -spice port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on -vnc 127.0.0.1:1 -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -chardev spicevmc,id=charredir0,name=usbredir -device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=1 -chardev spicevmc,id=charredir1,name=usbredir -device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=2 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg timestamp=on
2019-02-25T22:03:10.528309Z qemu-system-x86_64: -chardev pty,id=charserial0: char device redirected to /dev/pts/2 (label charserial0)
KVM internal error. Suberror: 1
emulation failure
EAX=00000001 EBX=000f7e63 ECX=00000001 EDX=00000001
ESI=000069c6 EDI=0ffbdb1a EBP=00006986 ESP=000a8000
EIP=000fd06a EFL=00010046 [---Z-P-] CPL=0 II=0 A20=1 SMM=1 HLT=0
ES =0010 00000000 ffffffff 00c09300
CS =0000 00000000 00000fff 00809b00
SS =0010 00000000 ffffffff 00c09300
DS =0010 00000000 ffffffff 00c09300
FS =0010 00000000 ffffffff 00c09300
GS =0010 00000000 ffffffff 00c09300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008b00
GDT=     10387cfe 0000fe6c
IDT=     0010387c 00003810
CR0=00000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000fffecffc DR7=000000000e1e0400
EFER=0000000000000000
Code=cb 66 ba 60 d0 0f 00 e9 cb fe bc 00 80 0a 00 e8 4e 52 ff ff <0f> aa fa fc 66 ba 79 d0 0f 00 e9 b4 fe f3 90 f0 0f ba 2d a8 53 0f 00 00 72 f3 8b 25 a4 53

KKoukiou · 2019-02-27T12:38:41Z

Two new issues appeared now.. Phewwwww
https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190227-104040-10036380-verify-fedora-29/log.html#168

This is probably some ancient libvirt in debian stable, checking for open bug reports.
https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190227-104040-10036380-verify-debian-stable/TestMachinesVirsh-testCreate-debian-stable-127.0.0.2-2501-FAIL.png

is_filesystem_location was renamed to sourceType to be able to support easily more than two different installation methods.

'Filesystem' was renamed to 'Local Install Media' to not be confused with existing disks images as installation source.

…meter When needed it should be written explicitely because for some installation methods it's not neeeded; for example when importing existing disk image.

…empt superuser This is to see files in user-inaccessible paths.

Closes cockpit-project#11206 Fixes cockpit-project#11163 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666825

andreasn

Looks good to me!

KKoukiou · 2019-02-27T18:03:26Z

fedora-29 failing https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190227-165757-d7c13f32-verify-fedora-29/log.html#70

phrdina · 2019-02-28T13:26:57Z

@martinpitt reposted. So just for the failing test I disabled KVM by unloading the module.
Simple chmod 0 /dev/kvm does not work, libvirt will choose KVM type for domains if /dev/kvm exists.

You can use --virt-type qemu to tell libvirt that you don't want to use KVM for this specific guest or you can unload the module from kernel.

KKoukiou · 2019-02-28T13:28:42Z

@martinpitt reposted. So just for the failing test I disabled KVM by unloading the module.
Simple chmod 0 /dev/kvm does not work, libvirt will choose KVM type for domains if /dev/kvm exists.

You can use --virt-type qemu to tell libvirt that you don't want to use KVM for this specific guest or you can unload the module from kernel.

I am creating guests by clicking in the cockpit UI, it's the testCreate. I don't create the guest from the python test code so parameter customization is not possible

And I unloaded kvm module, it's allready pushed. But still creation of this last VM sometimes fails without real error from libvirt.
https://fedorapeople.org/groups/cockpit/logs/pull-11206-20190227-165757-d7c13f32-verify-fedora-29/TestMachinesDBus-testCreate-fedora-29-127.0.0.2-2301-FAIL.png

…th VNC console Related to: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=923398 Issue cockpit-project#11265

martinpitt

This LGTM now, and the tests also seem happy. @KKoukiou , is there anything you still want to change from your side, or was this just waiting on review?

martinpitt · 2019-03-01T06:43:58Z

test/verify/machineslib.py

-            b.select_from_dropdown("#storage-size-unit-select", self.storage_size_unit)
+            if self.storage_size is not None:
+                b.set_input_text("#storage-size", str(self.storage_size))
+                b.select_from_dropdown("#storage-size-unit-select", self.storage_size_unit)


Suggestion: If it's None, check that #storage-size is not present. This makes tests fail more meaningfully if the parameter is accidentally forgotten.

martinpitt · 2019-03-01T06:45:52Z

pkg/machines/scripts/create_machine.sh

 else
-    DISK_OPTIONS="size=$STORAGE_SIZE,format=qcow2"
+    COMPARISON=$(awk 'BEGIN{ print "'$STORAGE_SIZE'"<=0 }')
+    if [ "$COMPARISON" -eq 1 ]; then


It's not new code, so no need to change it in this PR, but this is "eww". if [ "$STORAGE_SIZE" -le 0 ] will do just fine.

martinpitt · 2019-03-01T06:50:33Z

test/verify/machineslib.py

+        # This is applicable only for the following test so let's keep it last,
+        # in order to allow the rest of the tests to run faster with QEMU KVM
+        # Run modprobe -r in retry loop because it fails sometimes with 'Module kvm is in use'
+        self.machine.execute("for i in 1 2 3 4 5; do modprobe -r kvm_intel && modprobe -r kvm && break || sleep 1; done")


Style nitpick for the future: machine.execute() already has a builtin timeout, and there is no reason to be overly parsimonious about it. Also, for this we have testlib.wait():

wait(lambda: self.machine.execute("modprobe -r kvm_intel && modprobe -r kvm"))

Closes #11206 Fixes #11163 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666825

KKoukiou · 2019-03-01T11:00:38Z

All comments addressed in #11272

* testCreate test was adjusted to check that storage_size dialog entry is not present when importing existing disk * utilize testlib.wait instead of using for loop in shell and fix condition * introduced some extra expected journal messages from selinux warnings

KKoukiou force-pushed the create-vm-existing-disk branch from d96bdd5 to ef116bc Compare February 19, 2019 18:58

KKoukiou mentioned this pull request Feb 19, 2019

machines: create VM with installation source PXE #11179

Merged

martinpitt requested changes Feb 20, 2019

View reviewed changes

KKoukiou force-pushed the create-vm-existing-disk branch from ef116bc to 9995b98 Compare February 20, 2019 09:22

martinpitt previously approved these changes Feb 20, 2019

View reviewed changes

KKoukiou added the needswork label Feb 20, 2019

KKoukiou dismissed martinpitt’s stale review via d426da2 February 21, 2019 11:45

KKoukiou force-pushed the create-vm-existing-disk branch from 9995b98 to d426da2 Compare February 21, 2019 11:45

KKoukiou removed the needswork label Feb 21, 2019

KKoukiou force-pushed the create-vm-existing-disk branch from d426da2 to c4d6e70 Compare February 21, 2019 11:57

KKoukiou added needswork and removed needswork labels Feb 21, 2019

KKoukiou force-pushed the create-vm-existing-disk branch from c4d6e70 to d510cec Compare February 21, 2019 13:57

KKoukiou force-pushed the create-vm-existing-disk branch from d510cec to 19f84b5 Compare February 21, 2019 14:51

KKoukiou force-pushed the create-vm-existing-disk branch from 19f84b5 to ef5d4d4 Compare February 25, 2019 07:14

KKoukiou force-pushed the create-vm-existing-disk branch from ef5d4d4 to 17075e5 Compare February 25, 2019 13:54

KKoukiou added 5 commits February 27, 2019 14:40

tests: TestMachines: testCreate: rename is_filesystem_location variable

a2bffbb

is_filesystem_location was renamed to sourceType to be able to support easily more than two different installation methods.

machines: rename Filesystem select entry in Installation sources for VMs

8ad6ce5

'Filesystem' was renamed to 'Local Install Media' to not be confused with existing disks images as installation source.

tests: TestMachines: testCreate: storage size is not a mandatory para…

3fe67a5

…meter When needed it should be written explicitely because for some installation methods it's not neeeded; for example when importing existing disk image.

machines: FileAutoComplete component in VM creation dialog should att…

f4025bd

…empt superuser This is to see files in user-inaccessible paths.

machines: add option to import existing image when creating VM

84cd016

Closes cockpit-project#11206 Fixes cockpit-project#11163 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666825

KKoukiou force-pushed the create-vm-existing-disk branch from 1003638 to d7c13f3 Compare February 27, 2019 15:43

andreasn previously approved these changes Feb 27, 2019

View reviewed changes

KKoukiou added the needswork label Feb 27, 2019

bots: Add naughty override for virt-install failing to spawn guest wi…

e6bfbf3

…th VNC console Related to: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=923398 Issue cockpit-project#11265

KKoukiou dismissed andreasn’s stale review via e6bfbf3 February 28, 2019 16:40

KKoukiou force-pushed the create-vm-existing-disk branch from d7c13f3 to e6bfbf3 Compare February 28, 2019 16:40

KKoukiou removed the needswork label Feb 28, 2019

martinpitt approved these changes Mar 1, 2019

View reviewed changes

martinpitt merged commit 4fbee2a into cockpit-project:master Mar 1, 2019

martinpitt pushed a commit that referenced this pull request Mar 1, 2019

machines: add option to import existing image when creating VM

d04350a

Closes #11206 Fixes #11163 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666825

KKoukiou deleted the create-vm-existing-disk branch March 1, 2019 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

machines: add support for existing disk as installation mode for new VMs #11206

machines: add support for existing disk as installation mode for new VMs #11206

KKoukiou commented Feb 19, 2019 •

edited

Loading

martinpitt left a comment

martinpitt left a comment

KKoukiou commented Feb 20, 2019 •

edited

Loading

KKoukiou commented Feb 21, 2019

KKoukiou commented Feb 21, 2019

KKoukiou commented Feb 21, 2019

KKoukiou commented Feb 23, 2019 •

edited

Loading

KKoukiou commented Feb 25, 2019

KKoukiou commented Feb 27, 2019

andreasn left a comment

KKoukiou commented Feb 27, 2019

phrdina commented Feb 28, 2019

KKoukiou commented Feb 28, 2019 •

edited

Loading

martinpitt left a comment

martinpitt Mar 1, 2019

martinpitt Mar 1, 2019

martinpitt Mar 1, 2019

KKoukiou commented Mar 1, 2019

machines: add support for existing disk as installation mode for new VMs #11206

machines: add support for existing disk as installation mode for new VMs #11206

Conversation

KKoukiou commented Feb 19, 2019 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

KKoukiou commented Feb 20, 2019 • edited Loading

KKoukiou commented Feb 21, 2019

KKoukiou commented Feb 21, 2019

KKoukiou commented Feb 21, 2019

KKoukiou commented Feb 23, 2019 • edited Loading

KKoukiou commented Feb 25, 2019

KKoukiou commented Feb 27, 2019

andreasn left a comment

Choose a reason for hiding this comment

KKoukiou commented Feb 27, 2019

phrdina commented Feb 28, 2019

KKoukiou commented Feb 28, 2019 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Mar 1, 2019

Choose a reason for hiding this comment

martinpitt Mar 1, 2019

Choose a reason for hiding this comment

martinpitt Mar 1, 2019

Choose a reason for hiding this comment

KKoukiou commented Mar 1, 2019

KKoukiou commented Feb 19, 2019 •

edited

Loading

KKoukiou commented Feb 20, 2019 •

edited

Loading

KKoukiou commented Feb 23, 2019 •

edited

Loading

KKoukiou commented Feb 28, 2019 •

edited

Loading