Re: [libvirt] [PATCHv2 1/2] lxc: add support for docker-json Memory and VCPU conversion

On 06/27/2017 01:02 PM, Venkat Datta N H wrote: Should the drvlxc.html.in page be modified too? http://libvirt.org/drvlxc.html To include something that indicates docker config files being read and what fields are taken/used.. IOW how things are mapped. Stylistically, these should be: static int virLXCDockerParseCpu(virDomainDefPtr dom, virDomainXMLOptionPtr xmlopt, virJSONValuePtr prop) note the multiple lines for function and how the arguments are aligned. Similar point in every definition here. And yes, I know/see that lxc_driver.c doesn't follow that model - this is the newer style we like to see in general. s/ !=/</ e.g. ) < 0)" Why CpuShares and not CpuCount? What about CpusetCpus? > + > + if (virDomainDefSetVcpusMax(dom, vcpus, xmlopt) < 0) > + > + if (virDomainDefSetVcpus(dom, vcpus) < 0) This would seem to be more related to CpusetCpus from my quick read. Two blank lines between functions s/!=/</ > + > + virDomainDefSetMemoryTotal(dom, mem / 1024); > + dom->mem.cur_balloon = mem / 1024; > + Does ShmSize have any effect? (I'm not sure what it is, so I ask) Why go w/ UNUSED? For comparison, the lxcParseConfigString will pass @caps to virDomainDefPostParse after it builds it's @vmdef - there's some checking that goes in that function that you'd be possibly missing. Typically we keep definitions together without intervening lines of code. Curious why no virUUIDGenerate ? Domains typically have two keys UUID and Name - although I haven't dug through the details of the LXC driver to see what would happen if you had a non docker type container with a conflicting name, but no UUID supplied. This code doesn't even set a name? So no UUID and no Name, probably means no domain to be defined. You're setting some default and total, then possibly changing? The != NULL is really not necessary Long line - 80 cols please. Long line again. Setting the os.type, but there's nothing to run? [1] e.g. here for virDomainDefPostParse @json_obj is leaked (need a virJSONValueFree) here. Similarly more recent style has been just to copy the .c function and add the ; ... makes things easier. This comment doesn't matches the #ifndef above This ends up being a somewhat long line... The to stay within 80 char columns. You could also make the argument order the same as lxcParseConfigString - although it's not a requirement. Seems to me to be a lot more options in here than those read/handled in virLXCDockerParseJSONConfig (which only reads "HostConfig", "Memory", and "CpuShares")... I'm not as familiar with LXC as QEMU, but it would seem at the very least "Runtime" would equate to something... And certainly Path/Args would be useful. The only thing used is "Memory"? What is ShmSize? for? What about MemorySwap or MemoryReservation - is there any relationship with existing DOMAIN_MEMORY_* variables? As noted above CpuCount to me would be related to count/max of vcpus for the container, while CpuShares would be a scheduler type parameter. Perhaps even "CpusetCpus" would be even more in line with which vcpus would be used (e.g. a 4 vcpu system with only 0,1 active). The CpuPeriod and CpuQuota would seem to relate to VIR_DOMAIN_SCHEDULER* params.... Perhaps going through lxc_driver API's would be beneficial to see how many relate to existing/parsed fields. If I cut this XML and try to : virsh -c lxc:/// file.xml I get: error: Failed to define domain from docker.xml error: missing name information If I edit the XML and add a name, then try again I get: error: Failed to define domain from docker.xml error: XML error: init binary must be specified This one should have multi lines... > +{ > + /* Remove UUID randomness */ > + if (virUUIDParse("c7a5fdbd-edaf-9455-926a-d65c16db1809", vmdef->uuid) < 0) > + return 0; > +} > + Two blank lines. @expectError isn't really used and to me seems 'backwards' - you're expecting an error? Is there a way to test DO_TEST("less_simple", false) John