Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices

[John Ferlan]

On 01/28/2016 06:30 AM, Pavel Hrdina wrote:
> On Wed, Jan 27, 2016 at 03:26:12PM -0500, John Ferlan wrote:
>> ...

 However, after some more "investigation" of the environment - I think
 perhaps there's still a couple of loose ends. Keeping the 'segtype'
 field may be necessary/useful... Details follow if interested ;-)
>>>
>>> Yes, you're right and I did a bad job during review.  The segtype is 
>>> required
>>> and we should always consider it, there is like 20 segtypes right now and 
>>> some
>>> of them could also use 'stripes'.
>>>
>>
>> The 'segtype' is currently only required because commit id '82c1740a'
>> added 'segtype' and 'stripes' to resolve a bz (or more than one
>> depending on how far you chase) by using 'segtype' to determine whether
>> more than one existed. It also did quite a few things in one step which
>> by today's review standards is a bad thing ;-).
>>
>> However, that only worked for "striped" lv's. If there was a "mirror"
>> lv, then while the mirror could be found, the vol-dumpxml output is
>> wrong because it only parsed 1 extent (incorrectly at that):
> 
> That leads me to conclusion, that we should parse only the segtypes that we 
> know
> how to parse.  The rest should be ignored.
> 
>>
>>   
>> 
>>   
>> 
>>   
>>
>> instead of something like (if using 'stripes' to get nsegments):
>>
>>   
>> 
>>   
>> 
>> 
>>   
>> 
>>   
>>
>> Linking 'nsegments' to 'striped' lv's is a "limitation".
>>
>> Anyway, dropping 'segtype' wasn't necessarily bad unless you needed
>> "more information", such as perhaps the lv thin pool source when
>> nsegments == 0. This becomes obvious once the change to use "(\\S*)"
>> instead of "(\\S+)" hits. Now we can "see" thin lv's in a volume group.
>> Not sure what else becomes visible, though.  The problem is you then get:
> 
> But we need more information and we propagate those information to user via 
> our
> API and that's why I think that we should parse only those volume which we 
> know
> how to parse to pass correct values to user.

I understand your point, but what is that list? What happens if/when we
"choose" something that works today?

The issues with the code now:

1. It doesn't recognize a thin lv

2. It doesn't parse a mirror or raid segtype extent correctly

Both are based around using the 'segtype' to make a decision to use the
'stripes' field in order to get the number of segments found. While it
does accurately give the number of devices for striped, mirror, raid,
thin, thin-pool, and linear lv's - it's not documented that way.

So by using 'segtype' to limit we've already caused an issue, so I'm not
sure using that to limit what we parse.

Using it to make a decision about how to parse a specific segtype I
think would be OK.

