[libvirt] [libvirt-tck PATCH] TCK.pm: Define libvirt VMs with an RNG device

[Erik Skultety]

On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
> On 9/23/19 1:27 PM, Erik Skultety wrote:
> > The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to
> > the test VM. However, because the domain definition passed to libvirt
> > lacks an RNG device, the SSH server isn't started inside the guest
> > (even though that is the default on virt-builder images) and therefore:
> >
> > "ssh: connect to host 192.168.122.227 port 22: Connection refused"
>
>
> Strange that this has never happened to me. Is it perhaps because I'm using
> a very old cached image from virt-builder, and had started it up manually at
> some time in the past (thus giving it a long enough time to generate the
> keys, which are now stored away for posterity)?
>
>
> >
> > is returned which in turn makes a bunch of sub tests fail because the
> > very basic premise - a working SSH connection - is false.
> > This patch makes sure a virtio RNG is contained in the XML definition,
> > thus causing the SSH server to be started successfully, thus allowing
> > all the subtests to pass.
> > ---
> >   lib/Sys/Virt/TCK.pm               |  4 ++++
> >   lib/Sys/Virt/TCK/DomainBuilder.pm | 21 +++++++++++++++++++++
> >   2 files changed, 25 insertions(+)
> >
> > diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
> > index 389d5cc..3ea06cc 100644
> > --- a/lib/Sys/Virt/TCK.pm
> > +++ b/lib/Sys/Virt/TCK.pm
> > @@ -807,6 +807,8 @@ sub generic_machine_domain {
> >           $b->disk(src => $config{root},
> >                    dst => $config{dev},
> >                    type => "file");
> > +        $b->rng(backend_model => "random",
> > +                backend => "/dev/urandom");
> >           if ($config{firstboot}) {
> >               print "# Running the first boot\n";
> > @@ -865,6 +867,8 @@ sub generic_machine_domain {
> >                    dst => $config{dev},
> >                    type => "file",
> >                    shareable => $shareddisk);
> > +        $b->rng(backend_model => "random",
> > +                backend => "/dev/urandom");
> >           return $b;
> >       }
> >   }
> > diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm
> > index 45336b5..be8708f 100644
> > --- a/lib/Sys/Virt/TCK/DomainBuilder.pm
> > +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm
> > @@ -49,6 +49,7 @@ sub new {
> >           graphics => [],
> >           hostdevs => [],
> >           seclabel => {},
> > +        rng => {},
> >       };
> >       bless $self, $class;
> > @@ -328,6 +329,19 @@ sub seclabel {
> >       return $self;
> >   }
> > +sub rng {
> > +    my $self = shift;
> > +    my %params = @_;
> > +
> > +    die "backend model parameter is required" unless $params{backend_model};
> > +    die "backend parameter is required" unless $params{backend};
> > +
> > +    $self->{rng} = \%params;
> > +    $self->{rng}->{model} = "virtio" unless defined $self->{rng}->{model};
> > +
> > +    return $self;
> > +}
> > +
> >   sub as_xml {
> >       my $self = shift;
> > @@ -504,6 +518,13 @@ sub as_xml {
> >           $w->endTag("graphics");
> >       }
> >       $w->emptyTag("console", type => "pty");
> > +
> > +    $w->startTag("rng",
> > +                 model => $self->{rng}->{model});
> > +    $w->dataElement("backend", $self->{rng}->{backend},
> > +                    model => $self->{rng}->{backend_model});
> > +    $w->endTag("rng");
> > +
>
>
> Because the <rng> element is added unconditionally, t/070-domain-builder.t
> fails when you run "./prepare-release.sh"

Ah, missed that one, good catch :).

>
>
> I fixed this locally by making that bit of code conditional on having
> backen_model set (see the diff at the end). You could also modify the
> failing test to explicitly add an <rng> element, but I think we still should
> put it in conditionally, in case someone needs to *not* have it (e.g., there
> are xen tests run with libvirt-tck, and they might not like it if the domain
> had an <rng> element; I can't say that for sure, because I'm not setup to
> test Xen, and am too lazy to even look at the code and try to figure it out
> by hand :-)).
>
>
> From my POV, if you apply the diff at the end of this message, then:
>
>
> Reviewed-by: Laine Stump <laine at laine.org>
>
>
> You may or may not choose to add an rng to the domain-builder test (and may
> or may not get a complaint the next time someone runs a Xen test :-)
>
>
> diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm
> b/lib/Sys/Virt/TCK/DomainBuilder.pm
> index be8708f..9e0c49c 100644
> --- a/lib/Sys/Virt/TCK/DomainBuilder.pm
> +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm
> @@ -519,11 +519,13 @@ sub as_xml {
>      }
>      $w->emptyTag("console", type => "pty");
>
> -    $w->startTag("rng",
> -                 model => $self->{rng}->{model});
> -    $w->dataElement("backend", $self->{rng}->{backend},
> -                    model => $self->{rng}->{backend_model});
> -    $w->endTag("rng");
> +    if ($self->{rng}->{backend_model}) {

Hmm, wouldn't it be actually better to test for an empty hash instead? IOW
    if (%{$self->{rng}}) {
        ...

sounds a bit more generic to me rather than test presence of a specific
attribute within the hash and it seems to be working in context of both the
nwfilter test and the domain builder test with an Xen XML in prepare_release.sh
If you're okay with that kind of adjustment instead, I'll proceed with merging
the patch.

Thanks for review,
Erik

> +        $w->startTag("rng",
> +                     model => $self->{rng}->{model});
> +        $w->dataElement("backend", $self->{rng}->{backend},
> +                        model => $self->{rng}->{backend_model});
> +        $w->endTag("rng");
> +    }
>
>      $w->endTag("devices");
>      if ($self->{seclabel}->{model}) {
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list