Re: [libvirt] [tck PATCH 1/3] Add tests for virtual network <-> guest connections

On 11/2/18 11:52 AM, Daniel P. Berrangé wrote: > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > lib/Sys/Virt/TCK.pm | 33 ++++++++ > lib/Sys/Virt/TCK/NetworkBuilder.pm | 33 +++++++- > scripts/networks/300-guest-network-isolated.t | 82 ++++++++++++++++++ > scripts/networks/310-guest-network-nat.t | 83 +++++++++++++++++++ > scripts/networks/320-guest-network-route.t | 83 +++++++++++++++++++ > scripts/networks/330-guest-network-open.t | 83 +++++++++++++++++++ > scripts/networks/340-guest-network-bridge.t | 79 ++++++++++++++++++ > scripts/networks/350-guest-network-private.t | 81 ++++++++++++++++++ > scripts/networks/360-guest-network-vepa.t | 81 ++++++++++++++++++ > .../networks/370-guest-network-passthrough.t | 81 ++++++++++++++++++ > scripts/networks/380-guest-network-hostdev.t | 82 ++++++++++++++++++ > t/080-network-builder.t | 2 +- > 12 files changed, 800 insertions(+), 3 deletions(-) > create mode 100644 scripts/networks/300-guest-network-isolated.t > create mode 100644 scripts/networks/310-guest-network-nat.t > create mode 100644 scripts/networks/320-guest-network-route.t > create mode 100644 scripts/networks/330-guest-network-open.t > create mode 100644 scripts/networks/340-guest-network-bridge.t > create mode 100644 scripts/networks/350-guest-network-private.t > create mode 100644 scripts/networks/360-guest-network-vepa.t > create mode 100644 scripts/networks/370-guest-network-passthrough.t > create mode 100644 scripts/networks/380-guest-network-hostdev.t You added all these new tests, but forgot that you made the MANIFEST file static/manually edited in commit a140e4f61 back in May, so the new tests don't get installed.

Also, the tests don't actually boot up an OS and try to pass traffic; I guess that's something you're counting on someone adding later? :-)

> +sub find_free_ipv4_subnet { > + my $index; > + > + my %used; > + > + foreach my $iface (IO::Interface::Simple->interfaces()) { This works to eliminate conflicts with active interfaces, but doesn't take into account any routes that have the same destination address/prefix as the desired network. For example, you may have a static route for 192.168.125.0/25 pointing to another host that has a routed virtual network with that address. If that's the case, the test will fail. I don't have a quick alternative at hand to suggest though, and the likelyhood of a host having a static route that conflicts with the lowest numbered available 192.168.blah.0/24 network is probably pretty low, so I'm going to overlook this :-)

> + if ($iface->netmask eq "255.255.255.0" && > + $iface->address =~ /^192.168.(\d+).\d+/) { > + $used{"$1"} = 1; > + print "Used $1\n"; > + } else { > + print "Not used ", $iface->address, "\n"; > + } > + } > + > + for (my $i = 1; $i < 255; $i++) { > + if (!exists $used{"$i"}) { > + $index = $i; > + last; > + } > + } > + > + return () unless defined $index; > + > + return ( > + address => "192.168.$index.1", > + netmask => "255.255.255.0", > + dhcpstart => "192.168.$index.100", > + dhcpend => "192.168.$index.200" > + ); > +} > +

> diff --git a/scripts/networks/300-guest-network-isolated.t b/scripts/networks/300-guest-network-isolated.t > new file mode 100644 > index 0000000..487e864 > --- /dev/null > +++ b/scripts/networks/300-guest-network-isolated.t > @@ -0,0 +1,82 @@ > +# -*- perl -*- > +# > +# Copyright (C) 2018 Red Hat, Inc. > +# > +# This program is free software; You can redistribute it and/or modify > +# it under the GNU General Public License as published by the Free > +# Software Foundation; either version 2, or (at your option) any > +# later version > +# > +# The file "LICENSE" distributed along with this file provides full > +# details of the terms and conditions > +# > + > +=pod > + > +=head1 NAME > + > +network/300-guest-network-isolated.t - guest connect to isolated network > + > +=head1 DESCRIPTION > + > +This test case validates that a guest is connected to an isolated > +virtual network > + > +=cut > + > +use strict; > +use warnings; > + > +use Test::More tests => 4; > + > +use Sys::Virt::TCK; > + > +my $tck = Sys::Virt::TCK->new(); > +my $conn = eval { $tck->setup(); }; > +BAIL_OUT "failed to setup test harness: $@" if $@; > +END { $tck->cleanup if $tck; } > + > +my %subnet = Sys::Virt::TCK->find_free_ipv4_subnet(); > + > +SKIP: { > + skip "No available IPv4 subnet", 4 unless defined $subnet{address}; > + > + my $b = Sys::Virt::TCK::NetworkBuilder->new(name => "tck"); > + $b->bridge("tck"); > + $b->ipaddr($subnet{address}, $subnet{netmask}); > + $b->dhcp_range($subnet{dhcpstart}, $subnet{dhcpend}); > + my $xml = $b->as_xml(); > + > + diag "Creating a new transient network"; > + diag $xml; > + my $net; > + ok_network(sub { $net = $conn->create_network($xml) }, "created transient network object"); In the past (so long ago I don't remember the details) there has been different behavior between transient and persistent networks and domains. If we wanted to be pedantic, we could test it both/all ways. It would be overkill to do all the combinations for *every test* though. Since there are multiple tests here doing almost the same thing, we could make one of them with a persistent network/transient domain, one with transient/persistent, etc (or maybe just be sure there's one of each type, not necessarily all combinations)

