[libvirt] [PATCH] lxc: Fix 'domblkstat' error with attached disk devices.

[Cole Robinson]

On 10/20/19 11:54 PM, jcfaracco at gmail.com wrote:
> From: Julio Faracco <jcfaracco at gmail.com>
> 

I think if you set your gitconfig correctly, you can avoid this 'From:'
showing up. I have:

[sendemail]
    from = "Cole Robinson <crobinso at redhat.com>"
[user]
    name = Cole Robinson
    email = crobinso at redhat.com



> LXC was not working when you attached a new disk that points to block
> device. See: https://bugzilla.redhat.com/show_bug.cgi?id=1697115.
> Command line from virsh was showing problems with alias first (this
> feature is not supported) and after, problems to query block device. 

If you are fixing two distinct issues, this should at be at least two
separate patches. Otherwise review is more difficult among other things

It
> was happening because extra disks were not being included into
> cgroup blkio.throttle properties and libvirt could not query this info.
> Applying those devices into 'allowed' list is not enough, libvirt should
> reset blkio.throttle as a workaround to include disks into container's
> cgroup directory.
> 
> Before:
> 
>     virsh # domblkstat CentOS hda
>     error: Failed to get block stats for domain 'CentOS' device 'hda'
>     error: internal error: domain stats query failed
> 
> Now:
> 
>     virsh # domblkstat CentOS hda
>     hda rd_req 0
>     hda rd_bytes 0
>     hda wr_req 0
>     hda wr_bytes 0
> 
> Signed-off-by: Julio Faracco <jcfaracco at gmail.com>
> ---
>  src/lxc/lxc_cgroup.c | 29 ++++++++++++++++++++++++++++-
>  src/lxc/lxc_cgroup.h |  4 ++++
>  src/lxc/lxc_driver.c |  8 ++++----
>  3 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 5efb495b56..de6d892521 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -302,6 +302,24 @@ virLXCTeardownHostUSBDeviceCgroup(virUSBDevicePtr dev G_GNUC_UNUSED,
>      return 0;
>  }
>  
> +int
> +virLXCCgroupResetBlkioDeviceThrottle(virCgroupPtr cgroup,
> +                                     const char *path)
> +{
> +    if (virCgroupSetBlkioDeviceReadBps(cgroup, path, 0) < 0)
> +        return -1;
> +
> +    if (virCgroupSetBlkioDeviceWriteBps(cgroup, path, 0) < 0)
> +        return -1;
> +
> +    if (virCgroupSetBlkioDeviceReadIops(cgroup, path, 0) < 0)
> +        return -1;
> +
> +    if (virCgroupSetBlkioDeviceWriteIops(cgroup, path, 0) < 0)
> +        return -1;
> +
> +    return 0;
> +}
>  
>  static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
>                                        virCgroupPtr cgroup)
> @@ -309,6 +327,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
>      int capMknod = def->caps_features[VIR_DOMAIN_CAPS_FEATURE_MKNOD];
>      int ret = -1;
>      size_t i;
> +    const char *src_path = NULL;
>      static virLXCCgroupDevicePolicy devices[] = {
>          {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_NULL},
>          {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_ZERO},
> @@ -346,8 +365,16 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
>              !virStorageSourceIsBlockLocal(def->disks[i]->src))
>              continue;
>  
> +        /* Workaround to include disks into blkio.throttle.
> +         * To include it, we need to reset any feature of
> +         * blkio.throttle.* */
> +        src_path = virDomainDiskGetSource(def->disks[i]);
> +        if (virLXCCgroupResetBlkioDeviceThrottle(cgroup,
> +                                                 src_path) < 0)
> +            goto cleanup;
> +

I'll admit I don't really know how cgroups work here. But it seems odd.
Why exactly is this needed? How does blkstats fail without this, after
the alias piece is fixed? Is something similar needed for the qemu
driver, and if not, why the difference?

Also FWIW, not sure if you have fedora 31 installed anywhere, but seems
like the lxc driver is completely broken with cgroupv2:

https://bugzilla.redhat.com/show_bug.cgi?id=1770763

The suggestion in the bug sounds simple though. If you wanted to
separately take a stab at that I'm motivated to test and review. Just a
thought

>          if (virCgroupAllowDevicePath(cgroup,
> -                                     virDomainDiskGetSource(def->disks[i]),
> +                                     src_path,
>                                       (def->disks[i]->src->readonly ?
>                                        VIR_CGROUP_DEVICE_READ :
>                                        VIR_CGROUP_DEVICE_RW) |
> diff --git a/src/lxc/lxc_cgroup.h b/src/lxc/lxc_cgroup.h
> index 63e9e837b0..3d643a4fea 100644
> --- a/src/lxc/lxc_cgroup.h
> +++ b/src/lxc/lxc_cgroup.h
> @@ -46,3 +46,7 @@ int
>  virLXCTeardownHostUSBDeviceCgroup(virUSBDevicePtr dev,
>                                    const char *path,
>                                    void *opaque);
> +
> +int
> +virLXCCgroupResetBlkioDeviceThrottle(virCgroupPtr cgroup,
> +                                     const char *path);
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 204a3ed522..87da55f308 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -2339,14 +2339,14 @@ lxcDomainBlockStats(virDomainPtr dom,
>          goto endjob;
>      }
>  
> -    if (!disk->info.alias) {
> +    if (!disk->src->path) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("missing disk device alias name for %s"), disk->dst);
>          goto endjob;
>      }
> 

I think this whole conditional can be deleted. Earlier in the function
!path is already handled.

>      ret = virCgroupGetBlkioIoDeviceServiced(priv->cgroup,
> -                                            disk->info.alias,
> +                                            disk->src->path,
>                                              &stats->rd_bytes,
>                                              &stats->wr_bytes,
>                                              &stats->rd_req,
> @@ -2424,14 +2424,14 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
>              goto endjob;
>          }
>  
> -        if (!disk->info.alias) {
> +        if (!disk->src->path) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("missing disk device alias name for %s"), disk->dst);
>              goto endjob;
>          }
>  

Same with this block. The alias changes make sense though

Thanks,
Cole

>          if (virCgroupGetBlkioIoDeviceServiced(priv->cgroup,
> -                                              disk->info.alias,
> +                                              disk->src->path,
>                                                &rd_bytes,
>                                                &wr_bytes,
>                                                &rd_req,
>