Chunyan Liu wrote:

Add migration APIs for libxl driver. Implemented in migration version 3. Based on xen 4.1.

I didn't get a chance to test this yet, but have some initial review comments.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_driver.c | 617 ++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_driver.h | 17 ++- 2 files changed, 632 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d5fa64a..4037635 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -30,6 +30,12 @@ #include <math.h> #include <libxl.h> #include <fcntl.h> +#include <stdlib.h> +#include <string.h> +#include <sys/types.h> +#include <sys/socket.h> +#include <arpa/inet.h> +#include <netdb.h>

#include "internal.h" #include "logging.h" @@ -60,6 +66,13 @@ #define XEN_SCHED_CREDIT_NPARAM 2

static libxlDriverPrivatePtr libxl_driver = NULL; +static virThread migrate_receive_thread;

This prevents receiving concurrent migrations.

+ +typedef struct migrate_receive_args { + virConnectPtr conn; + virDomainObjPtr vm; + int sockfd; +} migrate_receive_args;

If there is a future need to extend this structure, will it cause incompatibility issues between a source with the extensions and a destination without? Or vise versa?

/* Function declarations */ static int @@ -1095,6 +1108,17 @@ libxlClose(virConnectPtr conn ATTRIBUTE_UNUSED) return 0; }

+static int +libxlSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED, int feature) +{ + switch (feature) { + case VIR_DRV_FEATURE_MIGRATION_V3: + return 1; + default: + return 0; + } +} + static const char * libxlGetType(virConnectPtr conn ATTRIBUTE_UNUSED) { @@ -3842,12 +3866,600 @@ libxlIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) return 1; }

+static int libxlReadFixedMessage(int fd, const void *msg, int msgsz) +{ + char buf[msgsz]; + + if (saferead(fd, buf, msgsz) != msgsz || memcmp(buf, msg, msgsz)) { + return -1; + } + + return 0; +} + +static int doParseURI(const char *uri, char **p_hostname, int *p_port) +{ + char *p, *hostname; + char port[16] = "0"; + + if (strstr (uri, "//")) { /* Full URI. */ + virURIPtr uriptr = virURIParse(uri); + if (!uriptr) { + libxlError(VIR_ERR_INVALID_ARG, + _("Invalid URI")); + return -1; + } + if (uriptr->scheme && STRCASENEQ(uriptr->scheme, "xlmigr")) { + libxlError(VIR_ERR_INVALID_ARG, + _("Only xlmigr://" + " migrations are supported by Xen")); + xmlFreeURI(uriptr); + return -1; + }

This is just tcp migration. Any reason for requiring the "xlmigr" scheme?

+ if (!uriptr->server) { + libxlError(VIR_ERR_INVALID_ARG, + _("A hostname must be" + " specified in the URI")); + xmlFreeURI(uriptr); + return -1; + } + hostname = strdup(uriptr->server);

I think it would be better to rework this, and other uses of strdup() and snprintf() in this function, to use virAsprintf().

+ if (!hostname) { + virReportOOMError(); + xmlFreeURI(uriptr); + return -1; + } + if (uriptr->port) + snprintf(port, sizeof(port), "%d", uriptr->port); + xmlFreeURI(uriptr); + } + else if ((p = strrchr(uri, ':')) != NULL) { /* "hostname:port" */ + int port_nr, n; + + if (virStrToLong_i(p+1, NULL, 10, &port_nr) < 0) { + libxlError(VIR_ERR_INVALID_ARG, + _("Invalid port number")); + return -1; + } + snprintf(port, sizeof(port), "%d", port_nr); + + /* Get the hostname. */ + n = p - uri; /* n = Length of hostname in bytes. */ + hostname = strdup(uri); + if (!hostname) { + virReportOOMError(); + return -1; + } + hostname[n] = '\0'; + } + else {/* "hostname" (or IP address) */ + hostname = strdup(uri); + if (!hostname) { + virReportOOMError(); + return -1; + } + } + *p_hostname = hostname; + *p_port = atoi(port); + return 0; +} + +static bool libxlMigrationFromIsAllowed(libxlDriverPrivatePtr driver, virDomainObjPtr vm) +{ + /* to be extended */ + VIR_DEBUG("driver=%p, vm=%p", driver, vm); + return true; +} + +static bool libxlMigrationToIsAllowed(libxlDriverPrivatePtr driver, virDomainDefPtr def) +{ + /* to be extended */ + VIR_DEBUG("driver=%p, def=%p", driver, def); + return true; +}

