Re: [PATCH v2 0/5] qemu: Support rbd namespace attribute

[Han Han]

ping

On Tue, Apr 21, 2020 at 8:11 PM Han Han  wrote:

> Change from v1:
> - Add rbd namespace member to aarch64 capability replies. Add rbd
>   namespace capability test for aarch64.
> - Provide more details in news xml
>
> v1: https://www.redhat.com/archives/libvir-list/2020-April/msg00388.html
> git repo: https://github.com/qiankehan/libvirt/tree/rbd-namespace-v2
>
> Han Han (5):
>   qemu_capabilities: Add QEMU_CAPS_RBD_NAMESPACE
>   conf: Support to parse rbd namespace attribute
>   qemu: Implement rbd namespace attribute
>   doc: rng schemas and html doc for rbd namespace attribute
>   news: qemu: Support rbd namespace
>
>  docs/formatdomain.html.in |  5 +++
>  docs/news.xml | 10 +
>  docs/schemas/domaincommon.rng |  3 ++
>  src/conf/domain_conf.c|  4 ++
>  src/qemu/qemu_block.c |  1 +
>  src/qemu/qemu_capabilities.c  |  2 +
>  src/qemu/qemu_capabilities.h  |  1 +
>  src/qemu/qemu_command.c   | 14 ++-
>  src/util/virstoragefile.h |  1 +
>  .../caps_5.0.0.aarch64.replies|  5 +++
>  .../caps_5.0.0.aarch64.xml|  1 +
>  .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |  1 +
>  .../caps_5.0.0.x86_64.xml |  1 +
>  ...k-network-rbd-namespace.x86_64-latest.args | 41 +++
>  .../disk-network-rbd-namespace.xml| 33 +++
>  tests/qemuxml2argvtest.c  |  1 +
>  ...sk-network-rbd-namespace.x86_64-latest.xml | 41 +++
>  tests/qemuxml2xmltest.c   |  1 +
>  18 files changed, 165 insertions(+), 1 deletion(-)
>  create mode 100644
> tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
>  create mode 100644
> tests/qemuxml2xmloutdata/disk-network-rbd-namespace.x86_64-latest.xml
>
> --
> 2.25.0
>
>

-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333

Live migration garp

[Ignazio Cassano]

Hello All, I am facing issues under openstack rocky with last libvirt
available on centos 7.
There is an open discussione here:

https://bugs.launchpad.net/neutron/+bug/1866139

About sending garp when an instance live migrate.
What must send garp ?

The problem is this: when live migrate a vm from a node to another it stops
to responds to ping requests. It starts to respond only when it send a
network request for example when its chrony daemon pools a server.
Anyone could help me, please?
Best Regards
Ignazio

[PATCH] docs: Describe protected virtualization guest setup

[Boris Fiuczynski]

From: Viktor Mihajlovski 

Protected virtualization/IBM Secure Execution for Linux protects
guest memory and state from the host.

Add some basic information about technology and a brief guide
on setting up secure guests with libvirt.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Paulo de Rezende Pinatti 
---
 docs/kbase.html.in  |   3 +
 docs/kbase/protected_virtualization.rst | 188 
 2 files changed, 191 insertions(+)
 create mode 100644 docs/kbase/protected_virtualization.rst

diff --git a/docs/kbase.html.in b/docs/kbase.html.in
index c586e0f676..05a3239224 100644
--- a/docs/kbase.html.in
+++ b/docs/kbase.html.in
@@ -14,6 +14,9 @@
 Secure usage
 Secure usage of the libvirt APIs
 
+Protected 
virtualization
+Running secure guests with IBM Secure Execution
+
 Launch security
 Securely launching VMs with AMD SEV
 
diff --git a/docs/kbase/protected_virtualization.rst 
b/docs/kbase/protected_virtualization.rst
new file mode 100644
index 00..48f2add14e
--- /dev/null
+++ b/docs/kbase/protected_virtualization.rst
@@ -0,0 +1,188 @@
+
+Protected Virtualization
+
+
+.. contents::
+
+Overview
+
+
+Protected virtualization, also known as IBM Secure Execution is a
+hardware-based privacy protection technology for s390x (IBM Z).
+It allows to execute virtual machines such that the host system
+has no access to a VM's state and memory contents.
+
+Unlike other similar technologies, the memory of a running guest
+is not encrypted but protected by hardware access controls, which
+may only be manipulated by trusted system firmware, called
+ultravisor.
+
+For the cases where the host needs access to guest memory (e.g. for
+paging), it can request pages to be exported to it. The exported page
+will be encrypted with a unique key for the running guest by the
+ultravisor. The ultravisor also computes an integrity value for
+the page, and stores it in a special table, together with the page
+index and a counter. This way it can verify the integrity of
+the page content upon re-import into the guest.
+
+In other cases it may be necessary for a guest to grant the host access
+to dedicated memory regions (e.g. for I/O). The guest can request
+that the ultravisor removes the memory protection from individual
+pages, so that they can be shared with the host. Likewise, the
+guest can undo the sharing.
+
+A secure guest will initially start in a regular non-protected VM.
+The start-up is controlled by a small bootstrap program loaded
+into memory together with encrypted operating system components and
+a control structure (the PV header).
+The operating system components (e.g. Linux kernel, initial RAM
+file system, kernel parameters) are encrypted and integrity
+protected. The component encryption keys and integrity values are
+stored in the PV header.
+The PV header is wrapped with a public key belonging to a specific
+system (in fact it can be wrapped with multiple such keys). The
+matching private key is only accessible by trusted hardware and
+firmware in that specific system.
+Consequently, such a secure guest boot image can only be run on the
+systems it has been prepared for. Its contents can't be decrypted
+without access to the private key and it can't be modified as
+it is integrity protected.
+
+Host Requirements
+=
+
+IBM Secure Execution for Linux has some hardware and firmware
+requirements. The system hardware must be an IBM z15 (or newer),
+or an IBM LinuxONE III (or newer).
+
+It is also necessary that the IBM Secure Execution feature is
+enabled for that system. Check for facility '158', e.g.
+
+::
+
+   $ grep facilities /proc/cpuinfo | grep 158
+
+The kernel must include the protected virtualization support
+which can be verified by checking for the presence of directory
+``/sys/firmware/uv``. It will only be present when both the
+hardware and the kernel support are available.
+
+Finally, the host operating system must donate some memory to
+the ultravisor needed to store memory security information.
+This is achieved by specifying the following kernel command
+line parameter to the host boot configuration
+
+::
+
+   prot_virt=1
+
+
+Guest Requirements
+==
+
+Guest Boot
+--
+
+To start a guest in protected virtualization secure mode, the
+boot image must have been prepared first with the program
+``genprotimg`` using the correct public key for this host.
+``genprotimg`` is part of the package ``s390-tools``, or
+``s390-utils``, depending on the Linux distribution being used.
+It can also be found at
+``_
+
+The guests have to be configured to use the host CPU model, which
+must contain the ``unpack`` facility indicating ultravisor guest support.
+
+With the following command it's possible to check whether the host

Re: Races / crashes in shutdown of libvirtd daemon

[Nikolay Shirokovskiy]

On 27.04.2020 18:54, Daniel P. Berrangé wrote:
> We got a new BZ filed about a libvirtd crash in shutdown
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1828207
> 
> We can see from the stack trace that the "interface" driver is in
> the middle of servicing an RPC call for virConnectListAllInterfaces()
> 
> Meanwhile the libvirtd daemon is doing virObjectUnref(dmn) on the
> virNetDaemonPtr object.
> 
> The fact that it is doing this unref, means that it must have already
> call virStateCleanup(), given the code sequence:
> 
> 
> /* Run event loop. */
> virNetDaemonRun(dmn);
> 
> ret = 0;
> 
> virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_SHUTDOWN,
> 0, "shutdown", NULL, NULL);
> 
>  cleanup:
> /* Keep cleanup order in inverse order of startup */
> virNetDaemonClose(dmn);
> 
> virNetlinkEventServiceStopAll();
> 
> if (driversInitialized) {
> /* NB: Possible issue with timing window between driversInitialized
>  * setting if virNetlinkEventServerStart fails */
> driversInitialized = false;
> virStateCleanup();
> }
> 
> virObjectUnref(adminProgram);
> virObjectUnref(srvAdm);
> virObjectUnref(qemuProgram);
> virObjectUnref(lxcProgram);
> virObjectUnref(remoteProgram);
> virObjectUnref(srv);
> virObjectUnref(dmn);
> 
> 
> Unless I'm missing something non-obvious, this cleanup code path is
> inherantly broken & racy.   When virNetDaemonRun() returns the RPC
> worker threads are all still active. They are all liable to still
> be executing RPC calls, which means any of the drivers may be in
> use. So calling  virStateCleanup() is an inherantly dangerous
> thing to do. There is the further complication that once we have
> exitted the main loop we may prevent the RPC calls from ever
> completing, as they may be waiting on an event to be dispatched.
> 
> I know we're had various patch proposals in the past to improve the
> robustness of shutdown cleanup but I can't remember the outcome of the
> reviews. Hopefully people involved in those threads can jump in here...

