[Libguestfs] [PATCH v2 2/2] v2v: -o rhv-upload: Use Unix domain socket to access imageio (RHBZ#1588088).

[Richard W.M. Jones]

On Tue, Jun 26, 2018 at 02:28:15AM +0300, Nir Soffer wrote:
> On Fri, Jun 22, 2018 at 2:59 PM Richard W.M. Jones <rjones at redhat.com>
> wrote:
> 
> > In the case where virt-v2v runs on the same server as the imageio
> > daemon that we are talking to, it may be possible to optimize access
> > using a Unix domain socket.
> >
> > This is only an optimization.  If it fails or if we're not running on
> > the same server it will fall back to the usual HTTPS over TCP
> > connection.
> >
> 
> I wonder how this can happen. If we do:
> 
> - get current host hardware id
> - ask engine to start transfer on this host
> 
> Then there is no way that the transfer will not run on the current host,
> and we can use unix socket if imageio supports it.

There are other use cases, primarily running virt-v2v from another
host on the command line.

> > +    # Get the current host.  If it fails, don't worry.
> > +    host = None
> > +    try:
> > +        with builtin_open("/etc/vdsm/vdsm.id") as f:
> >
> 
> Why use builtin_open() instead of open() or io.open()?

open() is a top-level function in the file (and cannot be changed
because the nbdkit python plugin calls it by name).  Therefore to call
"real" open we have to use the builtin_open alias imported at the top
of the file.

> 
> > +            vdsm_id = f.readline().strip()
> >
> 
> debug log with the host hardware id would be nice here.

OK I will add that.

> > +
> > +        hosts_service = connection.system_service().hosts_service()
> > +        hosts = hosts_service.list(
> > +            search="hw_id=%s" % vdsm_id,
> > +            case_sensitive=False,
> > +        )
> > +        if len(hosts) > 0:
> > +            host = hosts[0]
> > +            debug("host.id = %r" % host.id)
> > +        else:
> > +            debug("could not retrieve host with hw_id=%s" % vdsm_id)
> > +    except:
> > +        pass
> >
> 
> This will make it vey hard to debug slow uploads. I agree that optimization
> can be skipped on errors, but we must log a detailed error message
> before we fallback to https.
> 
> Also bare except should be used only in one case, when you do:
> 
>     try:
>         ...
>     except:
>         cleanup
>         raise
> 
> If you don't raise, you should use Exception. Otherwise you will swallow
> KeyboardInterrupt, SystemExit, and GeneratorExit and any other
> user defined exceptions inheriting from BaseException.
> 
> Also it will be easier to maintain this if this will be in a separate
> function
> like:
> 
> def find_host():
>     read hardware id or return None...
>     get hosts from engine or return None...
>     return types.Host(host.id)

OK I think I understand this and the rest.  I'll have a go at it in
the second version.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html