I think these functions (and their call sites) can be added in a future patch that provides an implementation.

+ +static char * +libxlDomainMigrateBegin3(virDomainPtr domain, + const char *xmlin, + char **cookieout ATTRIBUTE_UNUSED, + int *cookieoutlen ATTRIBUTE_UNUSED, + unsigned long flags, + const char *dname ATTRIBUTE_UNUSED, + unsigned long resource ATTRIBUTE_UNUSED) +{ + libxlDriverPrivatePtr driver = domain->conn->privateData; + virDomainObjPtr vm; + virDomainDefPtr def = NULL; + char *xml = NULL; + + virCheckFlags(LIBXL_MIGRATION_FLAGS, NULL); + + libxlDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(domain->uuid, uuidstr); + libxlError(VIR_ERR_OPERATION_INVALID, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (!virDomainObjIsActive(vm)) { + libxlError(VIR_ERR_OPERATION_INVALID, + _("domain is not running")); + goto cleanup; + } + + if (!libxlMigrationFromIsAllowed(driver, vm)) { + libxlError(VIR_ERR_OPERATION_INVALID, + _("domain cannot do migration")); + goto cleanup; + } + + if (xmlin) { + if (!(def = virDomainDefParseString(driver->caps, xml, + 1 << VIR_DOMAIN_VIRT_XEN, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE);

Do we need to ensure the name provided in xmlin is the same as def->name?

+ } else { + xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE); + } + +cleanup: + if (vm) + virDomainObjUnlock(vm); + libxlDriverUnlock(driver); + return xml; +} + +static void doMigrateReceive(void *opaque) +{ + migrate_receive_args *data = opaque; + virConnectPtr conn = data->conn; + int sockfd = data->sockfd; + virDomainObjPtr vm = data->vm; + libxlDriverPrivatePtr driver = conn->privateData; + int recv_fd; + struct sockaddr_in new_addr; + socklen_t socklen = sizeof(new_addr); + int len; + + do { + recv_fd = accept(sockfd, (struct sockaddr *)&new_addr, &socklen); + } while(recv_fd < 0 && errno == EINTR); + + VIR_DEBUG("Accepted migration\n");

This should be moved after checking for a valid recv_fd.

+ if (recv_fd < 0) { + VIR_DEBUG("Could not accept migration connection\n");

libxlError?

+ goto cleanup; + } + + len = sizeof(migrate_receiver_banner); + if (safewrite(recv_fd, migrate_receiver_banner, len) != len) { + libxlError(VIR_ERR_OPERATION_FAILED, + _("Failed to write migrate_receiver_banner")); + goto cleanup; + } + + if (libxlVmStart(driver, vm, false, recv_fd) < 0) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restore domain with libxenlight")); + if (!vm->persistent) { + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; + } + goto cleanup; + } + + len = sizeof(migrate_receiver_ready); + if (safewrite(recv_fd, migrate_receiver_ready, len) != len) { + libxlError(VIR_ERR_OPERATION_FAILED, + _("Failed to write migrate_receiver_ready")); + } + +cleanup: + if (VIR_CLOSE(recv_fd) < 0) + virReportSystemError(errno, "%s", _("cannot close recv_fd")); + if (VIR_CLOSE(sockfd) < 0) + virReportSystemError(errno, "%s", _("cannot close sockfd")); + if (vm) + virDomainObjUnlock(vm); + VIR_FREE(opaque); + return; +} + +static int doMigrateSend(virDomainPtr dom, unsigned long flags, int sockfd) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + libxlDomainObjPrivatePtr priv; + libxl_domain_suspend_info suspinfo; + virDomainEventPtr event = NULL; + int live = 0; + int ret = -1; + + if (flags & VIR_MIGRATE_LIVE) + live = 1; + + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + libxlError(VIR_ERR_OPERATION_INVALID, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + priv = vm->privateData; + + /* read fixed message from dest (ready to receive) */ + if (libxlReadFixedMessage(sockfd, migrate_receiver_banner, + sizeof(migrate_receiver_banner))) { + goto cleanup; + } + + /* send suspend data */ + memset(&suspinfo, 0, sizeof(suspinfo)); + if (live == 1) + suspinfo.flags |= XL_SUSPEND_LIVE; + if (libxl_domain_suspend(&priv->ctx, &suspinfo, vm->def->id, sockfd) != 0) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("Failed to save domain '%d' with libxenlight"), + vm->def->id); + goto cleanup; + } + + /* read fixed message from dest (receive completed) */ + if (libxlReadFixedMessage(sockfd, migrate_receiver_ready, + sizeof(migrate_receiver_ready))) { + /* Src side should be resumed, but for ret < 0, virsh won't call Src side + * Confirm3, handle it here. + */ + libxl_domain_resume(&priv->ctx, vm->def->id); + goto cleanup; + } + + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + if (event) + libxlDomainEventQueue(driver, event); + return ret; +} + +static int +libxlDomainMigratePrepare3(virConnectPtr dconn, + const char *cookiein ATTRIBUTE_UNUSED, + int cookieinlen ATTRIBUTE_UNUSED, + char **cookieout ATTRIBUTE_UNUSED, + int *cookieoutlen ATTRIBUTE_UNUSED, + const char *uri_in, + char **uri_out, + unsigned long flags, + const char *dname, + unsigned long resource ATTRIBUTE_UNUSED, + const char *dom_xml) +{ + libxlDriverPrivatePtr driver = dconn->privateData; + virDomainDefPtr def = NULL; + virDomainObjPtr vm = NULL; + int port = 0; + int sockfd = -1; + struct sockaddr_in addr; + migrate_receive_args *args; + int ret = -1; + + virCheckFlags(LIBXL_MIGRATION_FLAGS, -1); + + libxlDriverLock(driver); + if (!dom_xml) { + libxlError(VIR_ERR_OPERATION_INVALID, + _("no domain XML passed")); + goto cleanup; + } + + def = virDomainDefParseString(driver->caps, dom_xml, + 1 << VIR_DOMAIN_VIRT_XEN, + VIR_DOMAIN_XML_INACTIVE); + + if (!libxlMigrationToIsAllowed(driver, def)) { + libxlError(VIR_ERR_OPERATION_INVALID, + _("cannot migrate domain to this host")); + goto cleanup; + } + + /* Target domain name, maybe renamed. */ + if (dname) { + def->name = strdup(dname);

This leaks the original name.

+ if (def->name == NULL) + goto cleanup; + } + + if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) + goto cleanup; + + if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, def, true))) + goto cleanup; + + def = NULL; + + /* Create socket connection to receive migration data */ + if (!uri_in) { + *uri_out = virGetHostname(dconn); + if (*uri_out == NULL) + goto cleanup; + } else { + char *hostname = NULL; + doParseURI(uri_in, &hostname, &port); + VIR_FREE (hostname); + } + + if (port <= 0) + port = DEFAULT_MIGRATION_PORT; + + sockfd = socket(AF_INET, SOCK_STREAM, 0); + if (sockfd == -1) { + libxlError(VIR_ERR_OPERATION_INVALID, + _("Failed to create socket"));

This, and the following errors, could use a little more information. E.g. "Failed to create socket for incoming migration".

+ goto cleanup; + } + + memset(&addr, 0, sizeof(addr)); + addr.sin_family = AF_INET; + addr.sin_port = htons(port); + addr.sin_addr.s_addr = htonl(INADDR_ANY); + + if (bind(sockfd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { + libxlError(VIR_ERR_OPERATION_FAILED, + _("Bind error")); + goto cleanup; + } + + if (listen(sockfd, MAXCONN_NUM) < 0){ + libxlError(VIR_ERR_OPERATION_FAILED, + _("Listen error")); + goto cleanup; + } + + if (VIR_ALLOC(args) < 0) { + virReportOOMError(); + goto cleanup; + } + + args->conn = dconn; + args->vm = vm; + args->sockfd = sockfd; + if (virThreadCreate(&migrate_receive_thread, + true, + doMigrateReceive, args) < 0 ) { + virReportSystemError(errno, "%s", + _("Unable to create migration thread")); + VIR_FREE(args); + goto cleanup; + } + + ret = 0; + goto end; + +cleanup: + if (VIR_CLOSE(sockfd) < 0) + virReportSystemError(errno, "%s", _("cannot close sockfd")); + if (vm) + virDomainObjUnlock(vm); + +end: + libxlDriverUnlock(driver); + return ret; +} + +static int +libxlDomainMigratePerform3(virDomainPtr dom, + const char *xmlin ATTRIBUTE_UNUSED, + const char *cookiein ATTRIBUTE_UNUSED, + int cookieinlen ATTRIBUTE_UNUSED, + char **cookieout ATTRIBUTE_UNUSED, + int *cookieoutlen ATTRIBUTE_UNUSED, + const char *dconnuri ATTRIBUTE_UNUSED, + const char *uri, + unsigned long flags, + const char *dname ATTRIBUTE_UNUSED, + unsigned long resource ATTRIBUTE_UNUSED) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + char *hostname = NULL; + int port; + char *servname = NULL; + struct addrinfo hints, *res; + int sockfd = -1; + int ret = -1; + + virCheckFlags(LIBXL_MIGRATION_FLAGS, -1); + + libxlDriverLock(driver); + + if (doParseURI(uri, &hostname, &port)) + goto cleanup; + + VIR_DEBUG("hostname = %s, port = %d", hostname, port); + + if (virAsprintf(&servname, "%d", port ? port : DEFAULT_MIGRATION_PORT) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((sockfd = socket(AF_INET, SOCK_STREAM, 0)) < 0 ){ + libxlError(VIR_ERR_OPERATION_FAILED, + _("Failed to create socket")); + goto cleanup; + } + + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_INET; + hints.ai_socktype = SOCK_STREAM; + if (getaddrinfo(hostname, servname, &hints, &res) || res == NULL) { + libxlError(VIR_ERR_OPERATION_FAILED, + _("IP address lookup failed")); + goto cleanup; + } + + if (connect(sockfd, res->ai_addr, res->ai_addrlen) < 0) { + libxlError(VIR_ERR_OPERATION_FAILED, + _("Connect error")); + goto cleanup; + } + + ret = doMigrateSend(dom, flags, sockfd);

Hmm, the entire driver is locked during the migration. I just noticed libxlDomainSave{Flags} also holds the driver lock during save :-(. libxlDomainCoreDump drops the lock during the dump and IMO migration should follow the same pattern.

+ +cleanup: + if (VIR_CLOSE(sockfd) < 0) + virReportSystemError(errno, "%s", _("cannot close sockfd")); + VIR_FREE (hostname);

Extra whitespace.

+ VIR_FREE(servname); + freeaddrinfo(res); + libxlDriverUnlock(driver); + return ret; +} + +static virDomainPtr +libxlDomainMigrateFinish3(virConnectPtr dconn, + const char *dname, + const char *cookiein ATTRIBUTE_UNUSED, + int cookieinlen ATTRIBUTE_UNUSED, + char **cookieout ATTRIBUTE_UNUSED, + int *cookieoutlen ATTRIBUTE_UNUSED, + const char *dconnuri ATTRIBUTE_UNUSED, + const char *uri ATTRIBUTE_UNUSED, + unsigned long flags, + int cancelled) +{ + libxlDriverPrivatePtr driver = dconn->privateData; + virDomainObjPtr vm = NULL; + virDomainPtr dom = NULL; + libxlDomainObjPrivatePtr priv; + virDomainEventPtr event = NULL; + int rc; + + virCheckFlags(LIBXL_MIGRATION_FLAGS, NULL); + virThreadJoin(&migrate_receive_thread); + + libxlDriverLock(driver); + vm = virDomainFindByName(&driver->domains, dname); + if (!vm) + goto cleanup; + + if (!cancelled) { + if (!(flags & VIR_MIGRATE_PAUSED)) { + priv = vm->privateData; + rc = libxl_domain_unpause(&priv->ctx, vm->def->id); + if (rc) { + libxlError(VIR_ERR_OPERATION_FAILED, + _("Failed to unpause domain")); + goto error; + } + + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED); + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + goto error; + } + + dom = virGetDomain (dconn, vm->def->name, vm->def->uuid);

Extra whitespace.

+ goto cleanup; + } + +error: + if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_SAVED)) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("Failed to destroy domain '%d'"), vm->def->id); + goto cleanup; + } + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_SAVED);

