[PATCH v3 0/9] ch: support restore with network devices

Current ch driver supports restore only for domains without any network configuration defined. This was because libvirt explicitly passes network fds and CH did not had support to restore with new net FDS. This support has been added recently, https://github.com/cloud-hypervisor/cloud-hypervisor/pull/6402 The changes in this patch series includes moving to socket communication for restore api, create new net fds and pass them via SCM_RIGHTS to CH. New changes in v3: * Rebase on latest master * Fixe typos * Mention improvements in NEWS.rst v2: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/6TMM... New changes in v2: * Reword of few commints * Add version checks in save/restore validations * Add use_timeout in chSocketRecv * Address Praveen Paladugu's comments v1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/PT77V... Purna Pavan Chandra (9): ch: report response message instead of just code ch: Pass net ids explicitly during vm creation ch: refactor chProcessAddNetworkDevices ch: support poll with -1 in chSocketRecv ch: use monitor socket fd to send restore request ch: refactor virCHMonitorSaveVM ch: support restore with net devices ch: kill CH process if restore fails NEWS: Mention restore with n/w devices support for ch NEWS.rst | 6 + src/ch/ch_capabilities.c | 6 + src/ch/ch_capabilities.h | 1 + src/ch/ch_driver.c | 29 +++-- src/ch/ch_monitor.c | 62 +++++++---- src/ch/ch_monitor.h | 6 +- src/ch/ch_process.c | 233 +++++++++++++++++++++++++++++++-------- 7 files changed, 260 insertions(+), 83 deletions(-) -- 2.34.1

The response message from CH for vm.add-net api will be more helpful in debugging. Hence, log the message instead of just response code. Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 48190a1273..6f99fec900 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -665,7 +665,7 @@ chProcessAddNetworkDevices(virCHDriver *driver, } if (http_res != 204 && http_res != 200) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected response from CH: %1$d"), http_res); + _("Unexpected response from CH: %1$s"), response); return -1; } } -- 2.34.1

