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

test: Boot real VMs for testing virtual machines #7117

Merged
merged 5 commits into from Jul 11, 2017

Conversation

Projects
None yet
5 participants
@stefwalter
Contributor

stefwalter commented Jun 27, 2017

Use the same image that we booted as the testing virtual machine in a nested manner in check-machines in order to test virtual machine functionality in Cockpit.

This means migrating away from virt-install

Depends on:

@stefwalter stefwalter changed the title from WIP: Boot real VMs for testing virtual machines to test: Boot real VMs for testing virtual machines Jun 27, 2017

@sgratch sgratch referenced this pull request Jun 27, 2017

Closed

machines: Send Non-Maskable interrupt feature added #6722

2 of 2 tasks complete

@stefwalter stefwalter added the blocked label Jun 27, 2017

@martinpitt martinpitt removed the blocked label Jun 28, 2017

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 28, 2017

Member

I force-pushed a rebase to master, as #7118 now landed.

Member

martinpitt commented Jun 28, 2017

I force-pushed a rebase to master, as #7118 now landed.

Show outdated Hide outdated test/verify/check-machines
Show outdated Hide outdated test/verify/check-machines
Show outdated Hide outdated test/verify/check-machines
@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Jun 29, 2017

Member

I have pushed FIXUPs for me review comments. Let's see if they help...

Member

mvollmer commented Jun 29, 2017

I have pushed FIXUPs for me review comments. Let's see if they help...

@martinpitt martinpitt referenced this pull request Jun 29, 2017

Merged

machines: Replace state polling with event handling #7141

5 of 5 tasks complete
@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Jun 30, 2017

Member

What abou using virt-install --import? That way we wouldn't need to construct the XML ourselves, It seems to be non-trivial to get the logging right.

Member

mvollmer commented Jun 30, 2017

What abou using virt-install --import? That way we wouldn't need to construct the XML ourselves, It seems to be non-trivial to get the logging right.

@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Jun 30, 2017

Member

The cirros image doesn't seem to boot on fedora-i386

Member

mvollmer commented Jun 30, 2017

The cirros image doesn't seem to boot on fedora-i386

@michalskrivanek

This comment has been minimized.

Show comment
Hide comment
@michalskrivanek

michalskrivanek Jun 30, 2017

Contributor

isn't it the x86_64 image? They release a i386 version too, though I'm not sure how much value i386 still has these days.

Contributor

michalskrivanek commented Jun 30, 2017

isn't it the x86_64 image? They release a i386 version too, though I'm not sure how much value i386 still has these days.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 30, 2017

Member

virt-install --import sounds okay to me, given that on the hackfest we pretty much agreed that virt-install and virt-xml are the preferred interfaces for Cockpit to define/change machine definitions.

Member

martinpitt commented Jun 30, 2017

virt-install --import sounds okay to me, given that on the hackfest we pretty much agreed that virt-install and virt-xml are the preferred interfaces for Cockpit to define/change machine definitions.

@mykaul

