On 6/28/2024 2:26 AM, Purna Pavan Chandra wrote:

Move socket connection code to a new function. There could be other functions which might want to perform similar socket connection operation. Hence, move it to a new function. Similarly move http response processing and closing FDs to new functions.

Please consider rewording this commit message to be more definitive. Something like: Move monitor socket connection and response handling 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..668a53a1c2 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 reposible for closing the socketfd

nit: s/reposible/responsible

+ * + * 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;

-- Regards, Praveen

On 6/28/2024 2:26 AM, Purna Pavan Chandra wrote:

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, pass -1 as timeout to poll to make wait until there's a response from server.

Signed-off-by: Purna Pavan Chandra <paekkaladevi@linux.microsoft.com> --- src/ch/ch_process.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 668a53a1c2..3f2a3f81e5 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -530,9 +530,6 @@ chMonitorSocketConnect(virCHMonitor *mon) return -1; }

- -#define PKT_TIMEOUT_MS 500 /* ms */ - static char * chSocketRecv(int sock) { @@ -547,7 +544,7 @@ chSocketRecv(int sock) pfds[0].events = POLLIN;

do { - ret = poll(pfds, G_N_ELEMENTS(pfds), PKT_TIMEOUT_MS); + ret = poll(pfds, G_N_ELEMENTS(pfds), -1);

As we'd like "restore" operation to be synchronous we can keep polling until libvirt gets a response. But for rest of the operations, we should retain the timeout. For example, if CH process crashes after receiving the request to add net FDs, we'd like Libvirt to continue instead of waiting for a response forever. You can add an additional argument to this function to choose if timeout should be used or not. And not use the timeout during restore operation.

} while (ret < 0 && errno == EINTR);

if (ret <= 0) { @@ -571,8 +568,6 @@ chSocketRecv(int sock) return g_steal_pointer(&buf); }

-#undef PKT_TIMEOUT_MS - static int chSocketProcessHttpResponse(int sock) {

-- Regards, Praveen

On 6/28/2024 2:26 AM, Purna Pavan Chandra wrote:

This change will later help in passing new net fds to CH during restore operation.

Please consider rewording this commit message to contain a 'what' and a 'why'. Something to the effect of: Instead of curl, use low-level socket connections to restore VMs. This is required to pass net FDs to CH while restoring net devices.

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 | 39 +++++++++++++++++++++++++++++++++------ 3 files changed, 45 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 3f2a3f81e5..b8c6080d5c 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -903,6 +903,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 */ @@ -917,12 +923,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 */ @@ -933,6 +933,33 @@ 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; + } + + if (chSocketProcessHttpResponse(mon_sockfd) < 0) + return -1; + if (virCHProcessSetup(vm) < 0) return -1;

-- Regards, Praveen

On Fri, Jun 28, 2024 at 09:59:29AM -0500, Praveen K Paladugu wrote:

On 6/28/2024 2:26 AM, Purna Pavan Chandra 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_driver.c | 9 +--- src/ch/ch_monitor.c | 21 ++++++++- src/ch/ch_monitor.h | 2 +- src/ch/ch_process.c | 104 ++++++++++++++++++++++++++++++++++++-------- 4 files changed, 107 insertions(+), 29 deletions(-)

diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index fbeac60825..7857515766 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -683,14 +683,9 @@ static int chDomainSaveAdditionalValidation(virDomainDef *vmdef) { /* - SAVE and RESTORE are functional only without any networking and - device passthrough configuration + SAVE and RESTORE are functional only without any host device + passthrough configuration */

Add a version based check as well here. We should check cloud-hypervisor version >= 40 here. For reference: https://github.com/libvirt/libvirt/commit/b2e43609fd68b84f626d63e836acd870c8...

Sure, I'll add this change.

Similar to chDomainSaveAdditionalValidation, consider adding a validation step during Restore as well. This is required, if a user tries to save the VM on one host and restore it on a different host.

Even if a user saves a VM on one host and tries to restore in another one (along with migrating the saved state files), the expectation is that it would work, right ?

...

- if (vmdef->nnets > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot save domain with network interfaces")); - return -1; - } if (vmdef->nhostdevs > 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot save domain with host devices")); 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 b8c6080d5c..cb3539ebe5 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" @@ -707,6 +708,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 @@ -908,14 +959,19 @@ 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 */ if (!(priv->monitor = virCHProcessConnectMonitor(driver, vm))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to create connection to CH socket")); - return -1; + goto cleanup;

you can continue returning -1 here, as tapfds have not yet been created.

The next commit "ch: kill CH process if restore fails" kills the process incase of any errors during restore. Hence, all failures after creating priv->monitor needs to do 'goto cleanup'. But I guess this is not correct commit to make this change. I'll move this change to the above mentioned commit.

...

} } @@ -923,20 +979,10 @@ 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; + goto cleanup;

Same as above comment.

...

} virBufferAddLit(&http_headers, "PUT /api/v1/vm.restore HTTP/1.1\r\n"); @@ -949,21 +995,41 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from payload = virBufferContentAndReset(&buf); if ((mon_sockfd = chMonitorSocketConnect(priv->monitor)) < 0) - return -1; + goto cleanup;

Ditto

...

- 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; } if (chSocketProcessHttpResponse(mon_sockfd) < 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