[PATCH v2 0/8] New changes in v2:
* 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... 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. Purna Pavan Chandra (8): 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 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 +++++++++++++++++++++++++++++++-------- 6 files changed, 254 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..0ac0a5c9a2 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 syncronous operationin CH. so, pass false to wait until there's a reponse */ + if (chSocketProcessHttpResponse(mon_sockfd, false) < 0) + return -1; + if (virCHProcessSetup(vm) < 0) return -1; -- 2.34.1
On 7/4/2024 6:13 AM, Purna Pavan Chandra wrote:
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..0ac0a5c9a2 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 syncronous operationin CH. so, pass false to wait until there's a reponse */ typos: s/syncronous/synchronous/ s/operationin/operation in/ s/reponse/response + if (chSocketProcessHttpResponse(mon_sockfd, false) < 0) + return -1; + if (virCHProcessSetup(vm) < 0) return -1;
-- Regards, Praveen
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 0ac0a5c9a2..540bf6801d 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 syncronous operationin CH. so, pass false to wait until there's a reponse */ 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
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 0ac0a5c9a2..540bf6801d 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; Instead of duplicating these steps from chProcessAddNetworkDevices,
On 7/4/2024 6:13 AM, Purna Pavan Chandra wrote: please consider refactoring them into a different to be used here and in chProcessAddNetworkDevices.
+ 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; }
After sending the FDs, they have to closed on Libvirt side, even if the send was successful. Please check chProcessAddNetworkDevices for details on it.
/* Restore is a syncronous operationin CH. so, pass false to wait until there's a reponse */ 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; }
-- Regards, Praveen
On Thu, Jul 25, 2024 at 12:04:47PM -0500, Praveen K Paladugu wrote:
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 0ac0a5c9a2..540bf6801d 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; Instead of duplicating these steps from chProcessAddNetworkDevices,
On 7/4/2024 6:13 AM, Purna Pavan Chandra wrote: please consider refactoring them into a different to be used here and in chProcessAddNetworkDevices.
chProcessAddNetworkDevices is already refactored and the common code is moved to new functions. Since the action performed inside the loop are different here, this can not be refactored further.
+ 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; }
After sending the FDs, they have to closed on Libvirt side, even if the send was successful. Please check chProcessAddNetworkDevices for details on it.
This is taken care in the cleanup section.
/* Restore is a syncronous operationin CH. so, pass false to wait until there's a reponse */ 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; }
-- Regards, Praveen
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 540bf6801d..b77ab97a9f 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
Hi, The subject for the V2 was messed up before I posted patches for review. Please review my re-sent v2 at https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/6TMMK... Regards, Pavan On Thu, Jul 04, 2024 at 11:13:30AM +0000, Purna Pavan Chandra wrote:
* 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...
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.
Purna Pavan Chandra (8): 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
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 +++++++++++++++++++++++++++++++-------- 6 files changed, 254 insertions(+), 83 deletions(-)
-- 2.34.1
participants (3)
-
Praveen K Paladugu -
Purna Pavan Chandra -
Purna Pavan Chandra Aekkaladevi