Hi, all!

Yeah I and John Ferlan attempted to address the issue in the past.

The last reference I found is [1]. One can dig down the history
of the issue thru the links inside.

IIRC the latest approach was similar to what you suggested below namely
run event loop for a while after quit is seen in order to let
RPC worker threads waiting for event to finish.

[1] https://www.redhat.com/archives/libvir-list/2018-July/msg00382.html

> 
> 
> IMHO the key problem here is the virNetDeamonRun() method which just
> looks at the "quit" flag and immediately returns if it is set.
> This needs to be changed so that when it sees quit == true, it takes
> the following actions
> 
>   1. Call virNetDaemonClose() to drop all RPC clients and thus prevent
>  new RPC calls arriving
>   2. Flush any RPC calls which are queued but not yet assigned to a
>  worker thread
>   3. Tell worker threads to exit after finishing their current job
>   4. Wait for all worker threads to exit
>   5. Now virNetDaemonRun may return
> 
> At this point we can call virStateCleanup and the various other
> things, as we know no drivers are still active in RPC calls.
> 
> Having said that, there could be background threads in the the
> drivers which are doing work that uses the event loop thread.
> 
> So we probably need a virStateClose() method that we call from
> virNetDaemonRun, *after* all worker threads are gone, which would
> cleanup any background threads while the event loop is still
> running.

I guess RPC worker threads may need some signal to finish in time too.
For example in case of migration.

Nikolay

> 
> 
> The issue is that step 4 above ("Wait for all worker threads to exit")
> may take too long, or indeed never complete.  To deal with this, it
> will need a timeout. In the remote_daemon.c cleanup code path, if
> there are still worker threads present, then we need to skip all
> cleanup and simply call _exit(0) to terminate the process with no
> attempt at cleanup, since it would be unsafe to try anything else.
> 
> Regards,
> Daniel
> 

Re: [PATCH 1/3] Improve blockpull man entry

[Eric Blake]

On 4/28/20 9:19 AM, Sebastian Mitterle wrote:

So bandwidth indeed is a positional argument. Since all of the manpage
uses the syntax we've had here originally fixing just this place would
be wrong. The only fix that should be done here is to group the --bytes
flag under bandwidth as specifying --bytes is mandatory.


I think there's misunderstanding:

# virsh blockpull fedora vda vda[1]


We should really encourage users to properly quote their command line to 
avoid unintentional globbing:


# virsh blockpull fedora vda "vda[1]"

(Otherwise, if I 'touch vda1', the unquoted command will see the 
argument 'vda1')



error: Scaled numeric value 'vda[1]' for <--bandwidth> option is
malformed or out of range

# virsh blockpull fedora vda 1024 vda[1]
Block Pull started

I'll change to [bandwidth [--bytes] [base]]




--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2] Improve blockpull man entry

[Sebastian Mitterle]

1. Fix usage of bandwidth, base arguments.
2. Explain valid arguments for `base`.
3. Move explanation for `--keep-relative` to end considering it's
   not a very frequent use case.
4. Add reference to documentation for relative paths in backing chains.

Signed-off-by: Sebastian Mitterle 
---
 docs/manpages/virsh.rst | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index dc404ddfe8..71bd968fab 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -1345,7 +1345,7 @@ blockpull
 
 .. code-block::
 
-   blockpull domain path [bandwidth] [--bytes] [base]
+   blockpull domain path [bandwidth [--bytes] [base]]
   [--wait [--verbose] [--timeout seconds] [--async]]
   [--keep-relative]
 
@@ -1356,6 +1356,12 @@ the new backing file and only the intermediate portion 
of the chain is
 pulled.  Once all requested data from the backing image chain has been
 pulled, the disk no longer depends on that portion of the backing chain.
 
+*base* can be specified in two ways: either as indexed target name 'name[i]'
+where 'name' corresponds to the disk target name () and
+'i' corresponds to the 'index' of the ''; or as the file name
+of the backing file ().
+
+
 By default, this command returns as soon as possible, and data for
 the entire disk is pulled in the background; the progress of the
 operation can be checked with ``blockjob``.  However, if *--wait* is
@@ -1367,16 +1373,19 @@ is triggered, *--async* will return control to the user 
as fast as
 possible, otherwise the command may continue to block a little while
 longer until the job is done cleaning up.
 
-Using the *--keep-relative* flag will keep the backing chain names
-relative.
-
 *path* specifies fully-qualified path of the disk; it corresponds
 to a unique target name () or source file () for one of the disk devices attached to *domain* (see
 also ``domblklist`` for listing these names).
+
 *bandwidth* specifies copying bandwidth limit in MiB/s. For further information
 on the *bandwidth* argument see the corresponding section for the ``blockjob``
-command.
+command. Using *--bytes* flag indicates the value in *bandwidth* is given in
+bytes.
+
+Using the *--keep-relative* flag will keep the backing chain names
+relative (details on `https://www.libvirt.org/kbase/backing_chains.html
+`__).
 
 
 blockresize
-- 
2.25.2

Re: [PATCH 1/3] Improve blockpull man entry

[Sebastian Mitterle]

> So bandwidth indeed is a positional argument. Since all of the manpage
> uses the syntax we've had here originally fixing just this place would
> be wrong. The only fix that should be done here is to group the --bytes
> flag under bandwidth as specifying --bytes is mandatory.

I think there's misunderstanding:

# virsh blockpull fedora vda vda[1]
error: Scaled numeric value 'vda[1]' for <--bandwidth> option is
malformed or out of range

# virsh blockpull fedora vda 1024 vda[1]
Block Pull started

I'll change to [bandwidth [--bytes] [base]]



On Fri, Apr 24, 2020 at 9:35 AM Peter Krempa  wrote:
>
> On Wed, Apr 15, 2020 at 11:34:04 +, Sebastian Mitterle wrote:
> > 1. Mention usage of `--base` and `--bandwidth` and fix cmd syntax.
> > 2. Explain valid arguments for `base`.
> > 3. Move explanation for `--keep-relative` to end considering it
> >less frequent use case because libvirt doesn't create relative
> >backing chain names.
> > 4. Add reference to documentation for relative paths in backing chains.
> >
> > Signed-off-by: Sebastian Mitterle 
> > ---
> >  docs/manpages/virsh.rst | 19 +--
> >  1 file changed, 13 insertions(+), 6 deletions(-)
>
> Given new information about the virsh argument parser I've figured out
> I'll re-review this patch:
>
> > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> > index dc404ddfe8..27ecc53d56 100644
> > --- a/docs/manpages/virsh.rst
> > +++ b/docs/manpages/virsh.rst
> > @@ -1345,7 +1345,7 @@ blockpull
> >
> >  .. code-block::
> >
> > -   blockpull domain path [bandwidth] [--bytes] [base]
> > +   blockpull domain path { [bandwidth [--bytes] [base]] | [--bandwidth 
> > [--bytes]] [--base] }
>
> So bandwidth indeed is a positional argument. Since all of the manpage
> uses the syntax we've had here originally fixing just this place would
> be wrong. The only fix that should be done here is to group the --bytes
> flag under bandwidth as specifying --bytes is mandatory.
>
> >[--wait [--verbose] [--timeout seconds] [--async]]
> >[--keep-relative]
> >
> > @@ -1353,7 +1353,7 @@ Populate a disk from its backing image chain. By 
> > default, this command
> >  flattens the entire chain; but if *base* is specified, containing the
> >  name of one of the backing files in the chain, then that file becomes
> >  the new backing file and only the intermediate portion of the chain is
> > -pulled.  Once all requested data from the backing image chain has been
> > +pulled. Once all requested data from the backing image chain has been
> >  pulled, the disk no longer depends on that portion of the backing chain.
>
> As mentioned previously some of the docs use two spaces between
> sentences. If it should be fixed then all of it should be fixed in one
> patch.
>
> Please drop this hunk.
>
> >
> >  By default, this command returns as soon as possible, and data for
> > @@ -1367,16 +1367,23 @@ is triggered, *--async* will return control to the 
> > user as fast as
> >  possible, otherwise the command may continue to block a little while
> >  longer until the job is done cleaning up.
> >
> > -Using the *--keep-relative* flag will keep the backing chain names
> > -relative.
> > -
> >  *path* specifies fully-qualified path of the disk; it corresponds
> >  to a unique target name () or source file ( >  file='name'/>) for one of the disk devices attached to *domain* (see
> >  also ``domblklist`` for listing these names).
> > +
> >  *bandwidth* specifies copying bandwidth limit in MiB/s. For further 
> > information
> >  on the *bandwidth* argument see the corresponding section for the 
> > ``blockjob``
> > -command.
> > +command. Using *--bytes* flag indicates the value in *bandwidth* is given 
> > in
> > +bytes.
> > +
> > +*base* specifies fully-qualified path of the backing file; it corresponds
> > +to a unique indexed target name 'name[i]' (..
> > +) or source file 'name' ().
>
> I'd move this hunk under the first paragraph in section about blockpull
> since it explains what 'base' actually is along with some rewording:
>
> Something like
>
> *base* can eithr be specified as indexed target name 'name[i]'
> where 'name corresponds to the disk target name ()
> and 'i' corresponds to the 'index' of the '', or the file
> name of backing file ().
> > +
> > +Using the *--keep-relative* flag will keep the backing chain names
> > +relative (details on `https://www.libvirt.org/kbase/backing_chains.html
> > +`__).
> >
> >
> >  blockresize
> > --
> > 2.25.2
> >
>


