[libvirt] [PATCH 2/3] storage: optional 'refresh_volume_allocation' attribute on pool

[Jason Dillaman]

On Tue, Mar 19, 2019 at 5:52 AM Michal Privoznik <mprivozn at redhat.com> wrote:
>
> On 3/15/19 4:17 PM, jdillama at redhat.com wrote:
> > From: Jason Dillaman <dillaman at redhat.com>
> >
> > When this attribute is set to 'capacity', refreshing volume
> > allocations within the pool will report the capacity for the
> > allocation. This is useful for certain backends where computing
> > the actual allocation of a volume might be an expensive
> > operation.
> >
> > Signed-off-by: Jason Dillaman <dillaman at redhat.com>
> > ---
> >   docs/formatstorage.html.in                    | 15 +++++++++---
> >   docs/schemas/storagecommon.rng                |  7 ++++++
> >   docs/schemas/storagepool.rng                  |  5 ++++
> >   src/conf/storage_conf.c                       | 23 +++++++++++++++++++
> >   src/conf/storage_conf.h                       |  9 ++++++++
> >   .../pool-rbd-refresh-volume-allocation.xml    | 12 ++++++++++
> >   .../pool-rbd-refresh-volume-allocation.xml    | 15 ++++++++++++
> >   tests/storagepoolxml2xmltest.c                |  1 +
> >   8 files changed, 84 insertions(+), 3 deletions(-)
> >   create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml
> >   create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml
> >
> > diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> > index 968651330f..d40e463167 100644
> > --- a/docs/formatstorage.html.in
> > +++ b/docs/formatstorage.html.in
> > @@ -16,7 +16,7 @@
> >       </p>
> >       <p>
> >         The top level tag for a storage pool document is 'pool'. It has
> > -      a single attribute <code>type</code>, which is one of <code>dir</code>,
> > +      an attribute <code>type</code>, which is one of <code>dir</code>,
> >         <code>fs</code>, <code>netfs</code>, <code>disk</code>,
> >         <code>iscsi</code>, <code>logical</code>, <code>scsi</code>
> >         (all <span class="since">since 0.4.1</span>),
> > @@ -27,8 +27,17 @@
> >         <code>zfs</code> (<span class="since">since 1.2.8</span>),
> >         <code>vstorage</code> (<span class="since">since 3.1.0</span>),
> >         or <code>iscsi-direct</code> (<span class="since">since 4.7.0</span>).
> > -      This corresponds to the
> > -      storage backend drivers listed further along in this document.
> > +      This corresponds to the storage backend drivers listed further along in
> > +      this document.
> > +    </p>
> > +    <p>
> > +      The 'pool' element may also have an optional
> > +      <code>refresh_volume_allocation</code> attribute to control how
> > +      volume allocation is computed during a refresh. Valid values
> > +      are <code>default</code> to compute the actual usage or
> > +      <code>capacity</code> to default the allocation to the logical
> > +      capacity for cases where computing the allocation is too expensive.
> > +      <span class="since">Since 5.1.1</span>
>
> Since 5.2.0 ;-) We change minor for every release, and micro is for
> asynchronous releases, where we backport some patches (not that we've
> ever done that, except maybe for v3.2.1).
>
> Anyway, I don't like the name of this attribute. In my book, if an
> attribute has to consist of two or more words that calls for having an
> element for each of the words but the last which then can be as an
> attribute. So how about:
>
> <pool>
>    <refresh method='default|capacity'/>
>    ...
> </pool>

Thanks for the suggestion ... I didn't love it either.

> This is also more future proof IMO as it allows us to expand <refresh/>
> should we ever need that.
>
> The coode looks good otherwise.
>
> Michal



-- 
Jason