Secondarily, using 'stripes' as the count of extents is probably wrong
too. The count of segments should probably be made by actively parsing
the devices field and doing the VIR_APPEND_ELEMENT as you originally
suggested each time we parse something that has "string(#)", where
string and (#) are the parsed elements.

Let's see where this takes me ;-)

John
> 
> Pavel
> 
>>
>>  
>>  
>>
>> But that's fixable... The interesting part about  is that it's
>> an output only (virStorageVolDefParseXML doesn't read it). So, by adding
>> a new parse field 'pool_lv', we can then check 'segtype' to be "thin"
>> and if so, store the new field in a new vol->source.thin_pool field.
>>
>> Still cannot create a thin lv pool/volume without using the lvcreate
>> command. Nor can one create a mirror or stripe lv using libvirt's
>> vol-create command. One just cannot "find" a thin lv right now...
>>
>> John
>>

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

Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices

[Pavel Hrdina]

On Wed, Jan 27, 2016 at 03:26:12PM -0500, John Ferlan wrote:
> ...
> >>
> >> However, after some more "investigation" of the environment - I think
> >> perhaps there's still a couple of loose ends. Keeping the 'segtype'
> >> field may be necessary/useful... Details follow if interested ;-)
> > 
> > Yes, you're right and I did a bad job during review.  The segtype is 
> > required
> > and we should always consider it, there is like 20 segtypes right now and 
> > some
> > of them could also use 'stripes'.
> > 
> 
> The 'segtype' is currently only required because commit id '82c1740a'
> added 'segtype' and 'stripes' to resolve a bz (or more than one
> depending on how far you chase) by using 'segtype' to determine whether
> more than one existed. It also did quite a few things in one step which
> by today's review standards is a bad thing ;-).
> 
> However, that only worked for "striped" lv's. If there was a "mirror"
> lv, then while the mirror could be found, the vol-dumpxml output is
> wrong because it only parsed 1 extent (incorrectly at that):

That leads me to conclusion, that we should parse only the segtypes that we know
how to parse.  The rest should be ignored.

> 
>   
> 
>   
> 
>   
> 
> instead of something like (if using 'stripes' to get nsegments):
> 
>   
> 
>   
> 
> 
>   
> 
>   
> 
> Linking 'nsegments' to 'striped' lv's is a "limitation".
> 
> Anyway, dropping 'segtype' wasn't necessarily bad unless you needed
> "more information", such as perhaps the lv thin pool source when
> nsegments == 0. This becomes obvious once the change to use "(\\S*)"
> instead of "(\\S+)" hits. Now we can "see" thin lv's in a volume group.
> Not sure what else becomes visible, though.  The problem is you then get:

But we need more information and we propagate those information to user via our
API and that's why I think that we should parse only those volume which we know
how to parse to pass correct values to user.

Pavel

> 
>  
>  
> 
> But that's fixable... The interesting part about  is that it's
> an output only (virStorageVolDefParseXML doesn't read it). So, by adding
> a new parse field 'pool_lv', we can then check 'segtype' to be "thin"
> and if so, store the new field in a new vol->source.thin_pool field.
> 
> Still cannot create a thin lv pool/volume without using the lvcreate
> command. Nor can one create a mirror or stripe lv using libvirt's
> vol-create command. One just cannot "find" a thin lv right now...
> 
> John
> 

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

Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices

[Pavel Hrdina]

On Wed, Jan 27, 2016 at 10:05:36AM -0500, John Ferlan wrote:
> 
> 
> On 01/26/2016 09:55 AM, Pavel Hrdina wrote:
> > On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote:
> >> From: Joe Harvell 
> >>
> >> Since our 'devices' parsing logic now will use the 'nextents' (or
> >> lvs 'stripes' output) to decide whether or not to parse the field,
> >> use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)"
> >> (1 or more) when grabbing the 'groups[3]' or 'devices' field.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/storage/storage_backend_logical.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/storage/storage_backend_logical.c 
> >> b/src/storage/storage_backend_logical.c
> >> index 3010f58..2af3e69 100644
> >> --- a/src/storage/storage_backend_logical.c
> >> +++ b/src/storage/storage_backend_logical.c
> >> @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr 
> >> pool,
> >>   *striped, so "," is not a suitable separator either (rhbz 
> >> 727474).
> >>   */
> >>  const char *regexes[] = {
> >> -   
> >> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
> >> +/* name   orig   uuid   devs  stripes  segsz  vgextsz   
> >> sz lvattr */
> >> +   
> >> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
> >>  };
> >>  int vars[] = {
> >>  9
> > 
> > This could be squashed into the previous commit and if you are creating a
> > comment with labels, please use the exact names that lvs uses.
> > 
> 
> I'll just remove the comment - yes it's shorthand, but "vg_extent_size"
> is a bit long for the "([0-9]+)"...
> 
> BTW: I went separately mainly to make it more obvious of the change.
> 
> Patch 3 was just dealing with removing the need for the 'segtype' field
> as it was made obsolete by just using the stripes field to fill in the
> 'nextents'.
> 
> Patch 4 alters the regex to allow 0 or more, rather than at least 1.
> Although I do understand why combining them is also perfectly
> reasonable. I was trying not to do two things at one time...
> 
> However, after some more "investigation" of the environment - I think
> perhaps there's still a couple of loose ends. Keeping the 'segtype'
> field may be necessary/useful... Details follow if interested ;-)

Yes, you're right and I did a bad job during review.  The segtype is required
and we should always consider it, there is like 20 segtypes right now and some
of them could also use 'stripes'.

> 
> A 'vol-dumpxml' on a "found" thin lv from a thin-pool will provide an
> empty "" and the capacity == allocation; whereas, a
> 'vol-dumpxml' on a snapshot lv will provide the "... path='/dev/sdX'> with the  where the size of
> the extent of course matches the allocation value when seen with vol-info.
> 
> The  of a thin lv is a lv thin-pool and there is no extents data
> (e.g. nextents == 0). However, there is "pool_lv" data for a "thin"
> segtype which could then be used for display in the
> "...". There's still no way to "create" a thin pool from
> , but finding/displaying one is still possible... The details of
> which "/dev/sdX" is used is hidden by the lv mgr thin_pool code.
> 
> We don't display the thin_pool because by itself since it cannot be used
> as a volume, but that pool contains the information about the real
> sizes, so it's useful. Right now there's no way to get that useful
> information so going with the "size" (e.g. allocation) value would have
> to suffice. Future patches can perhaps adjust this.
> 
> John
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices

[John Ferlan]

...
>>
>> However, after some more "investigation" of the environment - I think
>> perhaps there's still a couple of loose ends. Keeping the 'segtype'
>> field may be necessary/useful... Details follow if interested ;-)
> 
> Yes, you're right and I did a bad job during review.  The segtype is required
> and we should always consider it, there is like 20 segtypes right now and some
> of them could also use 'stripes'.
> 

The 'segtype' is currently only required because commit id '82c1740a'
added 'segtype' and 'stripes' to resolve a bz (or more than one
depending on how far you chase) by using 'segtype' to determine whether
more than one existed. It also did quite a few things in one step which
by today's review standards is a bad thing ;-).

However, that only worked for "striped" lv's. If there was a "mirror"
lv, then while the mirror could be found, the vol-dumpxml output is
wrong because it only parsed 1 extent (incorrectly at that):

  

  

  

instead of something like (if using 'stripes' to get nsegments):

  

  


  

  