--
Best,
Sebastian

Re: [PATCH v5 0/4] introduction of migration_version attribute for VFIO live migration

[Dr. David Alan Gilbert]

* Yan Zhao (yan.y.z...@intel.com) wrote:
> On Mon, Apr 27, 2020 at 11:37:43PM +0800, Dr. David Alan Gilbert wrote:
> > * Yan Zhao (yan.y.z...@intel.com) wrote:
> > > On Sat, Apr 25, 2020 at 03:10:49AM +0800, Dr. David Alan Gilbert wrote:
> > > > * Yan Zhao (yan.y.z...@intel.com) wrote:
> > > > > On Tue, Apr 21, 2020 at 08:08:49PM +0800, Tian, Kevin wrote:
> > > > > > > From: Yan Zhao
> > > > > > > Sent: Tuesday, April 21, 2020 10:37 AM
> > > > > > > 
> > > > > > > On Tue, Apr 21, 2020 at 06:56:00AM +0800, Alex Williamson wrote:
> > > > > > > > On Sun, 19 Apr 2020 21:24:57 -0400
> > > > > > > > Yan Zhao  wrote:
> > > > > > > >
> > > > > > > > > On Fri, Apr 17, 2020 at 07:24:57PM +0800, Cornelia Huck wrote:
> > > > > > > > > > On Fri, 17 Apr 2020 05:52:02 -0400
> > > > > > > > > > Yan Zhao  wrote:
> > > > > > > > > >
> > > > > > > > > > > On Fri, Apr 17, 2020 at 04:44:50PM +0800, Cornelia Huck 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Mon, 13 Apr 2020 01:52:01 -0400
> > > > > > > > > > > > Yan Zhao  wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > This patchset introduces a migration_version 
> > > > > > > > > > > > > attribute under sysfs
> > > > > > > of VFIO
> > > > > > > > > > > > > Mediated devices.
> > > > > > > > > > > > >
> > > > > > > > > > > > > This migration_version attribute is used to check 
> > > > > > > > > > > > > migration
> > > > > > > compatibility
> > > > > > > > > > > > > between two mdev devices.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Currently, it has two locations:
> > > > > > > > > > > > > (1) under mdev_type node,
> > > > > > > > > > > > > which can be used even before device creation, 
> > > > > > > > > > > > > but only for
> > > > > > > mdev
> > > > > > > > > > > > > devices of the same mdev type.
> > > > > > > > > > > > > (2) under mdev device node,
> > > > > > > > > > > > > which can only be used after the mdev devices are 
> > > > > > > > > > > > > created, but
> > > > > > > the src
> > > > > > > > > > > > > and target mdev devices are not necessarily be of 
> > > > > > > > > > > > > the same
> > > > > > > mdev type
> > > > > > > > > > > > > (The second location is newly added in v5, in order 
> > > > > > > > > > > > > to keep
> > > > > > > consistent
> > > > > > > > > > > > > with the migration_version node for migratable 
> > > > > > > > > > > > > pass-though
> > > > > > > devices)
> > > > > > > > > > > >
> > > > > > > > > > > > What is the relationship between those two attributes?
> > > > > > > > > > > >
> > > > > > > > > > > (1) is for mdev devices specifically, and (2) is provided 
> > > > > > > > > > > to keep the
> > > > > > > same
> > > > > > > > > > > sysfs interface as with non-mdev cases. so (2) is for 
> > > > > > > > > > > both mdev
> > > > > > > devices and
> > > > > > > > > > > non-mdev devices.
> > > > > > > > > > >
> > > > > > > > > > > in future, if we enable vfio-pci vendor ops, (i.e. a 
> > > > > > > > > > > non-mdev device
> > > > > > > > > > > is binding to vfio-pci, but is able to register migration 
> > > > > > > > > > > region and do
> > > > > > > > > > > migration transactions from a vendor provided affiliate 
> > > > > > > > > > > driver),
> > > > > > > > > > > the vendor driver would export (2) directly, under device 
> > > > > > > > > > > node.
> > > > > > > > > > > It is not able to provide (1) as there're no mdev devices 
> > > > > > > > > > > involved.
> > > > > > > > > >
> > > > > > > > > > Ok, creating an alternate attribute for non-mdev devices 
> > > > > > > > > > makes sense.
> > > > > > > > > > However, wouldn't that rather be a case (3)? The change 
> > > > > > > > > > here only
> > > > > > > > > > refers to mdev devices.
> > > > > > > > > >
> > > > > > > > > as you pointed below, (3) and (2) serve the same purpose.
> > > > > > > > > and I think a possible usage is to migrate between a non-mdev 
> > > > > > > > > device and
> > > > > > > > > an mdev device. so I think it's better for them both to use 
> > > > > > > > > (2) rather
> > > > > > > > > than creating (3).
> > > > > > > >
> > > > > > > > An mdev type is meant to define a software compatible 
> > > > > > > > interface, so in
> > > > > > > > the case of mdev->mdev migration, doesn't migrating to a 
> > > > > > > > different type
> > > > > > > > fail the most basic of compatibility tests that we expect 
> > > > > > > > userspace to
> > > > > > > > perform?  IOW, if two mdev types are migration compatible, it 
> > > > > > > > seems a
> > > > > > > > prerequisite to that is that they provide the same software 
> > > > > > > > interface,
> > > > > > > > which means they should be the same mdev type.
> > > > > > > >
> > > > > > > > In the hybrid cases of mdev->phys or phys->mdev, how does a
> > > > > > > management
> > > > > > > > tool begin to even guess what might be compatible?  Are we 
> > > > > > > > expecting
> > > > > > > > libvirt to probe ever device with this attribute in the system? 
> > > > > > > >  Is
> > > > > > > > 

[libvirt-ci PATCH 4/4] lcitool: Store paths information as a dictionary

[Andrea Bolognani]

Ansible and Python both support actual dictionaries, so make use
of them in the inventory instead of having a bunch of randomly
named variables lumped together.

This commit is best viewed with 'git show -w'.

Signed-off-by: Andrea Bolognani 
---
 guests/host_vars/libvirt-centos-7/main.yml| 19 ---
 guests/host_vars/libvirt-centos-8/main.yml| 19 ---
 guests/host_vars/libvirt-debian-10/main.yml   | 19 ---
 guests/host_vars/libvirt-debian-9/main.yml| 19 ---
 guests/host_vars/libvirt-debian-sid/main.yml  | 19 ---
 guests/host_vars/libvirt-fedora-30/main.yml   | 19 ---
 guests/host_vars/libvirt-fedora-31/main.yml   | 19 ---
 .../host_vars/libvirt-fedora-rawhide/main.yml | 19 ---
 guests/host_vars/libvirt-freebsd-11/main.yml  | 19 ---
 guests/host_vars/libvirt-freebsd-12/main.yml  | 19 ---
 .../libvirt-freebsd-current/main.yml  | 19 ---
 .../host_vars/libvirt-opensuse-151/main.yml   | 19 ---
 guests/host_vars/libvirt-ubuntu-1604/main.yml | 19 ---
 guests/host_vars/libvirt-ubuntu-1804/main.yml | 19 ---
 guests/lcitool| 24 +--
 guests/playbooks/update/tasks/global.yml  |  4 ++--
 guests/playbooks/update/tasks/jenkins.yml |  2 +-
 guests/playbooks/update/tasks/users.yml   |  4 ++--
 guests/playbooks/update/templates/bashrc.j2   |  6 ++---
 .../update/templates/jenkins.service.j2   |  2 +-
 20 files changed, 161 insertions(+), 147 deletions(-)

diff --git a/guests/host_vars/libvirt-centos-7/main.yml 
b/guests/host_vars/libvirt-centos-7/main.yml
index 66cb113..d6efd00 100644
--- a/guests/host_vars/libvirt-centos-7/main.yml
+++ b/guests/host_vars/libvirt-centos-7/main.yml
@@ -23,15 +23,16 @@ packaging:
   format: rpm
   command: yum
 
