Re: [PATCH 1/5] conf: allow different resource registration modes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




I know it's a POC type series, but figured I'd take a look primarily at
the first 4 patches, learn a bit in the 5th one, and provide some review
feedback in order to help move things along...  There's perhaps a few
adjustments in here that could be made into multiple patches.

On 12/20/2017 11:47 AM, Daniel P. Berrange wrote:
> Currently the QEMU driver has three ways of setting up cgroups. It either
> skips them entirely (if non-root), or uses systemd-machined, or uses
> cgroups directly.
> 
> It is further possible to register directly with systemd and bypass
> machined. We don't support this by systemd-nsspawn does and we ought

s/by/but/   (I think)

Still a bit odd to add an option that isn't supported unless there was a
plan to add the support when formulating the non POC patches...

> to.
> 
> This change adds ability to configure the mechanism for registering
> resources between all these options explicitly. via

s/. via/ via:/  (e.g. remove the '.' and add ':')

> 
>   <resource register="none|direct|machined|systemd"/>
> 
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
>  src/conf/domain_conf.c   | 42 ++++++++++++++++++++++-------
>  src/conf/domain_conf.h   | 12 +++++++++
>  src/libvirt_private.syms |  2 ++
>  src/lxc/lxc_cgroup.c     | 34 ++++++++++++++++++++++++
>  src/lxc/lxc_cgroup.h     |  3 +++
>  src/lxc/lxc_process.c    | 11 ++++----
>  src/qemu/qemu_cgroup.c   | 69 +++++++++++++++++++++++++++++++++++++++++-------
>  src/util/vircgroup.c     | 55 ++++++++++++++++++++++++--------------
>  src/util/vircgroup.h     | 10 ++++++-
>  9 files changed, 194 insertions(+), 44 deletions(-)
> 


formatdomain.html.in would need to be augmented to describe the new
attribute.

domaincommon.rng modified to add the new attribute.