Linking 'nsegments' to 'striped' lv's is a "limitation".

Anyway, dropping 'segtype' wasn't necessarily bad unless you needed
"more information", such as perhaps the lv thin pool source when
nsegments == 0. This becomes obvious once the change to use "(\\S*)"
instead of "(\\S+)" hits. Now we can "see" thin lv's in a volume group.
Not sure what else becomes visible, though.  The problem is you then get:

 
 

But that's fixable... The interesting part about  is that it's
an output only (virStorageVolDefParseXML doesn't read it). So, by adding
a new parse field 'pool_lv', we can then check 'segtype' to be "thin"
and if so, store the new field in a new vol->source.thin_pool field.

Still cannot create a thin lv pool/volume without using the lvcreate
command. Nor can one create a mirror or stripe lv using libvirt's
vol-create command. One just cannot "find" a thin lv right now...

John

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

Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices

[John Ferlan]

On 01/26/2016 09:55 AM, Pavel Hrdina wrote:
> On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote:
>> From: Joe Harvell 
>>
>> Since our 'devices' parsing logic now will use the 'nextents' (or
>> lvs 'stripes' output) to decide whether or not to parse the field,
>> use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)"
>> (1 or more) when grabbing the 'groups[3]' or 'devices' field.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/storage/storage_backend_logical.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/storage/storage_backend_logical.c 
>> b/src/storage/storage_backend_logical.c
>> index 3010f58..2af3e69 100644
>> --- a/src/storage/storage_backend_logical.c
>> +++ b/src/storage/storage_backend_logical.c
>> @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr 
>> pool,
>>   *striped, so "," is not a suitable separator either (rhbz 727474).
>>   */
>>  const char *regexes[] = {
>> -   
>> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
>> +/* name   orig   uuid   devs  stripes  segsz  vgextsz   sz 
>> lvattr */
>> +   
>> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
>>  };
>>  int vars[] = {
>>  9
> 
> This could be squashed into the previous commit and if you are creating a
> comment with labels, please use the exact names that lvs uses.
> 

I'll just remove the comment - yes it's shorthand, but "vg_extent_size"
is a bit long for the "([0-9]+)"...

BTW: I went separately mainly to make it more obvious of the change.

Patch 3 was just dealing with removing the need for the 'segtype' field
as it was made obsolete by just using the stripes field to fill in the
'nextents'.

Patch 4 alters the regex to allow 0 or more, rather than at least 1.
Although I do understand why combining them is also perfectly
reasonable. I was trying not to do two things at one time...

However, after some more "investigation" of the environment - I think
perhaps there's still a couple of loose ends. Keeping the 'segtype'
field may be necessary/useful... Details follow if interested ;-)

A 'vol-dumpxml' on a "found" thin lv from a thin-pool will provide an
empty "" and the capacity == allocation; whereas, a
'vol-dumpxml' on a snapshot lv will provide the "... with the  where the size of
the extent of course matches the allocation value when seen with vol-info.

The  of a thin lv is a lv thin-pool and there is no extents data
(e.g. nextents == 0). However, there is "pool_lv" data for a "thin"
segtype which could then be used for display in the
"...". There's still no way to "create" a thin pool from
, but finding/displaying one is still possible... The details of
which "/dev/sdX" is used is hidden by the lv mgr thin_pool code.

We don't display the thin_pool because by itself since it cannot be used
as a volume, but that pool contains the information about the real
sizes, so it's useful. Right now there's no way to get that useful
information so going with the "size" (e.g. allocation) value would have
to suffice. Future patches can perhaps adjust this.

John

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

Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices

[Pavel Hrdina]

On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote:
> From: Joe Harvell 
> 
> Since our 'devices' parsing logic now will use the 'nextents' (or
> lvs 'stripes' output) to decide whether or not to parse the field,
> use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)"
> (1 or more) when grabbing the 'groups[3]' or 'devices' field.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend_logical.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_backend_logical.c 
> b/src/storage/storage_backend_logical.c
> index 3010f58..2af3e69 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
>   *striped, so "," is not a suitable separator either (rhbz 727474).
>   */
>  const char *regexes[] = {
> -   
> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
> +/* name   orig   uuid   devs  stripes  segsz  vgextsz   sz 
> lvattr */
> +   
> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
>  };
>  int vars[] = {
>  9

This could be squashed into the previous commit and if you are creating a
comment with labels, please use the exact names that lvs uses.

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

[libvirt] [PATCH 4/4] logical: Adjust regex for devices

[John Ferlan]

From: Joe Harvell 

Since our 'devices' parsing logic now will use the 'nextents' (or
lvs 'stripes' output) to decide whether or not to parse the field,
use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)"
(1 or more) when grabbing the 'groups[3]' or 'devices' field.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend_logical.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index 3010f58..2af3e69 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
  *striped, so "," is not a suitable separator either (rhbz 727474).
  */
 const char *regexes[] = {
-   
"^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
+/* name   orig   uuid   devs  stripes  segsz  vgextsz   sz 
lvattr */
+   
"^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
 };
 int vars[] = {
 9
-- 
2.5.0

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