[libvirt] [PATCH v2 0/5] vz: fix some CT disk representation cases and its statistics

v1-v2 changes - comments addressed Maxim Nestratov (5): vz: report "scsi" bus for disks when nothing was set explixitly vz: don't add implicit devices for CTs vz: report disks either as disks or filesystems depending on original xml vz: set boot from disk for CT only when there is no root filesystem vz: get disks statistics for CTs src/vz/vz_driver.c | 10 ++++- src/vz/vz_sdk.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++------- src/vz/vz_sdk.h | 2 +- 3 files changed, 121 insertions(+), 20 deletions(-) -- 2.4.11

This is necessary to show CTs created out of libvirt correctly. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 64748fa..2c8ce03 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -563,6 +563,7 @@ prlsdkGetDiskId(PRL_HANDLE disk, int *bus, char **dst) *dst = virIndexToDiskName(pos, "hd"); break; case PMS_SCSI_DEVICE: + case PMS_UNKNOWN_DEVICE: *bus = VIR_DOMAIN_DISK_BUS_SCSI; *dst = virIndexToDiskName(pos, "sd"); break; -- 2.4.11

On Wed, Dec 21, 2016 at 10:38:48PM +0300, Maxim Nestratov wrote:
This is necessary to show CTs created out of libvirt correctly.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 64748fa..2c8ce03 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -563,6 +563,7 @@ prlsdkGetDiskId(PRL_HANDLE disk, int *bus, char **dst) *dst = virIndexToDiskName(pos, "hd"); break; case PMS_SCSI_DEVICE: + case PMS_UNKNOWN_DEVICE: *bus = VIR_DOMAIN_DISK_BUS_SCSI; *dst = virIndexToDiskName(pos, "sd"); break;
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