Pass "net_<index>" as net id to CH. This is to have better control over the network configs. This id can be further used in performing operations like restore etc. Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_monitor.c | 7 ++++++- src/ch/ch_monitor.h | 4 +++- src/ch/ch_process.c | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 7b6b77de1c..6af5b29d82 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -278,18 +278,23 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) /** * virCHMonitorBuildNetJson: * @net: pointer to a guest network definition + * @netindex: index of the guest network definition * @jsonstr: returned network json * * Build net json to send to CH * Returns 0 on success or -1 in case of error */ int -virCHMonitorBuildNetJson(virDomainNetDef *net, char **jsonstr) +virCHMonitorBuildNetJson(virDomainNetDef *net, int netindex, char **jsonstr) { char macaddr[VIR_MAC_STRING_BUFLEN]; g_autoptr(virJSONValue) net_json = virJSONValueNewObject(); virDomainNetType actualType = virDomainNetGetActualType(net); + g_autofree char *id = g_strdup_printf("%s_%d", CH_NET_ID_PREFIX, netindex); + if (virJSONValueObjectAppendString(net_json, "id", id) < 0) + return -1; + if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET && net->guestIP.nips == 1) { const virNetDevIPAddr *ip; diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index ea6b2a771b..bac80b5fcb 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -42,6 +42,8 @@ #define VIRCH_THREAD_NAME_LEN 16 +#define CH_NET_ID_PREFIX "net" + typedef enum { virCHThreadTypeEmulator, virCHThreadTypeVcpu, @@ -125,4 +127,4 @@ size_t virCHMonitorGetThreadInfo(virCHMonitor *mon, bool refresh, int virCHMonitorGetIOThreads(virCHMonitor *mon, virDomainIOThreadInfo ***iothreads); int -virCHMonitorBuildNetJson(virDomainNetDef *netdef, char **jsonstr); +virCHMonitorBuildNetJson(virDomainNetDef *netdef, int netindex, char **jsonstr); diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 6f99fec900..5907a6e81f 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -621,7 +621,7 @@ chProcessAddNetworkDevices(virCHDriver *driver, nicindexes, nnicindexes) < 0) return -1; - if (virCHMonitorBuildNetJson(vmdef->nets[i], &payload) < 0) { + if (virCHMonitorBuildNetJson(vmdef->nets[i], i, &payload) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to build net json")); return -1; -- 2.34.1

Move monitor socket connection, response handling and closing FDs code into new functions in preparation for adding restore support for net devices. Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_process.c | 121 +++++++++++++++++++++++++++++--------------- 1 file changed, 80 insertions(+), 41 deletions(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 5907a6e81f..1943c4381b 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -490,6 +490,47 @@ virCHProcessSetup(virDomainObj *vm) } +/** + * chMonitorSocketConnect: + * @mon: pointer to monitor object + * + * Connects to the monitor socket. Caller is responsible for closing the socketfd + * + * Returns socket fd on success, -1 on error + */ +static int +chMonitorSocketConnect(virCHMonitor *mon) +{ + struct sockaddr_un server_addr; + int sock; + + sock = socket(AF_UNIX, SOCK_STREAM, 0); + if (sock < 0) { + virReportSystemError(errno, "%s", _("Failed to open a UNIX socket")); + return -1; + } + + memset(&server_addr, 0, sizeof(server_addr)); + server_addr.sun_family = AF_UNIX; + if (virStrcpyStatic(server_addr.sun_path, mon->socketpath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("UNIX socket path '%1$s' too long"), mon->socketpath); + goto error; + } + + if (connect(sock, (struct sockaddr *)&server_addr, + sizeof(server_addr)) == -1) { + virReportSystemError(errno, "%s", _("Failed to connect to mon socket")); + goto error; + } + + return sock; + error: + VIR_FORCE_CLOSE(sock); + return -1; +} + + #define PKT_TIMEOUT_MS 500 /* ms */ static char * @@ -532,6 +573,42 @@ chSocketRecv(int sock) #undef PKT_TIMEOUT_MS +static int +chSocketProcessHttpResponse(int sock) +{ + g_autofree char *response = NULL; + int http_res; + + response = chSocketRecv(sock); + if (response == NULL) { + return -1; + } + + /* Parse the HTTP response code */ + if (sscanf(response, "HTTP/1.%*d %d", &http_res) != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to parse HTTP response code")); + return -1; + } + if (http_res != 204 && http_res != 200) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected response from CH: %1$s"), response); + return -1; + } + + return 0; +} + +static int +chCloseFDs(int *fds, size_t nfds) +{ + size_t i; + for (i = 0; i < nfds; i++) { + VIR_FORCE_CLOSE(fds[i]); + } + return 0; +} + /** * chProcessAddNetworkDevices: * @driver: pointer to ch driver object @@ -554,7 +631,6 @@ chProcessAddNetworkDevices(virCHDriver *driver, { size_t i; VIR_AUTOCLOSE mon_sockfd = -1; - struct sockaddr_un server_addr; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) http_headers = VIR_BUFFER_INITIALIZER; @@ -564,25 +640,8 @@ chProcessAddNetworkDevices(virCHDriver *driver, return -1; } - mon_sockfd = socket(AF_UNIX, SOCK_STREAM, 0); - if (mon_sockfd < 0) { - virReportSystemError(errno, "%s", _("Failed to open a UNIX socket")); - return -1; - } - - memset(&server_addr, 0, sizeof(server_addr)); - server_addr.sun_family = AF_UNIX; - if (virStrcpyStatic(server_addr.sun_path, mon->socketpath) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("UNIX socket path '%1$s' too long"), mon->socketpath); - return -1; - } - - if (connect(mon_sockfd, (struct sockaddr *)&server_addr, - sizeof(server_addr)) == -1) { - virReportSystemError(errno, "%s", _("Failed to connect to mon socket")); + if ((mon_sockfd = chMonitorSocketConnect(mon)) < 0) return -1; - } virBufferAddLit(&http_headers, "PUT /api/v1/vm.add-net HTTP/1.1\r\n"); virBufferAddLit(&http_headers, "Host: localhost\r\n"); @@ -592,11 +651,9 @@ chProcessAddNetworkDevices(virCHDriver *driver, g_autofree int *tapfds = NULL; g_autofree char *payload = NULL; g_autofree char *response = NULL; - size_t j; size_t tapfd_len; size_t payload_len; int saved_errno; - int http_res; int rc; if (vmdef->nets[i]->driver.virtio.queues == 0) { @@ -640,9 +697,7 @@ chProcessAddNetworkDevices(virCHDriver *driver, saved_errno = errno; /* Close sent tap fds in Libvirt, as they have been dup()ed in CH */ - for (j = 0; j < tapfd_len; j++) { - VIR_FORCE_CLOSE(tapfds[j]); - } + chCloseFDs(tapfds, tapfd_len); if (rc < 0) { virReportSystemError(saved_errno, "%s", @@ -650,24 +705,8 @@ chProcessAddNetworkDevices(virCHDriver *driver, return -1; } - /* Process the response from CH */ - response = chSocketRecv(mon_sockfd); - if (response == NULL) { + if (chSocketProcessHttpResponse(mon_sockfd) < 0) return -1; - } - - /* Parse the HTTP response code */ - rc = sscanf(response, "HTTP/1.%*d %d", &http_res); - if (rc != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to parse HTTP response code")); - return -1; - } - if (http_res != 204 && http_res != 200) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected response from CH: %1$s"), response); - return -1; - } } return 0; -- 2.34.1

chSocketRecv fn can be used by operations such as restore, which cannot have a specific poll timeout. The runtime of these operations at server side (vmm) cannot be determined or capped as it depends on the guest configuration. Hence, add a new parameter 'use_timeout' which when set will pass -1 as timeout to poll, otherwise the default PKT_TIMEOUT_MS is used. Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_process.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 1943c4381b..851ab7545c 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -534,11 +534,12 @@ chMonitorSocketConnect(virCHMonitor *mon) #define PKT_TIMEOUT_MS 500 /* ms */ static char * -chSocketRecv(int sock) +chSocketRecv(int sock, bool use_timeout) { struct pollfd pfds[1]; char *buf = NULL; size_t buf_len = 1024; + int timeout = PKT_TIMEOUT_MS; int ret; buf = g_new0(char, buf_len); @@ -546,8 +547,11 @@ chSocketRecv(int sock) pfds[0].fd = sock; pfds[0].events = POLLIN; + if (!use_timeout) + timeout = -1; + do { - ret = poll(pfds, G_N_ELEMENTS(pfds), PKT_TIMEOUT_MS); + ret = poll(pfds, G_N_ELEMENTS(pfds), timeout); } while (ret < 0 && errno == EINTR); if (ret <= 0) { @@ -574,12 +578,12 @@ chSocketRecv(int sock) #undef PKT_TIMEOUT_MS static int -chSocketProcessHttpResponse(int sock) +chSocketProcessHttpResponse(int sock, bool use_poll_timeout) { g_autofree char *response = NULL; int http_res; - response = chSocketRecv(sock); + response = chSocketRecv(sock, use_poll_timeout); if (response == NULL) { return -1; } @@ -705,7 +709,7 @@ chProcessAddNetworkDevices(virCHDriver *driver, return -1; } - if (chSocketProcessHttpResponse(mon_sockfd) < 0) + if (chSocketProcessHttpResponse(mon_sockfd, true) < 0) return -1; } -- 2.34.1

Instead of curl, use low-level socket connections to make restore api request to CH. This will enable passing new net FDs to CH while restoring domains with network configuration. Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_monitor.c | 13 +++++++++++-- src/ch/ch_monitor.h | 2 +- src/ch/ch_process.c | 40 ++++++++++++++++++++++++++++++++++------ 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 6af5b29d82..0875887fd8 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -991,9 +991,18 @@ virCHMonitorSaveVM(virCHMonitor *mon, const char *to) } int -virCHMonitorRestoreVM(virCHMonitor *mon, const char *from) +virCHMonitorBuildRestoreJson(const char *from, char **jsonstr) { - return virCHMonitorSaveRestoreVM(mon, from, false); + g_autoptr(virJSONValue) restore_json = virJSONValueNewObject(); + + g_autofree char *path_url = g_strdup_printf("file://%s", from); + if (virJSONValueObjectAppendString(restore_json, "source_url", path_url)) + return -1; + + if (!(*jsonstr = virJSONValueToString(restore_json, false))) + return -1; + + return 0; } /** diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index bac80b5fcb..375b7a49a4 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -115,7 +115,6 @@ int virCHMonitorRebootVM(virCHMonitor *mon); int virCHMonitorSuspendVM(virCHMonitor *mon); int virCHMonitorResumeVM(virCHMonitor *mon); int virCHMonitorSaveVM(virCHMonitor *mon, const char *to); -int virCHMonitorRestoreVM(virCHMonitor *mon, const char *from); int virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info); void virCHMonitorCPUInfoFree(virCHMonitorCPUInfo *cpus); @@ -128,3 +127,4 @@ int virCHMonitorGetIOThreads(virCHMonitor *mon, virDomainIOThreadInfo ***iothreads); int virCHMonitorBuildNetJson(virDomainNetDef *netdef, int netindex, char **jsonstr); +int virCHMonitorBuildRestoreJson(const char *from, char **jsonstr); diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 851ab7545c..09c8be6c56 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -912,6 +912,12 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from { virCHDomainObjPrivate *priv = vm->privateData; g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(priv->driver); + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) http_headers = VIR_BUFFER_INITIALIZER; + g_autofree char *payload = NULL; + g_autofree char *response = NULL; + VIR_AUTOCLOSE mon_sockfd = -1; + size_t payload_len; if (!priv->monitor) { /* Get the first monitor connection if not already */ @@ -926,12 +932,6 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from vm->def->id = vm->pid; priv->machineName = virCHDomainGetMachineName(vm); - if (virCHMonitorRestoreVM(priv->monitor, from) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to restore domain")); - return -1; - } - /* Pass 0, NULL as restore only works without networking support */ if (virDomainCgroupSetupCgroup("ch", vm, 0, NULL, /* nnicindexes, nicindexes */ @@ -942,6 +942,34 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from priv->machineName) < 0) return -1; + if (virCHMonitorBuildRestoreJson(from, &payload) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to restore domain")); + return -1; + } + + virBufferAddLit(&http_headers, "PUT /api/v1/vm.restore HTTP/1.1\r\n"); + virBufferAddLit(&http_headers, "Host: localhost\r\n"); + virBufferAddLit(&http_headers, "Content-Type: application/json\r\n"); + virBufferAsprintf(&buf, "%s", virBufferCurrentContent(&http_headers)); + virBufferAsprintf(&buf, "Content-Length: %ld\r\n\r\n", strlen(payload)); + virBufferAsprintf(&buf, "%s", payload); + payload_len = virBufferUse(&buf); + payload = virBufferContentAndReset(&buf); + + if ((mon_sockfd = chMonitorSocketConnect(priv->monitor)) < 0) + return -1; + + if (virSocketSendMsgWithFDs(mon_sockfd, payload, payload_len, NULL, 0) < 0) { + virReportSystemError(errno, "%s", + _("Failed to send restore request to CH")); + return -1; + } + + /* Restore is a synchronous operation in CH. so, pass false to wait until there's a response */ + if (chSocketProcessHttpResponse(mon_sockfd, false) < 0) + return -1; + if (virCHProcessSetup(vm) < 0) return -1; -- 2.34.1