-bash: /bin/bash
-cc: /usr/bin/gcc
-ccache: /usr/bin/ccache
-java: /usr/bin/java
-make: /usr/bin/make
-ninja: /usr/bin/ninja-build
-python: /usr/bin/python3
-su: /bin/su
-sudoers: /etc/sudoers
+paths:
+  bash: /bin/bash
+  cc: /usr/bin/gcc
+  ccache: /usr/bin/ccache
+  java: /usr/bin/java
+  make: /usr/bin/make
+  ninja: /usr/bin/ninja-build
+  python: /usr/bin/python3
+  su: /bin/su
+  sudoers: /etc/sudoers
 
 ansible_python_package: python2
 ansible_python_interpreter: /usr/bin/python2
diff --git a/guests/host_vars/libvirt-centos-8/main.yml 
b/guests/host_vars/libvirt-centos-8/main.yml
index e48c1ff..debc6df 100644
--- a/guests/host_vars/libvirt-centos-8/main.yml
+++ b/guests/host_vars/libvirt-centos-8/main.yml
@@ -23,15 +23,16 @@ packaging:
   format: 'rpm'
   command: 'dnf'
 
-bash: /bin/bash
-cc: /usr/bin/gcc
-ccache: /usr/bin/ccache
-java: /usr/bin/java
-make: /usr/bin/make
-ninja: /usr/bin/ninja
-python: /usr/bin/python3
-su: /bin/su
-sudoers: /etc/sudoers
+paths:
+  bash: /bin/bash
+  cc: /usr/bin/gcc
+  ccache: /usr/bin/ccache
+  java: /usr/bin/java
+  make: /usr/bin/make
+  ninja: /usr/bin/ninja
+  python: /usr/bin/python3
+  su: /bin/su
+  sudoers: /etc/sudoers
 
 ansible_python_package: python3
 ansible_python_interpreter: /usr/bin/python3
diff --git a/guests/host_vars/libvirt-debian-10/main.yml 
b/guests/host_vars/libvirt-debian-10/main.yml
index da30dac..4d1f9cb 100644
--- a/guests/host_vars/libvirt-debian-10/main.yml
+++ b/guests/host_vars/libvirt-debian-10/main.yml
@@ -25,15 +25,16 @@ packaging:
   format: 'deb'
   command: 'apt-get'
 
-bash: /bin/bash
-cc: /usr/bin/gcc
-ccache: /usr/bin/ccache
-java: /usr/bin/java
-make: /usr/bin/make
-ninja: /usr/bin/ninja
-python: /usr/bin/python3
-su: /bin/su
-sudoers: /etc/sudoers
+paths:
+  bash: /bin/bash
+  cc: /usr/bin/gcc
+  ccache: /usr/bin/ccache
+  java: /usr/bin/java
+  make: /usr/bin/make
+  ninja: /usr/bin/ninja
+  python: /usr/bin/python3
+  su: /bin/su
+  sudoers: /etc/sudoers
 
 ansible_python_package: python3
 ansible_python_interpreter: /usr/bin/python3
diff --git a/guests/host_vars/libvirt-debian-9/main.yml 
b/guests/host_vars/libvirt-debian-9/main.yml
index 82ed8b2..5bcb75d 100644
--- a/guests/host_vars/libvirt-debian-9/main.yml
+++ b/guests/host_vars/libvirt-debian-9/main.yml
@@ -25,15 +25,16 @@ packaging:
   format: 'deb'
   command: 'apt-get'
 
-bash: /bin/bash
-cc: /usr/bin/gcc
-ccache: /usr/bin/ccache
-java: /usr/bin/java
-make: /usr/bin/make
-ninja: /usr/bin/ninja
-python: /usr/bin/python3
-su: /bin/su
-sudoers: /etc/sudoers
+paths:
+  bash: /bin/bash
+  cc: /usr/bin/gcc
+  ccache: /usr/bin/ccache
+  java: /usr/bin/java
+  make: /usr/bin/make
+  ninja: /usr/bin/ninja
+  python: /usr/bin/python3
+  su: /bin/su
+  sudoers: /etc/sudoers
 
 ansible_python_package: python3
 ansible_python_interpreter: /usr/bin/python3
diff --git a/guests/host_vars/libvirt-debian-sid/main.yml 
b/guests/host_vars/libvirt-debian-sid/main.yml
index f0c0c8e..2616678 100644
--- a/guests/host_vars/libvirt-debian-sid/main.yml
+++ b/guests/host_vars/libvirt-debian-sid/main.yml
@@ -25,15 

[libvirt-ci PATCH 2/4] lcitool: Store OS information a dictionary

[Andrea Bolognani]

Ansible and Python both support actual dictionaries, so make use
of them in the inventory instead of emulating them by using
variable names sharing the same prefix.

Signed-off-by: Andrea Bolognani 
---
 guests/host_vars/libvirt-centos-7/main.yml|  6 ++-
 guests/host_vars/libvirt-centos-8/main.yml|  6 ++-
 guests/host_vars/libvirt-debian-10/main.yml   |  6 ++-
 guests/host_vars/libvirt-debian-9/main.yml|  6 ++-
 guests/host_vars/libvirt-debian-sid/main.yml  |  6 ++-
 guests/host_vars/libvirt-fedora-30/main.yml   |  6 ++-
 guests/host_vars/libvirt-fedora-31/main.yml   |  6 ++-
 .../host_vars/libvirt-fedora-rawhide/main.yml |  6 ++-
 guests/host_vars/libvirt-freebsd-11/main.yml  |  6 ++-
 guests/host_vars/libvirt-freebsd-12/main.yml  |  6 ++-
 .../libvirt-freebsd-current/main.yml  |  6 ++-
 .../host_vars/libvirt-opensuse-151/main.yml   |  6 ++-
 guests/host_vars/libvirt-ubuntu-1604/main.yml |  6 ++-
 guests/host_vars/libvirt-ubuntu-1804/main.yml |  6 ++-
 guests/lcitool| 49 ---
 guests/playbooks/update/tasks/base.yml| 44 -
 guests/playbooks/update/tasks/bootloader.yml  |  2 +-
 guests/playbooks/update/tasks/gitlab.yml  |  2 +-
 guests/playbooks/update/tasks/kludges.yml |  8 +--
 guests/playbooks/update/tasks/packages.yml| 16 +++---
 20 files changed, 112 insertions(+), 93 deletions(-)

diff --git a/guests/host_vars/libvirt-centos-7/main.yml 
b/guests/host_vars/libvirt-centos-7/main.yml
index 104b702..ac40c7c 100644
--- a/guests/host_vars/libvirt-centos-7/main.yml
+++ b/guests/host_vars/libvirt-centos-7/main.yml
@@ -15,10 +15,12 @@ projects:
   - osinfo-db-tools
   - virt-viewer
 
+os:
+  name: 'CentOS'
+  version: '7'
+
 package_format: 'rpm'
 package_manager: 'yum'
-os_name: 'CentOS'
-os_version: '7'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-centos-8/main.yml 
b/guests/host_vars/libvirt-centos-8/main.yml
index 441802c..77ba6b1 100644
--- a/guests/host_vars/libvirt-centos-8/main.yml
+++ b/guests/host_vars/libvirt-centos-8/main.yml
@@ -15,10 +15,12 @@ projects:
   - virt-manager
   - virt-viewer
 
+os:
+  name: 'CentOS'
+  version: '8'
+
 package_format: 'rpm'
 package_manager: 'dnf'
-os_name: 'CentOS'
-os_version: '8'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-debian-10/main.yml 
b/guests/host_vars/libvirt-debian-10/main.yml
index e3aca55..cb859d1 100644
--- a/guests/host_vars/libvirt-debian-10/main.yml
+++ b/guests/host_vars/libvirt-debian-10/main.yml
@@ -17,10 +17,12 @@ projects:
   - virt-manager
   - virt-viewer
 
+os:
+  name: 'Debian'
+  version: '10'
+
 package_format: 'deb'
 package_manager: 'apt-get'
-os_name: 'Debian'
-os_version: '10'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-debian-9/main.yml 
b/guests/host_vars/libvirt-debian-9/main.yml
index 0599aa4..c9b739d 100644
--- a/guests/host_vars/libvirt-debian-9/main.yml
+++ b/guests/host_vars/libvirt-debian-9/main.yml
@@ -17,10 +17,12 @@ projects:
   - virt-manager
   - virt-viewer
 
+os:
+  name: 'Debian'
+  version: '9'
+
 package_format: 'deb'
 package_manager: 'apt-get'
-os_name: 'Debian'
-os_version: '9'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-debian-sid/main.yml 
b/guests/host_vars/libvirt-debian-sid/main.yml
index 565b0be..6e1adda 100644
--- a/guests/host_vars/libvirt-debian-sid/main.yml
+++ b/guests/host_vars/libvirt-debian-sid/main.yml
@@ -17,10 +17,12 @@ projects:
   - virt-manager
   - virt-viewer
 
+os:
+  name: 'Debian'
+  version: 'Sid'
+
 package_format: 'deb'
 package_manager: 'apt-get'
