On Fri, Nov 02, 2018 at 04:23:02PM -0400, Laine Stump wrote:
> On 11/2/18 11:52 AM, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > 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.
Hah, forgot that :-)
> 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? :-)
I was really only interested in testing the code in libvirtd that
connects guests to virtual networks, to exercise code paths i'm
refactoring right now.
I'll leave testing of guest traffic as an exercise for future contribs.
> > +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 :-)
We could allow for a config file parameter to override this for
people how have that scenario.
> > + 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)
For the sake of testing setup of guest NICs with networks, I don't
think there's significant difference.
> > 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).
Opps, these comments remind me that I never actually tested
the hostdev scenario.
>
>
> > + $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?
Yeah, the Perl XML generator doesn't omit the tag pair
when there's no children.
>
>
> > <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@xxxxxxxxx>
>
> Reviewed-by: Laine Stump <laine@xxxxxxxxx>
>
>
> (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@xxxxxxxxx>
> sub 2048R/0x92B8139F103E1242 2018-11-01 [expires: 2019-11-01]
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
