On Wed, Apr 15, 2015 at 6:43 PM, Michal Privoznik <[email protected]> wrote:
> On 15.04.2015 16:38, Pavel Boldin wrote: > > Michal, > > > > On Wed, Apr 15, 2015 at 10:54 AM, Michal Privoznik <[email protected]> > > wrote: > > > >> On 26.03.2015 15:48, Pavel Boldin wrote: > >>> Dear Libvirt Developers, > >>> > >>> > >>> I'm working to implement feature request [1]. The feature request > >> proposes > >>> to enhance `libvirt' code so the API caller can specify which block > >> devices > >>> are to be migrated using e.g. parameters in the > `virDomainMigrateToURI3' > >>> call. > >> > >> Correct. My idea is to add pairs of typed parameters to the > >> virDomainMigrate3() and virDomainMigrateToURI3() API. These are the only > >> migrate APIs that accept virTypedParameter. The rest of migrate APIs are > >> just too old and not extensible, so they must continue working as they > >> are now. However, those two APIs - they can be passed new pairs, e.g. > >> ("migrate_disk", "vda"). Now, mgmt applications will want to specify > >> more than one disk to migrate, obviously. We have two options: > >> > >> 1) make "migrate_disk" accept stringyfied array of disk targets, e.g. > >> "migrate_disk": "vda,vdb,sda". This is not so pleasant to work with from > >> client applications. Constructing the string would be awful. > >> > > My concern was that some drivers may allow commas inside the drive names. > > So this is obviously wrong thing to do then. > > That's why I've chosen to work purely with disk target at NBD level. We > have strong rules what characters can occur there. Moreover, it's fairly > easy to derive qemu disk ID from the target. Oh, and we require targets > to be unique throughout the domain. So I think it's the best option for > the extension you're planning. > As far as I remember, libvirt-1.2.2 does not support tunneled migration through NBD. > > > > > > >> > >> 2) make virTypedParam* APIs accept more than one values to a key, e.g. > >> {("migrate_disk", "vda"), ("migrate_disk", "vdb"), ("migrate_disk", > >> "sda")}. Currently, this is supported on RPC, but some virTypedParam* > >> APIs are not really prepared for this. If I had to name some from the > >> top of my head: virTypedParamsGet(). > >> > > As far as I see there is a line 420 in src/util/virtypedparam.c: > > /* The following APIs are public and their signature may never change. */ > > Of course. They would still return the first value found. But that's not > a problem. The paragraph is more like an intro than a suggestion to > change them. > > > > > So, the functions need to be implemented anew. > > Some of them. Correct. > > > > > > > > >> > >> I view virTyped* as an associative array. So 2) is practically enabling > >> multi value associative array. So I guess the first step would be to > >> introduce new API (maybe set of APIs?) that are aware of this. > >> > > I guess the first function to implement would be virTypedParamsGetNth() > > that is able to retrieve nth parameter with that name. > > While this is easy to implement there are details remain: > > > > 1. Should we implement a new set of virTypedParamsGetNth<TYPE> for > each > > of the TYPE or should we start with just the 'String'? > > 2. Since virTypedParamsGet<TYPE> is just a virTypedParamsGetNth<TYPE> > > with N=0 should we make old functions just call new ones with N=0? > > > > Hm.. what about this, introduce just this new function: > > virTypedParamsGetArrayForKey(.., const char *key, ...) > > which will iterate through an array of typed params, producing a new > array which will, however, contain only the selected key. For instance, > if called over > > {(key1, val1), (key1, val2), (key2, val3), (key4, val4)} > > with key==key1 it would create array > > {(key1, val1), (key1, val2)} > > with key==key2 it would create array > > {(key2, val3)} > > And so on. For selecting Nth item in the array, we can just use basic C > array selector []. Oh, I don't like the function name much btw, but it > serves the example sake here. > Well, this approach allows us to return virTypedParamPtr array of any type. So this is more general and the latter can be implemented as a call to this one. > > The other option would be to have a different function: > > char ** > virTypedParamGetStringList( ..., const char *key) > > which will produce a string list of values assigned with the @key. The > list will be NULL terminated of course. Again, the function name is > horrible, but it's just an example. > > In fact, the latter approach would be much more easier to use, so I vote > for it. > Yeah, and it is much easier to add an extra parameter `char **migrate_disk' to all the QEMU functions stack. I think I will implement both, second one calling first one. > > Oh, and these functions I came up with - they don't even need to be > public! We can keep them private and live happily ever after. Well, not > quite - the virTypedParamsValidate() will require some changes too, but > whatever. > We will need to add a `virTypedParamsValidateDups' function or something like that. I'm going to start looking into the code now. Hopefully, I will have an API implementation till the end of the week. No comments below this line. > > > > > > >> > >> Then, virDomainMigrate*3() can just use this in the following way: > >> > >> a) when no "migrate_disk" is specified, the current behaviour is > preserved > >> > >> b) when there are some disk selected for storage migration, since at > >> this point we have virTyped* APIs to work with multivalue, we can get > >> the array of disk targets to migrate and instruct qemu to just migrate > >> them. > >> > >> Now, qemu has split storage and internal state migration into two > >> streams. The first one is called NBD and libvirt selectively choses > >> disks to migrate. For the other stream you don't have to care about. > >> Look at qemuMigrationDriveMirror() and you'll see how libvirt instructs > >> qemu to selectively migrate only some disks. The "migrate_disk" array > >> would need to be propagated down here then. > >> > > My concern is I would, most likely, have to backport these to our > versions. > > I'm not sure onto how old release, but in general, the smaller the patch > is, the easier to backport it is also ;-) > > Michal > Pavel
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