You are getting closer and closer to reinventing Lago (http://lago.readthedocs.io/en/latest/README.html ).
Perhaps we should look into what you need from Lago to make you happy and join forces.

Show outdated Hide outdated test/verify/check-machines
<disk type='file' snapshot='external'>
<driver name='qemu' type='qcow2'/>
<source file='{image}'/>
<target dev='hdb' bus='ide'/>

This comment has been minimized.

@mykaul

mykaul Jul 2, 2017

Why IDE and not virtio or virtio-SCSI?

@mykaul

mykaul Jul 2, 2017

Why IDE and not virtio or virtio-SCSI?

This comment has been minimized.

@stefwalter

stefwalter Jul 2, 2017

Contributor

That's what the earlier integration test by @mareklibra expected. They expected 'hda' and 'hdb' devices. This is the minimal set of changes in this PR. An additional pull request could change this later.

@stefwalter

stefwalter Jul 2, 2017

Contributor

That's what the earlier integration test by @mareklibra expected. They expected 'hda' and 'hdb' devices. This is the minimal set of changes in this PR. An additional pull request could change this later.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Jul 2, 2017

Contributor

You are getting closer and closer to reinventing Lago (http://lago.readthedocs.io/en/latest/README.html ).

And Lago is close to reinventing Vagrant, and so on ;)

What we're really after with this pull request is the minimum amount of code modified in the current integration tests to get this up and running.

It would probably take a couple months of work to tear out all the launching, setup, boostrapping, access, libvirt, qemu stuff in the Cockpit integration tests. I'm not against such work, but unwilling to put work towards that right now.

Contributor

stefwalter commented Jul 2, 2017

You are getting closer and closer to reinventing Lago (http://lago.readthedocs.io/en/latest/README.html ).

And Lago is close to reinventing Vagrant, and so on ;)

What we're really after with this pull request is the minimum amount of code modified in the current integration tests to get this up and running.

It would probably take a couple months of work to tear out all the launching, setup, boostrapping, access, libvirt, qemu stuff in the Cockpit integration tests. I'm not against such work, but unwilling to put work towards that right now.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Jul 2, 2017

Contributor

The cirros image doesn't seem to boot on fedora-i386

Thanks for catching that, we can get another image, or just use i386 image for now, since this VM doesn't have to do anything.

isn't it the x86_64 image? They release a i386 version too, though I'm not sure how much value i386 still has these days.

i386 is important to Cockpit because it boots without emulation on x86_64 infrastructure while still finding multi-arch bugs. This doesn't seem to be true of any other architecture.

Contributor

stefwalter commented Jul 2, 2017

The cirros image doesn't seem to boot on fedora-i386

Thanks for catching that, we can get another image, or just use i386 image for now, since this VM doesn't have to do anything.

isn't it the x86_64 image? They release a i386 version too, though I'm not sure how much value i386 still has these days.

i386 is important to Cockpit because it boots without emulation on x86_64 infrastructure while still finding multi-arch bugs. This doesn't seem to be true of any other architecture.

Show outdated Hide outdated test/verify/check-machines
Show outdated Hide outdated pkg/machines/services.es6
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jul 6, 2017

Member

In the light of recent discussions, that virt-install and virt-xml are cockpit's main interfaces, I think we should revert the virt-install → XML bits. @stefwalter , if you agree I can look into this, and also resolve the conflict.

Member

martinpitt commented Jul 6, 2017

In the light of recent discussions, that virt-install and virt-xml are cockpit's main interfaces, I think we should revert the virt-install → XML bits. @stefwalter , if you agree I can look into this, and also resolve the conflict.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Jul 7, 2017

Contributor

In the light of recent discussions, that virt-install and virt-xml are cockpit's main interfaces, I think we should revert the virt-install → XML bits. @stefwalter , if you agree I can look into this, and also resolve the conflict.

TBH, my gut feeling is that this conclusion doesn't follow from the discussions. But if you're willing to put in the effort, and it solves problems, then I won't object.

Contributor

stefwalter commented Jul 7, 2017

In the light of recent discussions, that virt-install and virt-xml are cockpit's main interfaces, I think we should revert the virt-install → XML bits. @stefwalter , if you agree I can look into this, and also resolve the conflict.

TBH, my gut feeling is that this conclusion doesn't follow from the discussions. But if you're willing to put in the effort, and it solves problems, then I won't object.

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jul 10, 2017

test: Boot real VMs for testing virtual machines
Use the same image that we booted as the testing virtual
machine in a nested manner in check-machines in order
to test virtual machine functionality in Cockpit.

This means migrating away from virt-install

Closes #7117

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jul 10, 2017

test: Boot real VMs for testing virtual machines
Use the same image that we booted as the testing virtual
machine in a nested manner in check-machines in order
to test virtual machine functionality in Cockpit.

This means migrating away from virt-install

Closes #7117
}
console.error(`spawn '${cmd}' process returned error: "${JSON.stringify(exception)}", data: "${JSON.stringify(data)}"`);
console.warn(`spawn '${cmd}' process returned error: "${JSON.stringify(exception)}", data: "${JSON.stringify(data)}"`);

This comment has been minimized.

@@ -41,7 +40,7 @@ export function spawnScript({ script }) {
return spawn(cockpit.script(spawnArgs, [], { err: "message", environ: ['LC_ALL=C'] }))
.fail((ex, data) =>
console.error(`spawn '${script}' script error: "${JSON.stringify(ex)}", data: "${JSON.stringify(data)}"`));
console.warn(`spawn '${script}' script error: "${JSON.stringify(ex)}", data: "${JSON.stringify(data)}"`));

This comment has been minimized.

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jul 10, 2017

test: Boot real VMs for testing virtual machines
Use the same image that we booted as the testing virtual
machine in a nested manner in check-machines in order
to test virtual machine functionality in Cockpit.

This means migrating away from virt-install

Closes #7117
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jul 10, 2017

Member

This bit from testBasic (check_machines.TestMachines) fails now because apparently the new VMs here now use 256 MiB:

    b.wait_in_text("tbody.open .listing-ct-body td:nth-child(1) .usage-donut-caption", "128 MiB")

... but only sometimes, e. g. on debian-testing it is fine. We might need to relax that check and just see that it shows some plausible value?

Member

martinpitt commented Jul 10, 2017

This bit from testBasic (check_machines.TestMachines) fails now because apparently the new VMs here now use 256 MiB:

    b.wait_in_text("tbody.open .listing-ct-body td:nth-child(1) .usage-donut-caption", "128 MiB")

... but only sometimes, e. g. on debian-testing it is fine. We might need to relax that check and just see that it shows some plausible value?

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Jul 10, 2017

Contributor

This bit from testBasic (check_machines.TestMachines) fails now because apparently the new VMs here now use 256 MiB:

I updated that many hours ago. Are you still seeing the failure?

Contributor

stefwalter commented Jul 10, 2017

This bit from testBasic (check_machines.TestMachines) fails now because apparently the new VMs here now use 256 MiB:

I updated that many hours ago. Are you still seeing the failure?

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jul 10, 2017

Member

Indeed, I reloaded and it looks much greener now. Sorry for the noise!

Member

martinpitt commented Jul 10, 2017

