Re: [PATCH RFC v2 07/12] schema: Add new domain elements to support multiple throttle filters
On Thu, Jun 06, 2024 at 08:07:37 -0000, Chun Feng Wu wrote:
for comment
"Preferrably all these definitions should be shared with <iotune> as any
change would now need to change two places."
Trimming the context and mentioning this random bit makes it rather hard
to remember what I've based that comment on ...
do you mean we use domain xml like the following one(which reuse <iotune>
definition):
... so I have no idea how to respond to this without going back myself.
Plese preferably keep the context you are responding to rather than
starting a random new message.
Let me do that for you now:
> @@ -6637,6 +6641,164 @@
> </interleave>
> </element>
> </define>
> + <!--
> + throttlegroup is similar to <iotune>, however, group_name is required,
which is different from <iotune>
> + -->
> + <define name="throttlegroup">
> + <element name="throttlegroup">
> + <interleave>
> + <!-- required -->
> + <element name="group_name">
> + <text/>
> + </element>
> + <choice>
> + <element name="total_bytes_sec">
> + <data type="unsignedLong"/>
> + </element>
Preferrably all these definitions should be shared with <iotune> as any
change would now need to change two places.
It is okay if the schema is more lax (e.g. allowing 'group_name' to be
optional in the schema, which is then strictly required by the parser),
but it's not okay to have to change two distinct places.
> + <group>
> + <interleave>
> + <optional>
> + <element name="read_bytes_sec">
> + <data type="unsignedLong"/>
> + </element>
do you mean we use domain xml like the following one(which reuse
<iotune> definition):
<domain>
...
<iotunes>
<iotune>
<group_name>limit0</group_name>
<total_bytes_sec>10000000</total_bytes_sec>
<read_iops_sec>400000</read_iops_sec>
<write_iops_sec>100000</write_iops_sec>
</iotune>
</iotunes>
...
</devices>
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2' />
<source file='/var/lib/libvirt/images/disk.qcow2'/>
<target dev='vdh' bus='virtio'/>
<throttlefilters>
<throttlefilter iotune='limit2'/>
<throttlefilter iotune='limit012'/>
No, what I meant is to not reimplement the whole definition on the
schema jus tto make 'group_name' required by the schema itself.
You can make it required in the parser, and keep it optional in the
schema so that the schema can be reused in both places rather than
having two distinct copies of it that differ just in the optionality of
'group_name'.