[libvirt] Refactoring of storage pool
John Ferlan
jferlan at redhat.com
Thu Jan 26 22:23:32 UTC 2017
On 01/26/2017 11:16 AM, Olga Krishtal wrote:
> On 26/01/17 17:49, Daniel P. Berrange wrote:
>> On Wed, Jan 25, 2017 at 08:02:30PM +0300, Olga Krishtal wrote:
>>> Hi everyone!
>>> Half a year ago we started discussion about filesystem pools:
>>> https://www.redhat.com/archives/libvir-list/2016-April/msg01941.html
>>> https://www.redhat.com/archives/libvir-list/2016-May/msg00208.html
>>> https://www.redhat.com/archives/libvir-list/2016-September/msg00463.html
>>>
>>> At the end we have decided not to use copy/paste and try to merge code:
>>> https://www.redhat.com/archives/libvir-list/2016-December/msg00051.html
>>>
>>> However, after first try it had became obvious (the discussion is in
>>> the link above)
>>> that pools can be describe as a separate object, that later will be
>>> used in storage pool,
>>> filesystem pool, vgpu pool, etc. This latter is an extension of
>>> previous discussion;
>>> I just want to separate refactoring of storage pools and the
>>> implementation of
>>> filesystem pools a bit.
>> I'm not really in favour of trying to merge the code for storage pools
>> and filesystem pools at the XML parsing level. While they do share a
>> lot of common concepts, I expect that they'll need to grow apart over
>> time. Having one parsing two to deal with what is going to be two
>> similar, but none the less, distinct formats is going to cause as
>> many problems as it solves IMHO.
>>
>>> /*
>>> * virpool.h: pool utility to manage structures commonly used for
>>> pools.
>>> *
>>> */
>>>
>>> /* The dscription of pool element
>>> * For storage pool it used to be virStorageVolDef
>>> * instead of key we use uuid, because key is specific
>>> * for storage pools, and for other pool types (filesystem pools)
>>> * or gpu pool we do nor have key.
>> This kind of difference is just one example of why I think it is counter-
>> productive to merge these XML parses into one.
>>
>>> * type field is also specific - that is why we put it into
>>> * void* ElementTypeDef.
>>> * ElementTypeDef - stores internal information about pool
>>> element(volume, item, etc)
>>> * (the same as privateData for virConnectPtr)
>>> * For example, for storage pool voluems it consists from:
>>> * int type;
>>> * virStorageVolSource source;
>>> * virStorageSource target;
>>> */
>>>
>>> #include "virhash.h"
>>>
>>> typedef enum {
>>> VIR_POOL_ELEMENT_VOLUME,
>>> VIR_POOL_ELEMENT_FILESYSTEM,
>>>
>>> VIR_POOL_ELEMENT_LAST,
>>> } virPoolElemenType;
>>> VIR_ENUM_DECL(virPoolElementType)
>>>
>>> typedef void (*virPoolElementTypeDefFree) (void *PoolElementDef);
>>>
>>> typedef struct _virPoolElementDef virPoolElementDef;
>>> typedef virPoolElementDef *virPoolElementDefPtr;
>>> struct _virPoolElementDef {
>>> char *name;
>>> char *uuid;
>>> bool building;
>>> unsigned int in_use;
>>> int type;
>>> void* ElementTypeDef;
>>> virPoolElementTypeDefFree PoolElementDefFree;
>>> };
>>>
>>> typedef struct _virPoolElementDefList virPoolElementDefList;
>>> typedef virPoolElementDefList *virPoolElementDefListPtr;
>>> struct _virPoolElementDefList {
>>> size_t count;
>>> virPoolElementDefPtr *elements;
>>> };
>>>
>>> /* General information about pool source*/
>>> typedef struct _virPoolSourceHost virPoolSourceHost;
>>> typedef virPoolSourceHost *virPoolSourceHostPtr;
>>> struct _virPoolSourceHost {
>>> char *name;
>>> int port;
>>> };
>>>
>>> typedef struct _virPoolAuthDef virPoolAuthDef;
>>> typedef virPoolAuthDef *virPoolAuthDefPtr;
>>> struct _virPoolAuthDef {
>>> char *username;
>>> char *secrettype; /* <secret type='%s' for disk source */
>>> int authType; /* virStorageAuthType */
>>> virSecretLookupTypeDef seclookupdef;
>>> };
>>>
>>> typedef enum {
>>> VIR_POOL_SOURCE_DIR,
>>> VIR_POOL_SOURCE_DEVICE,
>>> VIR_POOL_SOURCE_NAME,
>>>
>>> VIR_POOL_SOURCE_LAST,
>>> } virPoolSourcePathType;
>>> VIR_ENUM_DECL(virPoolSourcePathType)
>>>
>>> typedef struct _virPoolSourcePath virPoolSourcePath;
>>> typedef virPoolSourcePath *virPoolSourcePathPtr;
>>> struct _virPoolSourcePath {
>>> virPoolSourcePathType sourcetype;
>>> char *path;
>>> };
>>>
>>> typedef void(*virPoolPrivDataFree) (void *privePoolData);
>>>
>>> typedef struct _virPoolSource virPoolSource;
>>> typedef virPoolSource *virPoolSourcePtr;
>>> struct _virPoolSource{
>>> /* One or more host definitions */
>>> size_t nhost;
>>> virPoolSourceHostPtr hosts;
>>>
>>> /* Authentication information */
>>> virPoolAuthDefPtr auth;
>>>
>>> /* One or more devices */
>>> size_t nsrcpath;
>>> virPoolSourcePathPtr srcpaths;
>>>
>>> /* Name of the source */
>>> char *name;
>>> /* Vendor of the source */
>>> char *vendor;
>>>
>>> /* Product name of the source */
>>> char *product;
>>> /*Some private data */
>>> void *privatePoolData;
>>> virPoolPrivDataFree privDataFree;
>>> };
>>>
>>> typedef struct _virPoolPathPerms virPoolPathPerms;
>>> typedef virPoolPathPerms *virPoolPathPermsPtr;
>>> struct _virPoolPathPerms {
>>> mode_t mode;
>>> uid_t uid;
>>> gid_t gid;
>>> char *label;
>>> }
>>>
>>>
>>> typedef struct _virPoolTarget virPoolTarget;
>>> typedef virPoolTarget *virPoolTargetPtr;
>>> struct _virPoolTarget{
>>> char *path; /* Optional path to target */
>>> virPoolPathPerms perms; /* Default permissions for path */
>>> };
>>> typedef struct _virPoolDef virPoolDef;
>>> typedef virPoolDef *virPoolDefPtr;
>>> struct _virPoolDef {
>>> char *name;
>>> unsigned char uuid[VIR_UUID_BUFLEN];
>>>
>>> virPoolSource source;
>>> virPoolTarget target;
>>> };
>>>
>>> typedef struct _virPoolInfo virPoolInfo; // used to be virPoolObj
>>> typedef virPoolInfo *virPoolInfoPtr;
>>> struct _virPoolInfo {
>>>
>>> char *configFile;
>>> char *autostartLink;
>>> bool active;
>>> int autostart;
>>> unsigned int asyncjobs;
>>>
>>> virPoolDefPtr def;
>>> virPoolDefPtr newDef;
>>>
>>> virPoolElementDefList elementsdef;
>>> };
>> This is all still pretty specific to storage pools / filesystme pools. I
>> don't think it is going to nicely extend to cover other things like GPU
>> pools.
>
> I completely understand you, because it is difficult to predict and
> describe
> features for vGPU pools without any example. So yes, it is mostly
> storage/filesystem pools
> specific. There still left private structs for more freedom.
>
Sorry the GPU stuff was just "in my head" at the time. One of the ideas
being floated was (ahem) borrowing the StoragePool code in order to make
a pool of GPU's, but I haven't followed it closely since then. So, yeah
let's not worry about it. Actually some other work I started may help
that effort more.
>>
>> My preference is still to stick with the patches you previously
>> proposed at
>>
>>
>> https://www.redhat.com/archives/libvir-list/2016-December/msg00051.html
>>
>> so that future changes made to help storage pools don't inadvetantly
>> affect filesystem code or vica-verca.
Still there are some concepts that are duplicated and a bug fix for one
would be beneficial to have in the other rather than having to make the
same change in two places because there's just piles of cut-n-paste code
or not making in one place and having the same bug in the other place.
The task is to figure out what those could be.
>
> I like this version too, (it should be slightly changed and rebased).
> However, this refactoring example is the result of discussions with
> John Ferlan.
> (He actually got pretty good ideas changes in storage pools).
> We need John's opinion so we all know that we are on the same page.
Even after reading a couple of times, I cannot recall the entire frame
of reference from my responses to the patch stream. What I do recall is
seeing so much cut-n-paste code that it concerned me. Now for external
API's there's probably no way around that. However, for the internal
inner workings I would think if you find yourself essentially copying
the *whole* module or *whole* data structure to be used by the FSPool,
then that's where a data structures and code could be made common. For
data structures that includes (at least and off the top of my head)
things such as:
virStorageVolDef
virStorageVolObj
virStoragePoolDef
virStoragePoolObj
virStorageSourcePtr
virStoragePoolTargetPtr
The bugger is *how* best to do that. I don't have all the answers here,
but I would prefer to see a logical approach that will allow you to
continue to make progress while also leaving me enough time to work
through stuff I'm on the hook for ;-).
One hard part of this for me is the "FS" nomenclature being used - I
keep thinking about the WITH_STORAGE_FS type code. Still with all the
changes for the vstorage changes "in" now - I hope you can understand
what type and a methodology to "share" data structures/code.
FWIW: Beyond your changes - there's been even more changes made to the
code since you posted in December and other changes I have floating
around in a couple git branches which were started because of thinking
about what you were trying to accomplish.
One set of changes is to try to "objectify" the guts (e.g.
virStorage*Obj[List]Ptr) in order to make a more consistent API to
access driver objects. I have that code compiling, passing the local
test infrastructure, but it's not complete yet. I know that will be
useful for the FSPool - the bulk of those virFSPoolObj would get common
code to use. My plan is to post an RFC soon, but I want the vHBA test
and node_device changes I posted yesterday to get pushed since my branch
touches the same area...
The second set of changes is to make accessors for all virStorageSource
data rather than having so many places "think" it knows what to do (I've
already found/fixed a couple of places where the wrong type field was
being used). I need to update those changes to merge in Peter's
storage_util changes. But given your patch 1 - I think those would be a
good starting point.
Not sure I answered the base question. You have in your mind where you
need to get to with the FSPool and what's been essentially copied. With
more time now looking at things - what changes do you believe could make
use of common code.
John
More information about the libvir-list
mailing list