A tests either added or existing one adjusted to exhibit the new
attribute option(s).

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9a62bc472c..fb8e7a0ec7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -910,6 +910,14 @@ VIR_ENUM_IMPL(virDomainHPTResizing,
>                "required",
>  );
>  
> +VIR_ENUM_IMPL(virDomainResourceRegister,
> +              VIR_DOMAIN_RESOURCE_REGISTER_LAST,
> +              "default",
> +              "none",
> +              "cgroup",
> +              "machined",
> +              "systemd");
> +
>  /* Internal mapping: subset of block job types that can be present in
>   * <mirror> XML (remaining types are not two-phase). */
>  VIR_ENUM_DECL(virDomainBlockJob)
> @@ -17698,16 +17706,20 @@ virDomainResourceDefParse(xmlNodePtr node,
>  {
>      virDomainResourceDefPtr def = NULL;
>      xmlNodePtr tmp = ctxt->node;
> +    char *reg;

= NULL;

>  
>      ctxt->node = node;
>  
>      if (VIR_ALLOC(def) < 0)
>          goto error;
>  
> -    /* Find out what type of virtualization to use */
> -    if (!(def->partition = virXPathString("string(./partition)", ctxt))) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       "%s", _("missing resource partition attribute"));
> +    def->partition = virXPathString("string(./partition)", ctxt);

It seems partition is now optional, although formatdomain seems to have
already indicated that... However, that's not a perfect match to
domaincommon.rng where only "respartition" is optional. (This type of
change could perhaps be it's own patch).

> +
> +    reg = virXMLPropString(node, "register");
> +    if (reg != NULL &&
> +        (def->reg = virDomainResourceRegisterTypeFromString(reg)) <= 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       "%s", _("Invalid register attribute"));
>          goto error;
>      }

Need a VIR_FREE(reg); since we haven't GO-ified yet in both error and
normal paths unless this is rewritten a bit...

>  
> @@ -25568,11 +25580,23 @@ static void
>  virDomainResourceDefFormat(virBufferPtr buf,
>                             virDomainResourceDefPtr def)
>  {
> -    virBufferAddLit(buf, "<resource>\n");
> -    virBufferAdjustIndent(buf, 2);
> -    virBufferEscapeString(buf, "<partition>%s</partition>\n", def->partition);
> -    virBufferAdjustIndent(buf, -2);
> -    virBufferAddLit(buf, "</resource>\n");
> +    if (def->reg == VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT &&
> +        def->partition == NULL)
> +        return;
> +
> +    virBufferAddLit(buf, "<resource");
> +    if (def->reg != VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT)
> +        virBufferAsprintf(buf, " register='%s'", virDomainResourceRegisterTypeToString(def->reg));
> +
> +    if (def->partition) {
> +        virBufferAddLit(buf, ">\n");
> +        virBufferAdjustIndent(buf, 2);
> +        virBufferEscapeString(buf, "<partition>%s</partition>\n", def->partition);
> +        virBufferAdjustIndent(buf, -2);
> +        virBufferAddLit(buf, "</resource>\n");
> +    } else {
> +        virBufferAddLit(buf, "/>\n");
> +    }
>  }
>  
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 6f7f96b3dd..a7a6628a36 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2145,9 +2145,20 @@ struct _virDomainPanicDef {
>  void virBlkioDeviceArrayClear(virBlkioDevicePtr deviceWeights,
>                                int ndevices);
>  
> +typedef enum {
> +    VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT,
> +    VIR_DOMAIN_RESOURCE_REGISTER_NONE,
> +    VIR_DOMAIN_RESOURCE_REGISTER_CGROUP,
> +    VIR_DOMAIN_RESOURCE_REGISTER_MACHINED,
> +    VIR_DOMAIN_RESOURCE_REGISTER_SYSTEMD,
> +
> +    VIR_DOMAIN_RESOURCE_REGISTER_LAST,
> +} virDomainResourceRegister;
> +
>  typedef struct _virDomainResourceDef virDomainResourceDef;
>  typedef virDomainResourceDef *virDomainResourceDefPtr;
>  struct _virDomainResourceDef {
> +    int reg;    /* enum virDomainResourceRegister */
>      char *partition;
>  };
>  
> @@ -3325,6 +3336,7 @@ VIR_ENUM_DECL(virDomainMemorySource)
>  VIR_ENUM_DECL(virDomainMemoryAllocation)
>  VIR_ENUM_DECL(virDomainIOMMUModel)
>  VIR_ENUM_DECL(virDomainShmemModel)
> +VIR_ENUM_DECL(virDomainResourceRegister)
>  /* from libvirt.h */
>  VIR_ENUM_DECL(virDomainState)
>  VIR_ENUM_DECL(virDomainNostateReason)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d5c3b9abb5..a0fde65dba 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -489,6 +489,8 @@ virDomainRedirdevBusTypeToString;
>  virDomainRedirdevDefFind;
>  virDomainRedirdevDefFree;
>  virDomainRedirdevDefRemove;
> +virDomainResourceRegisterTypeFromString;
> +virDomainResourceRegisterTypeToString;
>  virDomainRNGBackendTypeToString;
>  virDomainRNGDefFree;
>  virDomainRNGFind;
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 3369801870..7bd479df1b 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -478,6 +478,35 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
>      return ret;
>  }
>  
> +int virLXCCgroupMode(virDomainResourceRegister reg,
> +                     virCgroupRegister *cgreg)
> +{
> +    switch (reg) {
> +    case VIR_DOMAIN_RESOURCE_REGISTER_NONE:
> +        goto unsupported;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT:
> +        *cgreg = VIR_CGROUP_REGISTER_DEFAULT;
> +        break;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_MACHINED:
> +        *cgreg = VIR_CGROUP_REGISTER_MACHINED;
> +        break;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_CGROUP:
> +        *cgreg = VIR_CGROUP_REGISTER_DIRECT;
> +        break;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_SYSTEMD:
> +    default:

Use VIR_DOMAIN_RESOURCE_REGISTER_LAST instead of default

> +        goto unsupported;
> +    }
> +
> +    return 0;
> +
> + unsupported:
> +    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                   _("Resource register '%s' not available"),
> +                   virDomainResourceRegisterTypeToString(reg));
> +    return -1;
> +}
> +
>  
>  virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
>                                  pid_t initpid,
> @@ -485,11 +514,15 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
>                                  int *nicindexes)
>  {
>      virCgroupPtr cgroup = NULL;
> +    virCgroupRegister reg;
>      char *machineName = virLXCDomainGetMachineName(def, 0);
>  
>      if (!machineName)
>          goto cleanup;
>  
> +    if (virLXCCgroupMode(def->resource->reg, &reg) < 0)
> +        goto cleanup;
> +
>      if (def->resource->partition[0] != '/') {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("Resource partition '%s' must start with '/'"),
> @@ -504,6 +537,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
>                              initpid,
>                              true,
>                              nnicindexes, nicindexes,
> +                            &reg,
>                              def->resource->partition,
>                              -1,
>                              &cgroup) < 0)
> diff --git a/src/lxc/lxc_cgroup.h b/src/lxc/lxc_cgroup.h
> index e85f21c47d..979d6a154b 100644
> --- a/src/lxc/lxc_cgroup.h
> +++ b/src/lxc/lxc_cgroup.h
> @@ -27,6 +27,9 @@
>  # include "lxc_fuse.h"
>  # include "virusb.h"
>  
> +int virLXCCgroupMode(virDomainResourceRegister reg,
> +                     virCgroupRegister *cgreg);
> +
>  virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
>                                  pid_t initpid,
>                                  size_t nnicindexes,
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index efd8a69000..24aa0cb0bf 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -1166,6 +1166,7 @@ virLXCProcessEnsureRootFS(virDomainObjPtr vm)
>      return -1;
>  }
>  
> +
>  /**
>   * virLXCProcessStart:
>   * @conn: pointer to connection
> @@ -1260,13 +1261,13 @@ int virLXCProcessStart(virConnectPtr conn,
>          if (VIR_ALLOC(res) < 0)
>              goto cleanup;
>  
> -        if (VIR_STRDUP(res->partition, "/machine") < 0) {
> -            VIR_FREE(res);
> -            goto cleanup;
> -        }
> -
>          vm->def->resource = res;
>      }

So the above could be shortened to just:

    if (!vm->def->resource &&
        VIR_ALLOC(vm->def->resource) < 0)
        goto cleanup;

> +    if (vm->def->resource->reg != VIR_DOMAIN_RESOURCE_REGISTER_NONE &&
> +        !vm->def->resource->partition) {
> +        if (VIR_STRDUP(vm->def->resource->partition, "/machine") < 0)
> +            goto cleanup;
> +    }

This could be similar too w/r/t single if condition...

>  
>      if (virAsprintf(&logfile, "%s/%s.log",
>                      cfg->logDir, vm->def->name) < 0)
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 19252ea239..5167d7fee1 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -834,6 +834,46 @@ qemuSetupCpuCgroup(virDomainObjPtr vm)
>  }
>  
>  

There could be a helper here that would be callable by qemuConnectCgroup
that would return the @avail value.

> +static int qemuGetCgroupMode(virDomainObjPtr vm,
> +                             virDomainResourceRegister reg,
> +                             virCgroupRegister *cgreg)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    bool avail = virQEMUDriverIsPrivileged(priv->driver) &&
> +        virCgroupAvailable();
> +
> +    switch (reg) {
> +    case VIR_DOMAIN_RESOURCE_REGISTER_NONE:
> +        return 0;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT:
> +        if (!avail)
> +            return 0;
> +        *cgreg = VIR_CGROUP_REGISTER_DEFAULT;
> +        break;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_MACHINED:
> +        if (!avail)
> +            goto unsupported;
> +        *cgreg = VIR_CGROUP_REGISTER_MACHINED;
> +        break;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_CGROUP:
> +        if (!avail)
> +            goto unsupported;
> +        *cgreg = VIR_CGROUP_REGISTER_DIRECT;
> +        break;
> +    case VIR_DOMAIN_RESOURCE_REGISTER_SYSTEMD:
> +    default:

Use VIR_DOMAIN_RESOURCE_REGISTER_LAST instead of default

> +        goto unsupported;
> +    }
> +
> +    return 1;
> +
> + unsupported:
> +    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                   _("Resource register '%s' not available"),
> +                   virDomainResourceRegisterTypeToString(reg));
> +    return -1;
> +}
> +
>  static int
>  qemuInitCgroup(virDomainObjPtr vm,
>                 size_t nnicindexes,
> @@ -842,11 +882,17 @@ qemuInitCgroup(virDomainObjPtr vm,
>      int ret = -1;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(priv->driver);
> +    virCgroupRegister reg;
> +    int rv;
>  
> -    if (!virQEMUDriverIsPrivileged(priv->driver))
> -        goto done;
> -
> -    if (!virCgroupAvailable())
> +    rv = qemuGetCgroupMode(vm,
> +                           vm->def->resource ?
> +                           vm->def->resource->reg :
> +                           VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT,
> +                           &reg);
> +    if (rv < 0)
> +        goto cleanup;
> +    if (rv == 0)
>          goto done;
>  
>      virCgroupFree(&priv->cgroup);
> @@ -857,13 +903,12 @@ qemuInitCgroup(virDomainObjPtr vm,
>          if (VIR_ALLOC(res) < 0)
>              goto cleanup;
>  
> -        if (VIR_STRDUP(res->partition, "/machine") < 0) {
> -            VIR_FREE(res);
> -            goto cleanup;
> -        }
> -
>          vm->def->resource = res;
>      }

Similar to above, we now have:

    if (!vm->def->resource &&
        VIR_ALLOC(vm->def->resource) < 0)
        goto cleanup;

> +    if (!vm->def->resource->partition) {
> +        if (VIR_STRDUP(vm->def->resource->partition, "/machine") < 0)
> +            goto cleanup;
> +    }
>  

and similar construct if desired.

>      if (vm->def->resource->partition[0] != '/') {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -879,6 +924,7 @@ qemuInitCgroup(virDomainObjPtr vm,
>                              vm->pid,
>                              false,
>                              nnicindexes, nicindexes,
> +                            &reg,
>                              vm->def->resource->partition,
>                              cfg->cgroupControllers,
>                              &priv->cgroup) < 0) {
> @@ -980,6 +1026,11 @@ qemuConnectCgroup(virDomainObjPtr vm)
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(priv->driver);
>      int ret = -1;
>  
> +    if (vm->def->resource &&
> +        vm->def->resource->reg == VIR_DOMAIN_RESOURCE_REGISTER_NONE) {
> +        goto done;
> +    }
> +

The subsequent checks could use the @avail helper mentioned above...

John

>      if (!virQEMUDriverIsPrivileged(priv->driver))
>          goto done;
>  
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 0a31947b0d..07ffb78c78 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1733,6 +1733,7 @@ virCgroupNewMachine(const char *name,
>                      bool isContainer,
>                      size_t nnicindexes,
>                      int *nicindexes,
> +                    virCgroupRegister *reg,
>                      const char *partition,
>                      int controllers,
>                      virCgroupPtr *group)
> @@ -1741,28 +1742,42 @@ virCgroupNewMachine(const char *name,
>  
>      *group = NULL;
>  
> -    if ((rv = virCgroupNewMachineSystemd(name,
> -                                         drivername,
> -                                         uuid,
> -                                         rootdir,
> -                                         pidleader,
> -                                         isContainer,
> -                                         nnicindexes,
> -                                         nicindexes,
> -                                         partition,
> -                                         controllers,
> -                                         group)) == 0)
> -        return 0;
> +    if (*reg == VIR_CGROUP_REGISTER_DEFAULT ||
> +        *reg == VIR_CGROUP_REGISTER_MACHINED) {
> +        if ((rv = virCgroupNewMachineSystemd(name,
> +                                             drivername,
> +                                             uuid,
> +                                             rootdir,
> +                                             pidleader,
> +                                             isContainer,
> +                                             nnicindexes,
> +                                             nicindexes,
> +                                             partition,
> +                                             controllers,
> +                                             group)) == 0) {
> +            *reg = VIR_CGROUP_REGISTER_MACHINED;
> +            return 0;
> +        }
>  
> -    if (rv == -1)
> -        return -1;
> +        if (rv == -1)
> +            return -1;
> +
> +        if (*reg == VIR_CGROUP_REGISTER_MACHINED) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           "%s", _("Systemd machined requested, but not available"));
> +            return -1;
> +        }
> +    }
>  
> -    return virCgroupNewMachineManual(name,
> -                                     drivername,
> -                                     pidleader,
> -                                     partition,
> -                                     controllers,
> -                                     group);
> +    rv = virCgroupNewMachineManual(name,
> +                                   drivername,
> +                                   pidleader,
> +                                   partition,
> +                                   controllers,
> +                                   group);
> +    if (rv == 0)
> +        *reg = VIR_CGROUP_REGISTER_DIRECT;
> +    return rv;
>  }
>  
>  
> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
> index d833927678..63ee1aba5c 100644
> --- a/src/util/vircgroup.h
> +++ b/src/util/vircgroup.h
> @@ -46,7 +46,8 @@ enum {
>      VIR_CGROUP_CONTROLLER_LAST
>  };
>  
> -VIR_ENUM_DECL(virCgroupController);
> +VIR_ENUM_DECL(virCgroupController)
> +
>  /* Items of this enum are used later in virCgroupNew to create
>   * bit array stored in int. Like this:
>   *   1 << VIR_CGROUP_CONTROLLER_CPU
> @@ -103,6 +104,12 @@ virCgroupNewDetectMachine(const char *name,
>                            virCgroupPtr *group)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> +typedef enum {
> +    VIR_CGROUP_REGISTER_DEFAULT,
> +    VIR_CGROUP_REGISTER_DIRECT,
> +    VIR_CGROUP_REGISTER_MACHINED,
> +} virCgroupRegister;
> +
>  int virCgroupNewMachine(const char *name,
>                          const char *drivername,
>                          const unsigned char *uuid,
> @@ -111,6 +118,7 @@ int virCgroupNewMachine(const char *name,
>                          bool isContainer,
>                          size_t nnicindexes,
>                          int *nicindexes,
> +                        virCgroupRegister *reg,
>                          const char *partition,
>                          int controllers,
>                          virCgroupPtr *group)
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux