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@xxxxxxxxx>
>
>
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
