9:26 a.m.
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 majorly include 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: 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_driver.c | 9 +-
src/ch/ch_monitor.c | 62 +++++++-----
src/ch/ch_monitor.h | 6 +-
src/ch/ch_process.c | 225 ++++++++++++++++++++++++++++++++++----------
4 files changed, 223 insertions(+), 79 deletions(-)
--
2.34.1
9:26 a.m.
New subject: [PATCH 1/8] ch: report response message instead of just code
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(a)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
1:29 p.m.
New subject: [PATCH 1/8] ch: report response message instead of just code
On 6/28/2024 2:26 AM, Purna Pavan Chandra wrote:
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(a)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);
Does cloud-hypervisor provide any additional information related to the
failure? From what I remember cloud-hypervisor only provides HTTP
response code in case of failures.
return -1;
}
}
--
Regards,
Praveen
9:26 a.m.
New subject: [PATCH 2/8] ch: Pass net ids explicitly during vm creation
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(a)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
9:26 a.m.
New subject: [PATCH 3/8] ch: refactor chProcessAddNetworkDevices
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.
Signed-off-by: Purna Pavan Chandra <paekkaladevi(a)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
+ *
+ * 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
2:17 p.m.
New subject: [PATCH 3/8] ch: refactor chProcessAddNetworkDevices
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(a)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
9:26 a.m.
New subject: [PATCH 4/8] ch: poll with -1 in chSocketRecv
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(a)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);
} 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)
{
--
2.34.1
2:29 p.m.
New subject: [PATCH 4/8] ch: poll with -1 in chSocketRecv
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(a)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
9:26 a.m.
New subject: [PATCH 5/8] ch: use monitor socket fd to send restore request
This change will later help in passing new net fds to CH during
restore operation.
Signed-off-by: Purna Pavan Chandra <paekkaladevi(a)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;
--
2.34.1
3:15 p.m.
New subject: [PATCH 5/8] ch: use monitor socket fd to send restore request
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(a)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
9:26 a.m.
New subject: [PATCH 6/8] ch: refactor virCHMonitorSaveVM
Remove the unwanted utility function and make api calls directly from
virCHMonitorSaveVM fn
Signed-off-by: Purna Pavan Chandra <paekkaladevi(a)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
9:26 a.m.
New subject: [PATCH 7/8] ch: support restore with net devices
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(a)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
*/
- 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;
}
}
@@ -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;
}
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;
- 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;
}
--
2.34.1
4:59 p.m.
New subject: [PATCH 7/8] ch: support restore with net devices
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(a)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/b2e43609fd68b84f626d63e836acd87...
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.
- 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.
}
}
@@ -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
5:52 p.m.
New subject: [PATCH 7/8] ch: support restore with net devices
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(a)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/b2e43609fd68b84f626d63e836acd87...
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
9:26 a.m.
New subject: [PATCH 8/8] ch: kill CH process if restore fails
Invoke virCHProcessStop to kill CH process incase of any failures during
restore operation.
Signed-off-by: Purna Pavan Chandra <paekkaladevi(a)linux.microsoft.com>
---
src/ch/ch_process.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index cb3539ebe5..acd542b24e 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -1031,5 +1031,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
84
days inactive
84
days old
14 comments
3 participants
participants (3)
-
Praveen K Paladugu -
Purna Pavan Chandra -
Purna Pavan Chandra Aekkaladevi