Remove the unwanted utility function and make api calls directly from virCHMonitorSaveVM fn Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_monitor.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 0875887fd8..8d8be1f46e 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -925,8 +925,8 @@ virCHMonitorResumeVM(virCHMonitor *mon) return virCHMonitorPutNoContent(mon, URL_VM_RESUME); } -static int -virCHMonitorSaveRestoreVM(virCHMonitor *mon, const char *path, bool save) +int +virCHMonitorSaveVM(virCHMonitor *mon, const char *to) { g_autofree char *url = NULL; int responseCode = 0; @@ -936,22 +936,15 @@ virCHMonitorSaveRestoreVM(virCHMonitor *mon, const char *path, bool save) struct curl_slist *headers = NULL; struct curl_data data = {0}; - if (save) - url = g_strdup_printf("%s/%s", URL_ROOT, URL_VM_SAVE); - else - url = g_strdup_printf("%s/%s", URL_ROOT, URL_VM_RESTORE); + url = g_strdup_printf("%s/%s", URL_ROOT, URL_VM_SAVE); headers = curl_slist_append(headers, "Accept: application/json"); headers = curl_slist_append(headers, "Content-Type: application/json"); - path_url = g_strdup_printf("file://%s", path); - if (save) { - if (virCHMonitorBuildKeyValueStringJson(&payload, "destination_url", path_url) != 0) - return -1; - } else { - if (virCHMonitorBuildKeyValueStringJson(&payload, "source_url", path_url) != 0) - return -1; - } + path_url = g_strdup_printf("file://%s", to); + if (virCHMonitorBuildKeyValueStringJson(&payload, "destination_url", path_url) != 0) + return -1; + VIR_WITH_OBJECT_LOCK_GUARD(mon) { /* reset all options of a libcurl session handle at first */ @@ -984,12 +977,6 @@ virCHMonitorSaveRestoreVM(virCHMonitor *mon, const char *path, bool save) return ret; } -int -virCHMonitorSaveVM(virCHMonitor *mon, const char *to) -{ - return virCHMonitorSaveRestoreVM(mon, to, true); -} - int virCHMonitorBuildRestoreJson(const char *from, char **jsonstr) { -- 2.34.1

Cloud-hypervisor now supports restoring with new net fds. Ref: https://github.com/cloud-hypervisor/cloud-hypervisor/pull/6402 So, pass new tap fds via SCM_RIGHTS to CH's restore api. Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_capabilities.c | 6 +++ src/ch/ch_capabilities.h | 1 + src/ch/ch_driver.c | 29 +++++++----- src/ch/ch_monitor.c | 21 ++++++++- src/ch/ch_monitor.h | 2 +- src/ch/ch_process.c | 98 +++++++++++++++++++++++++++++++++------- 6 files changed, 126 insertions(+), 31 deletions(-) diff --git a/src/ch/ch_capabilities.c b/src/ch/ch_capabilities.c index 5941851500..f3765a8095 100644 --- a/src/ch/ch_capabilities.c +++ b/src/ch/ch_capabilities.c @@ -65,6 +65,12 @@ virCHCapsInitCHVersionCaps(int version) if (version >= 36000000) virCHCapsSet(chCaps, CH_SOCKET_BACKEND_SERIAL_PORT); + /* Starting v40, Cloud-Hypervisor supports restore with new net fds. + * This is required to be able to restore a guest with network config define. + * https://github.com/cloud-hypervisor/cloud-hypervisor/releases/tag/v40.0 */ + if (version >= 40000000) + virCHCapsSet(chCaps, CH_RESTORE_WITH_NEW_TAPFDS); + return g_steal_pointer(&chCaps); } diff --git a/src/ch/ch_capabilities.h b/src/ch/ch_capabilities.h index 03932511f6..e5efb14b68 100644 --- a/src/ch/ch_capabilities.h +++ b/src/ch/ch_capabilities.h @@ -28,6 +28,7 @@ typedef enum { CH_SERIAL_CONSOLE_IN_PARALLEL, /* Serial and Console ports can work in parallel */ CH_MULTIFD_IN_ADDNET, /* Cloud-hypervisor can accept multiple FDs in add-net api */ CH_SOCKET_BACKEND_SERIAL_PORT, /* Support Unix socket as a backend for a serial port */ + CH_RESTORE_WITH_NEW_TAPFDS, /* Cloud-hypervisor support for restore with new net fds */ CH_CAPS_LAST /* this must always be the last item */ } virCHCapsFlags; diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index fbeac60825..24aed471c5 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -680,22 +680,24 @@ chDomainDestroy(virDomainPtr dom) } static int -chDomainSaveAdditionalValidation(virDomainDef *vmdef) +chDomainSaveRestoreAdditionalValidation(virCHDriver *driver, virDomainDef *vmdef) { - /* - SAVE and RESTORE are functional only without any networking and - device passthrough configuration - */ - if (vmdef->nnets > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot save domain with network interfaces")); - return -1; - } + /* SAVE and RESTORE are functional only without any host device + * passthrough configuration */ if (vmdef->nhostdevs > 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot save domain with host devices")); + _("cannot save/restore domain with host devices")); return -1; } + + if (vmdef->nnets > 0) { + if (!virBitmapIsBitSet(driver->chCaps, CH_RESTORE_WITH_NEW_TAPFDS)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot save/restore domain with network devices")); + return -1; + } + } + return 0; } @@ -728,7 +730,7 @@ chDoDomainSave(virCHDriver *driver, VIR_AUTOCLOSE fd = -1; int ret = -1; - if (chDomainSaveAdditionalValidation(vm->def) < 0) + if (chDomainSaveRestoreAdditionalValidation(driver, vm->def) < 0) goto end; domainState = virDomainObjGetState(vm, NULL); @@ -1087,6 +1089,9 @@ chDomainRestoreFlags(virConnectPtr conn, if (virDomainRestoreFlagsEnsureACL(conn, def) < 0) goto cleanup; + if (chDomainSaveRestoreAdditionalValidation(driver, def) < 0) + goto cleanup; + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 8d8be1f46e..3af5e7f2d1 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -978,14 +978,31 @@ virCHMonitorSaveVM(virCHMonitor *mon, const char *to) } int -virCHMonitorBuildRestoreJson(const char *from, char **jsonstr) +virCHMonitorBuildRestoreJson(virDomainDef *vmdef, const char *from, char **jsonstr) { + size_t i; g_autoptr(virJSONValue) restore_json = virJSONValueNewObject(); - g_autofree char *path_url = g_strdup_printf("file://%s", from); + if (virJSONValueObjectAppendString(restore_json, "source_url", path_url)) return -1; + /* Pass the netconfig needed to restore with new netfds */ + if (vmdef->nnets) { + g_autoptr(virJSONValue) nets = virJSONValueNewArray(); + for (i = 0; i < vmdef->nnets; i++) { + g_autoptr(virJSONValue) net_json = virJSONValueNewObject(); + g_autofree char *id = g_strdup_printf("%s_%ld", CH_NET_ID_PREFIX, i); + if (virJSONValueObjectAppendString(net_json, "id", id) < 0) + return -1; + if (virJSONValueObjectAppendNumberInt(net_json, "num_fds", vmdef->nets[i]->driver.virtio.queues)) + return -1; + virJSONValueArrayAppend(nets, &net_json); + } + if (virJSONValueObjectAppend(restore_json, "net_fds", &nets)) + return -1; + } + if (!(*jsonstr = virJSONValueToString(restore_json, false))) return -1; diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index 375b7a49a4..02a6685d58 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -127,4 +127,4 @@ int virCHMonitorGetIOThreads(virCHMonitor *mon, virDomainIOThreadInfo ***iothreads); int virCHMonitorBuildNetJson(virDomainNetDef *netdef, int netindex, char **jsonstr); -int virCHMonitorBuildRestoreJson(const char *from, char **jsonstr); +int virCHMonitorBuildRestoreJson(virDomainDef *vmdef, const char *from, char **jsonstr); diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 09c8be6c56..ff0fc5ca63 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -29,6 +29,7 @@ #include "ch_process.h" #include "domain_cgroup.h" #include "domain_interface.h" +#include "viralloc.h" #include "virerror.h" #include "virfile.h" #include "virjson.h" @@ -716,6 +717,56 @@ chProcessAddNetworkDevices(virCHDriver *driver, return 0; } +/** + * virCHRestoreCreateNetworkDevices: + * @driver: pointer to driver structure + * @vmdef: pointer to domain definition + * @vmtapfds: returned array of FDs of guest interfaces + * @nvmtapfds: returned number of network indexes + * @nicindexes: returned array of network indexes + * @nnicindexes: returned number of network indexes + * + * Create network devices for the domain. This function is called during + * domain restore. + * + * Returns 0 on success or -1 in case of error +*/ +static int +virCHRestoreCreateNetworkDevices(virCHDriver *driver, + virDomainDef *vmdef, + int **vmtapfds, + size_t *nvmtapfds, + int **nicindexes, + size_t *nnicindexes) +{ + size_t i, j; + size_t tapfd_len; + size_t index_vmtapfds; + for (i = 0; i < vmdef->nnets; i++) { + g_autofree int *tapfds = NULL; + tapfd_len = vmdef->nets[i]->driver.virtio.queues; + if (virCHDomainValidateActualNetDef(vmdef->nets[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("net definition failed validation")); + return -1; + } + tapfds = g_new0(int, tapfd_len); + memset(tapfds, -1, (tapfd_len) * sizeof(int)); + + /* Connect Guest interfaces */ + if (virCHConnetNetworkInterfaces(driver, vmdef, vmdef->nets[i], tapfds, + nicindexes, nnicindexes) < 0) + return -1; + + index_vmtapfds = *nvmtapfds; + VIR_EXPAND_N(*vmtapfds, *nvmtapfds, tapfd_len); + for (j = 0; j < tapfd_len; j++) { + VIR_APPEND_ELEMENT_INPLACE(*vmtapfds, index_vmtapfds, tapfds[j]); + } + } + return 0; +} + /** * virCHProcessStartValidate: * @driver: pointer to driver structure @@ -917,7 +968,12 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from g_autofree char *payload = NULL; g_autofree char *response = NULL; VIR_AUTOCLOSE mon_sockfd = -1; + g_autofree int *tapfds = NULL; + g_autofree int *nicindexes = NULL; size_t payload_len; + size_t ntapfds = 0; + size_t nnicindexes = 0; + int ret = -1; if (!priv->monitor) { /* Get the first monitor connection if not already */ @@ -932,17 +988,7 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from vm->def->id = vm->pid; priv->machineName = virCHDomainGetMachineName(vm); - /* Pass 0, NULL as restore only works without networking support */ - if (virDomainCgroupSetupCgroup("ch", vm, - 0, NULL, /* nnicindexes, nicindexes */ - &priv->cgroup, - cfg->cgroupControllers, - 0, /*maxThreadsPerProc*/ - priv->driver->privileged, - priv->machineName) < 0) - return -1; - - if (virCHMonitorBuildRestoreJson(from, &payload) < 0) { + if (virCHMonitorBuildRestoreJson(vm->def, from, &payload) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to restore domain")); return -1; @@ -960,20 +1006,40 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from if ((mon_sockfd = chMonitorSocketConnect(priv->monitor)) < 0) return -1; - if (virSocketSendMsgWithFDs(mon_sockfd, payload, payload_len, NULL, 0) < 0) { + if (virCHRestoreCreateNetworkDevices(driver, vm->def, &tapfds, &ntapfds, &nicindexes, &nnicindexes) < 0) + goto cleanup; + + if (virDomainCgroupSetupCgroup("ch", vm, + nnicindexes, nicindexes, + &priv->cgroup, + cfg->cgroupControllers, + 0, /*maxThreadsPerProc*/ + priv->driver->privileged, + priv->machineName) < 0) + goto cleanup; + + /* Bring up netdevs before restoring vm */ + if (virDomainInterfaceStartDevices(vm->def) < 0) + goto cleanup; + + if (virSocketSendMsgWithFDs(mon_sockfd, payload, payload_len, tapfds, ntapfds) < 0) { virReportSystemError(errno, "%s", _("Failed to send restore request to CH")); - return -1; + goto cleanup; } /* Restore is a synchronous operation in CH. so, pass false to wait until there's a response */ if (chSocketProcessHttpResponse(mon_sockfd, false) < 0) - return -1; + goto cleanup; if (virCHProcessSetup(vm) < 0) - return -1; + goto cleanup; virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + ret = 0; - return 0; + cleanup: + if (tapfds) + chCloseFDs(tapfds, ntapfds); + return ret; } -- 2.34.1

Invoke virCHProcessStop to kill CH process incase of any failures during restore operation. Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index ff0fc5ca63..38216bbf8d 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -980,7 +980,7 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from if (!(priv->monitor = virCHProcessConnectMonitor(driver, vm))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to create connection to CH socket")); - return -1; + goto cleanup; } } @@ -991,7 +991,7 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from if (virCHMonitorBuildRestoreJson(vm->def, from, &payload) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to restore domain")); - return -1; + goto cleanup; } virBufferAddLit(&http_headers, "PUT /api/v1/vm.restore HTTP/1.1\r\n"); @@ -1004,7 +1004,7 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from payload = virBufferContentAndReset(&buf); if ((mon_sockfd = chMonitorSocketConnect(priv->monitor)) < 0) - return -1; + goto cleanup; if (virCHRestoreCreateNetworkDevices(driver, vm->def, &tapfds, &ntapfds, &nicindexes, &nnicindexes) < 0) goto cleanup; @@ -1041,5 +1041,7 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from cleanup: if (tapfds) chCloseFDs(tapfds, ntapfds); + if (ret) + virCHProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); return ret; } -- 2.34.1

Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- NEWS.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 44df5d08f7..bbd30ab3b5 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -19,6 +19,12 @@ v10.7.0 (unreleased) * **Improvements** + * ch: support restore with network devices + + Cloud-Hypervisor starting from V40.0 supports restoring file descriptor + backed network devices. So, create new net fds and pass them via + SCM_RIGHTS to CH during restore operation. + * **Bug fixes** -- 2.34.1