-os_name: 'Debian'
-os_version: 'Sid'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-fedora-30/main.yml 
b/guests/host_vars/libvirt-fedora-30/main.yml
index d31a84a..2a317c3 100644
--- a/guests/host_vars/libvirt-fedora-30/main.yml
+++ b/guests/host_vars/libvirt-fedora-30/main.yml
@@ -30,10 +30,12 @@ projects:
   - virt-viewer+mingw32
   - virt-viewer+mingw64
 
+os:
+  name: 'Fedora'
+  version: '30'
+
 package_format: 'rpm'
 package_manager: 'dnf'
-os_name: 'Fedora'
-os_version: '30'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-fedora-31/main.yml 
b/guests/host_vars/libvirt-fedora-31/main.yml
index 389cfac..ade64ed 100644
--- a/guests/host_vars/libvirt-fedora-31/main.yml
+++ b/guests/host_vars/libvirt-fedora-31/main.yml
@@ -18,10 +18,12 @@ projects:
   - virt-manager
   - virt-viewer
 
+os:
+  name: 'Fedora'
+  version: '31'
+
 package_format: 'rpm'
 package_manager: 'dnf'
-os_name: 'Fedora'
-os_version: '31'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-fedora-rawhide/main.yml 
b/guests/host_vars/libvirt-fedora-rawhide/main.yml
index 374cb4c..b11e37c 100644
--- a/guests/host_vars/libvirt-fedora-rawhide/main.yml
+++ b/guests/host_vars/libvirt-fedora-rawhide/main.yml
@@ -18,10 +18,12 @@ projects:
   - virt-manager
   - virt-viewer
 

[libvirt-ci PATCH 0/4] lcitool: Use more dictionaries in the inventory

[Andrea Bolognani]

It's the most sensible way to represent related data in YAML.

Andrea Bolognani (4):
  lcitool: Tweak formatting
  lcitool: Store OS information a dictionary
  lcitool: Store packaging information as a dictionary
  lcitool: Store paths information as a dictionary

 guests/host_vars/libvirt-centos-7/main.yml|  30 ++--
 guests/host_vars/libvirt-centos-8/main.yml|  30 ++--
 guests/host_vars/libvirt-debian-10/main.yml   |  30 ++--
 guests/host_vars/libvirt-debian-9/main.yml|  30 ++--
 guests/host_vars/libvirt-debian-sid/main.yml  |  30 ++--
 guests/host_vars/libvirt-fedora-30/main.yml   |  30 ++--
 guests/host_vars/libvirt-fedora-31/main.yml   |  30 ++--
 .../host_vars/libvirt-fedora-rawhide/main.yml |  30 ++--
 guests/host_vars/libvirt-freebsd-11/main.yml  |  30 ++--
 guests/host_vars/libvirt-freebsd-12/main.yml  |  30 ++--
 .../libvirt-freebsd-current/main.yml  |  30 ++--
 .../host_vars/libvirt-opensuse-151/main.yml   |  30 ++--
 guests/host_vars/libvirt-ubuntu-1604/main.yml |  30 ++--
 guests/host_vars/libvirt-ubuntu-1804/main.yml |  30 ++--
 guests/lcitool| 154 --
 guests/playbooks/update/tasks/base.yml|  72 
 guests/playbooks/update/tasks/bootloader.yml  |   2 +-
 guests/playbooks/update/tasks/bootstrap.yml   |   6 +-
 guests/playbooks/update/tasks/gitlab.yml  |   2 +-
 guests/playbooks/update/tasks/global.yml  |   4 +-
 guests/playbooks/update/tasks/jenkins.yml |   2 +-
 guests/playbooks/update/tasks/kludges.yml |   8 +-
 guests/playbooks/update/tasks/packages.yml|  24 +--
 guests/playbooks/update/tasks/services.yml|   2 +-
 guests/playbooks/update/tasks/users.yml   |   4 +-
 guests/playbooks/update/templates/bashrc.j2   |  12 +-
 .../update/templates/jenkins.service.j2   |   2 +-
 27 files changed, 380 insertions(+), 334 deletions(-)

-- 
2.25.4

[libvirt-ci PATCH 1/4] lcitool: Tweak formatting

[Andrea Bolognani]

This will make further changes nicer.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index dfb1ebc..f4565bc 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -714,7 +714,12 @@ class Application:
 pkgs = {}
 cross_pkgs = {}
 pip_pkgs = {}
-base_keys = ["default", package_format, os_name, os_full]
+base_keys = [
+"default",
+package_format,
+os_name,
+os_full,
+]
 cross_keys = []
 if cross_arch:
 keys = base_keys + [cross_arch + "-" + k for k in base_keys]
@@ -782,7 +787,12 @@ class Application:
 pkgs = {}
 cross_pkgs = {}
 pip_pkgs = {}
-keys = ["default", package_format, os_name, os_full]
+keys = [
+"default",
+package_format,
+os_name,
+os_full,
+]
 
 # We need to add the base project manually here: the standard
 # machinery hides it because it's an implementation detail
-- 
2.25.4

[libvirt-ci PATCH 3/4] lcitool: Store packaging information as a dictionary

[Andrea Bolognani]

Ansible and Python both support actual dictionaries, so make use
of them in the inventory instead of emulating them by using
variable names sharing the same prefix.

Signed-off-by: Andrea Bolognani 
---
 guests/host_vars/libvirt-centos-7/main.yml|  5 +-
 guests/host_vars/libvirt-centos-8/main.yml|  5 +-
 guests/host_vars/libvirt-debian-10/main.yml   |  5 +-
 guests/host_vars/libvirt-debian-9/main.yml|  5 +-
 guests/host_vars/libvirt-debian-sid/main.yml  |  5 +-
 guests/host_vars/libvirt-fedora-30/main.yml   |  5 +-
 guests/host_vars/libvirt-fedora-31/main.yml   |  5 +-
 .../host_vars/libvirt-fedora-rawhide/main.yml |  5 +-
 guests/host_vars/libvirt-freebsd-11/main.yml  |  5 +-
 guests/host_vars/libvirt-freebsd-12/main.yml  |  5 +-
 .../libvirt-freebsd-current/main.yml  |  5 +-
 .../host_vars/libvirt-opensuse-151/main.yml   |  5 +-
 guests/host_vars/libvirt-ubuntu-1604/main.yml |  5 +-
 guests/host_vars/libvirt-ubuntu-1804/main.yml |  5 +-
 guests/lcitool| 79 ---
 guests/playbooks/update/tasks/base.yml| 28 +++
 guests/playbooks/update/tasks/bootstrap.yml   |  6 +-
 guests/playbooks/update/tasks/packages.yml|  8 +-
 guests/playbooks/update/tasks/services.yml|  2 +-
 guests/playbooks/update/templates/bashrc.j2   |  6 +-
 20 files changed, 101 insertions(+), 98 deletions(-)

diff --git a/guests/host_vars/libvirt-centos-7/main.yml 
b/guests/host_vars/libvirt-centos-7/main.yml
index ac40c7c..66cb113 100644
--- a/guests/host_vars/libvirt-centos-7/main.yml
+++ b/guests/host_vars/libvirt-centos-7/main.yml
@@ -19,8 +19,9 @@ os:
   name: 'CentOS'
   version: '7'
 
-package_format: 'rpm'
-package_manager: 'yum'
+packaging:
+  format: rpm
+  command: yum
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-centos-8/main.yml 
b/guests/host_vars/libvirt-centos-8/main.yml
index 77ba6b1..e48c1ff 100644
--- a/guests/host_vars/libvirt-centos-8/main.yml
+++ b/guests/host_vars/libvirt-centos-8/main.yml
@@ -19,8 +19,9 @@ os:
   name: 'CentOS'
   version: '8'
 
-package_format: 'rpm'
-package_manager: 'dnf'
+packaging:
+  format: 'rpm'
+  command: 'dnf'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-debian-10/main.yml 
b/guests/host_vars/libvirt-debian-10/main.yml
index cb859d1..da30dac 100644
--- a/guests/host_vars/libvirt-debian-10/main.yml
+++ b/guests/host_vars/libvirt-debian-10/main.yml
@@ -21,8 +21,9 @@ os:
   name: 'Debian'
   version: '10'
 
-package_format: 'deb'
-package_manager: 'apt-get'
+packaging:
+  format: 'deb'
+  command: 'apt-get'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-debian-9/main.yml 
b/guests/host_vars/libvirt-debian-9/main.yml
index c9b739d..82ed8b2 100644
--- a/guests/host_vars/libvirt-debian-9/main.yml
+++ b/guests/host_vars/libvirt-debian-9/main.yml
@@ -21,8 +21,9 @@ os:
   name: 'Debian'
   version: '9'
 
-package_format: 'deb'
-package_manager: 'apt-get'
+packaging:
+  format: 'deb'
+  command: 'apt-get'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-debian-sid/main.yml 
b/guests/host_vars/libvirt-debian-sid/main.yml
index 6e1adda..f0c0c8e 100644
--- a/guests/host_vars/libvirt-debian-sid/main.yml
+++ b/guests/host_vars/libvirt-debian-sid/main.yml
@@ -21,8 +21,9 @@ os:
   name: 'Debian'
   version: 'Sid'
 
