Re: [PATCH RFC v3 07/16] remote: New APIs for ThrottleGroup lifecycle management

26 Jul 2024

On Wed, Jun 12, 2024 at 03:02:15 -0700, wucf@linux.ibm.com wrote:
...
From: Chun Feng Wu <wucf@linux.ibm.com>
Defined new public APIs:
* virDomainSetThrottleGroup to add or update throttlegroup within specific domain, it will be referenced by throttlefilter later in disk to do limits
* virDomainGetThrottleGroup to get throttlegroup info, old-style is discarded(APIs to query first for the number of parameters and then give it a reasonably-sized pointer), instead, the new approach is adopted that API returns allocated array of fields and number of fileds that are in it.
* virDomainDelThrottleGroup to delete throttlegroup, it fails if this throttlegroup is still referenced by some throttlefilter
Please use shorter lines in commit message.
...
Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com>
---
 include/libvirt/libvirt-domain.h    |  21 +++
 src/driver-hypervisor.h             |  22 ++++
 src/libvirt-domain.c                | 196 ++++++++++++++++++++++++++++
 src/libvirt_private.syms            |   9 ++
 src/libvirt_public.syms             |   7 +
 src/remote/remote_daemon_dispatch.c |  44 +++++++
 src/remote/remote_driver.c          |  40 ++++++
 src/remote/remote_protocol.x        |  48 ++++++-
 src/remote_protocol-structs         |  28 ++++
 9 files changed, 414 insertions(+), 1 deletion(-)
[...]
...

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 7c6b93963c..37ba587b57 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -14162,3 +14162,199 @@ virDomainGraphicsReload(virDomainPtr domain,
     virDispatchError(domain->conn);
     return -1;
 }
