On 09.12.2016 17:36, Maxim Nestratov wrote:

This is necessary for to show CTs created out of libvirt correctly.

Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index d5688e1..9976e4c 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -562,18 +562,15 @@ prlsdkGetDiskId(PRL_HANDLE disk, int *bus, char **dst) *bus = VIR_DOMAIN_DISK_BUS_IDE; *dst = virIndexToDiskName(pos, "hd"); break; - case PMS_SCSI_DEVICE: - *bus = VIR_DOMAIN_DISK_BUS_SCSI; - *dst = virIndexToDiskName(pos, "sd"); - break; case PMS_SATA_DEVICE: *bus = VIR_DOMAIN_DISK_BUS_SATA; *dst = virIndexToDiskName(pos, "sd"); break; + case PMS_SCSI_DEVICE: default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown disk bus: %X"), ifType); - return -1; + *bus = VIR_DOMAIN_DISK_BUS_SCSI; + *dst = virIndexToDiskName(pos, "sd"); + break; }

if (NULL == *dst)

So this is special case only for containers and only for special 'undefined' value of bus type (we don't set/report bus type if create containers with help of virtuozzo tools). I would code the condition exactly as it is. This patch can turn into scsi unexpected cases. Nikolay

20-Dec-16 13:50, Nikolay Shirokovskiy пишет:

On 09.12.2016 17:36, Maxim Nestratov wrote:

...

Before, boot devices information for CTs was always empty and we didn't indicate that containers always boot from disk.

Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 9976e4c..a6236d0 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1634,7 +1634,7 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, }

static int -prlsdkConvertBootOrder(PRL_HANDLE sdkdom, virDomainDefPtr def) +prlsdkConvertBootOrderVm(PRL_HANDLE sdkdom, virDomainDefPtr def) { int ret = -1; PRL_RESULT pret; @@ -1792,9 +1792,14 @@ prlsdkLoadDomain(vzDriverPtr driver, if (prlsdkAddDomainHardware(driver, sdkdom, def) < 0) goto error;

- /* depends on prlsdkAddDomainHardware */ - if (prlsdkConvertBootOrder(sdkdom, def) < 0) - goto error; + if (IS_CT(def)) { + def->os.nBootDevs = 1; + def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK; + } else { + /* depends on prlsdkAddDomainHardware */ + if (prlsdkConvertBootOrderVm(sdkdom, def) < 0) + goto error; + }

pret = PrlVmCfg_GetEnvId(sdkdom, &envId); prlsdkCheckRetGoto(pret, error);

I think we'd better make this code symmetrical to prlsdkSetBootOrderCt, that is report boot order only if there is no filesystem mounted to root. Also we need to fix prlsdkCheckUnsupportedParams to allow boot order if there is no root filesystems but still forbid boot from network etc.

Nikolay

Ok. Makes sense to move this change after #4 and rely on def->fss previously constructed

On 09.12.2016 17:36, Maxim Nestratov wrote:

Implicit devices like controllers are confusing for CTs as they are faked. Other devices like video we require to be specified explicitly.

I would drop last sentence. prlsdkApplyVideoParams shows that we ignore video params for containers. Let's reword commit message as we discussed offline - virDomainDefAddImplicitDevices is just never intended for containers.

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 a6236d0..1030a1b 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1816,7 +1816,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

21-Dec-16 10:25, Nikolay Shirokovskiy пишет:

On 09.12.2016 17:36, 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, 86 insertions(+), 10 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 1030a1b..9b8b48e 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 @@ -754,7 +757,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; @@ -1551,6 +1555,75 @@ 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; + PRL_UINT32 sdkIndex, bootIndex; + PRL_UINT32 pos, targetpos; + PRL_HANDLE dev = PRL_INVALID_HANDLE; + size_t i; + + pret = PrlVmDev_GetStackIndex(sdktargetdev, &targetpos); + prlsdkCheckRetExit(pret, -1); + + pret = PrlVmCfg_GetBootDevCount(sdkdom, &bootNum); + prlsdkCheckRetExit(pret, -1); + + if (bootNum > VIR_DOMAIN_MAX_BOOT_DEVS) { + bootNum = VIR_DOMAIN_MAX_BOOT_DEVS; + VIR_WARN("Too many boot devices"); + } This is not nesessary. We have VIR_DOMAIN_MAX_BOOT_DEVS limit only if we want to present boot order in domain definition.

Ok. Seems to be true.

...

+ + 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_GetSequenceIndex(bootDev, &bootIndex); + prlsdkCheckRetGoto(pret, cleanup); bootIndex is not used

Yeah, my first intention was to return it, then I reconsidered it but left the variable.

...

+ + 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; + + pret = PrlVmDev_GetStackIndex(dev, &pos); + prlsdkCheckRetExit(pret, -1); I would use PrlVmDev_GetType and PrlVmDev_GetIndex for given device and compare obtained values against sdkType and sdkIndex. It is more straight forward.

Ok. I'll try to do it this way.

...

+ + PrlHandle_Free(bootDev); + bootDev = PRL_INVALID_HANDLE; + + if (pos == targetpos) { + 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) @@ -3741,23 +3814,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 */ let's move this comment to 'if (!rootfs) {'

Ok

...

for (i = 0; i < def->nfss; i++) { + + pret = prlsdkAddDeviceToBootList(sdkdom, i, PDE_HARD_DISK, i + 1); + prlsdkCheckRetExit(pret, -1);

As this relies on the fact that we add disks for filesystems first and then disks for disks let's add comment to prlsdkDoApplyConfig that this order is now significant.

Makes sense

...

+ 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) { + pret = PrlVmCfg_GetHardDisk(sdkdom, 0, &hdd); + prlsdkCheckRetExit(pret, -1); If we mix filesystems and disks then first harddisk will represent filesystem. So we should use def->nfss instead of 0. It is a matter of a distinct patch I guess, nevertheless let's fix this place too.

Nikolay

Agree. See in the next version

...

- PrlVmDevHd_SetMountPoint(hdd, "/"); - prlsdkCheckRetGoto(pret, cleanup); + PrlVmDevHd_SetMountPoint(hdd, "/"); + prlsdkCheckRetGoto(pret, cleanup); + }

ret = 0;

On 09.12.2016 17:36, 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(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index a634ed2..3a22b07 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 9b8b48e..dff531f 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4360,7 +4360,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; @@ -4379,7 +4380,7 @@ prlsdkGetBlockStats(PRL_HANDLE sdkstats, idx = address->unit; break; case VIR_DOMAIN_DISK_BUS_SCSI: - prefix = "scsi"; + prefix = isCt ? "hdd" : "scsi";

I would calculate prefix for containers outside the scope of this switch. Even if it means bigger diff, the switch deserves its own function anyway. Otherwise ACK.

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