On 2/6/24 10:34, Purna Pavan Chandra Aekkaladevi wrote:
From: Purna Pavan Chandra Aekkaladevi <paekkaladevi@microsoft.com>
save, managedsave and restore is supported for domains without any network, hostdev config defined. The `path` input to the save command should be a directory path since cloud-hypervisor expects directory path.
Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com> --- src/ch/ch_conf.c | 6 + src/ch/ch_conf.h | 12 ++ src/ch/ch_driver.c | 511 +++++++++++++++++++++++++++++++++++++++++++- src/ch/ch_monitor.c | 97 ++++++++- src/ch/ch_monitor.h | 6 +- src/ch/ch_process.c | 101 +++++++-- src/ch/ch_process.h | 4 + 7 files changed, 715 insertions(+), 22 deletions(-)
There are multiple APIs implemented here which makes it unnecessarily hard to do a proper review. Can you please split this huge patch into smaller ones?
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index f421af5121..c109721a83 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -139,10 +139,12 @@ virCHDriverConfigNew(bool privileged) if (privileged) { cfg->logDir = g_strdup_printf("%s/log/libvirt/ch", LOCALSTATEDIR); cfg->stateDir = g_strdup_printf("%s/libvirt/ch", RUNSTATEDIR); + cfg->saveDir = g_strdup_printf("%s/lib/libvirt/ch/save", LOCALSTATEDIR);
} else { g_autofree char *rundir = NULL; g_autofree char *cachedir = NULL; + g_autofree char *configbasedir = NULL;
cachedir = virGetUserCacheDirectory();
@@ -150,6 +152,9 @@ virCHDriverConfigNew(bool privileged)
rundir = virGetUserRuntimeDirectory(); cfg->stateDir = g_strdup_printf("%s/ch/run", rundir); + + configbasedir = virGetUserConfigDirectory(); + cfg->saveDir = g_strdup_printf("%s/ch/save", configbasedir); }
return cfg; @@ -166,6 +171,7 @@ virCHDriverConfigDispose(void *obj) { virCHDriverConfig *cfg = obj;
+ g_free(cfg->saveDir); g_free(cfg->stateDir); g_free(cfg->logDir); } diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h index 579eca894e..a77cad7a2a 100644 --- a/src/ch/ch_conf.h +++ b/src/ch/ch_conf.h @@ -37,6 +37,7 @@ struct _virCHDriverConfig {
char *stateDir; char *logDir; + char *saveDir;
int cgroupControllers;
@@ -81,6 +82,17 @@ struct _virCHDriver ebtablesContext *ebtables; };
+#define CH_SAVE_MAGIC "libvirt-xml\n \0 \r" +#define CH_SAVE_XML "libvirt-save.xml" + +typedef struct _CHSaveXMLHeader CHSaveXMLHeader; +struct _CHSaveXMLHeader { + char magic[sizeof(CH_SAVE_MAGIC)-1]; + uint32_t xmlLen; + /* 20 bytes used, pad up to 64 bytes */ + uint32_t unused[11]; +}; + virCaps *virCHDriverCapsInit(void); virCaps *virCHDriverGetCapabilities(virCHDriver *driver, bool refresh); diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 96de5044ac..4413abfa79 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -20,6 +20,8 @@
#include <config.h>
+#include <fcntl.h> + #include "ch_capabilities.h" #include "ch_conf.h" #include "ch_domain.h" @@ -32,6 +34,7 @@ #include "viraccessapicheck.h" #include "virchrdev.h" #include "virerror.h" +#include "virfile.h" #include "virlog.h" #include "virobject.h" #include "virtypedparam.h" @@ -176,6 +179,13 @@ static char *chConnectGetCapabilities(virConnectPtr conn) return xml; }
+static char * +chDomainManagedSavePath(virCHDriver *driver, virDomainObj *vm) +{ + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); + return g_strdup_printf("%s/%s.save", cfg->saveDir, vm->def->name); +} + /** * chDomainCreateXML: * @conn: pointer to connection @@ -196,6 +206,7 @@ chDomainCreateXML(virConnectPtr conn, virDomainObj *vm = NULL; virDomainPtr dom = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + g_autofree char *managed_save_path = NULL;
virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL);
@@ -218,6 +229,15 @@ chDomainCreateXML(virConnectPtr conn, NULL))) goto cleanup;
+ /* cleanup if there's any stale managedsave dir */ + managed_save_path = chDomainManagedSavePath(driver, vm); + if (virFileDeleteTree(managed_save_path) < 0) { + virReportSystemError(errno, + _("Failed to cleanup stale managed save dir '%1$s'"), + managed_save_path); + goto cleanup; + }
I don't think we should do this. Users can start a transient domain with the same name and UUID as a defined domain, but a different XML. If we did this then previously saved data is lost (and domain disks are possibly left in an inconsistent state as the domain's memory would be removed here).
+ if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) goto cleanup;
@@ -242,6 +262,8 @@ chDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { virCHDriver *driver = dom->conn->privateData; virDomainObj *vm; + virCHDomainObjPrivate *priv; + g_autofree char *managed_save_path = NULL; int ret = -1;
virCheckFlags(0, -1); @@ -255,8 +277,33 @@ chDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) goto cleanup;
- ret = virCHProcessStart(driver, vm, VIR_DOMAIN_RUNNING_BOOTED); + if (vm->hasManagedSave) { + priv = vm->privateData; + managed_save_path = chDomainManagedSavePath(driver, vm); + if (virCHProcessStartRestore(driver, vm, managed_save_path) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to restore domain from managed save")); + goto endjob; + } + if (virCHMonitorResumeVM(priv->monitor) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to resume domain after restore from managed save")); + goto endjob; + } + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_RESTORED); + if (virFileDeleteTree(managed_save_path) < 0) { + virReportSystemError(errno, + _("Failed to remove managed save path '%1$s'"), + managed_save_path); + goto endjob; + } + vm->hasManagedSave = false; + ret = 0; + } else { + ret = virCHProcessStart(driver, vm, VIR_DOMAIN_RUNNING_BOOTED); + }
+ endjob: virDomainObjEndJob(vm);
cleanup: @@ -277,6 +324,7 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) g_autoptr(virDomainDef) vmdef = NULL; virDomainObj *vm = NULL; virDomainPtr dom = NULL; + g_autofree char *managed_save_path = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -299,6 +347,15 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) 0, NULL))) goto cleanup;
+ /* cleanup if there's any stale managedsave dir */ + managed_save_path = chDomainManagedSavePath(driver, vm); + if (virFileDeleteTree(managed_save_path) < 0) { + virReportSystemError(errno, + _("Failed to cleanup stale managed save dir '%1$s'"), + managed_save_path); + goto cleanup; + } + vm->persistent = 1;
dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); @@ -621,6 +678,449 @@ chDomainDestroy(virDomainPtr dom) return chDomainDestroyFlags(dom, 0); }
+static int +chDomainSaveAdditionalValidation(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; + } + if (vmdef->nhostdevs > 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot save domain with host devices")); + return -1; + } + return 0; +} + +/** + * chDoDomainSave: + * @driver: pointer to driver structure + * @vm: pointer to virtual machine structure. Must be locked before invocation. + * @to_dir: directory path (CH needs directory input) to save the domain
Ah, so it stores multiple files inside of the directory? Well, this calls for update of virDomainSave() documentation then. I don't think it's worth going through the hassle of packing everything under that dir into a single file (e.g. via 'tar'). This is where I stop my review. Sorry for letting this slip. Michal