> diff --git a/scripts/networks/380-guest-network-hostdev.t b/scripts/networks/380-guest-network-hostdev.t > new file mode 100644 > index 0000000..63fa0c9 > --- /dev/null > +++ b/scripts/networks/380-guest-network-hostdev.t > @@ -0,0 +1,82 @@ > +# -*- perl -*- > +# > +# Copyright (C) 2018 Red Hat, Inc. > +# > +# This program is free software; You can redistribute it and/or modify > +# it under the GNU General Public License as published by the Free > +# Software Foundation; either version 2, or (at your option) any > +# later version > +# > +# The file "LICENSE" distributed along with this file provides full > +# details of the terms and conditions > +# > + > +=pod > + > +=head1 NAME > + > +network/380-guest-network-hostdev.t - guest connect to a hostdev network > + > +=head1 DESCRIPTION > + > +This test case validates that a guest is connected to a hostdev > +virtual network > + > +=cut > + > +use strict; > +use warnings; > + > +use Test::More tests => 4; > + > +use Sys::Virt::TCK; > + > +my $tck = Sys::Virt::TCK->new(); > +my $conn = eval { $tck->setup(); }; > +BAIL_OUT "failed to setup test harness: $@" if $@; > +END { $tck->cleanup if $tck; } > + > +my ($domain, $bus, $slot, $func) = $tck->get_host_pci_device(); > + > +SKIP: { > + skip "No available PCI device", 4 unless defined $domain; > + > + my $b = Sys::Virt::TCK::NetworkBuilder->new(name => "tck"); > + $b->bridge("tck"); There shouldn't be a bridge device for a hostdev network. Removing the line above is sufficient. > + $b->forward(mode => "hostdev"); This should also have 'managed => "yes"' for maximum ease of use (you have to go to the trouble of blacklisting things or explicitly running nodedev-detach to get the VFs pre-detached from the hostside VF net driver).

> + $b->host_devices([$domain, $bus, $slot, $func]); > + my $xml = $b->as_xml(); > + > + diag "Creating a new transient network"; > + diag $xml; > + my $net; > + ok_network(sub { $net = $conn->create_network($xml) }, "created transient network object"); > + > + $b = $tck->generic_domain(name => "tck"); > + $b->interface(type => "network", > + source => "tck", > + model => "virtio", > + mac => "52:54:00:11:11:11"); Sigh. (That has nothing to do with this bit of code. I just happened to type it in for some reason while this review email was up on my screen. Probably it was meant for another window, I'm not sure. I was just kind of amused to find it sitting here as I scanned through the message before sending it, so thought I'd leave it in for comic relief). > + $xml = $b->as_xml(); > + > + diag "Creating a new transient domain"; > + diag $xml; > + my $dom; > + ok_domain(sub { $dom = $conn->create_domain($xml) }, "created transient domain object"); > + > + diag "Destroying the transient guest"; > + $dom->destroy; > + > + diag "Checking that transient domain has gone away"; > + ok_error(sub { $conn->get_domain_by_name("tck") }, "NO_DOMAIN error raised from missing domain", > + Sys::Virt::Error::ERR_NO_DOMAIN); > + > + diag "Destroying the transient network"; > + $net->destroy; > + > + diag "Checking that transient network has gone away"; > + ok_error(sub { $conn->get_network_by_name("tck") }, "NO_network error raised from missing network", > + Sys::Virt::Error::ERR_NO_NETWORK); > +} > + > +# end > diff --git a/t/080-network-builder.t b/t/080-network-builder.t > index ec2b70c..a99bc63 100644 > --- a/t/080-network-builder.t > +++ b/t/080-network-builder.t > @@ -23,7 +23,7 @@ my $xml = <<EOF; > <network> > <name>tck</name> > <bridge name="virbr0" /> > - <forward dev="eth0" /> > + <forward dev="eth0"></forward> Is the above some odd requirement of the way things are added into existing XML?

> <ip address="192.168.100.1" netmask="255.255.255.0"> > <dhcp> > <range start="192.168.100.50" end="192.168.100.70" /> Assuming the following: * add new test files to MANIFEST * remove $b->bridge("tck") from hostdev test * add 'managed => "yes"' to the $b->forward(...) in hostdev test Tested-by: Laine Stump <laine(a)laine.org&gt; Reviewed-by: Laine Stump <laine(a)laine.org&gt; (Yes, I actually added hostdevs and nics to the config file and ran *all* the tests :-)

pub 2048R/0xC4F5579A8DA0E3E5 2018-11-01 Laine Stump <laine(a)laine.org&gt; sub 2048R/0x92B8139F103E1242 2018-11-01 [expires: 2019-11-01]