[PATCH v5 0/5] migration/dirtyrate: Introduce APIs for getting domain memory dirty rate
V4 -> V5: squash 1/7 and bits of 5/7 and 6/7 into 2/7 in v4 (to be 1/5 in v5) squash left of 5/7 into 4/7 in v4 (to be 3/5 in v5) add VIR_DOMAIN_DIRTYRATE_DEFAULT flag remove redundant error report rename virsh api to "domdirtyrate" use vshTablePtr for virsh api output add description in docs/manpages/virsh.rst other format optimize V3 -> V4: define flags to unsigned int fix some compile warnings V2 -> V3: reorganize patchset to fix compile warning V1 -> V2: replace QEMU_JOB_ASYNC with QEMU_JOB_QUERY Sometimes domain's memory dirty rate is expected by user in order to decide whether it's proper to be migrated out or not. We have already completed the QEMU part of the capability: https://patchew.org/QEMU/1600237327-33618-1-git-send-email-zhengchuan@huawei... And this serial of patches introduce the corresponding LIBVIRT part -- DomainGetDirtyRateInfo API and corresponding virsh api -- "domdirtyrate". instructions: bash# virsh domdirtyrate --help NAME domdirtyrate - Get a vm's memory dirty rate SYNOPSIS domdirtyrate <domain> [--seconds <number>] [--calculate] [--query] DESCRIPTION Get memory dirty rate of a domain in order to decide whether it's proper to be migrated out or not. OPTIONS [--domain] <string> domain name, id or uuid --seconds <number> calculate memory dirty rate within specified seconds, the supported value range is [1, 60], default to 1s. --calculate calculate dirty rate only, can be used together with --query, either or both is expected, otherwise would default to both. --query query dirty rate only, can be used together with --calculate, either or both is expected, otherwise would default to both. example: bash# virsh domdirtyrate vm0 --calculate --query --seconds 1 Item Value ----------------------------- Status: measured Start time: 51585 Calculate time: 1 s Dirty rate: 18 MB/s Hao Wang (5): migration/dirtyrate: Introduce DomainGetDirtyRateInfo API migration/dirtyrate: Implement qemuDomainCalculateDirtyRate migration/dirtyrate: Implement qemuDomainQueryDirtyRate migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo migration/dirtyrate: Introduce domdirtyrate virsh api docs/manpages/virsh.rst | 17 +++++ include/libvirt/libvirt-domain.h | 59 ++++++++++++++ src/driver-hypervisor.h | 7 ++ src/libvirt-domain.c | 56 ++++++++++++++ src/libvirt_public.syms | 5 ++ src/qemu/qemu_driver.c | 67 ++++++++++++++++ src/qemu/qemu_migration.c | 48 ++++++++++++ src/qemu/qemu_migration.h | 10 +++ src/qemu/qemu_monitor.c | 24 ++++++ src/qemu/qemu_monitor.h | 8 ++ src/qemu/qemu_monitor_json.c | 99 ++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 8 ++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 21 ++++- tools/virsh-domain.c | 127 +++++++++++++++++++++++++++++++ 15 files changed, 556 insertions(+), 1 deletion(-) -- 2.23.0
Introduce DomainGetDirtyRateInfo API to get domain's memory dirty rate info which may be expected by user in order to decide whether it's proper to be migrated out or not. Using flags to control the action of the API: If the VIR_DOMAIN_DIRTYRATE_CALC flag is set, this will calculate domain's memory dirty rate within specific time. If the VIR_DOMAIN_DIRTYRATE_QUERY flag is set, this will query the dirty rate info calculated last time. The VIR_DOMAIN_DIRTYRATE_DEFAULT flag is equal to both VIR_DOMAIN_DIRTYRATE_CALC and VIR_DOMAIN_DIRTYRATE_QUERY. Signed-off-by: Hao Wang <wanghao232@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- include/libvirt/libvirt-domain.h | 59 ++++++++++++++++++++++++++++++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 21 +++++++++++- 6 files changed, 148 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index de2456812c..77b46c2018 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5119,4 +5119,63 @@ int virDomainAuthorizedSSHKeysSet(virDomainPtr domain, unsigned int nkeys, unsigned int flags); +/** + * virDomainDirtyRateStatus: + * + * Details on the cause of a dirty rate calculation status. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has + not been started */ + VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is + measuring */ + VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is + completed */ + +# ifdef VIR_ENUM_SENTINELS + VIR_DOMAIN_DIRTYRATE_LAST +# endif +} virDomainDirtyRateStatus; + +/** + * virDomainDirtyRateInfo: + * + * a virDomainDirtyRateInfo is a structure filled by virDomainGetDirtyRate() + * and extracting dirty rate infomation for a given active Domain. + */ + +typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo; +struct _virDomainDirtyRateInfo { + int status; /* the status of dirtyrate calculation, one of + virDomainDirtyRateStatus */ + long long dirtyRate; /* the dirtyrate in MB/s */ + long long startTime; /* the start time of dirtyrate calculation */ + int calcTime; /* the period of dirtyrate calculation */ +}; + +/** + * virDomainDirtyRateInfoPtr: + * + * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo structure. + */ + +typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr; + +/** + * virDomainDirtyRateFlags: + * + * Details on the flags used by getdirtyrate api. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_DEFAULT = 0, /* default domdirtyrate behavior: + calculate and query */ + VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirtyrate */ + VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */ +} virDomainDirtyRateFlags; + +int virDomainGetDirtyRateInfo(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags); + #endif /* LIBVIRT_DOMAIN_H */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 9e8fe89921..5ad681997b 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1400,6 +1400,12 @@ typedef int unsigned int nkeys, unsigned int flags); +typedef int +(*virDrvDomainGetDirtyRateInfo)(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags); + typedef struct _virHypervisorDriver virHypervisorDriver; typedef virHypervisorDriver *virHypervisorDriverPtr; @@ -1665,4 +1671,5 @@ struct _virHypervisorDriver { virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc; virDrvDomainAuthorizedSSHKeysGet domainAuthorizedSSHKeysGet; virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet; + virDrvDomainGetDirtyRateInfo domainGetDirtyRateInfo; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index c9f8ffdb56..777028c499 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13102,3 +13102,59 @@ virDomainAuthorizedSSHKeysSet(virDomainPtr domain, virDispatchError(conn); return -1; } + + +/** + * virDomainGetDirtyRateInfo: + * @domain: a domain object + * @info: pointer to current domain's memory dirty rate info + * @sec: show dirty rate within specified seconds + * @flags: extra flags; binary-OR of virDomainGetDirtyRateInfoFlags + * + * Get the current domain's memory dirty rate info. + * + * If the VIR_DOMAIN_DIRTYRATE_CALC flag is set, this will calculate + * domain's memory dirty rate within specific time (sec). + * + * If the VIR_DOMAIN_DIRTYRATE_QUERY flag is set, this will query the + * dirty rate info calculated last time and stored in 'info'. + * + * The VIR_DOMAIN_DIRTYRATE_DEFAULT flag is equal to both + * VIR_DOMAIN_DIRTYRATE_CALC and VIR_DOMAIN_DIRTYRATE_QUERY. + * + * Returns 0 in case of success, -1 otherwise. + */ +int +virDomainGetDirtyRateInfo(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "info=%p, sec=%d, flags=0x%x", + info, sec, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckNonNullArgGoto(info, error); + memset(info, 0, sizeof(*info)); + + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->driver->domainGetDirtyRateInfo) { + int ret; + ret = conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index cf31f937d5..155b75713b 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -879,4 +879,9 @@ LIBVIRT_6.10.0 { virDomainAuthorizedSSHKeysSet; } LIBVIRT_6.0.0; +LIBVIRT_7.1.0 { + global: + virDomainGetDirtyRateInfo; +} LIBVIRT_6.10.0; + # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1b784e61c7..4460d2c7ce 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8531,6 +8531,7 @@ static virHypervisorDriver hypervisor_driver = { .domainBackupGetXMLDesc = remoteDomainBackupGetXMLDesc, /* 6.0.0 */ .domainAuthorizedSSHKeysGet = remoteDomainAuthorizedSSHKeysGet, /* 6.10.0 */ .domainAuthorizedSSHKeysSet = remoteDomainAuthorizedSSHKeysSet, /* 6.10.0 */ + .domainGetDirtyRateInfo = remoteDomainGetDirtyRateInfo, /* 7.1.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 2df38cef77..965d05f68f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3799,6 +3799,19 @@ struct remote_domain_authorized_ssh_keys_set_args { unsigned int flags; }; +struct remote_domain_get_dirty_rate_info_args { + remote_nonnull_domain dom; + int sec; + unsigned int flags; +}; + +struct remote_domain_get_dirty_rate_info_ret { /* insert@1 */ + int status; + hyper dirtyRate; + hyper startTime; + int calcTime; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -6714,5 +6727,11 @@ enum remote_procedure { * @generate: none * @acl: domain:write */ - REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425 + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, + + /** + * @generate: both + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_DIRTY_RATE_INFO = 426 }; -- 2.23.0
This patch is making the 'check-remote-protocol' test error out in my env: Ok: 305 Expected Fail: 0 Fail: 1 Unexpected Pass: 0 Skipped: 4 Timeout: 0 The output from the failed tests: 12/310 libvirt / check-remote_protocol FAIL 0.27s (exit status 1) --- command --- 11:55:05 LC_CTYPE='en_US.UTF-8' LANG='C' LC_ALL='' /usr/bin/python3 /home/danielhb/kvm-project/libvirt/scripts/check-remote-protocol.py remote_protocol virt_remote_driver /home/danielhb/kvm-project/libvirt/build/src/remote/libvirt_remote_driver.a /usr/bin/pdwtags /home/danielhb/kvm-project/libvirt/build/../src/remote_protocol-structs --- stdout --- --- /home/danielhb/kvm-project/libvirt/build/../src/remote_protocol-structs 2020-11-19 13:27:02.018823909 -0300 +++ - 2021-02-01 08:55:05.827296710 -0300 @@ -3162,6 +3162,17 @@ } keys; u_int flags; }; +struct remote_domain_get_dirty_rate_info_args { + remote_nonnull_domain dom; + int sec; + u_int flags; +}; +struct remote_domain_get_dirty_rate_info_ret { + int status; + int64_t dirtyRate; + int64_t startTime; + int calcTime; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3588,4 +3599,5 @@ REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET = 424, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, + REMOTE_PROC_DOMAIN_GET_DIRTY_RATE_INFO = 426, }; ------- I'm not sure how to fix it. This error persists even if I apply all the patches, meaning that it's not something that is being declared too early. I'm afraid you're missing some step in the remote protocol implementation that is making this test fail .... or perhaps I'm the one messing this up. Let's see what others will say. Thanks, DHB On 2/1/21 6:55 AM, Hao Wang wrote:
Introduce DomainGetDirtyRateInfo API to get domain's memory dirty rate info which may be expected by user in order to decide whether it's proper to be migrated out or not. Using flags to control the action of the API:
If the VIR_DOMAIN_DIRTYRATE_CALC flag is set, this will calculate domain's memory dirty rate within specific time.
If the VIR_DOMAIN_DIRTYRATE_QUERY flag is set, this will query the dirty rate info calculated last time.
The VIR_DOMAIN_DIRTYRATE_DEFAULT flag is equal to both VIR_DOMAIN_DIRTYRATE_CALC and VIR_DOMAIN_DIRTYRATE_QUERY.
Signed-off-by: Hao Wang <wanghao232@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- include/libvirt/libvirt-domain.h | 59 ++++++++++++++++++++++++++++++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 21 +++++++++++- 6 files changed, 148 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index de2456812c..77b46c2018 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5119,4 +5119,63 @@ int virDomainAuthorizedSSHKeysSet(virDomainPtr domain, unsigned int nkeys, unsigned int flags);
+/** + * virDomainDirtyRateStatus: + * + * Details on the cause of a dirty rate calculation status. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has + not been started */ + VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is + measuring */ + VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is + completed */ + +# ifdef VIR_ENUM_SENTINELS + VIR_DOMAIN_DIRTYRATE_LAST +# endif +} virDomainDirtyRateStatus; + +/** + * virDomainDirtyRateInfo: + * + * a virDomainDirtyRateInfo is a structure filled by virDomainGetDirtyRate() + * and extracting dirty rate infomation for a given active Domain. + */ + +typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo; +struct _virDomainDirtyRateInfo { + int status; /* the status of dirtyrate calculation, one of + virDomainDirtyRateStatus */ + long long dirtyRate; /* the dirtyrate in MB/s */ + long long startTime; /* the start time of dirtyrate calculation */ + int calcTime; /* the period of dirtyrate calculation */ +}; + +/** + * virDomainDirtyRateInfoPtr: + * + * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo structure. + */ + +typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr; + +/** + * virDomainDirtyRateFlags: + * + * Details on the flags used by getdirtyrate api. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_DEFAULT = 0, /* default domdirtyrate behavior: + calculate and query */ + VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirtyrate */ + VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */ +} virDomainDirtyRateFlags; + +int virDomainGetDirtyRateInfo(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags); + #endif /* LIBVIRT_DOMAIN_H */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 9e8fe89921..5ad681997b 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1400,6 +1400,12 @@ typedef int unsigned int nkeys, unsigned int flags);
+typedef int +(*virDrvDomainGetDirtyRateInfo)(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags); + typedef struct _virHypervisorDriver virHypervisorDriver; typedef virHypervisorDriver *virHypervisorDriverPtr;
@@ -1665,4 +1671,5 @@ struct _virHypervisorDriver { virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc; virDrvDomainAuthorizedSSHKeysGet domainAuthorizedSSHKeysGet; virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet; + virDrvDomainGetDirtyRateInfo domainGetDirtyRateInfo; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index c9f8ffdb56..777028c499 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13102,3 +13102,59 @@ virDomainAuthorizedSSHKeysSet(virDomainPtr domain, virDispatchError(conn); return -1; } + + +/** + * virDomainGetDirtyRateInfo: + * @domain: a domain object + * @info: pointer to current domain's memory dirty rate info + * @sec: show dirty rate within specified seconds + * @flags: extra flags; binary-OR of virDomainGetDirtyRateInfoFlags + * + * Get the current domain's memory dirty rate info. + * + * If the VIR_DOMAIN_DIRTYRATE_CALC flag is set, this will calculate + * domain's memory dirty rate within specific time (sec). + * + * If the VIR_DOMAIN_DIRTYRATE_QUERY flag is set, this will query the + * dirty rate info calculated last time and stored in 'info'. + * + * The VIR_DOMAIN_DIRTYRATE_DEFAULT flag is equal to both + * VIR_DOMAIN_DIRTYRATE_CALC and VIR_DOMAIN_DIRTYRATE_QUERY. + * + * Returns 0 in case of success, -1 otherwise. + */ +int +virDomainGetDirtyRateInfo(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "info=%p, sec=%d, flags=0x%x", + info, sec, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckNonNullArgGoto(info, error); + memset(info, 0, sizeof(*info)); + + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->driver->domainGetDirtyRateInfo) { + int ret; + ret = conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index cf31f937d5..155b75713b 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -879,4 +879,9 @@ LIBVIRT_6.10.0 { virDomainAuthorizedSSHKeysSet; } LIBVIRT_6.0.0;
+LIBVIRT_7.1.0 { + global: + virDomainGetDirtyRateInfo; +} LIBVIRT_6.10.0; + # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1b784e61c7..4460d2c7ce 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8531,6 +8531,7 @@ static virHypervisorDriver hypervisor_driver = { .domainBackupGetXMLDesc = remoteDomainBackupGetXMLDesc, /* 6.0.0 */ .domainAuthorizedSSHKeysGet = remoteDomainAuthorizedSSHKeysGet, /* 6.10.0 */ .domainAuthorizedSSHKeysSet = remoteDomainAuthorizedSSHKeysSet, /* 6.10.0 */ + .domainGetDirtyRateInfo = remoteDomainGetDirtyRateInfo, /* 7.1.0 */ };
static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 2df38cef77..965d05f68f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3799,6 +3799,19 @@ struct remote_domain_authorized_ssh_keys_set_args { unsigned int flags; };
+struct remote_domain_get_dirty_rate_info_args { + remote_nonnull_domain dom; + int sec; + unsigned int flags; +}; + +struct remote_domain_get_dirty_rate_info_ret { /* insert@1 */ + int status; + hyper dirtyRate; + hyper startTime; + int calcTime; +}; + /*----- Protocol. -----*/
/* Define the program number, protocol version and procedure numbers here. */ @@ -6714,5 +6727,11 @@ enum remote_procedure { * @generate: none * @acl: domain:write */ - REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425 + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, + + /** + * @generate: both + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_DIRTY_RATE_INFO = 426 };
On 2/1/21 1:11 PM, Daniel Henrique Barboza wrote:
This patch is making the 'check-remote-protocol' test error out in my env:
Indeed, any change to remote_protocol.x has to be coupled with change to src/remote_protocol-structs. The idea for this test is that we take compiled version of our RPC and use pdwtags to "decompile" it. Then, the output generated by pwdtags is compared against well known output stored in git (src/remote_protocol-structs). The idea is that we will catch incompatible changes made by rpcgen/compiler/developer. BTW, that is the reasoning behind Dan's patch: commit e603efb6ec5d1a2295adfda934e79f022bb7bb0e Author: Daniel P. Berrangé <berrange@redhat.com> AuthorDate: Mon Jan 25 18:13:57 2021 +0000 Commit: Daniel P. Berrangé <berrange@redhat.com> CommitDate: Tue Jan 26 12:33:31 2021 +0000 gitlab: force dwarf4 format for debuginfo in Fedora rawhide Fedora 34 rawhide has pulled in a new GCC 11 build which now defaults to dwarf5 format. This format is not compatible with the pdwtags program used in our test suite to validate the RPC files. We have no need for debuginfo in CI except for pdwtags, so the simplest short term fix is to force the older dwarf version in the hope that a fixed dwarves release will arrive before Fedora 34 is released, or GCC 11 becomes more widespread. Eventually we might need to figure out a way to probe for compatibility but for now, we'll hope that any distro with GCC 11 will be able to have a fixed dwarves too. https://bugzilla.redhat.com/show_bug.cgi?id=1919965 Reviewed-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> In this case, it's developer's fault for not updating remote_protocol-structs to contain additions made to remote_protocol.x. Anyway, I don't think we will need new RPC anyway. Let me comment to the patch itself. Michal
Thanks for reviewing my patchset. Actually I remember that I have passed all tests in my env before uploading. However, I doubel-check the results after receving Daniel's reply, and found that 'check-remote-protocol'does not lie in my test list for the reason that dwarves is missing in my env. After installing dwarves, I got same test error with Daniel. Hao On 2021/2/1 21:24, Michal Privoznik wrote:
On 2/1/21 1:11 PM, Daniel Henrique Barboza wrote:
This patch is making the 'check-remote-protocol' test error out in my env:
Indeed, any change to remote_protocol.x has to be coupled with change to src/remote_protocol-structs. The idea for this test is that we take compiled version of our RPC and use pdwtags to "decompile" it. Then, the output generated by pwdtags is compared against well known output stored in git (src/remote_protocol-structs). The idea is that we will catch incompatible changes made by rpcgen/compiler/developer. BTW, that is the reasoning behind Dan's patch:
commit e603efb6ec5d1a2295adfda934e79f022bb7bb0e Author: Daniel P. Berrangé <berrange@redhat.com> AuthorDate: Mon Jan 25 18:13:57 2021 +0000 Commit: Daniel P. Berrangé <berrange@redhat.com> CommitDate: Tue Jan 26 12:33:31 2021 +0000
gitlab: force dwarf4 format for debuginfo in Fedora rawhide
Fedora 34 rawhide has pulled in a new GCC 11 build which now defaults to dwarf5 format. This format is not compatible with the pdwtags program used in our test suite to validate the RPC files.
We have no need for debuginfo in CI except for pdwtags, so the simplest short term fix is to force the older dwarf version in the hope that a fixed dwarves release will arrive before Fedora 34 is released, or GCC 11 becomes more widespread. Eventually we might need to figure out a way to probe for compatibility but for now, we'll hope that any distro with GCC 11 will be able to have a fixed dwarves too.
https://bugzilla.redhat.com/show_bug.cgi?id=1919965 Reviewed-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In this case, it's developer's fault for not updating remote_protocol-structs to contain additions made to remote_protocol.x.
Anyway, I don't think we will need new RPC anyway. Let me comment to the patch itself.
Michal
.
On 2/1/21 10:55 AM, Hao Wang wrote:
Introduce DomainGetDirtyRateInfo API to get domain's memory dirty rate info which may be expected by user in order to decide whether it's proper to be migrated out or not. Using flags to control the action of the API:
If the VIR_DOMAIN_DIRTYRATE_CALC flag is set, this will calculate domain's memory dirty rate within specific time.
If the VIR_DOMAIN_DIRTYRATE_QUERY flag is set, this will query the dirty rate info calculated last time.
The VIR_DOMAIN_DIRTYRATE_DEFAULT flag is equal to both VIR_DOMAIN_DIRTYRATE_CALC and VIR_DOMAIN_DIRTYRATE_QUERY.
Signed-off-by: Hao Wang <wanghao232@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- include/libvirt/libvirt-domain.h | 59 ++++++++++++++++++++++++++++++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 21 +++++++++++- 6 files changed, 148 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index de2456812c..77b46c2018 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5119,4 +5119,63 @@ int virDomainAuthorizedSSHKeysSet(virDomainPtr domain, unsigned int nkeys, unsigned int flags);
+/** + * virDomainDirtyRateStatus: + * + * Details on the cause of a dirty rate calculation status. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has + not been started */ + VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is + measuring */ + VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is + completed */ + +# ifdef VIR_ENUM_SENTINELS + VIR_DOMAIN_DIRTYRATE_LAST +# endif +} virDomainDirtyRateStatus; + +/** + * virDomainDirtyRateInfo: + * + * a virDomainDirtyRateInfo is a structure filled by virDomainGetDirtyRate() + * and extracting dirty rate infomation for a given active Domain. + */ + +typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo; +struct _virDomainDirtyRateInfo { + int status; /* the status of dirtyrate calculation, one of + virDomainDirtyRateStatus */ + long long dirtyRate; /* the dirtyrate in MB/s */
I guess you meant MiB/s.
+ long long startTime; /* the start time of dirtyrate calculation */ + int calcTime; /* the period of dirtyrate calculation */ +};
Do we need to expose this as a struct? IIRC, in review of v4 Peter was suggesting this to be exposed as a new set of virTypedParameter under virDomainListGetStats() and virConnectGetAllDomainStats(). Problem with structures is that once they are released, we can not ever change them (the best we can do is update comments), we can not even change order of members, because might break how structure is organized in memory (compiler might put padding at different place than originally) and thus we would break ABI. Therefore, if we ever need to report one member more, we can't. Well, various projects approach this differently. Some put intentional padding at the end of structure to reserve extra bytes for future use. That's ugly and not scalable. What we invented for this purpose are so called typed parameters: basically an array of <key, type, value> tuples. Users can then iterate over returned array and look for items interesting to them. For instance: virsh domstats fedora Domain: 'fedora' state.state=1 state.reason=1 cpu.time=77689980240 cpu.user=700000000 cpu.system=74490000000 cpu.cache.monitor.count=0 ...
+ +/** + * virDomainDirtyRateInfoPtr: + * + * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo structure. + */ + +typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr; + +/** + * virDomainDirtyRateFlags: + * + * Details on the flags used by getdirtyrate api. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_DEFAULT = 0, /* default domdirtyrate behavior: + calculate and query */ + VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirtyrate */ + VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */ +} virDomainDirtyRateFlags; + +int virDomainGetDirtyRateInfo(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags);
Looking into the future, this is supposed to work like this: with a single API call I can both set the time calc time and get the results back: virDomainGetDirtyRateInfo(dom, &info, 10, CALC | QUERY); This call will block for 11.5 seconds. That sounds like bad design, esp. since QEMU tells caller the state the calculation's in (measuring vs. measured). How about splitting this into two APIs? virDomainStartDirtyRateCalc(dom, seconds, flags); for starting the calculation, and then the second: new typed params for domstats mentioned above Note that @flags in new API will probably be unused for now and that's okay. It's more future proof this way. There's also virDomainSetMemoryParameters() which we could use instead of new API - if new typed param is introduced for starting calculation, but that feels wrong (I can't tell why really, it's just a gut feeling). Michal
This's quite helpful suggestions. I'll refactor the APIs following the advices. BR, Hao On 2021/2/1 22:32, Michal Privoznik wrote:
On 2/1/21 10:55 AM, Hao Wang wrote:
Introduce DomainGetDirtyRateInfo API to get domain's memory dirty rate info which may be expected by user in order to decide whether it's proper to be migrated out or not. Using flags to control the action of the API:
If the VIR_DOMAIN_DIRTYRATE_CALC flag is set, this will calculate domain's memory dirty rate within specific time.
If the VIR_DOMAIN_DIRTYRATE_QUERY flag is set, this will query the dirty rate info calculated last time.
The VIR_DOMAIN_DIRTYRATE_DEFAULT flag is equal to both VIR_DOMAIN_DIRTYRATE_CALC and VIR_DOMAIN_DIRTYRATE_QUERY.
Signed-off-by: Hao Wang <wanghao232@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- include/libvirt/libvirt-domain.h | 59 ++++++++++++++++++++++++++++++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 21 +++++++++++- 6 files changed, 148 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index de2456812c..77b46c2018 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5119,4 +5119,63 @@ int virDomainAuthorizedSSHKeysSet(virDomainPtr domain, unsigned int nkeys, unsigned int flags); +/** + * virDomainDirtyRateStatus: + * + * Details on the cause of a dirty rate calculation status. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has + not been started */ + VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is + measuring */ + VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is + completed */ + +# ifdef VIR_ENUM_SENTINELS + VIR_DOMAIN_DIRTYRATE_LAST +# endif +} virDomainDirtyRateStatus; + +/** + * virDomainDirtyRateInfo: + * + * a virDomainDirtyRateInfo is a structure filled by virDomainGetDirtyRate() + * and extracting dirty rate infomation for a given active Domain. + */ + +typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo; +struct _virDomainDirtyRateInfo { + int status; /* the status of dirtyrate calculation, one of + virDomainDirtyRateStatus */ + long long dirtyRate; /* the dirtyrate in MB/s */
I guess you meant MiB/s.
+ long long startTime; /* the start time of dirtyrate calculation */ + int calcTime; /* the period of dirtyrate calculation */ +};
Do we need to expose this as a struct? IIRC, in review of v4 Peter was suggesting this to be exposed as a new set of virTypedParameter under virDomainListGetStats() and virConnectGetAllDomainStats().
Problem with structures is that once they are released, we can not ever change them (the best we can do is update comments), we can not even change order of members, because might break how structure is organized in memory (compiler might put padding at different place than originally) and thus we would break ABI. Therefore, if we ever need to report one member more, we can't. Well, various projects approach this differently. Some put intentional padding at the end of structure to reserve extra bytes for future use. That's ugly and not scalable.
What we invented for this purpose are so called typed parameters: basically an array of <key, type, value> tuples. Users can then iterate over returned array and look for items interesting to them. For instance:
virsh domstats fedora Domain: 'fedora' state.state=1 state.reason=1 cpu.time=77689980240 cpu.user=700000000 cpu.system=74490000000 cpu.cache.monitor.count=0 ...
+ +/** + * virDomainDirtyRateInfoPtr: + * + * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo structure. + */ + +typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr; + +/** + * virDomainDirtyRateFlags: + * + * Details on the flags used by getdirtyrate api. + */ +typedef enum { + VIR_DOMAIN_DIRTYRATE_DEFAULT = 0, /* default domdirtyrate behavior: + calculate and query */ + VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirtyrate */ + VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */ +} virDomainDirtyRateFlags; + +int virDomainGetDirtyRateInfo(virDomainPtr domain, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags);
Looking into the future, this is supposed to work like this: with a single API call I can both set the time calc time and get the results back:
virDomainGetDirtyRateInfo(dom, &info, 10, CALC | QUERY);
This call will block for 11.5 seconds. That sounds like bad design, esp. since QEMU tells caller the state the calculation's in (measuring vs. measured). How about splitting this into two APIs?
virDomainStartDirtyRateCalc(dom, seconds, flags);
for starting the calculation, and then the second:
new typed params for domstats mentioned above
Note that @flags in new API will probably be unused for now and that's okay. It's more future proof this way. There's also virDomainSetMemoryParameters() which we could use instead of new API - if new typed param is introduced for starting calculation, but that feels wrong (I can't tell why really, it's just a gut feeling).
Michal
.
Implement qemuDomainCalculateDirtyRate which calculates domain's memory dirty rate calling qmp "calc-dirty-rate". Signed-off-by: Hao Wang <wanghao232@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- src/qemu/qemu_migration.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_migration.h | 5 +++++ src/qemu/qemu_monitor.c | 12 ++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ src/qemu/qemu_monitor_json.c | 22 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++++ 6 files changed, 72 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0adfdb9351..226fe5395e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5891,3 +5891,28 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver, virHashFree(blockinfo); return 0; } + + +int +qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int sec) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + VIR_DEBUG("Calculate dirty rate during %d seconds", sec); + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + return -1; + } + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorCalculateDirtyRate(priv->mon, sec); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + return ret; +} diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index fd9eb7cab0..6b1f4a8eea 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -258,3 +258,8 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, qemuDomainJobInfoPtr jobInfo); + +int +qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int sec); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 09b8617097..51bc9182f3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4713,3 +4713,15 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, return qemuMonitorJSONTransactionBackup(actions, device, jobname, target, bitmap, syncmode); } + + +int +qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, + int sec) +{ + VIR_DEBUG("seconds=%d", sec); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONCalculateDirtyRate(mon, sec); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index a07617ec28..a11a5dc21c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1516,3 +1516,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, const char *target, const char *bitmap, qemuMonitorTransactionBackupSyncMode syncmode); + +int +qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, + int sec); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8a75a2734e..bb85513aa6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9452,3 +9452,25 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, return virJSONValueGetBoolean(virJSONValueObjectGet(reply, "return"), migratable); } + + +int +qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, + int sec) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate", + "I:calc-time", sec, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ba1531fee8..cc59d2c078 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -705,3 +705,7 @@ int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, int qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, bool *migratable); + +int +qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, + int sec); -- 2.23.0
On 2/1/21 10:55 AM, Hao Wang wrote:
Implement qemuDomainCalculateDirtyRate which calculates domain's memory dirty rate calling qmp "calc-dirty-rate".
Signed-off-by: Hao Wang <wanghao232@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- src/qemu/qemu_migration.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_migration.h | 5 +++++ src/qemu/qemu_monitor.c | 12 ++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ src/qemu/qemu_monitor_json.c | 22 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++++ 6 files changed, 72 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8a75a2734e..bb85513aa6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9452,3 +9452,25 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, return virJSONValueGetBoolean(virJSONValueObjectGet(reply, "return"), migratable); } + + +int +qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, + int sec) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate", + "I:calc-time", sec,
I don't think that this is correct. "I" stands for long long, but @sec is just an int. I believe this would break on 32bits.
+ NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +}
Michal
Implement qemuDomainQueryDirtyRate which query domain's memory dirty rate calling qmp "query-dirty-rate". Signed-off-by: Hao Wang <wanghao232@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- src/qemu/qemu_migration.c | 23 +++++++++++ src/qemu/qemu_migration.h | 5 +++ src/qemu/qemu_monitor.c | 12 ++++++ src/qemu/qemu_monitor.h | 4 ++ src/qemu/qemu_monitor_json.c | 77 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++ 6 files changed, 125 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 226fe5395e..373896e408 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5916,3 +5916,26 @@ qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver, return ret; } + + +int +qemuDomainQueryDirtyRate(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDirtyRateInfoPtr info) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + return -1; + } + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorQueryDirtyRate(priv->mon, info); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + return ret; +} diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 6b1f4a8eea..a894c3c527 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -263,3 +263,8 @@ int qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver, virDomainObjPtr vm, int sec); + +int +qemuDomainQueryDirtyRate(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDirtyRateInfoPtr info); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 51bc9182f3..b8c428fca8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4725,3 +4725,15 @@ qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, return qemuMonitorJSONCalculateDirtyRate(mon, sec); } + + +int +qemuMonitorQueryDirtyRate(qemuMonitorPtr mon, + virDomainDirtyRateInfoPtr info) +{ + VIR_DEBUG("info=%p", info); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONQueryDirtyRate(mon, info); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index a11a5dc21c..9814fe0218 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1520,3 +1520,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions, int qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon, int sec); + +int +qemuMonitorQueryDirtyRate(qemuMonitorPtr mon, + virDomainDirtyRateInfoPtr info); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index bb85513aa6..ba301029b0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9474,3 +9474,80 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, return 0; } + + +VIR_ENUM_DECL(qemuDomainDirtyRateStatus); +VIR_ENUM_IMPL(qemuDomainDirtyRateStatus, + VIR_DOMAIN_DIRTYRATE_LAST, + "unstarted", + "measuring", + "measured"); + +static int +qemuMonitorJSONExtractDirtyRateInfo(virJSONValuePtr data, + virDomainDirtyRateInfoPtr info) +{ + const char *status; + int statusID; + + if (!(status = virJSONValueObjectGetString(data, "status"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'status' data")); + return -1; + } + + if ((statusID = qemuDomainDirtyRateStatusTypeFromString(status)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown dirty rate status: %s"), status); + return -1; + } + info->status = statusID; + + if ((info->status == VIR_DOMAIN_DIRTYRATE_MEASURED) && + (virJSONValueObjectGetNumberLong(data, "dirty-rate", &info->dirtyRate) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'dirty-rate' data")); + return -1; + } + + if (virJSONValueObjectGetNumberLong(data, "start-time", &info->startTime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'start-time' data")); + return -1; + } + + if (virJSONValueObjectGetNumberInt(data, "calc-time", &info->calcTime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'calc-time' data")); + return -1; + } + + return 0; +} + + +int +qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon, + virDomainDirtyRateInfoPtr info) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + virJSONValuePtr data = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-dirty-rate", NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + if (!(data = virJSONValueObjectGetObject(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-dirty-rate reply was missing 'return' data")); + return -1; + } + + return qemuMonitorJSONExtractDirtyRateInfo(data, info); +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index cc59d2c078..27ced5cd5e 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -709,3 +709,7 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon, int qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon, int sec); + +int +qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon, + virDomainDirtyRateInfoPtr info); -- 2.23.0
Implement qemuDomainGetDirtyRateInfo: using flags to control behaviors -- calculate and/or query dirtyrate. Default flag is both calculate and query. Signed-off-by: Hao Wang <wanghao232@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed840a5c8d..2b9ce1c386 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20289,6 +20289,72 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom, } +#define MIN_DIRTYRATE_CALCULATION_PERIOD 1 /* supported min dirtyrate calc time: 1s */ +#define MAX_DIRTYRATE_CALCULATION_PERIOD 60 /* supported max dirtyrate calc time: 60s */ + +static int +qemuDomainGetDirtyRateInfo(virDomainPtr dom, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + /* flags is default to both calculate and query */ + if (flags == VIR_DOMAIN_DIRTYRATE_DEFAULT) + flags |= VIR_DOMAIN_DIRTYRATE_CALC | VIR_DOMAIN_DIRTYRATE_QUERY; + + /* calculating */ + if (flags & VIR_DOMAIN_DIRTYRATE_CALC) { + if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || + sec > MAX_DIRTYRATE_CALCULATION_PERIOD) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("seconds=%d is invalid, please choose value within [%d, %d]."), + sec, + MIN_DIRTYRATE_CALCULATION_PERIOD, + MAX_DIRTYRATE_CALCULATION_PERIOD); + goto endjob; + } + + if (qemuDomainCalculateDirtyRate(driver, vm, sec) < 0) + goto endjob; + } + + /* querying */ + if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) { + /* wait sec and extra 50ms to let last calculation finish */ + if (flags & VIR_DOMAIN_DIRTYRATE_CALC) { + virObjectUnlock(vm); + g_usleep((sec * 1000 + 50) * 1000); + virObjectLock(vm); + } + + if (qemuDomainQueryDirtyRate(driver, vm, info) < 0) + goto endjob; + } + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, .connectURIProbe = qemuConnectURIProbe, @@ -20530,6 +20596,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */ .domainAuthorizedSSHKeysGet = qemuDomainAuthorizedSSHKeysGet, /* 6.10.0 */ .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */ + .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 7.1.0 */ }; -- 2.23.0
On 2/1/21 10:55 AM, Hao Wang wrote:
Implement qemuDomainGetDirtyRateInfo: using flags to control behaviors -- calculate and/or query dirtyrate. Default flag is both calculate and query.
Signed-off-by: Hao Wang <wanghao232@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed840a5c8d..2b9ce1c386 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20289,6 +20289,72 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom, }
+#define MIN_DIRTYRATE_CALCULATION_PERIOD 1 /* supported min dirtyrate calc time: 1s */ +#define MAX_DIRTYRATE_CALCULATION_PERIOD 60 /* supported max dirtyrate calc time: 60s */ + +static int +qemuDomainGetDirtyRateInfo(virDomainPtr dom, + virDomainDirtyRateInfoPtr info, + int sec, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1;
Here should be virCheckFlags() call with all flags supported enumarated. The idea is that if a new flag is ever invented then instead of ignoring it silently an error is produced.
+ + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + /* flags is default to both calculate and query */ + if (flags == VIR_DOMAIN_DIRTYRATE_DEFAULT) + flags |= VIR_DOMAIN_DIRTYRATE_CALC | VIR_DOMAIN_DIRTYRATE_QUERY; + + /* calculating */ + if (flags & VIR_DOMAIN_DIRTYRATE_CALC) { + if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || + sec > MAX_DIRTYRATE_CALCULATION_PERIOD) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("seconds=%d is invalid, please choose value within [%d, %d]."), + sec, + MIN_DIRTYRATE_CALCULATION_PERIOD, + MAX_DIRTYRATE_CALCULATION_PERIOD); + goto endjob; + } + + if (qemuDomainCalculateDirtyRate(driver, vm, sec) < 0) + goto endjob; + } + + /* querying */ + if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) { + /* wait sec and extra 50ms to let last calculation finish */ + if (flags & VIR_DOMAIN_DIRTYRATE_CALC) { + virObjectUnlock(vm); + g_usleep((sec * 1000 + 50) * 1000); + virObjectLock(vm);
I know this will probably change, but anyway - waiting X seconds is not good IMO. Also, it performs two operations at once. What if the calculation was successfully started but then querying fails? How can a caller distinguish this from a case where the calculation failed to start? Michal
The new virsh command are: domdirtyrate <domain> [--calculate] [--query] [--seconds <sec>] Signed-off-by: Hao Wang <wanghao232@huawei.com> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com> --- docs/manpages/virsh.rst | 17 ++++++ tools/virsh-domain.c | 127 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index e3afa48f7b..c7cff8a647 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1704,6 +1704,23 @@ states other than "ok" or "error" the command also prints number of seconds elapsed since the control interface entered its current state. +domdirtyrate +------------ + +**Syntax:** + +:: + + domdirtyrate <domain> [--calculate] [--query] [--seconds <sec>] + +Calculate and/or query an active domain's memory dirty rate which may be +expected by user in order to decide whether it's proper to be migrated out +or not. Either or both *--calculate* and *--query* flags are expected, and +it would be default to both if no flags is specified. And the ``seconds`` +parameter can be used to calculate dirty rate in a specific time which allows +60s at most now and would be default to 1s if missing. + + domdisplay ---------- diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2bb136333f..2cdd26b3b6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -14416,6 +14416,127 @@ cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd) } +/* + * "domdirtyrate" command + */ +static const vshCmdInfo info_domdirtyrate[] = { + {.name = "help", + .data = N_("Get a vm's memory dirty rate") + }, + {.name = "desc", + .data = N_("Get memory dirty rate of a domain in order to decide" + " whether it's proper to be migrated out or not.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_domdirtyrate[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "seconds", + .type = VSH_OT_INT, + .help = N_("calculate memory dirty rate within specified seconds," + " the supported value range is [1, 60], default to 1s.") + }, + {.name = "calculate", + .type = VSH_OT_BOOL, + .help = N_("calculate dirty rate only, can be used together with --query," + " either or both is expected, otherwise would default to both.") + }, + {.name = "query", + .type = VSH_OT_BOOL, + .help = N_("query dirty rate only, can be used together with --calculate," + " either or both is expected, otherwise would default to both.") + }, + {.name = NULL} +}; + +static bool +cmdDomDirtyRateInfo(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + virDomainDirtyRateInfo info; + const char *status = NULL; + unsigned int flags = VIR_DOMAIN_DIRTYRATE_DEFAULT; + int sec; + int rc; + vshTablePtr table = NULL; + g_autofree char *startTimeStr = NULL; + g_autofree char *calcTimeStr = NULL; + g_autofree char *dirtyRateStr = NULL; + bool ret = false; + bool calc = vshCommandOptBool(cmd, "calculate"); + bool query = vshCommandOptBool(cmd, "query"); + + if (calc) + flags |= VIR_DOMAIN_DIRTYRATE_CALC; + + if (query) + flags |= VIR_DOMAIN_DIRTYRATE_QUERY; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + rc = vshCommandOptInt(ctl, cmd, "seconds", &sec); + if (rc < 0) + goto cleanup; + + /* if no inputted seconds, default to 1s */ + if (!rc) + sec = 1; + + if (virDomainGetDirtyRateInfo(dom, &info, sec, flags) < 0) + goto cleanup; + + if (flags == VIR_DOMAIN_DIRTYRATE_DEFAULT || + flags & VIR_DOMAIN_DIRTYRATE_QUERY) { + table = vshTableNew(_("Item"), _("Value"), NULL); + if (!table) + goto cleanup; + + switch (info.status) { + case VIR_DOMAIN_DIRTYRATE_UNSTARTED: + status = _("unstarted"); + break; + case VIR_DOMAIN_DIRTYRATE_MEASURING: + status = _("measuring"); + break; + case VIR_DOMAIN_DIRTYRATE_MEASURED: + status = _("measured"); + dirtyRateStr = g_strdup_printf("%lld MB/s", info.dirtyRate); + break; + default: + status = _("unknown"); + } + + if (vshTableRowAppend(table, _("Status:"), status, NULL) < 0) + goto cleanup; + + startTimeStr = g_strdup_printf("%lld", info.startTime); + if (vshTableRowAppend(table, _("Start time:"), startTimeStr, NULL) < 0) + goto cleanup; + + calcTimeStr = g_strdup_printf("%d s", info.calcTime); + if (vshTableRowAppend(table, _("Calculate time:"), calcTimeStr, NULL) < 0) + goto cleanup; + + if (info.status == VIR_DOMAIN_DIRTYRATE_MEASURED && + vshTableRowAppend(table, _("Dirty rate:"), dirtyRateStr, NULL) < 0) + goto cleanup; + + vshTablePrintToStdout(table, ctl); + } else { + vshPrint(ctl, _("Memory dirty rate is calculating, use --query option to display results.\n")); + } + + ret = true; + + cleanup: + vshTableFree(table); + virshDomainFree(dom); + return ret; +} + + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -15055,5 +15176,11 @@ const vshCmdDef domManagementCmds[] = { .info = info_guestinfo, .flags = 0 }, + {.name = "domdirtyrate", + .handler = cmdDomDirtyRateInfo, + .opts = opts_domdirtyrate, + .info = info_domdirtyrate, + .flags = 0 + }, {.name = NULL} }; -- 2.23.0
participants (3)
-
Daniel Henrique Barboza -
Hao Wang -
Michal Privoznik