-package_format: 'deb'
-package_manager: 'apt-get'
+packaging:
+  format: 'deb'
+  command: 'apt-get'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-fedora-30/main.yml 
b/guests/host_vars/libvirt-fedora-30/main.yml
index 2a317c3..d9e539e 100644
--- a/guests/host_vars/libvirt-fedora-30/main.yml
+++ b/guests/host_vars/libvirt-fedora-30/main.yml
@@ -34,8 +34,9 @@ os:
   name: 'Fedora'
   version: '30'
 
-package_format: 'rpm'
-package_manager: 'dnf'
+packaging:
+  format: 'rpm'
+  command: 'dnf'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-fedora-31/main.yml 
b/guests/host_vars/libvirt-fedora-31/main.yml
index ade64ed..b884e7f 100644
--- a/guests/host_vars/libvirt-fedora-31/main.yml
+++ b/guests/host_vars/libvirt-fedora-31/main.yml
@@ -22,8 +22,9 @@ os:
   name: 'Fedora'
   version: '31'
 
-package_format: 'rpm'
-package_manager: 'dnf'
+packaging:
+  format: 'rpm'
+  command: 'dnf'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-fedora-rawhide/main.yml 
b/guests/host_vars/libvirt-fedora-rawhide/main.yml
index b11e37c..732b988 100644
--- a/guests/host_vars/libvirt-fedora-rawhide/main.yml
+++ b/guests/host_vars/libvirt-fedora-rawhide/main.yml
@@ -22,8 +22,9 @@ os:
   name: 'Fedora'
   version: 'Rawhide'
 
-package_format: 'rpm'
-package_manager: 'dnf'
+packaging:
+  format: 'rpm'
+  command: 'dnf'
 
 bash: /bin/bash
 cc: /usr/bin/gcc
diff --git a/guests/host_vars/libvirt-freebsd-11/main.yml 
b/guests/host_vars/libvirt-freebsd-11/main.yml
index b01c62b..e22b688 100644
--- a/guests/host_vars/libvirt-freebsd-11/main.yml
+++ 

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Peter Krempa]

On Tue, Apr 28, 2020 at 15:27:18 +0200, Michal Privoznik wrote:
> On 4/28/20 1:19 PM, Peter Krempa wrote:
> > On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote:
> > > On a Tuesday in 2020, Michal Privoznik wrote:
> > > > On 4/27/20 10:22 PM, tobin wrote:
> > > > 
> > > > > Yeah fine with me. Thank You.
> > > > > 
> > > > > When it's a positive capability, you don't even need
> > > > > virQEMUCapsProbeQMPTCGState,
> > > > > you can just add the capability to virQEMUCapsObjectTypes.
> > > > > 
> > > > 
> > > > Yep. I've went with that. This is now pushed.
> > > > 
> > > 
> > > Umm, how do you know then if the capability is not missing because the
> > > QEMU is too old to support it?
> > 
> > Yeah, when inverting it, the capability should be assumed by a version
> > check (yuck) with old qemu.
> > 
> 
> Yeah, the qemu commit in question is v2.10.0-rc0~93^2~18 and before that it
> wasn't possible to compile out TCG. And what do you mean "too old to support
> TCG"? Isn't TCG how QEMU started (with adaptation to KVM happening later)?
> The oldest mention of TCG that I bothered to find is in
> v0.14.0-rc0-936-g303d4e865b which is way older than current minimal 1.5.0 so
> I think we are safe, aren't we? Or is there something that I am missing?

No, this is the artifact of you taking patches and modifying them and me
not checking the pushed code.

You correctly added that prior to 2.10 the capability is assumed. I was
refering to the need to have such a thing if you want to do a positive
capability to prevent regressing with old qemus.

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Michal Privoznik]

On 4/28/20 1:19 PM, Peter Krempa wrote:

On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote:

On a Tuesday in 2020, Michal Privoznik wrote:

On 4/27/20 10:22 PM, tobin wrote:


Yeah fine with me. Thank You.

When it's a positive capability, you don't even need
virQEMUCapsProbeQMPTCGState,
you can just add the capability to virQEMUCapsObjectTypes.



Yep. I've went with that. This is now pushed.



Umm, how do you know then if the capability is not missing because the
QEMU is too old to support it?


Yeah, when inverting it, the capability should be assumed by a version
check (yuck) with old qemu.



Yeah, the qemu commit in question is v2.10.0-rc0~93^2~18 and before that 
it wasn't possible to compile out TCG. And what do you mean "too old to 
support TCG"? Isn't TCG how QEMU started (with adaptation to KVM 
happening later)? The oldest mention of TCG that I bothered to find is 
in v0.14.0-rc0-936-g303d4e865b which is way older than current minimal 
1.5.0 so I think we are safe, aren't we? Or is there something that I am 
missing?


Michal

Entering freeze for libvirt 6.3.0

[Daniel Veillard]

  We are getting close to the end of the month, so I tagged RC1 in git
and pushed signed source tarball and rpms to the usual place:

   https://libvirt.org/sources/

Seems to work fine in my very limited testing, CI seems green except for a
couple of mingw tests, so that looks good from a distance.

Please give it some testing, RC2 should land on Thursday, and then if
everything looks fine I could push the final version over the coming week-end.

  Stay safe, please test it,

   thanks,

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Peter Krempa]

On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote:
> On a Tuesday in 2020, Michal Privoznik wrote:
> > On 4/27/20 10:22 PM, tobin wrote:
> > 
> > > Yeah fine with me. Thank You.
> > > 
> > > When it's a positive capability, you don't even need
> > > virQEMUCapsProbeQMPTCGState,
> > > you can just add the capability to virQEMUCapsObjectTypes.
> > > 
> > 
> > Yep. I've went with that. This is now pushed.
> > 
> 
> Umm, how do you know then if the capability is not missing because the
> QEMU is too old to support it?

Yeah, when inverting it, the capability should be assumed by a version
check (yuck) with old qemu.

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Ján Tomko]

On a Tuesday in 2020, Michal Privoznik wrote:

On 4/27/20 10:22 PM, tobin wrote:


Yeah fine with me. Thank You.

When it's a positive capability, you don't even need 
virQEMUCapsProbeQMPTCGState,

you can just add the capability to virQEMUCapsObjectTypes.



Yep. I've went with that. This is now pushed.



Umm, how do you know then if the capability is not missing because the
QEMU is too old to support it?

Jano


Reviewed-by: Michal Privoznik 

Congratulations on your first libvirt contribution!

Michal



signature.asc
Description: PGP signature

Re: [PATCH V3 5/5] cpu: Introduce getHost support for ARM CPU driver

[Zhenyu Zheng]

Thanks alot I will check again about the patch series.

On Tue, Apr 28, 2020 at 6:39 PM Jiri Denemark  wrote:

> On Wed, Apr 22, 2020 at 15:14:01 +0800, Zhenyu Zheng wrote:
> >  gitlab CI testing as suggested:
> > https://gitlab.com/ZhengZhenyu/libvirt/pipelines/134657317
>
> Sending results of CI pipeline makes sense only when the code submitted
> for CI is exactly the same as submitted for review. You should just push
> the branch with your local changes to your gitlab repo rather than
> somehow combining all patches you sent into one or even make additional
> changes.
>
> Apparently the patch pushed to gitlab is a bit different than this
> series as this series cannot be compiled due to a missing "return NULL;"
> line in patch 4/5.
>
> Anyway, I'm trying to get some ARM hosts and hoping to find at least one
> where your series would actually detect the host CPU to see how this
> works in real life and to confirm (or not) my suspicions. No need to
> resent this series to fix the compilation error yet, I'll fix it myself
> for testing.
>
> Thank you for your patience.
>
> Jirka
>

Re: [PATCH V3 5/5] cpu: Introduce getHost support for ARM CPU driver

[Jiri Denemark]

On Wed, Apr 22, 2020 at 15:14:01 +0800, Zhenyu Zheng wrote:
>  gitlab CI testing as suggested:
> https://gitlab.com/ZhengZhenyu/libvirt/pipelines/134657317

Sending results of CI pipeline makes sense only when the code submitted
for CI is exactly the same as submitted for review. You should just push
the branch with your local changes to your gitlab repo rather than
somehow combining all patches you sent into one or even make additional
changes.

Apparently the patch pushed to gitlab is a bit different than this
series as this series cannot be compiled due to a missing "return NULL;"
line in patch 4/5.

Anyway, I'm trying to get some ARM hosts and hoping to find at least one
where your series would actually detect the host CPU to see how this
works in real life and to confirm (or not) my suspicions. No need to
resent this series to fix the compilation error yet, I'll fix it myself
for testing.

Thank you for your patience.

Jirka