+
+
+/**
+ * virDomainSetThrottleGroup:
+ * @dom: pointer to domain object
+ * @group: throttle group name
+ * @params: Pointer to blkio parameter objects
+ * @nparams: Number of blkio parameters (this value can be the same or
+ *           less than the number of parameters supported)
+ * @flags: bitwise-OR of virDomainModificationImpact
+ *
+ * Add throttlegroup or change all or a subset of the throttlegroup options
+ * within specific domain
+ *
+ * The @group parameter is the name for new or existing throttlegroup,
+ * it cannot be NULL, detailed throttlegroup info is included in @params,
+ * it either creates new throttlegroup with @params or updates existing
+ * throttlegroup with @params, throttlegroup can be referenced by throttle
+ * filter in attached disk to do limits, the difference from iotune is that
+ * multiple throttlegroups can be referenced within attached disk
+ *
+ * Returns -1 in case of error, 0 in case of success.
+ *
+ * Since: 10.5.0
+ */
+int
+virDomainSetThrottleGroup(virDomainPtr dom,
+                          const char *group,
+                          virTypedParameterPtr params,
+                          int nparams,
+                          unsigned int flags)
+{
+    virConnectPtr conn;
+
+    VIR_DOMAIN_DEBUG(dom, "params=%p, nparams=%d, flags=0x%x",
+                     params, nparams, flags);
+    VIR_TYPED_PARAMS_DEBUG(params, nparams);
+
+    virResetLastError();
+
+    virCheckDomainReturn(dom, -1);
+    conn = dom->conn;
+
+    virCheckReadOnlyGoto(conn->flags, error);
+    virCheckNonNullArgGoto(group, error);
+    virCheckPositiveArgGoto(nparams, error);
+    virCheckNonNullArgGoto(params, error);
+
+    if (virTypedParameterValidateSet(dom->conn, params, nparams) < 0)
+        goto error;
+
+    if (conn->driver->domainSetThrottleGroup) {
+        int ret;
+        ret = conn->driver->domainSetThrottleGroup(dom, group, params, nparams, flags);
+        if (ret < 0)
+            goto error;
+        return ret;
+    }
+
+    virReportUnsupportedError();
+
+ error:
+    virDispatchError(dom->conn);
+    return -1;
+}
+
+
+/**
+ * virDomainGetThrottleGroup:
+ * @dom: pointer to domain object
+ * @group: throttle group name
+ * @params: pointer that will be filled with an array of typed parameters
+ * @nparams: pointer filled with number of elements in @params
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
Typed parameter flags are not to be passed by user.
...
+ *
+ * Get all block IO tunable parameters for specific throttle group. @group cannot be NULL.
+ * @nparams gives how many slots were filled with parameter information
+ *
+ *
+ * Returns -1 in case of error, 0 in case of success.
+ *
+ * Since: 10.5.0
+ */
+int
+virDomainGetThrottleGroup(virDomainPtr dom,
+                          const char *group,
+                          virTypedParameterPtr *params,
+                          int *nparams,
+                          unsigned int flags)
+{
+    virConnectPtr conn;
+    int rc;
+
+    VIR_DOMAIN_DEBUG(dom, "params=%p, nparams=%d, flags=0x%x",
+                     params, (nparams) ? *nparams : -1, flags);
+
+    virResetLastError();
+
+    virCheckDomainReturn(dom, -1);
+    virCheckNonNullArgGoto(group, error);
+    virCheckNonNullArgGoto(nparams, error);
+    virCheckNonNullArgGoto(params, error);
+
+    rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn,
+                                  VIR_DRV_FEATURE_TYPED_PARAM_STRING);
All libvirt versions supporting this API support also string typed
params, so this code is not needed and you can assume support.
...
+    if (rc < 0)
+        goto error;
+    if (rc)
+        flags |= VIR_TYPED_PARAM_STRING_OKAY;
+
+    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE,
+                             VIR_DOMAIN_AFFECT_CONFIG,
+                             error);
You explicitly document that both can be used.
...
+
+    conn = dom->conn;
+
+    if (conn->driver->domainGetThrottleGroup) {
+        int ret;
+        ret = conn->driver->domainGetThrottleGroup(dom, group, params, nparams, flags);
+        if (ret < 0)
+            goto error;
+        return ret;
+    }
+
+    virReportUnsupportedError();
+
+ error:
+    virDispatchError(dom->conn);
+    return -1;
+}
+
+
+/**
+ * virDomainDelThrottleGroup:
+ * @dom: pointer to domain object
+ * @group: throttle group name
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
This takes not typed params.
...
+ *
+ * Delete an throttlegroup from the domain. @group cannot be NULL,
+ * and the @group to be deleted must not have a throttlefilter associated with
+ * it and can be any of the current valid group.
+ *
+ * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG.
+ * Both flags may be set.
[1].
...
+ * If VIR_DOMAIN_AFFECT_LIVE is set, the change affects a running domain
+ * and may fail if domain is not alive.
+ * If VIR_DOMAIN_AFFECT_CONFIG is set, the change affects persistent state,
+ * and will fail for transient domains. If neither flag is specified (that is,
+ * @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain modifies
+ * persistent setup, while an active domain is hypervisor-dependent on whether
+ * just live or both live and persistent state is changed.
+ *
+ * Returns -1 in case of error, 0 in case of success.
+ *
+ * Since: 10.5.0
+ */
+int
+virDomainDelThrottleGroup(virDomainPtr dom,
+                          const char *group,
+                          unsigned int flags)
+{
+    virConnectPtr conn;
+    int rc;
+
+    virResetLastError();
+
+    virCheckDomainReturn(dom, -1);
+    virCheckNonNullArgGoto(group, error);
+
+    rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn,
+                                  VIR_DRV_FEATURE_TYPED_PARAM_STRING);
This API is not passing any typed parameters so all of this code is not
actually used.
...
+    if (rc < 0)
+        goto error;
+    if (rc)
+        flags |= VIR_TYPED_PARAM_STRING_OKAY;
+
+    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE,
+                             VIR_DOMAIN_AFFECT_CONFIG,
+                             error);
This API is updating state of a VM thus must not be allowed on read-only
connections. You'll need to also use proper flags in the .x file (which
I've deleted while replying).

Also note that many of existing APIs allow dual use of _LIVE and _CONFIG
flags. You even document it [1].
...
+
+    conn = dom->conn;
+
+    if (conn->driver->domainDelThrottleGroup) {
+        int ret;
+        ret = conn->driver->domainDelThrottleGroup(dom, group, flags);
+        if (ret < 0)
+            goto error;
+        return ret;
+    }
+
+    virReportUnsupportedError();
+
+ error:
+    virDispatchError(dom->conn);
+    return -1;
+}
...
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 7a3492d9d7..2b37f29a28 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -948,4 +948,11 @@ LIBVIRT_10.2.0 {
         virDomainGraphicsReload;
 } LIBVIRT_10.1.0;