Parameter alignment.

+ if (!vm->persistent) { + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; + } + +cleanup: + if (vm) + virDomainObjUnlock(vm); + if (event) + libxlDomainEventQueue(driver, event); + libxlDriverUnlock(driver); + return dom; +} + +static int +libxlDomainMigrateConfirm3(virDomainPtr domain, + const char *cookiein ATTRIBUTE_UNUSED, + int cookieinlen ATTRIBUTE_UNUSED, + unsigned long flags, + int cancelled) +{ + libxlDriverPrivatePtr driver = domain->conn->privateData; + virDomainObjPtr vm; + libxlDomainObjPrivatePtr priv; + virDomainEventPtr event = NULL; + int ret = -1; + + virCheckFlags(LIBXL_MIGRATION_FLAGS, -1); + + libxlDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(domain->uuid, uuidstr); + libxlError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr);

Parameter alignment.

+ goto cleanup; + } + + if (cancelled) { + priv = vm->privateData; + libxlError(VIR_ERR_INTERNAL_ERROR, + _("migration failed, try to resume on our end")); + if (!libxl_domain_resume(&priv->ctx, vm->def->id)) + ret = 0; + + goto cleanup; + } + + if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_SAVED)) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("Failed to destroy domain '%d'"), vm->def->id); + goto cleanup; + } + + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_SAVED);