Re: [libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method

[Andrea Bolognani]

On Tue, 2020-04-28 at 10:20 +0200, Erik Skultety wrote:
> On Tue, Apr 28, 2020 at 09:34:54AM +0200, Andrea Bolognani wrote:
> > There are also two uses of get_flavor() remaining after this
> > patch:
> > 
> >   get_gitlab_runner_token_file()
> >   get_gitlab_url_file()
> 
> Yeah, does it matter since I'm dropping those 2 patches later?

In the grand scheme of things? No, not really :) But removing all
calls to a function when you remove the function itself is just the
right thing to do.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Michal Privoznik]

On 4/27/20 10:22 PM, tobin wrote:


Yeah fine with me. Thank You.

When it's a positive capability, you don't even need 
virQEMUCapsProbeQMPTCGState,

you can just add the capability to virQEMUCapsObjectTypes.



Yep. I've went with that. This is now pushed.

Reviewed-by: Michal Privoznik 

Congratulations on your first libvirt contribution!

Michal

Re: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config

[Andrea Bolognani]

On Tue, 2020-04-28 at 10:19 +0200, Erik Skultety wrote:
> On Mon, Apr 27, 2020 at 07:47:13PM +0200, Andrea Bolognani wrote:
> > On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> > > +except Exception as ex:
> > > +raise Exception(
> > > +"Missing or invalid config.toml file: {}".format(ex)
> > > +)
> > 
> > This will cause lcitool to blow up pretty spectacularly if you
> > haven't created a configuration file already:
> > 
> >   $ ./lcitool update all all
> >   Traceback (most recent call last):
> > File "./lcitool", line 141, in __init__
> >   self.dict = toml.load(path)
> > File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load
> >   with io.open(f, encoding='utf-8') as ffile:
> >   FileNotFoundError: [Errno 2] No such file or directory: 
> > '/home/abologna/.config/lcitool/config.toml'
> > 
> >   During handling of the above exception, another exception occurred:
> > 
> >   Traceback (most recent call last):
> > File "./lcitool", line 1043, in 
> >   Application().run()
> > File "./lcitool", line 395, in __init__
> >   self._config = Config()
> > File "./lcitool", line 146, in __init__
> >   "Missing or invalid config.toml file: {}".format(ex)
> >   Exception: Missing or invalid config.toml file: [Errno 2] No such file or 
> > directory: '/home/abologna/.config/lcitool/config.toml'
> > 
> > This could technically happen already, but that would have been as
> > the result of invalid data being committed to the repository, so it
> > never showed up in practice.
> > 
> > You'll need to refactor the script so that exceptions can be caught
> > and pretty-printed even when they occur outside of Application.run().
> 
> I guess that in this case if config is missing, it should print a nice error,
> in general, I don't like us re-raising exceptions and wrapping them with
> Exception - this is not the only occurrence where ^this kind of horrible
> backtrace is to be seen. In fact, I've never seen or heard of this practice,
> instead, depending on what type of project it is - if it's a standalone
> program, you let python fail with its standard exception mechanism as it's 
> most
> likely a result of user input; if it's a library, you define your own 
> exception
> class so that projects consuming the library can catch specific errors, e.g.
> config is invalid and cannot be parsed...
> I'll fix the issue pointed out, but the exception overhaul is a refactor for
> another day.

I don't think it's reasonable to get a Python stack trace as a result
of some failure scenario that can be easily anticipated, such as the
configuration file not being present. In other words, the fact that
we see two exceptions above is not the issue: if there was a single
exception, I would still consider it a problem.

If you have identified other code paths that can result in a Python
stack trace instead of a nice, readable error message, then we should
address those too.

> > > +# fill in default options
> > > +self._fill_default_options(self.dict)
> > 
> > You don't need to pass the dictionary as argument - it's already
> > part of self at this point.
> 
> I actually do because it's a static method, it doesn't get a reference to 
> self,
> and I believe that's the correct design, since it's just a validation helper,
> it validates a combination of key-value pairs and as such does not modify the
> instance state and thus does not need nor should have access to the instance
> itself.

I don't really see the point in making it static, because it's a
method that's internal to the class and will only ever be used to
validate a dictionary that's part of the object.

The same consideration applies to _validate_section(), especially
since _validate() - which is the only caller for it - actually uses
self.dict instead of being static itself.

> > > +@staticmethod
> > > +def _validate_section(cfg, section, *args):
> > 
> > I don't like *args very much, and would prefer if this was just a
> > regular list.
> 
> Passing list has a side-effect that args could be accidentally modified, *args
> prevents that. Is there a specific reason why *args is not suitable as a
> parameter in this case?

I just don't like it very much, because when looking at the call site
I find something like

  do_something(with, these, values)

not as clear as something like

  do_something(with, [these, values])

because in the latter it's obvious that the last two values are going
to be used for the same purpose, whereas in the former they could be
completely unrelated for all you know.

Values being modified by mistake is kind of a moot point in Python
anyway: in this specific case, I would be more concerned about the
dictionary being modified by a function that is supposed to only
validate it, but I don't think there is a good way to prevent that
from happening, and with that off the table worrying about some
other argument that's never even 

Re: [libvirt-ci PATCH 13/13] guests: README: Document the existence and usage of config.toml

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> +`ansible`, `python3-toml` and `virt-install` need to be available on the 
> host,
> +the former two can be either installed system-wide using your package manager
> +or using by `pip` (see the provided requirements.txt file).

s/using by/using/

You should also add backticks around requirements.txt.

> The latter can only
> +be installed with your package manager as `virt-install` is not distributed 
> via
> +PyPi.

s/PyPi/PyPI/

> +Before you can start bringing up guests, you need to create (ideally by
> +copying the `config.toml` template) ~/.config/lcitool/config.toml and set at

Backticks around /.config/lcitool/config.toml.

With these nits fixed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 12/13] lcitool: Use the config install options rather than install facts

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> Now that we have the corresponding options available in the config,
> let's start using them.
> 
> Signed-off-by: Erik Skultety 
> ---
>  guests/lcitool | 30 +++---
>  1 file changed, 11 insertions(+), 19 deletions(-)

At this point the file

  group_vars/all/install.yml

is no longer used and you should drop it. I also don't think it makes
sense to split this changes from the ones in 11/13, so please squash
the two patches together.

With that done,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 11/13] config: Move the virt-install settings from install.yml to the config

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> +++ b/guests/lcitool
> @@ -175,6 +175,33 @@ class Config:
>  flavor = cfg.get("install").get("flavor", "test")
>  cfg["install"]["flavor"] = flavor
>  
> +virt_type = cfg.get("install").get("virt_type", "kvm")
> +cfg["install"]["virt_type"] = virt_type
> +
> +arch = cfg.get("install").get("arch", "x86_64")
> +cfg["install"]["arch"] = arch
> +
> +machine = cfg.get("install").get("machine", "pc")
> +cfg["install"]["machine"] = machine
> +
> +cpu_model = cfg.get("install").get("cpu_model", "host-passthrough")
> +cfg["install"]["cpu_model"] = cpu_model
> +
> +vcpus = cfg.get("install").get("vcpus", 2)
> +cfg["install"]["vcpus"] = vcpus
> +
> +memory_size = cfg.get("install").get("memory_size", 2)
> +cfg["install"]["memory_size"] = memory_size
> +
> +disk_size = cfg.get("install").get("disk_size", 15)
> +cfg["install"]["disk_size"] = disk_size
> +
> +storage_pool = cfg.get("install").get("storage_pool", "default")
> +cfg["install"]["storage_pool"] = storage_pool
> +
> +network = cfg.get("install").get("network", "default")
> +cfg["install"]["network"] = network

All this code will go away once you start obtaining default from the
in-repo config.toml.

For the remaining changes,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 10/13] lcitool: Use d.update() on extra_vars for options coming from the config

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> +# start with generic items not coming from the config
>  extra_vars = {
>  "base": base,
>  "playbook_base": playbook_base,
> -"root_password": 
> self._config.dict["install"]["root_password"],
> -"flavor": self._config.dict["install"]["flavor"],
>  "selected_projects": selected_projects,
>  "git_remote": git_remote,
>  "git_branch": git_branch,
> -"gitlab_url": self._config.dict["gitlab"]["url"],
> -"gitlab_runner_secret": self._config.dict["gitlab"]["token"],
>  }
> +
> +# now add the config vars
> +extra_vars.update(self._config.dict["install"])
> +
> +if extra_vars["flavor"] == "gitlab":
> +extra_vars.update(self._config.dict["gitlab"])
> +

Mh, I don't think I like this.

First of all, it forces you to replicate the "if gitlab" conditional
in one additional place, and most importantly it completely flattens
the resulting dictionary, eg.

  config.dict["gitlab"]["url"] -> extra_vars["url"]

Both of these issues will disappear if you follow the suggestion I
made in 6/13 and load the default config.toml first: at that point,
you can unconditionally copy entire sections from the TOML into
extra_vars, and later in the playbook you can simply refer to them
along the lines of

  - shell: gitlab-runner {{ gitlab.url }} {{ gitlab.runner_secret }}
when:
  - install.flavor == "gitlab"