Indeed, I reloaded and it looks much greener now. Sorry for the noise!

@martinpitt

As I said before I'm not convinced that moving from virt-install to raw XML for the tests is a good long-term move, as the Machines page is moving towards using virt-install and virt-xml as their primary interface. However, this isn't a reason to block this PR, and it's becoming pleasantly green now. I just have some minor nitpicks and cleanups.

if os.path.exists(path):
os.unlink(path)
if path:
(unused, image) = tempfile.mkstemp(suffix='.qcow2', prefix=os.path.basename(path), dir=self.run_dir)

This comment has been minimized.

@martinpitt

martinpitt Jul 10, 2017

Member

Isn't this supposed to be dir=os.path.dirname(path)? If path does not specify the full path then it should be renamed to filename, and the basename() would be unnecessary.

@martinpitt

martinpitt Jul 10, 2017

Member

Isn't this supposed to be dir=os.path.dirname(path)? If path does not specify the full path then it should be renamed to filename, and the basename() would be unnecessary.

This comment has been minimized.

@stefwalter

stefwalter Jul 11, 2017

Contributor

I think the run_dir is where we want the temporary images. This is intentional.

@stefwalter

stefwalter Jul 11, 2017

Contributor

I think the run_dir is where we want the temporary images. This is intentional.

@@ -304,7 +304,7 @@ function spawnVirsh({connectionName, method, failHandler, args}) {
logDebug(msg);
return ;
}
console.error(msg);
console.warn(msg);

This comment has been minimized.

@martinpitt

martinpitt Jul 10, 2017

Member

Nitpick: s/indistinguish/distinguish/ in the commit message :-)

@martinpitt

martinpitt Jul 10, 2017

Member

Nitpick: s/indistinguish/distinguish/ in the commit message :-)

This comment has been minimized.

@stefwalter

stefwalter Jul 11, 2017

Contributor

Done.

@stefwalter

stefwalter Jul 11, 2017

Contributor

Done.

@@ -101,6 +101,7 @@ depcomp
/bots/images/*.qcow2
/bots/images/*.partial
/bots/images/*.xz
/bots/images/*.img

This comment has been minimized.

@martinpitt

martinpitt Jul 10, 2017

Member

Please fix the commit message to say "a CirrOS image", not "same image".

@martinpitt

martinpitt Jul 10, 2017

Member

Please fix the commit message to say "a CirrOS image", not "same image".

This comment has been minimized.

@stefwalter

stefwalter Jul 11, 2017

Contributor

Done.

@stefwalter

stefwalter Jul 11, 2017

Contributor

Done.

Show outdated Hide outdated bots/images/scripts/fedora-24.setup
Show outdated Hide outdated test/verify/check-machines

stefwalter added some commits Jun 27, 2017

test: Refactor how the tests add disks
Return a more fully featured disk object from
VirtMachine.add_disk() and accept more detailed arguments
so we can be more opinionated about what happens.
test: Add a function to VirtMachine to pull an image
VirtMachine.pull() pulls an image from our image-download tooling
and will then return the image filename.
machines: Use console.warn instead of console.error
Use of console.error() indicates a failure similar to an uncaught
exception. This is definitely the case in phantomjs where the
two are hard to distingudish in phantom-driver.js.

We still use console.error() for programmer errors, but not for
validating data from the system.
machines: Allow failHandler to return a resolved promise
This allows us to ignore failures and propogate them as successes
if they are expected. This helps fix a race when 'virsh dommemstate'
fails during virtual machine shutdown.
test: Boot real VMs for testing virtual machines
Use a CirrOS image as the testing virtual machine. We boot
it in a nested manner in check-machines in order to test
virtual machine functionality in Cockpit.

Closes #7117
@martinpitt

Thanks for fixing! Looks good enough now, if there are more nitpicks let's fix them in a follow-up.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jul 11, 2017

Member

This is "green enough" now. The debian-stable failures of check-machines are known and being addressed in #7192 or alternatively #7193. The centos-7 failure is a flake, I just sent PR #7195 to fix that.

Member

martinpitt commented Jul 11, 2017

This is "green enough" now. The debian-stable failures of check-machines are known and being addressed in #7192 or alternatively #7193. The centos-7 failure is a flake, I just sent PR #7195 to fix that.

@martinpitt martinpitt merged commit 60b9711 into cockpit-project:master Jul 11, 2017

14 of 16 checks passed

verify/centos-7 Not yet tested
Details
verify/debian-stable Not yet tested
Details
avocado/fedora-24 Tests passed
Details
container/kubernetes Tests passed
Details
selenium/chrome Tests passed
Details
selenium/firefox Tests passed
Details
semaphoreci The build passed on Semaphore.
Details
verify/debian-testing Tests passed
Details
verify/fedora-26 Tests passed
Details
verify/fedora-atomic Tests passed
Details
verify/fedora-i386 Tests passed
Details
verify/rhel-7 Tests passed
Details
verify/rhel-7-4 Tests passed
Details
verify/rhel-atomic Tests passed
Details
verify/ubuntu-1604 Tests passed
Details
verify/ubuntu-stable Tests passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment