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

2018-11-12 Thread Daniel P . Berrangé
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é 
> > ---
> >  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 000..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 

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

2018-11-02 Thread Laine Stump
On 11/2/18 11:52 AM, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé 
> ---
>  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? :-)


(Same comment for adding the proper iptables rules, but we need to
consider what to do about that, because what gets added will in the
*very near* future be different depending on version of libvirt and/or
whether or not firewalld is using nftables).


Those last two shouldn't keep you from pushing the patch though (see my
list at the end for the few things that do need to be changed before
pushing)


>
> diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
> index 04244bd..9457836 100644
> --- a/lib/Sys/Virt/TCK.pm
> +++ b/lib/Sys/Virt/TCK.pm
> @@ -29,6 +29,7 @@ use File::Path qw(mkpath);
>  use File::Spec::Functions qw(catfile catdir rootdir);
>  use Cwd qw(cwd);
>  use LWP::UserAgent;
> +use IO::Interface::Simple;
>  use IO::Uncompress::Gunzip qw(gunzip);
>  use IO::Uncompress::Bunzip2 qw(bunzip2);
>  use XML::XPath;
> @@ -1285,6 +1286,38 @@ sub get_ip_from_leases{
>  }
>  
>  
> +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"
> + );
> +}
> +
>  sub shutdown_vm_gracefully {
>  my $dom = shift;
>  
> diff --git a/lib/Sys/Virt/TCK/NetworkBuilder.pm 
> b/lib/Sys/Virt/TCK/NetworkBuilder.pm
> index 09ca6b7..ad0cab8 100644
> --- a/lib/Sys/Virt/TCK/NetworkBuilder.pm
> +++ b/lib/Sys/Virt/TCK/NetworkBuilder.pm
> @@ -61,6 +61,22 @@ sub forward {
>  return $self;
>  }
>  
> +sub interfaces {
> +my $self = shift;
> +
> +$self->{interfaces} = [@_];
> +
> +return $self;
> +}
> +
> +sub host_devices {
> +my $self = shift;
> +
> +$self->{host_devices} = [@_];
> +
> +return $self;
> +}
> +
>  sub ipaddr {
>  my $self = shift;
>  my $address = shift;
> @@ -98,8 +114,21 @@ sub as_xml {
>  $w->emptyTag("bridge", %{$self->{bridge}})
>  if $self->{bridge};
>  
> -$w->emptyTag("forward", %{$self->{forward}})
> -if 

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

2018-11-02 Thread Daniel P . Berrangé
Signed-off-by: Daniel P. Berrangé 
---
 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

diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index 04244bd..9457836 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -29,6 +29,7 @@ use File::Path qw(mkpath);
 use File::Spec::Functions qw(catfile catdir rootdir);
 use Cwd qw(cwd);
 use LWP::UserAgent;
+use IO::Interface::Simple;
 use IO::Uncompress::Gunzip qw(gunzip);
 use IO::Uncompress::Bunzip2 qw(bunzip2);
 use XML::XPath;
@@ -1285,6 +1286,38 @@ sub get_ip_from_leases{
 }
 
 
+sub find_free_ipv4_subnet {
+my $index;
+
+my %used;
+
+foreach my $iface (IO::Interface::Simple->interfaces()) {
+   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"
+   );
+}
+
 sub shutdown_vm_gracefully {
 my $dom = shift;
 
diff --git a/lib/Sys/Virt/TCK/NetworkBuilder.pm 
b/lib/Sys/Virt/TCK/NetworkBuilder.pm
index 09ca6b7..ad0cab8 100644
--- a/lib/Sys/Virt/TCK/NetworkBuilder.pm
+++ b/lib/Sys/Virt/TCK/NetworkBuilder.pm
@@ -61,6 +61,22 @@ sub forward {
 return $self;
 }
 
+sub interfaces {
+my $self = shift;
+
+$self->{interfaces} = [@_];
+
+return $self;
+}
+
+sub host_devices {
+my $self = shift;
+
+$self->{host_devices} = [@_];
+
+return $self;
+}
+
 sub ipaddr {
 my $self = shift;
 my $address = shift;
@@ -98,8 +114,21 @@ sub as_xml {
 $w->emptyTag("bridge", %{$self->{bridge}})
 if $self->{bridge};
 
-$w->emptyTag("forward", %{$self->{forward}})
-if exists $self->{forward};
+if (exists $self->{forward}) {
+   $w->startTag("forward", %{$self->{forward}});
+   foreach (@{$self->{interfaces}}) {
+   $w->emptyTag("interface", dev => $_);
+   }
+   foreach (@{$self->{host_devices}}) {
+   $w->emptyTag("address",
+type => "pci",
+domain => $_->[0],
+bus => $_->[1],
+slot => $_->[2],
+function => $_->[3]);
+   }
+   $w->endTag("forward");
+}
 
 if ($self->{ipaddr}) {
 $w->startTag("ip",
diff --git a/scripts/networks/300-guest-network-isolated.t 
b/scripts/networks/300-guest-network-isolated.t
new file mode 100644
index 000..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