LGTM! On 8/5/2024 9:40 AM, Purna Pavan Chandra wrote:
Current ch driver supports restore only for domains without any network configuration defined. This was because libvirt explicitly passes network fds and CH did not had support to restore with new net FDS. This support has been added recently, https://github.com/cloud-hypervisor/cloud-hypervisor/pull/6402 The changes in this patch series includes moving to socket communication for restore api, create new net fds and pass them via SCM_RIGHTS to CH.
New changes in v3: * Rebase on latest master * Fixe typos * Mention improvements in NEWS.rst
v2: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/6TMM...
New changes in v2: * Reword of few commints * Add version checks in save/restore validations * Add use_timeout in chSocketRecv * Address Praveen Paladugu's comments
v1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/PT77V...
Purna Pavan Chandra (9): ch: report response message instead of just code ch: Pass net ids explicitly during vm creation ch: refactor chProcessAddNetworkDevices ch: support poll with -1 in chSocketRecv ch: use monitor socket fd to send restore request ch: refactor virCHMonitorSaveVM ch: support restore with net devices ch: kill CH process if restore fails NEWS: Mention restore with n/w devices support for ch
NEWS.rst | 6 + src/ch/ch_capabilities.c | 6 + src/ch/ch_capabilities.h | 1 + src/ch/ch_driver.c | 29 +++-- src/ch/ch_monitor.c | 62 +++++++---- src/ch/ch_monitor.h | 6 +- src/ch/ch_process.c | 233 +++++++++++++++++++++++++++++++-------- 7 files changed, 260 insertions(+), 83 deletions(-)
-- Regards, Praveen