Parameter alignment.

+ + if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) + virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm); + + if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) { + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; + } + + VIR_DEBUG("Migration successful.\n"); + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + libxlDriverUnlock(driver); + return ret; +}

static virDriver libxlDriver = { .no = VIR_DRV_LIBXL, .name = "xenlight", .open = libxlOpen, /* 0.9.0 */ .close = libxlClose, /* 0.9.0 */ + .supports_feature = libxlSupportsFeature, /* 0.9.9 */ .type = libxlGetType, /* 0.9.0 */ .version = libxlGetVersion, /* 0.9.0 */ .getHostname = virGetHostname, /* 0.9.0 */ @@ -3906,6 +4518,11 @@ static virDriver libxlDriver = { .domainGetSchedulerParametersFlags = libxlDomainGetSchedulerParametersFlags, /* 0.9.2 */ .domainSetSchedulerParameters = libxlDomainSetSchedulerParameters, /* 0.9.0 */ .domainSetSchedulerParametersFlags = libxlDomainSetSchedulerParametersFlags, /* 0.9.2 */ + .domainMigrateBegin3 = libxlDomainMigrateBegin3, /* 0.9.9 */ + .domainMigratePrepare3 = libxlDomainMigratePrepare3, /* 0.9.0 */ + .domainMigratePerform3 = libxlDomainMigratePerform3, /* 0.9.9 */ + .domainMigrateFinish3 = libxlDomainMigrateFinish3, /* 0.9.9 */ + .domainMigrateConfirm3 = libxlDomainMigrateConfirm3, /* 0.9.9 */

All of the additions will need updated to 0.9.11, or perhaps 0.9.12 depending on when the patch is merged. Regards, Jim

.nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */ .domainEventRegister = libxlDomainEventRegister, /* 0.9.0 */ .domainEventDeregister = libxlDomainEventDeregister, /* 0.9.0 */ diff --git a/src/libxl/libxl_driver.h b/src/libxl/libxl_driver.h index 4632d33..8a5fa6b 100644 --- a/src/libxl/libxl_driver.h +++ b/src/libxl/libxl_driver.h @@ -21,9 +21,22 @@ /*---------------------------------------------------------------------------*/

#ifndef LIBXL_DRIVER_H -# define LIBXL_DRIVER_H +#define LIBXL_DRIVER_H

-# include <config.h> +#include <config.h> + +#define LIBXL_MIGRATION_FLAGS \ + (VIR_MIGRATE_LIVE | \ + VIR_MIGRATE_UNDEFINE_SOURCE | \ + VIR_MIGRATE_PAUSED) + +#define DEFAULT_MIGRATION_PORT 8002 +#define MAXCONN_NUM 10 + +static const char migrate_receiver_banner[]= + "xl migration receiver ready, send binary domain data"; +static const char migrate_receiver_ready[]= + "domain received, ready to unpause";

int libxlRegister(void);