[libvirt] [PATCH v5 2/4] event: introduce new event for tunable values

Pavel Hrdina phrdina at redhat.com
Tue Sep 23 21:26:34 UTC 2014


On 09/23/2014 10:08 PM, Eric Blake wrote:
> On 09/23/2014 12:46 PM, Pavel Hrdina wrote:
>> This new event will use typedParameters to expose what has been actually
>> updated and the reason is that we can in the future extend any tunable
>> values or add new tunable values. With typedParameters we don't have to
>> worry about creating some other events, we will just use this universal
>> event to inform user about updates.
>>
>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>> ---
>>
>
> When the event is issued, does it contain ONLY parameters for what just
> changed, or does it list ALL tunables including the unchanged ones?
>
> It feels like your API is only capable of listing the new tunable value.
>   Is anyone using this event going to want to know both former and new
> value at the same time?
>

The API returns only the new tunable values. I've inspired myself with 
other APIs that they also returns only the updated values without the 
old ones.

The only user that I know right now will be oVirt and they seems to be 
care only about new values.

>>
>> +/**
>> + * virConnectDomainEventTunableCallback:
>> + * @conn: connection object
>> + * @dom: domain on which the event occurred
>> + * @params: changed tunable values stored as array of virTypedParameter
>> + * @nparams: size of the array
>> + * @opaque: application specified data
>> + *
>> + * This callback occurs when tunable values are updated. The params must not
>> + * be freed in the callback handler as it's done internally after the callback
>> + * handler is executed.
>> + *
>> + * The callback signature to use when registering for an event of type
>> + * VIR_DOMAIN_EVENT_ID_TUNABLE with virConnectDomainEventRegisterAny()
>> + */
>> +typedef void (*virConnectDomainEventTunableCallback)(virConnectPtr conn,
>> +                                                     virDomainPtr dom,
>> +                                                     virTypedParameterPtr params,
>> +                                                     int nparams,
>> +                                                     void *opaque);
>
> Where do we document what names of tunables to expect?

It's documented in the 4/4 patch as I'm adding for now only cputune and 
it will be documented in the description of this callback as existing 
namespace and also there will be definitions for each tunable value that 
we will return.

>
>> +++ b/daemon/remote.c
>
>> +static int
>> +remoteRelayDomainEventTunable(virConnectPtr conn,
>> +                              virDomainPtr dom,
>> +                              virTypedParameterPtr params,
>> +                              int nparams,
>> +                              void *opaque)
>> +{
>> +    daemonClientEventCallbackPtr callback = opaque;
>> +    remote_domain_event_callback_tunable_msg data;
>> +
>> +    if (callback->callbackID < 0 ||
>> +        !remoteRelayDomainEventCheckACL(callback->client, conn, dom))
>> +        return -1;
>> +
>> +    VIR_DEBUG("Relaying domain tunable event %s %d, callback %d",
>> +              dom->name, dom->id, callback->callbackID);
>> +
>
> Might also be nice to log "%p %n", params, nparams

Yes, that would be probably nice, but since I've pushed this patch 
already I can create a following patch with this small update?

>
>
>> +++ b/src/remote/remote_protocol.x
>> @@ -247,6 +247,9 @@ const REMOTE_NETWORK_DHCP_LEASES_MAX = 65536;
>>   /* Upper limit on count of parameters returned via bulk stats API */
>>   const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096;
>>
>> +/* Upper limit of message size for tunable event. */
>> +const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 8388608;
>
> That feels excessive...
>
>> +
>>   /* UUID.  VIR_UUID_BUFLEN definition comes from libvirt.h */
>>   typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>>
>> @@ -2990,6 +2993,12 @@ struct remote_domain_event_block_job_2_msg {
>>       int status;
>>   };
>>
>> +struct remote_domain_event_callback_tunable_msg {
>> +    int callbackID;
>> +    remote_nonnull_domain dom;
>> +    remote_typed_param params<REMOTE_DOMAIN_EVENT_TUNABLE_MAX>;
>
> ...each param in the array will occupy multiple bytes.  I think that
> something as low as 2048 for REMOTE_DOMAIN_EVENT_TUNABLE_MAX is still
> plenty (we don't have that many tunables yet); even if each tunable
> requires 64 bytes to transmit (mostly in the name of the parameter, but
> also in the type and value), that's still well under a megabyte limit of
> information passed on an instance of the event.
>

Well, yes and no :). Let's say, that we will add in the future (and I'm 
planning to do it) blkiotune where you can update at the same time all 
of the tunables for all host's disks where all params for now will be 
only VIR_TYPED_PARAM_STRING and that could consume a lot of memory. I 
know that it will probably never be that much, but I wanted to be sure 
that we will have enough space for all possible tunable events.

Pavel

> The code looks okay, but I'm still a bit worried about the design.





More information about the libvir-list mailing list