Implicit devices like controllers are confusing for CTs and function virDomainDefAddImplicitDevices never intended to be called for CTs. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 2c8ce03..8d75399 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1815,7 +1815,7 @@ prlsdkLoadDomain(vzDriverPtr driver, if (prlsdkGetDomainState(dom, sdkdom, &domainState) < 0) goto error; - if (virDomainDefAddImplicitDevices(def) < 0) + if (!IS_CT(def) && virDomainDefAddImplicitDevices(def) < 0) goto error; if (def->ngraphics > 0) { -- 2.4.11

On Wed, Dec 21, 2016 at 10:38:49PM +0300, Maxim Nestratov wrote:
Implicit devices like controllers are confusing for CTs and function virDomainDefAddImplicitDevices never intended to be called for CTs.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 2c8ce03..8d75399 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1815,7 +1815,7 @@ prlsdkLoadDomain(vzDriverPtr driver, if (prlsdkGetDomainState(dom, sdkdom, &domainState) < 0) goto error;
- if (virDomainDefAddImplicitDevices(def) < 0) + if (!IS_CT(def) && virDomainDefAddImplicitDevices(def) < 0) goto error;
if (def->ngraphics > 0) {
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

Virtuozzo SDK interface doesn't differ filesystems from disks and sees them as disks. Before, we always mistakenly presented disks based on files as filesystems, which is not completely correct. Now we are going to show either disks or filesystems depending on a hint, which uses boot device section of VZ config. Though this information doesn't change booting order of a CT, it is used by vz libvirt interface as a hint for libvirt representation of disks. Since now, if we have filesystems in input xml, then we add them to VZ booting devices list and rely on this information to show corresponding libvirt xml. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 11 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 8d75399..54a21a3 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -50,6 +50,9 @@ static PRL_HANDLE prlsdkFindNetByMAC(PRL_HANDLE sdkdom, virMacAddrPtr mac); static PRL_HANDLE prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk); +static bool +prlsdkInBootList(PRL_HANDLE sdkdom, + PRL_HANDLE sdktargetdev); /* * Log error description @@ -758,7 +761,8 @@ prlsdkAddDomainHardDisksInfo(vzDriverPtr driver, PRL_HANDLE sdkdom, virDomainDef pret = PrlVmDev_GetEmulatedType(hdd, &emulatedType); prlsdkCheckRetGoto(pret, error); - if (PDT_USE_REAL_DEVICE != emulatedType && IS_CT(def)) { + if (IS_CT(def) && + prlsdkInBootList(sdkdom, hdd)) { if (!(fs = virDomainFSDefNew())) goto error; @@ -1555,6 +1559,69 @@ virFindDiskBootIndex(virDomainDefPtr def, virDomainDiskDevice type, int index) return NULL; } +static bool +prlsdkInBootList(PRL_HANDLE sdkdom, + PRL_HANDLE sdktargetdev) +{ + bool ret = false; + PRL_RESULT pret; + PRL_UINT32 bootNum; + PRL_HANDLE bootDev = PRL_INVALID_HANDLE; + PRL_BOOL inUse; + PRL_DEVICE_TYPE sdkType, targetType; + PRL_UINT32 sdkIndex, targetIndex; + PRL_HANDLE dev = PRL_INVALID_HANDLE; + size_t i; + + pret = PrlVmDev_GetType(sdktargetdev, &targetType); + prlsdkCheckRetExit(pret, -1); + + pret = PrlVmDev_GetIndex(sdktargetdev, &targetIndex); + prlsdkCheckRetExit(pret, -1); + + pret = PrlVmCfg_GetBootDevCount(sdkdom, &bootNum); + prlsdkCheckRetExit(pret, -1); + + for (i = 0; i < bootNum; ++i) { + pret = PrlVmCfg_GetBootDev(sdkdom, i, &bootDev); + prlsdkCheckRetGoto(pret, cleanup); + + pret = PrlBootDev_IsInUse(bootDev, &inUse); + prlsdkCheckRetGoto(pret, cleanup); + + if (!inUse) { + PrlHandle_Free(bootDev); + bootDev = PRL_INVALID_HANDLE; + continue; + } + + pret = PrlBootDev_GetType(bootDev, &sdkType); + prlsdkCheckRetGoto(pret, cleanup); + + pret = PrlBootDev_GetIndex(bootDev, &sdkIndex); + prlsdkCheckRetGoto(pret, cleanup); + + dev = prlsdkGetDevByDevIndex(sdkdom, sdkType, sdkIndex); + if (dev == PRL_INVALID_HANDLE) + goto cleanup; + + PrlHandle_Free(dev); + dev = PRL_INVALID_HANDLE; + + PrlHandle_Free(bootDev); + bootDev = PRL_INVALID_HANDLE; + + if (sdkIndex == targetIndex && sdkType == targetType) { + ret = true; + break; + } + } + + cleanup: + PrlHandle_Free(dev); + PrlHandle_Free(bootDev); + return ret; +} static int prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, virDomainDefPtr def, int bootIndex) @@ -3740,23 +3807,26 @@ prlsdkSetBootOrderCt(PRL_HANDLE sdkdom, virDomainDefPtr def) size_t i; PRL_HANDLE hdd = PRL_INVALID_HANDLE; PRL_RESULT pret; + bool rootfs = false; int ret = -1; - /* if we have root mounted we don't need to explicitly set boot order */ for (i = 0; i < def->nfss; i++) { + + pret = prlsdkAddDeviceToBootList(sdkdom, i, PDE_HARD_DISK, i + 1); + prlsdkCheckRetExit(pret, -1); + if (STREQ(def->fss[i]->dst, "/")) - return 0; + rootfs = true; } - /* else set first hard disk as boot device */ - pret = prlsdkAddDeviceToBootList(sdkdom, 0, PDE_HARD_DISK, 0); - prlsdkCheckRetExit(pret, -1); - - pret = PrlVmCfg_GetHardDisk(sdkdom, 0, &hdd); - prlsdkCheckRetExit(pret, -1); + if (!rootfs) { + /* if we have root mounted we don't need to explicitly set boot order */ + pret = PrlVmCfg_GetHardDisk(sdkdom, def->nfss, &hdd); + prlsdkCheckRetExit(pret, -1); - PrlVmDevHd_SetMountPoint(hdd, "/"); - prlsdkCheckRetGoto(pret, cleanup); + PrlVmDevHd_SetMountPoint(hdd, "/"); + prlsdkCheckRetGoto(pret, cleanup); + } ret = 0; @@ -3926,11 +3996,15 @@ prlsdkDoApplyConfig(vzDriverPtr driver, goto error; } + /* It is important that we add filesystems first and then disks as we rely + * on this information in prlsdkSetBootOrderCt */ for (i = 0; i < def->nfss; i++) { if (prlsdkAddFS(sdkdom, def->fss[i]) < 0) goto error; } + /* filesystems first, disks go after them as we rely on this order in + * prlsdkSetBootOrderCt */ for (i = 0; i < def->ndisks; i++) { if (prlsdkConfigureDisk(driver, sdkdom, def->disks[i], true) < 0) -- 2.4.11

On Wed, Dec 21, 2016 at 10:38:50PM +0300, Maxim Nestratov wrote:
Virtuozzo SDK interface doesn't differ filesystems from disks and sees them as disks. Before, we always mistakenly presented disks based on files as filesystems, which is not completely correct. Now we are going to show either disks or filesystems depending on a hint, which uses boot device section of VZ config. Though this information doesn't change booting order of a CT, it is used by vz libvirt interface as a hint for libvirt representation of disks. Since now, if we have filesystems in input xml, then we add them to VZ booting devices list and rely on this information to show corresponding libvirt xml.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 11 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

Before, boot devices information for CTs was always empty and we didn't indicate that containers can boot from disk. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 54a21a3..cc077f5 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1704,8 +1704,23 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, return ret; } +static void +prlsdkConvertBootOrderCt(virDomainDefPtr def) +{ + size_t i; + for (i = 0; i < def->nfss; i++) { + + if (STREQ(def->fss[i]->dst, "/")) { + def->os.nBootDevs = 0; + return; + } + } + def->os.nBootDevs = 1; + def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK; +} + static int -prlsdkConvertBootOrder(PRL_HANDLE sdkdom, virDomainDefPtr def) +prlsdkConvertBootOrderVm(PRL_HANDLE sdkdom, virDomainDefPtr def) { int ret = -1; PRL_RESULT pret; @@ -1864,8 +1879,12 @@ prlsdkLoadDomain(vzDriverPtr driver, goto error; /* depends on prlsdkAddDomainHardware */ - if (prlsdkConvertBootOrder(sdkdom, def) < 0) - goto error; + if (IS_CT(def)) { + prlsdkConvertBootOrderCt(def); + } else { + if (prlsdkConvertBootOrderVm(sdkdom, def) < 0) + goto error; + } pret = PrlVmCfg_GetEnvId(sdkdom, &envId); prlsdkCheckRetGoto(pret, error); -- 2.4.11

On Wed, Dec 21, 2016 at 10:38:51PM +0300, Maxim Nestratov wrote:
Before, boot devices information for CTs was always empty and we didn't indicate that containers can boot from disk.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

A CT disk statistics is reported with prefix "hdd" and we should use it to extract data. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_driver.c | 10 ++++++++-- src/vz/vz_sdk.c | 5 +++-- src/vz/vz_sdk.h | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index af9ef7f..93d8552 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1743,7 +1743,10 @@ vzDomainBlockStatsImpl(virDomainObjPtr dom, virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); return -1; } - if (prlsdkGetBlockStats(privdom->stats, dom->def->disks[idx], stats) < 0) + if (prlsdkGetBlockStats(privdom->stats, + dom->def->disks[idx], + stats, + IS_CT(dom->def)) < 0) return -1; } else { virDomainBlockStatsStruct s; @@ -1756,7 +1759,10 @@ vzDomainBlockStatsImpl(virDomainObjPtr dom, #undef PARALLELS_ZERO_STATS for (i = 0; i < dom->def->ndisks; i++) { - if (prlsdkGetBlockStats(privdom->stats, dom->def->disks[i], &s) < 0) + if (prlsdkGetBlockStats(privdom->stats, + dom->def->disks[i], + &s, + IS_CT(dom->def)) < 0) return -1; #define PARALLELS_SUM_STATS(VAR, TYPE, NAME) \ diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index cc077f5..630a18f 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4376,7 +4376,8 @@ prlsdkExtractStatsParam(PRL_HANDLE sdkstats, const char *name, long long *val) int prlsdkGetBlockStats(PRL_HANDLE sdkstats, virDomainDiskDefPtr disk, - virDomainBlockStatsPtr stats) + virDomainBlockStatsPtr stats, + bool isCt) { virDomainDeviceDriveAddressPtr address; int idx; @@ -4395,7 +4396,7 @@ prlsdkGetBlockStats(PRL_HANDLE sdkstats, idx = address->unit; break; case VIR_DOMAIN_DISK_BUS_SCSI: - prefix = "scsi"; + prefix = isCt ? "hdd" : "scsi"; idx = address->unit; break; default: diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h index ef789ab..e4e46dc 100644 --- a/src/vz/vz_sdk.h +++ b/src/vz/vz_sdk.h @@ -63,7 +63,7 @@ prlsdkDetachDevice(vzDriverPtr driver, virDomainObjPtr dom, virDomainDeviceDefPt int prlsdkUpdateDevice(vzDriverPtr driver, virDomainObjPtr dom, virDomainDeviceDefPtr dev); int -prlsdkGetBlockStats(PRL_HANDLE sdkstats, virDomainDiskDefPtr disk, virDomainBlockStatsPtr stats); +prlsdkGetBlockStats(PRL_HANDLE sdkstats, virDomainDiskDefPtr disk, virDomainBlockStatsPtr stats, bool isCt); int prlsdkGetNetStats(PRL_HANDLE sdkstas, PRL_HANDLE sdkdom, const char *path, virDomainInterfaceStatsPtr stats); int -- 2.4.11

On Wed, Dec 21, 2016 at 10:38:52PM +0300, Maxim Nestratov wrote:
A CT disk statistics is reported with prefix "hdd" and we should use it to extract data.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_driver.c | 10 ++++++++-- src/vz/vz_sdk.c | 5 +++-- src/vz/vz_sdk.h | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

22-Dec-16 12:13, Daniel P. Berrange пишет:
On Wed, Dec 21, 2016 at 10:38:52PM +0300, Maxim Nestratov wrote:
A CT disk statistics is reported with prefix "hdd" and we should use it to extract data.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_driver.c | 10 ++++++++-- src/vz/vz_sdk.c | 5 +++-- src/vz/vz_sdk.h | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) ACK
Regards, Daniel Though you ACKed this version I changed it a bit addressing Nickolay's comments on this patch and pushed modified version. The new chunk is as follows:
address = &disk->info.addr.drive; - switch (disk->bus) { - case VIR_DOMAIN_DISK_BUS_IDE: - prefix = "ide"; - idx = address->bus * 2 + address->unit; - break; - case VIR_DOMAIN_DISK_BUS_SATA: - prefix = "sata"; - idx = address->unit; - break; - case VIR_DOMAIN_DISK_BUS_SCSI: - prefix = "scsi"; + + if (isCt) { + prefix = "hdd"; idx = address->unit; - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown disk bus: %X"), disk->bus); - goto cleanup; + } else { + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + prefix = "ide"; + idx = address->bus * 2 + address->unit; + break; + case VIR_DOMAIN_DISK_BUS_SATA: + prefix = "sata"; + idx = address->unit; + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + prefix = "scsi"; + idx = address->unit; + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown disk bus: %X"), disk->bus); + goto cleanup; + } } Thus, although we report CT's disk bus as scsi in most cases, we don't really care what exact bus is and explicitly set prefix to "hdd" to extract statistics. With this change I pushed the series. Maxim
participants (2)
-
Daniel P. Berrange
-
Maxim Nestratov