which is much nicer.

In fact, we should use the same approach for values in the
inventory, eg.

  os_name: "Debian"
  os_version: "10"

should become

  os:
name: "Debian"
version: "10"

Given that both Python and Ansible support proper dictionaries, we
might very well use them instead of using prefix notation to emulate
a worse version of them :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method

[Erik Skultety]

On Tue, Apr 28, 2020 at 09:34:54AM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> > @@ -617,7 +591,7 @@ class Application:
> >  def _action_install(self, args):
> >  base = Util.get_base()
> >  
> > -flavor = self._config.get_flavor()
> > +flavor = self._config.dict["install"]["flavor"]
> 
> You can remove this assignment and just use the value directly
> below, like you've done in _execute_playbook().

Okay, missed that one.

> 
> There are also two uses of get_flavor() remaining after this
> patch:
> 
>   get_gitlab_runner_token_file()
>   get_gitlab_url_file()

Yeah, does it matter since I'm dropping those 2 patches later?

-- 
Erik Skultety

Re: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config

[Erik Skultety]

On Mon, Apr 27, 2020 at 07:47:13PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> > +def __init__(self):
> > +path = self._get_config_file("config.toml")
> > +
> > +try:
> > +self.dict = toml.load(path)
> 
> I'm not too keen on the name "dict". What about "values"?

Sure, works for me :).

> 
> > +self._validate()
> > +
> > +except Exception as ex:
> > +raise Exception(
> > +"Missing or invalid config.toml file: {}".format(ex)
> > +)
> 
> This will cause lcitool to blow up pretty spectacularly if you
> haven't created a configuration file already:
> 
>   $ ./lcitool update all all
>   Traceback (most recent call last):
> File "./lcitool", line 141, in __init__
>   self.dict = toml.load(path)
> File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load
>   with io.open(f, encoding='utf-8') as ffile:
>   FileNotFoundError: [Errno 2] No such file or directory: 
> '/home/abologna/.config/lcitool/config.toml'
> 
>   During handling of the above exception, another exception occurred:
> 
>   Traceback (most recent call last):
> File "./lcitool", line 1043, in 
>   Application().run()
> File "./lcitool", line 395, in __init__
>   self._config = Config()
> File "./lcitool", line 146, in __init__
>   "Missing or invalid config.toml file: {}".format(ex)
>   Exception: Missing or invalid config.toml file: [Errno 2] No such file or 
> directory: '/home/abologna/.config/lcitool/config.toml'
> 
> This could technically happen already, but that would have been as
> the result of invalid data being committed to the repository, so it
> never showed up in practice.
> 
> You'll need to refactor the script so that exceptions can be caught
> and pretty-printed even when they occur outside of Application.run().

I guess that in this case if config is missing, it should print a nice error,
in general, I don't like us re-raising exceptions and wrapping them with
Exception - this is not the only occurrence where ^this kind of horrible
backtrace is to be seen. In fact, I've never seen or heard of this practice,
instead, depending on what type of project it is - if it's a standalone
program, you let python fail with its standard exception mechanism as it's most
likely a result of user input; if it's a library, you define your own exception
class so that projects consuming the library can catch specific errors, e.g.
config is invalid and cannot be parsed...
I'll fix the issue pointed out, but the exception overhaul is a refactor for
another day.

> 
> > +# fill in default options
> > +self._fill_default_options(self.dict)
> 
> You don't need to pass the dictionary as argument - it's already
> part of self at this point.

I actually do because it's a static method, it doesn't get a reference to self,
and I believe that's the correct design, since it's just a validation helper,
it validates a combination of key-value pairs and as such does not modify the
instance state and thus does not need nor should have access to the instance
itself.

> 
> > +def _fill_default_options(cfg):
> > +flavor = cfg.get("install").get("flavor", "test")
> > +cfg["install"]["flavor"] = flavor
> > +
> > +if flavor == "gitlab":
> > +url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com;)
> > +cfg["gitlab"]["gitlab_url"] = url
> 
> It would be neat if, instead of hardcoding our defaults in the
> script, we did something like
> 
>   cfg.get("gitlab").get("gitlab_url", default_gitlab_url)
> 
> where default_gitlab_url is obtained from the inventory.
> 
> Alternatively, and this is probably a cleaner approach, we could make
> it so lcitool reads config.toml from the source directory, which
> would be uncommented, first, and then the one in the user's home
> directory, adding / overriding only those keys that are found: this
> would ensure, among other things, that the comments in the sample
> configuration file never get out of sync with the actual defaults,
> and also I think the amount of boring code necessary in patch 11.

Okay, makes sense, I can go with that change I like that.

> 
> > +@staticmethod
> > +def _validate_section(cfg, section, *args):
> 
> I don't like *args very much, and would prefer if this was just a
> regular list.

Passing list has a side-effect that args could be accidentally modified, *args
prevents that. Is there a specific reason why *args is not suitable as a
parameter in this case?

> 
> > +if cfg.get(section, None) is None:
> > +raise KeyError("Section '[{}]' not found".format(section))
> 
> No need for square brackets around the name, you already mentioned
> that it's a section we're talking about.

Noted.

-- 
Erik Skultety

Re: [libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file

[Erik Skultety]

On Tue, Apr 28, 2020 at 09:39:31AM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> > +[gitlab]
> > +# GitLab runner agent registration options, applies only if flavor == 
> > 'gitlab'.
> > +
> > +# GitLab server URL to register the runner.
> > +#gitlab_url = "https://gitlab.com;
> > +
> > +# GitLab runner registration token. (mandatory)
> > +#gitlab_runner_secret = ""
> 
> Additional nit: since TOML is organized in sections, it doesn't make
> sense to include the section name in the key name, so this should
> look like
> 
>   [gitlab]
>   url = "https://gitlab.com;
>   runner_secret = "..."

Absolutely agreed. That was the initial approach, however it didn't pair
with the naming used within the Ansible playbooks, so I changed it. If I'm not
mistaken "url" will cause a collision with another variable, that's why I was
explicit with naming in the config, although I agree that from the config POV
it doesn't make sense to include the section prefix.

-- 
Erik Skultety

Re: [libvirt-ci PATCH 09/13] lcitool: Drop the gitlab-related getter methods

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> @@ -529,8 +489,8 @@ class Application:
>  "selected_projects": selected_projects,
>  "git_remote": git_remote,
>  "git_branch": git_branch,
> -"gitlab_url_file": gitlab_url_file,
> -"gitlab_runner_token_file": gitlab_runner_token_file,
> +"gitlab_url": self._config.dict["gitlab"]["url"],
> +"gitlab_runner_secret": self._config.dict["gitlab"]["token"],

The second key is "runner_secret", not "token".

With that fixed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> +@staticmethod
> +def _fill_default_options(cfg):
> +flavor = cfg.get("install").get("flavor", "test")
> +cfg["install"]["flavor"] = flavor
> +
> +if flavor == "gitlab":
> +url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com;)
> +cfg["gitlab"]["gitlab_url"] = url

The key should be "url" here...

> +def _validate(self):
> +
> +# verify the [install] section and its mandatory options
> +self._validate_section(self.dict, "install", "root_password")
> +
> +# we need to check flavor here, because later validations depend on 
> it
> +flavor = self.dict.get("install").get("flavor", "test")
> +if flavor not in ["test", "jenkins", "gitlab"]:
> +raise ValueError(
> +"Invalid value '{}' for 'install.flavor'".format(flavor)
> +)
> +
> +# verify the optional [gitlab] section and its mandatory options
> +if flavor == "gitlab":
> +self._validate_section(self.dict, "gitlab", 
> "gitlab_runner_secret")

... and "runner_secret" here.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> +[gitlab]
> +# GitLab runner agent registration options, applies only if flavor == 
> 'gitlab'.
> +
> +# GitLab server URL to register the runner.
> +#gitlab_url = "https://gitlab.com;
> +
> +# GitLab runner registration token. (mandatory)
> +#gitlab_runner_secret = ""

Additional nit: since TOML is organized in sections, it doesn't make
sense to include the section name in the key name, so this should
look like

  [gitlab]
  url = "https://gitlab.com;
  runner_secret = "..."

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 08/13] lcitool: Drop the get_root_password_file() method

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> We can now access this value directly in the config dictionary.
> 
> Signed-off-by: Erik Skultety 
> ---
>  guests/lcitool  | 19 +--
>  guests/playbooks/update/tasks/users.yml |  2 +-
>  2 files changed, 2 insertions(+), 19 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> @@ -617,7 +591,7 @@ class Application:
>  def _action_install(self, args):
>  base = Util.get_base()
>  
> -flavor = self._config.get_flavor()
> +flavor = self._config.dict["install"]["flavor"]

You can remove this assignment and just use the value directly
below, like you've done in _execute_playbook().

There are also two uses of get_flavor() remaining after this
patch:

  get_gitlab_runner_token_file()
  get_gitlab_url_file()

-- 
Andrea Bolognani / Red Hat / Virtualization