[libvirt] [PATCH v4 2/6] vz: add migration backbone code

[Nikolay Shirokovskiy]

On 03.09.2015 19:45, Daniel P. Berrange wrote:
> On Wed, Sep 02, 2015 at 03:09:23PM +0300, Nikolay Shirokovskiy wrote:
>> From: nshirokovskiy at virtuozzo.com <nshirokovskiy at virtuozzo.com>
>>
>> This patch makes basic vz migration possible. For example by virsh:
>>
>> virsh -c vz:///system migrate $NAME vz+ssh://$DST/system --p2p
>>
>> Vz migration is implemented as p2p migration. The reason
>> is that vz sdk do all the job. The question may arise then
>> why don't implement it as a direct migration. The reason
>> is that we want to leverage rich libvirt authentication abilities
>> we lack in vz sdk. We can do it because vz sdk can use tokens to
>> factor out authentication from migration command.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>  src/vz/vz_driver.c |  192 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/vz/vz_sdk.c    |   33 +++++++++
>>  src/vz/vz_sdk.h    |    2 +
>>  3 files changed, 227 insertions(+), 0 deletions(-)
>>
> 
>> +static int
>> +vzDomainMigratePrepare3Params(virConnectPtr conn,
>> +                                virTypedParameterPtr params ATTRIBUTE_UNUSED,
>> +                                int nparams ATTRIBUTE_UNUSED,
>> +                                const char *cookiein ATTRIBUTE_UNUSED,
>> +                                int cookieinlen ATTRIBUTE_UNUSED,
>> +                                char **cookieout,
>> +                                int *cookieoutlen,
>> +                                char **uri_out ATTRIBUTE_UNUSED,
>> +                                unsigned int flags)
>> +{
>> +    vzConnPtr privconn = conn->privateData;
>> +    int ret = -1;
>> +
>> +    virCheckFlags(0, -1);
>> +
>> +    if (!(*cookieout = vzFormatCookie(privconn->session_uuid)))
>> +        goto cleanup;
>> +    *cookieoutlen = strlen(*cookieout) + 1;
> 
> As mentioned before, you should fill in uri_out here with the
> hypervisor URI to use for migration, by filling in the URI of
> the current host (ie dest host). eg
> 
>      char *thishost = virGetHostname();
>      virAsprintf(uri_out, "tcp://%s", thishost);
>      VIR_FREE(thishost);
> 
>> +    ret = 0;
>> +
>> + cleanup:
>> +    if (ret != 0) {
>> +        VIR_FREE(*cookieout);
>> +        *cookieoutlen = 0;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
> 
>> +static int
>> +vzDomainMigratePerform3Params(virDomainPtr domain,
>> +                              const char *dconnuri,
>> +                              virTypedParameterPtr params,
>> +                              int nparams,
>> +                              const char *cookiein ATTRIBUTE_UNUSED,
>> +                              int cookieinlen ATTRIBUTE_UNUSED,
>> +                              char **cookieout ATTRIBUTE_UNUSED,
>> +                              int *cookieoutlen ATTRIBUTE_UNUSED,
>> +                              unsigned int flags)
>> +{
>> +    int ret = -1;
>> +    virDomainObjPtr dom = NULL;
>> +    virConnectPtr dconn = NULL;
>> +    virURIPtr vzuri = NULL;
>> +    unsigned char session_uuid[VIR_UUID_BUFLEN];
>> +    vzConnPtr privconn = domain->conn->privateData;
>> +    char *cookie = NULL;
>> +    int cookielen = 0;
>> +
>> +    virCheckFlags(VZ_MIGRATION_FLAGS, -1);
>> +
>> +    if (virTypedParamsValidate(params, nparams, VZ_MIGRATION_PARAMETERS) < 0)
>> +        goto cleanup;
>> +
>> +    if (!(vzuri = vzMakeVzUri(dconnuri)))
>> +        goto cleanup;
> 
> This is not right - you can't use the libvirt URI to form the
> hypervisor migration URI, since the libvirt URI may not in
> fact refer to the hypervisor host.
> 
> eg people may be accessing libvirt(d) via a SSH tunnel in
> which case the dconnuri would include 'localhost' and not
> the actual target host. This is why you must fill in the
> uri_out parameter in the Prepare() method and use that,
> if the "migrateuri" parameter is not provided in the
> virTypedParams array.
> 
>> +
>> +    if (!(dom = vzDomObjFromDomain(domain)))
>> +        goto cleanup;
>> +
>> +    dconn = virConnectOpen(dconnuri);
>> +    if (dconn == NULL) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED,
>> +                       _("Failed to connect to remote libvirt URI %s: %s"),
>> +                       dconnuri, virGetLastErrorMessage());
>> +        goto cleanup;
>> +    }
>> +
>> +    /* NULL and zero elements are unused */
>> +    if (virDomainMigratePrepare3Params(dconn, NULL, 0, NULL, 0,
>> +                                       &cookie, &cookielen, NULL, 0) < 0)
>> +        goto cleanup;
> 
> Since this is implementing v3, I'd prefer to see you provide the
> full set of 5 callbacks, even though they will currently be no-ops.
> This provides better future proofing for the migration impl in case
> those become neccessary later.
> 
> You can also then trivially implement the non-p2p plain migration
> in this method, by checking whether or not the PEER2PEER flag
> is set or not. If it is not set, you can just skip the connect
> open & prepare calls on the basis that libvirt will have done
> them for you.
Ok.
> 
>> +
>> +    if (vzParseCookie(cookie, session_uuid) < 0)
>> +        goto cleanup;
>> +
>> +    if (prlsdkMigrate(dom, vzuri, session_uuid) < 0)
>> +        goto cleanup;
>> +
>> +    virDomainObjListRemove(privconn->domains, dom);
>> +    dom = NULL;
>> +
>> +    ret = 0;
>> +
>> + cleanup:
>> +    if (dom)
>> +        virObjectUnlock(dom);
>> +    virObjectUnref(dconn);
>> +    virURIFree(vzuri);
>> +    VIR_FREE(cookie);
>> +
>> +    return ret;
>> +}
>> +
>>  static virHypervisorDriver vzDriver = {
>>      .name = "vz",
>>      .connectOpen = vzConnectOpen,            /* 0.10.0 */
>> @@ -1396,6 +1585,9 @@ static virHypervisorDriver vzDriver = {
>>      .domainBlockStatsFlags = vzDomainBlockStatsFlags, /* 1.2.17 */
>>      .domainInterfaceStats = vzDomainInterfaceStats, /* 1.2.17 */
>>      .domainMemoryStats = vzDomainMemoryStats, /* 1.2.17 */
>> +    .connectSupportsFeature = vzConnectSupportsFeature, /* 1.2.20 */
>> +    .domainMigratePrepare3Params = vzDomainMigratePrepare3Params, /* 1.2.20 */
>> +    .domainMigratePerform3Params = vzDomainMigratePerform3Params, /* 1.2.20 */
> 
> Somewhat annoyingly you also need to implement the callbacks for
> .domainMigratePrepare3 and .domainMigratePerform3, as we don't
> automatically convert non-params usage to the params based
> method AFAICT.
> 
> Your impl of .domainMigratePerform3 could pack the values into a
> virTypedParams array and then call .domainMigratePerform3Params,
> or do the reverse.
Yes, without plain(non-params) callbacks we get working only toURI3
API function and I create a patch not included in this patchset
to make toURI{1,2} work too. I take this approach of converting
parameters and use one common worker function but patch a different
place - API implementaion itself. So I'll include this patch
in next version of the set.

As in this case I need to patch 2 different sets of API implementation
*migrate{N} and *migrateURI{N} I'd rather put direct managed support
to a different patchset. Is it ok?


> 
> Regards,
> Daniel
>