On 8/5/24 16:40, Purna Pavan Chandra wrote:
Current ch driver supports restore only for domains without any network configuration defined. This was because libvirt explicitly passes network fds and CH did not had support to restore with new net FDS. This support has been added recently, https://github.com/cloud-hypervisor/cloud-hypervisor/pull/6402 The changes in this patch series includes moving to socket communication for restore api, create new net fds and pass them via SCM_RIGHTS to CH.
New changes in v3: * Rebase on latest master * Fixe typos * Mention improvements in NEWS.rst
v2: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/6TMM...
New changes in v2: * Reword of few commints * Add version checks in save/restore validations * Add use_timeout in chSocketRecv * Address Praveen Paladugu's comments
v1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/PT77V...
Purna Pavan Chandra (9): ch: report response message instead of just code ch: Pass net ids explicitly during vm creation ch: refactor chProcessAddNetworkDevices ch: support poll with -1 in chSocketRecv ch: use monitor socket fd to send restore request ch: refactor virCHMonitorSaveVM ch: support restore with net devices ch: kill CH process if restore fails NEWS: Mention restore with n/w devices support for ch
NEWS.rst | 6 + src/ch/ch_capabilities.c | 6 + src/ch/ch_capabilities.h | 1 + src/ch/ch_driver.c | 29 +++-- src/ch/ch_monitor.c | 62 +++++++---- src/ch/ch_monitor.h | 6 +- src/ch/ch_process.c | 233 +++++++++++++++++++++++++++++++-------- 7 files changed, 260 insertions(+), 83 deletions(-)
Fixed couple of formatting issues and merged. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Michal Prívozník
-
Praveen K Paladugu
-
Purna Pavan Chandra