+LIBVIRT_10.5.0 {
10.7.0
...
+    global:
+        virDomainSetThrottleGroup;
+        virDomainGetThrottleGroup;
+        virDomainDelThrottleGroup;
+} LIBVIRT_10.2.0;
+
 # .... define new API here using predicted next version number ....
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index cfc1067e40..f5a9e45303 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3614,6 +3614,50 @@ remoteDispatchDomainGetBlockIoTune(virNetServer *server G_GNUC_UNUSED,
     return rv;
 }
+
+static int
+remoteDispatchDomainGetThrottleGroup(virNetServer *server G_GNUC_UNUSED,
+                                     virNetServerClient *client,
+                                     virNetMessage *hdr G_GNUC_UNUSED,
+                                     struct virNetMessageError *rerr,
+                                     remote_domain_get_throttle_group_args *args,
+                                     remote_domain_get_throttle_group_ret *ret)
+{
+    virDomainPtr dom = NULL;
+    int rv = -1;
+    virTypedParameterPtr params = NULL;
+    int nparams = 0;
+    virConnectPtr conn = remoteGetHypervisorConn(client);
+
+    if (!conn)
+        goto cleanup;
+
+    if (!(dom = get_nonnull_domain(conn, args->dom)))
+        goto cleanup;
+
+    if (virDomainGetThrottleGroup(dom, args->group ? *args->group : NULL,
+                                  ¶ms, &nparams, args->flags) < 0)
+        goto cleanup;
+
+    /* Serialize the ThrottleGroup parameters. */
+    if (virTypedParamsSerialize(params, nparams,
+                                REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX,
+                                (struct _virTypedParameterRemote **) &ret->params.params_val,
+                                &ret->params.params_len,
+                                args->flags) < 0)
+        goto cleanup;
+
+    rv = 0;
+
+ cleanup:
+    if (rv < 0)
+        virNetMessageSaveError(rerr);
+    virTypedParamsFree(params, nparams);
+    virObjectUnref(dom);
+    return rv;
+}
+
+
 /*-------------------------------------------------------------*/
static int
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index e76d9e9ba4..4f2b28e662 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2583,6 +2583,43 @@ static int remoteDomainGetBlockIoTune(virDomainPtr domain,
     return 0;
 }
+
+static int
+remoteDomainGetThrottleGroup(virDomainPtr domain,
+                             const char *group,
+                             virTypedParameterPtr *params,
+                             int *nparams,
+                             unsigned int flags)
+{
+    remote_domain_get_throttle_group_args args = {0};
+    g_auto(remote_domain_get_throttle_group_ret) ret = {0};
+    struct private_data *priv = domain->conn->privateData;
+    VIR_LOCK_GUARD lock = remoteDriverLock(priv);
+
+    make_nonnull_domain(&args.dom, domain);
+    args.group = group ? (char **)&group : NULL;
+    args.flags = flags;
+
+    if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_THROTTLE_GROUP,
+             (xdrproc_t) xdr_remote_domain_get_throttle_group_args,
+               (char *) &args,
+             (xdrproc_t) xdr_remote_domain_get_throttle_group_ret,
+               (char *) &ret) == -1) {
+        return -1;
+    }
+
+    if (virTypedParamsDeserialize((struct _virTypedParameterRemote *) ret.params.params_val,
+                                  ret.params.params_len,
+                                  REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX,
+                                  params,
+                                  nparams) < 0)
+        return -1;
+
+    return 0;
+
+}
+
+
 static int remoteDomainGetCPUStats(virDomainPtr domain,
                                    virTypedParameterPtr params,
                                    unsigned int nparams,
@@ -7842,6 +7879,9 @@ static virHypervisorDriver hypervisor_driver = {
     .domainSetLaunchSecurityState = remoteDomainSetLaunchSecurityState, /* 8.0.0 */
     .domainFDAssociate = remoteDomainFDAssociate, /* 9.0.0 */
     .domainGraphicsReload = remoteDomainGraphicsReload, /* 10.2.0 */
+    .domainSetThrottleGroup = remoteDomainSetThrottleGroup, /* 10.5.0 */
+    .domainGetThrottleGroup = remoteDomainGetThrottleGroup, /* 10.5.0 */
+    .domainDelThrottleGroup = remoteDomainDelThrottleGroup, /* 10.5.0 */
Don't forget to update these to 10.7.0
...
};

    

Re: [PATCH RFC v3 07/16] remote: New APIs for ThrottleGroup lifecycle management

Peter Krempa