[libvirt] [PATCHv2 0/2] Adding support for LXC Docker-json configuration
Hi All, I added two patches to support conversion of Docker JSON configuration into LXC Native Domain XML. Venkat Datta N H (2): lxc: add support for docker-json Memory and VCPU conversion lxc: add support for docker-json command conversion po/POTFILES.in | 1 + src/Makefile.am | 1 + src/lxc/lxc_docker.c | 181 +++++++++++++++++++++ src/lxc/lxc_docker.h | 32 ++++ src/lxc/lxc_driver.c | 13 +- src/lxc/lxc_native.h | 1 + tests/Makefile.am | 8 +- .../lxcdocker2xmldata-command.json | 47 ++++++ .../lxcdocker2xmldata-command.xml | 17 ++ .../lxcdocker2xmldata-simple.json | 36 ++++ .../lxcdocker2xmldata/lxcdocker2xmldata-simple.xml | 15 ++ tests/lxcdocker2xmltest.c | 150 +++++++++++++++++ 12 files changed, 496 insertions(+), 6 deletions(-) create mode 100644 src/lxc/lxc_docker.c create mode 100644 src/lxc/lxc_docker.h create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-command.json create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-command.xml create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml create mode 100644 tests/lxcdocker2xmltest.c -- 2.7.4
Docker Memory and VCPU configuration is converted to fit for LXC container XML configuration --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/lxc/lxc_docker.c | 116 ++++++++++++++++ src/lxc/lxc_docker.h | 32 +++++ src/lxc/lxc_driver.c | 13 +- src/lxc/lxc_native.h | 1 + tests/Makefile.am | 8 +- .../lxcdocker2xmldata-simple.json | 36 +++++ .../lxcdocker2xmldata/lxcdocker2xmldata-simple.xml | 15 +++ tests/lxcdocker2xmltest.c | 149 +++++++++++++++++++++ 10 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 src/lxc/lxc_docker.c create mode 100644 src/lxc/lxc_docker.h create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml create mode 100644 tests/lxcdocker2xmltest.c diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f..421b32e 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -111,6 +111,7 @@ src/lxc/lxc_driver.c src/lxc/lxc_fuse.c src/lxc/lxc_hostdev.c src/lxc/lxc_native.c +src/lxc/lxc_docker.c src/lxc/lxc_process.c src/network/bridge_driver.c src/network/bridge_driver_linux.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc..1341e5a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -839,6 +839,7 @@ LXC_DRIVER_SOURCES = \ lxc/lxc_process.c lxc/lxc_process.h \ lxc/lxc_fuse.c lxc/lxc_fuse.h \ lxc/lxc_native.c lxc/lxc_native.h \ + lxc/lxc_docker.c lxc/lxc_docker.h \ lxc/lxc_driver.c lxc/lxc_driver.h LXC_CONTROLLER_SOURCES = \ diff --git a/src/lxc/lxc_docker.c b/src/lxc/lxc_docker.c new file mode 100644 index 0000000..dbb2a81 --- /dev/null +++ b/src/lxc/lxc_docker.c @@ -0,0 +1,116 @@ +/* + * lxc_docker.c: LXC native docker configuration import + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "util/viralloc.h" +#include "util/virfile.h" +#include "util/virstring.h" +#include "util/virconf.h" +#include "util/virjson.h" +#include "util/virutil.h" +#include "virerror.h" +#include "virlog.h" +#include "conf/domain_conf.h" +#include "lxc_docker.h" +#include "secret_conf.h" + +#define VIR_FROM_THIS VIR_FROM_LXC + +VIR_LOG_INIT("lxc.lxc_docker"); + +static int virLXCDockerParseCpu(virDomainDefPtr dom, + virDomainXMLOptionPtr xmlopt, + virJSONValuePtr prop) +{ + int vcpus; + + if (virJSONValueObjectGetNumberInt(prop, "CpuShares", &vcpus) != 0) + return -1; + + if (virDomainDefSetVcpusMax(dom, vcpus, xmlopt) < 0) + return -1; + + if (virDomainDefSetVcpus(dom, vcpus) < 0) + return -1; + + return 0; +} + +static int virLXCDockerParseMem(virDomainDefPtr dom, + virJSONValuePtr prop) +{ + unsigned long long mem; + + if (virJSONValueObjectGetNumberUlong(prop, "Memory", &mem) != 0) + return -1; + + virDomainDefSetMemoryTotal(dom, mem / 1024); + dom->mem.cur_balloon = mem / 1024; + + return 0; +} + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps ATTRIBUTE_UNUSED, + virDomainXMLOptionPtr xmlopt, + const char *config) +{ + virJSONValuePtr json_obj; + virJSONValuePtr host_config; + + if (!(json_obj = virJSONValueFromString(config))) + return NULL; + + virDomainDefPtr def; + + if (!(def = virDomainDefNew())) + goto error; + + def->id = -1; + def->mem.cur_balloon = 64*1024; + virDomainDefSetMemoryTotal(def, def->mem.cur_balloon); + + if ((host_config = virJSONValueObjectGetObject(json_obj, "HostConfig")) != NULL) { + if (virLXCDockerParseCpu(def, xmlopt, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse VCpu")); + goto error; + } + + if (virLXCDockerParseMem(def, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse Memory")); + goto error; + } + } + + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; + def->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY; + def->onPoweroff = VIR_DOMAIN_LIFECYCLE_DESTROY; + def->virtType = VIR_DOMAIN_VIRT_LXC; + def->os.type = VIR_DOMAIN_OSTYPE_EXE; + + return def; + + error: + virDomainDefFree(def); + return NULL; +} diff --git a/src/lxc/lxc_docker.h b/src/lxc/lxc_docker.h new file mode 100644 index 0000000..f081e07 --- /dev/null +++ b/src/lxc/lxc_docker.h @@ -0,0 +1,32 @@ +/* + * lxc_docker.h: Header file for LXC native docker configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#ifndef __LXC_DOCKER_H__ +# define __LXC_DOCKER_H__ + +# include "domain_conf.h" + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + const char *config); + +#endif /* __LXC_NATIVE_DOCKER_H__ */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 22c8b58..711bb16 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -53,6 +53,7 @@ #include "lxc_driver.h" #include "lxc_native.h" #include "lxc_process.h" +#include "lxc_docker.h" #include "viralloc.h" #include "virnetdevbridge.h" #include "virnetdevveth.h" @@ -1062,15 +1063,17 @@ static char *lxcConnectDomainXMLFromNative(virConnectPtr conn, if (virConnectDomainXMLFromNativeEnsureACL(conn) < 0) goto cleanup; - if (STRNEQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (STREQ(nativeFormat, DOCKER_CONFIG_FORMAT)) { + if (!(def = virLXCDockerParseJSONConfig(caps, driver->xmlopt, nativeConfig))) + goto cleanup; + } else if (STREQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) + goto cleanup; + } else { virReportError(VIR_ERR_INVALID_ARG, _("unsupported config type %s"), nativeFormat); goto cleanup; } - - if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) - goto cleanup; - xml = virDomainDefFormat(def, caps, 0); cleanup: diff --git a/src/lxc/lxc_native.h b/src/lxc/lxc_native.h index 15fa0d5..88263ae 100644 --- a/src/lxc/lxc_native.h +++ b/src/lxc/lxc_native.h @@ -26,6 +26,7 @@ # include "domain_conf.h" # define LXC_CONFIG_FORMAT "lxc-tools" +# define DOCKER_CONFIG_FORMAT "docker" virDomainDefPtr lxcParseConfigString(const char *config, virCapsPtr caps, diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc..0948bfa 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -93,6 +93,7 @@ EXTRA_DIST = \ capabilityschemadata \ commanddata \ cputestdata \ + lxcdocker2xmldata \ domaincapsschemadata \ domainconfdata \ domainschemadata \ @@ -295,7 +296,7 @@ test_libraries += libqemumonitortestutils.la \ endif WITH_QEMU if WITH_LXC -test_programs += lxcxml2xmltest lxcconf2xmltest +test_programs += lxcxml2xmltest lxcconf2xmltest lxcdocker2xmltest endif WITH_LXC if WITH_OPENVZ @@ -693,6 +694,11 @@ lxcconf2xmltest_SOURCES = \ lxcconf2xmltest.c testutilslxc.c testutilslxc.h \ testutils.c testutils.h lxcconf2xmltest_LDADD = $(lxc_LDADDS) + +lxcdocker2xmltest_SOURCES = \ + lxcdocker2xmltest.c testutilslxc.c testutilslxc.h \ + testutils.c testutils.h +lxcdocker2xmltest_LDADD = $(lxc_LDADDS) else ! WITH_LXC EXTRA_DIST += lxcxml2xmltest.c testutilslxc.c testutilslxc.h endif ! WITH_LXC diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json new file mode 100644 index 0000000..63470be --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json @@ -0,0 +1,36 @@ +{ + "Id": "dbb1ae21dac15973d66e6c2b8516d270b32ca766e0cf7551d8b7973513e5f079", + "Created": "2017-05-25T18:55:17.922934825Z", + "Path": "/bin/bash", + "Args": [], + "HostConfig": { + "Binds": null, + "ContainerIDFile": "", + "LogConfig": { + "Type": "json-file", + "Config": {} + }, + "NetworkMode": "default", + "PortBindings": {}, + "ShmSize": 67108864, + "Runtime": "runc", + "Isolation": "", + "CpuShares": 2, + "Memory": 1073741824, + "CgroupParent": "", + "CpuPeriod": 0, + "CpuQuota": 0, + "CpusetCpus": "", + "CpusetMems": "", + "KernelMemory": 0, + "MemoryReservation": 0, + "MemorySwap": -1, + "MemorySwappiness": -1, + "PidsLimit": 0, + "Ulimits": null, + "CpuCount": 0, + "CpuPercent": 0, + "IOMaximumIOps": 0, + "IOMaximumBandwidth": 0 + } +} diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml new file mode 100644 index 0000000..8be1ace --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml @@ -0,0 +1,15 @@ +<domain type='lxc'> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>2</vcpu> + <os> + <type>exe</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + </devices> +</domain> diff --git a/tests/lxcdocker2xmltest.c b/tests/lxcdocker2xmltest.c new file mode 100644 index 0000000..ccac4c4 --- /dev/null +++ b/tests/lxcdocker2xmltest.c @@ -0,0 +1,149 @@ +/* + * lxcdocker2xmltest.c: Test LXC Docker Configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "testutils.h" + +#ifdef WITH_LXC + +# include "lxc/lxc_docker.h" +# include "lxc/lxc_conf.h" +# include "testutilslxc.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +static virCapsPtr caps; +static virDomainXMLOptionPtr xmlopt; + +static int testSanitizeDef(virDomainDefPtr vmdef) +{ + /* Remove UUID randomness */ + if (virUUIDParse("c7a5fdbd-edaf-9455-926a-d65c16db1809", vmdef->uuid) < 0) + return -1; + return 0; +} + +static int +testCompareXMLToConfigFiles(const char *xmlfile, + const char *configfile, + bool expectError) +{ + int ret = -1; + char *config = NULL; + char *actualxml = NULL; + virDomainDefPtr vmdef = NULL; + + if (virTestLoadFile(configfile, &config) < 0) + goto fail; + + vmdef = virLXCDockerParseJSONConfig(caps, xmlopt, config); + + if (vmdef && expectError) { + if (testSanitizeDef(vmdef) < 0) + goto fail; + + if (!(actualxml = virDomainDefFormat(vmdef, caps, 0))) + goto fail; + + if (virTestCompareToFile(actualxml, xmlfile) < 0) + goto fail; + } + + ret = 0; + + fail: + VIR_FREE(actualxml); + VIR_FREE(config); + virDomainDefFree(vmdef); + return ret; +} + +struct testInfo { + const char *name; + bool expectError; +}; + +static int +testCompareXMLToConfigHelper(const void *data) +{ + int result = -1; + const struct testInfo *info = data; + char *xml = NULL; + char *config = NULL; + + if (virAsprintf(&xml, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&config, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.json", + abs_srcdir, info->name) < 0) + goto cleanup; + + result = testCompareXMLToConfigFiles(xml, config, info->expectError); + + cleanup: + VIR_FREE(xml); + VIR_FREE(config); + return result; +} + +static int +mymain(void) +{ + int ret = EXIT_SUCCESS; + + if (!(caps = testLXCCapsInit())) + return EXIT_FAILURE; + + if (!(xmlopt = lxcDomainXMLConfInit())) { + virObjectUnref(caps); + return EXIT_FAILURE; + } + + +# define DO_TEST(name, expectError) \ + do { \ + const struct testInfo info = { name, expectError }; \ + if (virTestRun("DOCKER JSON-2-XML " name, \ + testCompareXMLToConfigHelper, \ + &info) < 0) \ + ret = EXIT_FAILURE; \ + } while (0) + + DO_TEST("simple", true); + + virObjectUnref(xmlopt); + virObjectUnref(caps); + + return ret; +} + +VIR_TEST_MAIN(mymain) + +#else + +int +main(void) +{ + return EXIT_AM_SKIP; +} + +#endif /* WITH_LXC */ -- 2.7.4
On 06/27/2017 01:02 PM, Venkat Datta N H wrote:
Docker Memory and VCPU configuration is converted to fit for LXC container XML configuration --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/lxc/lxc_docker.c | 116 ++++++++++++++++ src/lxc/lxc_docker.h | 32 +++++ src/lxc/lxc_driver.c | 13 +- src/lxc/lxc_native.h | 1 + tests/Makefile.am | 8 +- .../lxcdocker2xmldata-simple.json | 36 +++++ .../lxcdocker2xmldata/lxcdocker2xmldata-simple.xml | 15 +++ tests/lxcdocker2xmltest.c | 149 +++++++++++++++++++++ 10 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 src/lxc/lxc_docker.c create mode 100644 src/lxc/lxc_docker.h create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml create mode 100644 tests/lxcdocker2xmltest.c
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.
diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f..421b32e 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -111,6 +111,7 @@ src/lxc/lxc_driver.c src/lxc/lxc_fuse.c src/lxc/lxc_hostdev.c src/lxc/lxc_native.c +src/lxc/lxc_docker.c src/lxc/lxc_process.c src/network/bridge_driver.c src/network/bridge_driver_linux.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc..1341e5a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -839,6 +839,7 @@ LXC_DRIVER_SOURCES = \ lxc/lxc_process.c lxc/lxc_process.h \ lxc/lxc_fuse.c lxc/lxc_fuse.h \ lxc/lxc_native.c lxc/lxc_native.h \ + lxc/lxc_docker.c lxc/lxc_docker.h \ lxc/lxc_driver.c lxc/lxc_driver.h
LXC_CONTROLLER_SOURCES = \ diff --git a/src/lxc/lxc_docker.c b/src/lxc/lxc_docker.c new file mode 100644 index 0000000..dbb2a81 --- /dev/null +++ b/src/lxc/lxc_docker.c @@ -0,0 +1,116 @@ +/* + * lxc_docker.c: LXC native docker configuration import + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "util/viralloc.h" +#include "util/virfile.h" +#include "util/virstring.h" +#include "util/virconf.h" +#include "util/virjson.h" +#include "util/virutil.h" +#include "virerror.h" +#include "virlog.h" +#include "conf/domain_conf.h" +#include "lxc_docker.h" +#include "secret_conf.h" + +#define VIR_FROM_THIS VIR_FROM_LXC + +VIR_LOG_INIT("lxc.lxc_docker"); + +static int virLXCDockerParseCpu(virDomainDefPtr dom, + virDomainXMLOptionPtr xmlopt, + virJSONValuePtr prop)
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.
+{ + int vcpus; + + if (virJSONValueObjectGetNumberInt(prop, "CpuShares", &vcpus) != 0)
s/ !=/</ e.g. ) < 0)"
+ return -1;
Why CpuShares and not CpuCount? What about CpusetCpus?
+ + if (virDomainDefSetVcpusMax(dom, vcpus, xmlopt) < 0) + return -1; + + if (virDomainDefSetVcpus(dom, vcpus) < 0) + return -1;
This would seem to be more related to CpusetCpus from my quick read.
+ + return 0; +} +
Two blank lines between functions
+static int virLXCDockerParseMem(virDomainDefPtr dom, + virJSONValuePtr prop) +{ + unsigned long long mem; + + if (virJSONValueObjectGetNumberUlong(prop, "Memory", &mem) != 0)
s/!=/</
+ return -1; + + 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)
+ return 0; +} + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps ATTRIBUTE_UNUSED,
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.
+ virDomainXMLOptionPtr xmlopt, + const char *config) +{ + virJSONValuePtr json_obj; + virJSONValuePtr host_config; + + if (!(json_obj = virJSONValueFromString(config))) + return NULL; + + virDomainDefPtr def;
Typically we keep definitions together without intervening lines of code.
+ + if (!(def = virDomainDefNew())) + goto error;> +
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.
+ def->id = -1; + def->mem.cur_balloon = 64*1024; + virDomainDefSetMemoryTotal(def, def->mem.cur_balloon);
You're setting some default and total, then possibly changing?
+ + if ((host_config = virJSONValueObjectGetObject(json_obj, "HostConfig")) != NULL) {
The != NULL is really not necessary
+ if (virLXCDockerParseCpu(def, xmlopt, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse VCpu"));
Long line - 80 cols please.
+ goto error; + } + + if (virLXCDockerParseMem(def, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse Memory"));
Long line again.
+ goto error; + } + } + + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; + def->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY; + def->onPoweroff = VIR_DOMAIN_LIFECYCLE_DESTROY; + def->virtType = VIR_DOMAIN_VIRT_LXC; + def->os.type = VIR_DOMAIN_OSTYPE_EXE; + Setting the os.type, but there's nothing to run?
[1] e.g. here for virDomainDefPostParse
+ return def; + + error:
@json_obj is leaked (need a virJSONValueFree) here.
+ virDomainDefFree(def); + return NULL; +} diff --git a/src/lxc/lxc_docker.h b/src/lxc/lxc_docker.h new file mode 100644 index 0000000..f081e07 --- /dev/null +++ b/src/lxc/lxc_docker.h @@ -0,0 +1,32 @@ +/* + * lxc_docker.h: Header file for LXC native docker configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#ifndef __LXC_DOCKER_H__ +# define __LXC_DOCKER_H__ + +# include "domain_conf.h" + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + const char *config);
Similarly more recent style has been just to copy the .c function and add the ; ... makes things easier.
+ +#endif /* __LXC_NATIVE_DOCKER_H__ */
This comment doesn't matches the #ifndef above
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 22c8b58..711bb16 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -53,6 +53,7 @@ #include "lxc_driver.h" #include "lxc_native.h" #include "lxc_process.h" +#include "lxc_docker.h" #include "viralloc.h" #include "virnetdevbridge.h" #include "virnetdevveth.h" @@ -1062,15 +1063,17 @@ static char *lxcConnectDomainXMLFromNative(virConnectPtr conn, if (virConnectDomainXMLFromNativeEnsureACL(conn) < 0) goto cleanup;
- if (STRNEQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (STREQ(nativeFormat, DOCKER_CONFIG_FORMAT)) { + if (!(def = virLXCDockerParseJSONConfig(caps, driver->xmlopt, nativeConfig)))
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.
+ goto cleanup; + } else if (STREQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) + goto cleanup; + } else { virReportError(VIR_ERR_INVALID_ARG, _("unsupported config type %s"), nativeFormat); goto cleanup; } - - if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) - goto cleanup; - xml = virDomainDefFormat(def, caps, 0);
cleanup: diff --git a/src/lxc/lxc_native.h b/src/lxc/lxc_native.h index 15fa0d5..88263ae 100644 --- a/src/lxc/lxc_native.h +++ b/src/lxc/lxc_native.h @@ -26,6 +26,7 @@ # include "domain_conf.h"
# define LXC_CONFIG_FORMAT "lxc-tools" +# define DOCKER_CONFIG_FORMAT "docker"
virDomainDefPtr lxcParseConfigString(const char *config, virCapsPtr caps, diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc..0948bfa 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -93,6 +93,7 @@ EXTRA_DIST = \ capabilityschemadata \ commanddata \ cputestdata \ + lxcdocker2xmldata \ domaincapsschemadata \ domainconfdata \ domainschemadata \ @@ -295,7 +296,7 @@ test_libraries += libqemumonitortestutils.la \ endif WITH_QEMU
if WITH_LXC -test_programs += lxcxml2xmltest lxcconf2xmltest +test_programs += lxcxml2xmltest lxcconf2xmltest lxcdocker2xmltest endif WITH_LXC
if WITH_OPENVZ @@ -693,6 +694,11 @@ lxcconf2xmltest_SOURCES = \ lxcconf2xmltest.c testutilslxc.c testutilslxc.h \ testutils.c testutils.h lxcconf2xmltest_LDADD = $(lxc_LDADDS) + +lxcdocker2xmltest_SOURCES = \ + lxcdocker2xmltest.c testutilslxc.c testutilslxc.h \ + testutils.c testutils.h +lxcdocker2xmltest_LDADD = $(lxc_LDADDS) else ! WITH_LXC EXTRA_DIST += lxcxml2xmltest.c testutilslxc.c testutilslxc.h endif ! WITH_LXC diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json new file mode 100644 index 0000000..63470be --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json @@ -0,0 +1,36 @@ +{ + "Id": "dbb1ae21dac15973d66e6c2b8516d270b32ca766e0cf7551d8b7973513e5f079", + "Created": "2017-05-25T18:55:17.922934825Z", + "Path": "/bin/bash", + "Args": [], + "HostConfig": { + "Binds": null, + "ContainerIDFile": "", + "LogConfig": { + "Type": "json-file", + "Config": {} + }, + "NetworkMode": "default", + "PortBindings": {}, + "ShmSize": 67108864, + "Runtime": "runc", + "Isolation": "", + "CpuShares": 2, + "Memory": 1073741824, + "CgroupParent": "", + "CpuPeriod": 0, + "CpuQuota": 0, + "CpusetCpus": "", + "CpusetMems": "", + "KernelMemory": 0, + "MemoryReservation": 0, + "MemorySwap": -1, + "MemorySwappiness": -1, + "PidsLimit": 0, + "Ulimits": null, + "CpuCount": 0, + "CpuPercent": 0, + "IOMaximumIOps": 0, + "IOMaximumBandwidth": 0 + } +}
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.
diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml new file mode 100644 index 0000000..8be1ace --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml @@ -0,0 +1,15 @@ +<domain type='lxc'> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>2</vcpu> + <os> + <type>exe</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + </devices> +</domain>
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
diff --git a/tests/lxcdocker2xmltest.c b/tests/lxcdocker2xmltest.c new file mode 100644 index 0000000..ccac4c4 --- /dev/null +++ b/tests/lxcdocker2xmltest.c @@ -0,0 +1,149 @@ +/* + * lxcdocker2xmltest.c: Test LXC Docker Configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "testutils.h" + +#ifdef WITH_LXC + +# include "lxc/lxc_docker.h" +# include "lxc/lxc_conf.h" +# include "testutilslxc.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +static virCapsPtr caps; +static virDomainXMLOptionPtr xmlopt; + +static int testSanitizeDef(virDomainDefPtr vmdef)
This one should have multi lines...
+{ + /* Remove UUID randomness */ + if (virUUIDParse("c7a5fdbd-edaf-9455-926a-d65c16db1809", vmdef->uuid) < 0) + return -1; + return 0; +} +
Two blank lines.
+static int +testCompareXMLToConfigFiles(const char *xmlfile, + const char *configfile, + bool expectError)
@expectError isn't really used and to me seems 'backwards' - you're expecting an error?
+{ + int ret = -1; + char *config = NULL; + char *actualxml = NULL; + virDomainDefPtr vmdef = NULL; + + if (virTestLoadFile(configfile, &config) < 0) + goto fail; + + vmdef = virLXCDockerParseJSONConfig(caps, xmlopt, config); + + if (vmdef && expectError) { + if (testSanitizeDef(vmdef) < 0) + goto fail; + + if (!(actualxml = virDomainDefFormat(vmdef, caps, 0))) + goto fail; + + if (virTestCompareToFile(actualxml, xmlfile) < 0) + goto fail; + } + + ret = 0; + + fail: + VIR_FREE(actualxml); + VIR_FREE(config); + virDomainDefFree(vmdef); + return ret; +} + +struct testInfo { + const char *name; + bool expectError; +}; + +static int +testCompareXMLToConfigHelper(const void *data) +{ + int result = -1; + const struct testInfo *info = data; + char *xml = NULL; + char *config = NULL; + + if (virAsprintf(&xml, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&config, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.json", + abs_srcdir, info->name) < 0) + goto cleanup; + + result = testCompareXMLToConfigFiles(xml, config, info->expectError); + + cleanup: + VIR_FREE(xml); + VIR_FREE(config); + return result; +} + +static int +mymain(void) +{ + int ret = EXIT_SUCCESS; + + if (!(caps = testLXCCapsInit())) + return EXIT_FAILURE; + + if (!(xmlopt = lxcDomainXMLConfInit())) { + virObjectUnref(caps); + return EXIT_FAILURE; + } + + +# define DO_TEST(name, expectError) \ + do { \ + const struct testInfo info = { name, expectError }; \ + if (virTestRun("DOCKER JSON-2-XML " name, \ + testCompareXMLToConfigHelper, \ + &info) < 0) \ + ret = EXIT_FAILURE; \ + } while (0) + + DO_TEST("simple", true);
Is there a way to test DO_TEST("less_simple", false) John
+ + virObjectUnref(xmlopt); + virObjectUnref(caps); + + return ret; +} + +VIR_TEST_MAIN(mymain) + +#else + +int +main(void) +{ + return EXIT_AM_SKIP; +} + +#endif /* WITH_LXC */
On Tue, 2017-06-27 at 16:07 -0400, John Ferlan wrote:
On 06/27/2017 01:02 PM, Venkat Datta N H wrote:
Docker Memory and VCPU configuration is converted to fit for LXC container XML configuration --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/lxc/lxc_docker.c | 116 ++++++++++++++++ src/lxc/lxc_docker.h | 32 +++++ src/lxc/lxc_driver.c | 13 +- src/lxc/lxc_native.h | 1 + tests/Makefile.am | 8 +- .../lxcdocker2xmldata-simple.json | 36 +++++ .../lxcdocker2xmldata/lxcdocker2xmldata-simple.xml | 15 +++ tests/lxcdocker2xmltest.c | 149 +++++++++++++++++++++ 10 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 src/lxc/lxc_docker.c create mode 100644 src/lxc/lxc_docker.h create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml create mode 100644 tests/lxcdocker2xmltest.c
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.
diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f..421b32e 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -111,6 +111,7 @@ src/lxc/lxc_driver.c src/lxc/lxc_fuse.c src/lxc/lxc_hostdev.c src/lxc/lxc_native.c +src/lxc/lxc_docker.c src/lxc/lxc_process.c src/network/bridge_driver.c src/network/bridge_driver_linux.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc..1341e5a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -839,6 +839,7 @@ LXC_DRIVER_SOURCES = \ lxc/lxc_process.c lxc/lxc_process.h \ lxc/lxc_fuse.c lxc/lxc_fuse.h \ lxc/lxc_native.c lxc/lxc_native.h \ + lxc/lxc_docker.c lxc/lxc_docker.h \ lxc/lxc_driver.c lxc/lxc_driver.h LXC_CONTROLLER_SOURCES = \ diff --git a/src/lxc/lxc_docker.c b/src/lxc/lxc_docker.c new file mode 100644 index 0000000..dbb2a81 --- /dev/null +++ b/src/lxc/lxc_docker.c @@ -0,0 +1,116 @@ +/* + * lxc_docker.c: LXC native docker configuration import + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "util/viralloc.h" +#include "util/virfile.h" +#include "util/virstring.h" +#include "util/virconf.h" +#include "util/virjson.h" +#include "util/virutil.h" +#include "virerror.h" +#include "virlog.h" +#include "conf/domain_conf.h" +#include "lxc_docker.h" +#include "secret_conf.h" + +#define VIR_FROM_THIS VIR_FROM_LXC + +VIR_LOG_INIT("lxc.lxc_docker"); + +static int virLXCDockerParseCpu(virDomainDefPtr dom, + virDomainXMLOptionPtr xmlopt, + virJSONValuePtr prop)
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.
+{ + int vcpus; + + if (virJSONValueObjectGetNumberInt(prop, "CpuShares", &vcpus) != 0)
s/ !=/</ e.g. ) < 0)"
+ return -1;
Why CpuShares and not CpuCount? What about CpusetCpus?
After some more digging around that, maxvcpus value isn't used in the lxc driver. It needs to be set to get the XML parser code happy. In the lxc configuration conversion code (lxc_native.c) I just set it to 1: a number of vcpus doesn't really make sense on a container. The cpu tune properties however should be handled in another commit. -- Cedric
+ + if (virDomainDefSetVcpusMax(dom, vcpus, xmlopt) < 0) + return -1; + + if (virDomainDefSetVcpus(dom, vcpus) < 0) + return -1;
This would seem to be more related to CpusetCpus from my quick read.
+ + return 0; +} +
Two blank lines between functions
+static int virLXCDockerParseMem(virDomainDefPtr dom, + virJSONValuePtr prop) +{ + unsigned long long mem; + + if (virJSONValueObjectGetNumberUlong(prop, "Memory", &mem) != 0)
s/!=/</
+ return -1; + + 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)
Docker mounts a tmpfs as /dev/shm and allows changing the size of this mount. This is what ShmSize does. It seems that we don't do anything similar at all and would just ignore ShmSize. See https://github.com/moby/moby/pull/4981 for reference
+ return 0; +} + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps ATTRIBUTE_UNUSED,
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.
+ virDomainXMLOptionPtr xmlopt, + const char *config) +{ + virJSONValuePtr json_obj; + virJSONValuePtr host_config; + + if (!(json_obj = virJSONValueFromString(config))) + return NULL; + + virDomainDefPtr def;
Typically we keep definitions together without intervening lines of code.
+ + if (!(def = virDomainDefNew())) + goto error;> +
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.
+ def->id = -1; + def->mem.cur_balloon = 64*1024; + virDomainDefSetMemoryTotal(def, def->mem.cur_balloon);
You're setting some default and total, then possibly changing?
+ + if ((host_config = virJSONValueObjectGetObject(json_obj, "HostConfig")) != NULL) {
The != NULL is really not necessary
+ if (virLXCDockerParseCpu(def, xmlopt, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse VCpu"));
Long line - 80 cols please.
+ goto error; + } + + if (virLXCDockerParseMem(def, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse Memory"));
Long line again.
+ goto error; + } + } + + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; + def->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY; + def->onPoweroff = VIR_DOMAIN_LIFECYCLE_DESTROY; + def->virtType = VIR_DOMAIN_VIRT_LXC; + def->os.type = VIR_DOMAIN_OSTYPE_EXE; +
Setting the os.type, but there's nothing to run?
That is handled by the other commit, but those two can be merged together to have a basic working conversion.
[1] e.g. here for virDomainDefPostParse
What do you mean exactly here?
+ return def; + + error:
@json_obj is leaked (need a virJSONValueFree) here.
+ virDomainDefFree(def); + return NULL; +} diff --git a/src/lxc/lxc_docker.h b/src/lxc/lxc_docker.h new file mode 100644 index 0000000..f081e07 --- /dev/null +++ b/src/lxc/lxc_docker.h @@ -0,0 +1,32 @@ +/* + * lxc_docker.h: Header file for LXC native docker configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#ifndef __LXC_DOCKER_H__ +# define __LXC_DOCKER_H__ + +# include "domain_conf.h" + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + const char *config);
Similarly more recent style has been just to copy the .c function and add the ; ... makes things easier.
+ +#endif /* __LXC_NATIVE_DOCKER_H__ */
This comment doesn't matches the #ifndef above
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 22c8b58..711bb16 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -53,6 +53,7 @@ #include "lxc_driver.h" #include "lxc_native.h" #include "lxc_process.h" +#include "lxc_docker.h" #include "viralloc.h" #include "virnetdevbridge.h" #include "virnetdevveth.h" @@ -1062,15 +1063,17 @@ static char *lxcConnectDomainXMLFromNative(virConnectPtr conn, if (virConnectDomainXMLFromNativeEnsureACL(conn) < 0) goto cleanup; - if (STRNEQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (STREQ(nativeFormat, DOCKER_CONFIG_FORMAT)) { + if (!(def = virLXCDockerParseJSONConfig(caps, driver->xmlopt, nativeConfig)))
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.
+ goto cleanup; + } else if (STREQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) + goto cleanup; + } else { virReportError(VIR_ERR_INVALID_ARG, _("unsupported config type %s"), nativeFormat); goto cleanup; } - - if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) - goto cleanup; - xml = virDomainDefFormat(def, caps, 0); cleanup: diff --git a/src/lxc/lxc_native.h b/src/lxc/lxc_native.h index 15fa0d5..88263ae 100644 --- a/src/lxc/lxc_native.h +++ b/src/lxc/lxc_native.h @@ -26,6 +26,7 @@ # include "domain_conf.h" # define LXC_CONFIG_FORMAT "lxc-tools" +# define DOCKER_CONFIG_FORMAT "docker" virDomainDefPtr lxcParseConfigString(const char *config, virCapsPtr caps, diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc..0948bfa 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -93,6 +93,7 @@ EXTRA_DIST = \ capabilityschemadata \ commanddata \ cputestdata \ + lxcdocker2xmldata \ domaincapsschemadata \ domainconfdata \ domainschemadata \ @@ -295,7 +296,7 @@ test_libraries += libqemumonitortestutils.la \ endif WITH_QEMU if WITH_LXC -test_programs += lxcxml2xmltest lxcconf2xmltest +test_programs += lxcxml2xmltest lxcconf2xmltest lxcdocker2xmltest endif WITH_LXC if WITH_OPENVZ @@ -693,6 +694,11 @@ lxcconf2xmltest_SOURCES = \ lxcconf2xmltest.c testutilslxc.c testutilslxc.h \ testutils.c testutils.h lxcconf2xmltest_LDADD = $(lxc_LDADDS) + +lxcdocker2xmltest_SOURCES = \ + lxcdocker2xmltest.c testutilslxc.c testutilslxc.h \ + testutils.c testutils.h +lxcdocker2xmltest_LDADD = $(lxc_LDADDS) else ! WITH_LXC EXTRA_DIST += lxcxml2xmltest.c testutilslxc.c testutilslxc.h endif ! WITH_LXC diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json b/tests/lxcdocker2xmldata/lxcdocker2xmldata- simple.json new file mode 100644 index 0000000..63470be --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json @@ -0,0 +1,36 @@ +{ + "Id": "dbb1ae21dac15973d66e6c2b8516d270b32ca766e0cf7551d8b7973513e5f079", + "Created": "2017-05-25T18:55:17.922934825Z", + "Path": "/bin/bash", + "Args": [], + "HostConfig": { + "Binds": null, + "ContainerIDFile": "", + "LogConfig": { + "Type": "json-file", + "Config": {} + }, + "NetworkMode": "default", + "PortBindings": {}, + "ShmSize": 67108864, + "Runtime": "runc", + "Isolation": "", + "CpuShares": 2, + "Memory": 1073741824, + "CgroupParent": "", + "CpuPeriod": 0, + "CpuQuota": 0, + "CpusetCpus": "", + "CpusetMems": "", + "KernelMemory": 0, + "MemoryReservation": 0, + "MemorySwap": -1, + "MemorySwappiness": -1, + "PidsLimit": 0, + "Ulimits": null, + "CpuCount": 0, + "CpuPercent": 0, + "IOMaximumIOps": 0, + "IOMaximumBandwidth": 0 + } +}
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 goal is to work on this iteratively do avoid the huge commit providing full support.
The only thing used is "Memory"? What is ShmSize? for? What about MemorySwap or MemoryReservation - is there any relationship with existing DOMAIN_MEMORY_* variables?
Surely... Could be handled in a later commit, right?
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....
I'm not even sure there is anything mapping the vcpumax in the docker json configuration. But the cpu tuning values for sure can be mapped.
Perhaps going through lxc_driver API's would be beneficial to see how many relate to existing/parsed fields.
+1
diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml b/tests/lxcdocker2xmldata/lxcdocker2xmldata- simple.xml new file mode 100644 index 0000000..8be1ace --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml @@ -0,0 +1,15 @@ +<domain type='lxc'> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>2</vcpu> + <os> + <type>exe</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + </devices> +</domain>
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
Indeed, we're missing the name here. Venkat, you'll need to set it if provided in the configuration. Otherwise I'ld rather leave it empty and let the user manually add it rather than generating some funky name like docker does.
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
The question is: should we merge this commit with the next one then?
diff --git a/tests/lxcdocker2xmltest.c b/tests/lxcdocker2xmltest.c new file mode 100644 index 0000000..ccac4c4 --- /dev/null +++ b/tests/lxcdocker2xmltest.c @@ -0,0 +1,149 @@ +/* + * lxcdocker2xmltest.c: Test LXC Docker Configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "testutils.h" + +#ifdef WITH_LXC + +# include "lxc/lxc_docker.h" +# include "lxc/lxc_conf.h" +# include "testutilslxc.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +static virCapsPtr caps; +static virDomainXMLOptionPtr xmlopt; + +static int testSanitizeDef(virDomainDefPtr vmdef)
This one should have multi lines...
+{ + /* Remove UUID randomness */ + if (virUUIDParse("c7a5fdbd-edaf-9455-926a-d65c16db1809", vmdef->uuid) < 0) + return -1; + return 0; +} +
Two blank lines.
+static int +testCompareXMLToConfigFiles(const char *xmlfile, + const char *configfile, + bool expectError)
@expectError isn't really used and to me seems 'backwards' - you're expecting an error?
That is a copy/paste from the lxc-native configuration test. But yes, expectError should be removed if not used in this test.
+{ + int ret = -1; + char *config = NULL; + char *actualxml = NULL; + virDomainDefPtr vmdef = NULL; + + if (virTestLoadFile(configfile, &config) < 0) + goto fail; + + vmdef = virLXCDockerParseJSONConfig(caps, xmlopt, config); + + if (vmdef && expectError) { + if (testSanitizeDef(vmdef) < 0) + goto fail; + + if (!(actualxml = virDomainDefFormat(vmdef, caps, 0))) + goto fail; + + if (virTestCompareToFile(actualxml, xmlfile) < 0) + goto fail; + } + + ret = 0; + + fail: + VIR_FREE(actualxml); + VIR_FREE(config); + virDomainDefFree(vmdef); + return ret; +} + +struct testInfo { + const char *name; + bool expectError; +}; + +static int +testCompareXMLToConfigHelper(const void *data) +{ + int result = -1; + const struct testInfo *info = data; + char *xml = NULL; + char *config = NULL; + + if (virAsprintf(&xml, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&config, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.json", + abs_srcdir, info->name) < 0) + goto cleanup; + + result = testCompareXMLToConfigFiles(xml, config, info->expectError); + + cleanup: + VIR_FREE(xml); + VIR_FREE(config); + return result; +} + +static int +mymain(void) +{ + int ret = EXIT_SUCCESS; + + if (!(caps = testLXCCapsInit())) + return EXIT_FAILURE; + + if (!(xmlopt = lxcDomainXMLConfInit())) { + virObjectUnref(caps); + return EXIT_FAILURE; + } + + +# define DO_TEST(name, expectError) \ + do { \ + const struct testInfo info = { name, expectError }; \ + if (virTestRun("DOCKER JSON-2-XML " name, \ + testCompareXMLToConfigHelper, \ + &info) < 0) \ + ret = EXIT_FAILURE; \ + } while (0) + + DO_TEST("simple", true);
Is there a way to test DO_TEST("less_simple", false)
How comes we are expecting an error here? -- Cedric
John
+ + virObjectUnref(xmlopt); + virObjectUnref(caps); + + return ret; +} + +VIR_TEST_MAIN(mymain) + +#else + +int +main(void) +{ + return EXIT_AM_SKIP; +} + +#endif /* WITH_LXC */
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/03/2017 11:27 AM, Cedric Bosdonnat wrote:
On Tue, 2017-06-27 at 16:07 -0400, John Ferlan wrote:
On 06/27/2017 01:02 PM, Venkat Datta N H wrote:
Docker Memory and VCPU configuration is converted to fit for LXC container XML configuration --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/lxc/lxc_docker.c | 116 ++++++++++++++++ src/lxc/lxc_docker.h | 32 +++++ src/lxc/lxc_driver.c | 13 +- src/lxc/lxc_native.h | 1 + tests/Makefile.am | 8 +- .../lxcdocker2xmldata-simple.json | 36 +++++ .../lxcdocker2xmldata/lxcdocker2xmldata-simple.xml | 15 +++ tests/lxcdocker2xmltest.c | 149 +++++++++++++++++++++ 10 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 src/lxc/lxc_docker.c create mode 100644 src/lxc/lxc_docker.h create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml create mode 100644 tests/lxcdocker2xmltest.c
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.
diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f..421b32e 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -111,6 +111,7 @@ src/lxc/lxc_driver.c src/lxc/lxc_fuse.c src/lxc/lxc_hostdev.c src/lxc/lxc_native.c +src/lxc/lxc_docker.c src/lxc/lxc_process.c src/network/bridge_driver.c src/network/bridge_driver_linux.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc..1341e5a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -839,6 +839,7 @@ LXC_DRIVER_SOURCES = \ lxc/lxc_process.c lxc/lxc_process.h \ lxc/lxc_fuse.c lxc/lxc_fuse.h \ lxc/lxc_native.c lxc/lxc_native.h \ + lxc/lxc_docker.c lxc/lxc_docker.h \ lxc/lxc_driver.c lxc/lxc_driver.h
LXC_CONTROLLER_SOURCES = \ diff --git a/src/lxc/lxc_docker.c b/src/lxc/lxc_docker.c new file mode 100644 index 0000000..dbb2a81 --- /dev/null +++ b/src/lxc/lxc_docker.c @@ -0,0 +1,116 @@ +/* + * lxc_docker.c: LXC native docker configuration import + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "util/viralloc.h" +#include "util/virfile.h" +#include "util/virstring.h" +#include "util/virconf.h" +#include "util/virjson.h" +#include "util/virutil.h" +#include "virerror.h" +#include "virlog.h" +#include "conf/domain_conf.h" +#include "lxc_docker.h" +#include "secret_conf.h" + +#define VIR_FROM_THIS VIR_FROM_LXC + +VIR_LOG_INIT("lxc.lxc_docker"); + +static int virLXCDockerParseCpu(virDomainDefPtr dom, + virDomainXMLOptionPtr xmlopt, + virJSONValuePtr prop)
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.
+{ + int vcpus; + + if (virJSONValueObjectGetNumberInt(prop, "CpuShares", &vcpus) != 0)
s/ !=/</ e.g. ) < 0)"
+ return -1;
Why CpuShares and not CpuCount? What about CpusetCpus?
After some more digging around that, maxvcpus value isn't used in the lxc driver. It needs to be set to get the XML parser code happy. In the lxc configuration conversion code (lxc_native.c) I just set it to 1: a number of vcpus doesn't really make sense on a container.
The cpu tune properties however should be handled in another commit.
-- Cedric
Ok - well it wasn't clear to me - hence the question... Still if CpuShares is not vCPU, then we shouldn't apply it that way; otherwise, we could be "stuck with it". I'm OK with cputune in another commit...
+ + if (virDomainDefSetVcpusMax(dom, vcpus, xmlopt) < 0) + return -1; + + if (virDomainDefSetVcpus(dom, vcpus) < 0) + return -1;
This would seem to be more related to CpusetCpus from my quick read.
+ + return 0; +} +
Two blank lines between functions
+static int virLXCDockerParseMem(virDomainDefPtr dom, + virJSONValuePtr prop) +{ + unsigned long long mem; + + if (virJSONValueObjectGetNumberUlong(prop, "Memory", &mem) != 0)
s/!=/</
+ return -1; + + 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)
Docker mounts a tmpfs as /dev/shm and allows changing the size of this mount. This is what ShmSize does. It seems that we don't do anything similar at all and would just ignore ShmSize.
See https://github.com/moby/moby/pull/4981 for reference
Hmm.. maybe I asked the wrong question or pointed out the wrong thing. The ->memory value is the total size of memory in use, so do we need to include that ShmSize in some way with Memory... Perhaps not based partially on what I read from the linked pull request. It's not memory the guest would be using per se, but would be memory the domain needs for some kind of sharing with that -o flag. Eventually that ShmSize value would need to be read and handled I assume.
+ return 0; +} + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps ATTRIBUTE_UNUSED,
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.
+ virDomainXMLOptionPtr xmlopt, + const char *config) +{ + virJSONValuePtr json_obj; + virJSONValuePtr host_config; + + if (!(json_obj = virJSONValueFromString(config))) + return NULL; + + virDomainDefPtr def;
Typically we keep definitions together without intervening lines of code.
+ + if (!(def = virDomainDefNew())) + goto error;> +
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.
+ def->id = -1; + def->mem.cur_balloon = 64*1024; + virDomainDefSetMemoryTotal(def, def->mem.cur_balloon);
You're setting some default and total, then possibly changing?
+ + if ((host_config = virJSONValueObjectGetObject(json_obj, "HostConfig")) != NULL) {
The != NULL is really not necessary
+ if (virLXCDockerParseCpu(def, xmlopt, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse VCpu"));
Long line - 80 cols please.
+ goto error; + } + + if (virLXCDockerParseMem(def, host_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse Memory"));
Long line again.
+ goto error; + } + } + + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; + def->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY; + def->onPoweroff = VIR_DOMAIN_LIFECYCLE_DESTROY; + def->virtType = VIR_DOMAIN_VIRT_LXC; + def->os.type = VIR_DOMAIN_OSTYPE_EXE; +
Setting the os.type, but there's nothing to run?
That is handled by the other commit, but those two can be merged together to have a basic working conversion.
But I assume you need .type set because of something else, but it's not clear... Strange to have a naked setting like this when there's more to the os.* fields.
[1] e.g. here for virDomainDefPostParse
What do you mean exactly here?
Hmm... some review line seems to have been deleted, but IIRC I was pointing at the top of virLXCDockerParseJSONConfig where you'll see I was making a comparison to lxcParseConfigString which does call PostParse which I guess I would expect this code to do the same thing. If not, then why not? I would think at this point we'd want to be able to have whatever "defaults" or [minimum] environment has been created by the above to be able to be usable.
+ return def; + + error:
@json_obj is leaked (need a virJSONValueFree) here.
+ virDomainDefFree(def); + return NULL; +} diff --git a/src/lxc/lxc_docker.h b/src/lxc/lxc_docker.h new file mode 100644 index 0000000..f081e07 --- /dev/null +++ b/src/lxc/lxc_docker.h @@ -0,0 +1,32 @@ +/* + * lxc_docker.h: Header file for LXC native docker configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#ifndef __LXC_DOCKER_H__ +# define __LXC_DOCKER_H__ + +# include "domain_conf.h" + +virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + const char *config);
Similarly more recent style has been just to copy the .c function and add the ; ... makes things easier.
+ +#endif /* __LXC_NATIVE_DOCKER_H__ */
This comment doesn't matches the #ifndef above
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 22c8b58..711bb16 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -53,6 +53,7 @@ #include "lxc_driver.h" #include "lxc_native.h" #include "lxc_process.h" +#include "lxc_docker.h" #include "viralloc.h" #include "virnetdevbridge.h" #include "virnetdevveth.h" @@ -1062,15 +1063,17 @@ static char *lxcConnectDomainXMLFromNative(virConnectPtr conn, if (virConnectDomainXMLFromNativeEnsureACL(conn) < 0) goto cleanup;
- if (STRNEQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (STREQ(nativeFormat, DOCKER_CONFIG_FORMAT)) { + if (!(def = virLXCDockerParseJSONConfig(caps, driver->xmlopt, nativeConfig)))
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.
+ goto cleanup; + } else if (STREQ(nativeFormat, LXC_CONFIG_FORMAT)) { + if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) + goto cleanup; + } else { virReportError(VIR_ERR_INVALID_ARG, _("unsupported config type %s"), nativeFormat); goto cleanup; } - - if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt))) - goto cleanup; - xml = virDomainDefFormat(def, caps, 0);
cleanup: diff --git a/src/lxc/lxc_native.h b/src/lxc/lxc_native.h index 15fa0d5..88263ae 100644 --- a/src/lxc/lxc_native.h +++ b/src/lxc/lxc_native.h @@ -26,6 +26,7 @@ # include "domain_conf.h"
# define LXC_CONFIG_FORMAT "lxc-tools" +# define DOCKER_CONFIG_FORMAT "docker"
virDomainDefPtr lxcParseConfigString(const char *config, virCapsPtr caps, diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc..0948bfa 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -93,6 +93,7 @@ EXTRA_DIST = \ capabilityschemadata \ commanddata \ cputestdata \ + lxcdocker2xmldata \ domaincapsschemadata \ domainconfdata \ domainschemadata \ @@ -295,7 +296,7 @@ test_libraries += libqemumonitortestutils.la \ endif WITH_QEMU
if WITH_LXC -test_programs += lxcxml2xmltest lxcconf2xmltest +test_programs += lxcxml2xmltest lxcconf2xmltest lxcdocker2xmltest endif WITH_LXC
if WITH_OPENVZ @@ -693,6 +694,11 @@ lxcconf2xmltest_SOURCES = \ lxcconf2xmltest.c testutilslxc.c testutilslxc.h \ testutils.c testutils.h lxcconf2xmltest_LDADD = $(lxc_LDADDS) + +lxcdocker2xmltest_SOURCES = \ + lxcdocker2xmltest.c testutilslxc.c testutilslxc.h \ + testutils.c testutils.h +lxcdocker2xmltest_LDADD = $(lxc_LDADDS) else ! WITH_LXC EXTRA_DIST += lxcxml2xmltest.c testutilslxc.c testutilslxc.h endif ! WITH_LXC diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json b/tests/lxcdocker2xmldata/lxcdocker2xmldata- simple.json new file mode 100644 index 0000000..63470be --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json @@ -0,0 +1,36 @@ +{ + "Id": "dbb1ae21dac15973d66e6c2b8516d270b32ca766e0cf7551d8b7973513e5f079", + "Created": "2017-05-25T18:55:17.922934825Z", + "Path": "/bin/bash", + "Args": [], + "HostConfig": { + "Binds": null, + "ContainerIDFile": "", + "LogConfig": { + "Type": "json-file", + "Config": {} + }, + "NetworkMode": "default", + "PortBindings": {}, + "ShmSize": 67108864, + "Runtime": "runc", + "Isolation": "", + "CpuShares": 2, + "Memory": 1073741824, + "CgroupParent": "", + "CpuPeriod": 0, + "CpuQuota": 0, + "CpusetCpus": "", + "CpusetMems": "", + "KernelMemory": 0, + "MemoryReservation": 0, + "MemorySwap": -1, + "MemorySwappiness": -1, + "PidsLimit": 0, + "Ulimits": null, + "CpuCount": 0, + "CpuPercent": 0, + "IOMaximumIOps": 0, + "IOMaximumBandwidth": 0 + } +}
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 goal is to work on this iteratively do avoid the huge commit providing full support.
The only thing used is "Memory"? What is ShmSize? for? What about MemorySwap or MemoryReservation - is there any relationship with existing DOMAIN_MEMORY_* variables?
Surely... Could be handled in a later commit, right?
I'm fine with getting something working, but I think you'd admit it's bare bones as is, right? I'm not all that Docker aware, but figured I'd at least do a review and perhaps become more knowledgeable. I think the usual "fear" over how long does it take to become more fully functional and whether there are any downsides to "less" functionality. What expectations are there to have a fully functioning environment in some sort of timely manner. The fear is always the drop in some bare bones code and then someone becomes too busy to do anything else so eventually the work falls to someone else or isn't done.
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....
I'm not even sure there is anything mapping the vcpumax in the docker json configuration. But the cpu tuning values for sure can be mapped.
Perhaps going through lxc_driver API's would be beneficial to see how many relate to existing/parsed fields.
+1
diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml b/tests/lxcdocker2xmldata/lxcdocker2xmldata- simple.xml new file mode 100644 index 0000000..8be1ace --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml @@ -0,0 +1,15 @@ +<domain type='lxc'> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>2</vcpu> + <os> + <type>exe</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + </devices> +</domain>
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
Indeed, we're missing the name here. Venkat, you'll need to set it if provided in the configuration. Otherwise I'ld rather leave it empty and let the user manually add it rather than generating some funky name like docker does.
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
The question is: should we merge this commit with the next one then?
Well at what point do you declare things functional. Can this current commit be pared back some to provide the framework, then add pieces, etc. before declaring in lxc_driver.c that it's functionally ready. IOW: Can the lxc_driver.c change be "pushed off" until later as the last patch that would also include the .json and .xml test files. It's all about ordering and having "small enough", but "complete enough" compilable, testable units along the way so that if some day someone needs to git bisect to determine where some fault was interjected into the system it'll be easier. It's the happy balance between building something up or just throwing it over the wall.
diff --git a/tests/lxcdocker2xmltest.c b/tests/lxcdocker2xmltest.c new file mode 100644 index 0000000..ccac4c4 --- /dev/null +++ b/tests/lxcdocker2xmltest.c @@ -0,0 +1,149 @@ +/* + * lxcdocker2xmltest.c: Test LXC Docker Configuration + * + * Copyright (C) 2017 Venkat Datta N H + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Venkat Datta N H <nhvenkatdatta@gmail.com> + */ + +#include <config.h> + +#include "testutils.h" + +#ifdef WITH_LXC + +# include "lxc/lxc_docker.h" +# include "lxc/lxc_conf.h" +# include "testutilslxc.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +static virCapsPtr caps; +static virDomainXMLOptionPtr xmlopt; + +static int testSanitizeDef(virDomainDefPtr vmdef)
This one should have multi lines...
+{ + /* Remove UUID randomness */ + if (virUUIDParse("c7a5fdbd-edaf-9455-926a-d65c16db1809", vmdef->uuid) < 0) + return -1; + return 0; +} +
Two blank lines.
+static int +testCompareXMLToConfigFiles(const char *xmlfile, + const char *configfile, + bool expectError)
@expectError isn't really used and to me seems 'backwards' - you're expecting an error?
That is a copy/paste from the lxc-native configuration test. But yes, expectError should be removed if not used in this test.
+{ + int ret = -1; + char *config = NULL; + char *actualxml = NULL; + virDomainDefPtr vmdef = NULL; + + if (virTestLoadFile(configfile, &config) < 0) + goto fail; + + vmdef = virLXCDockerParseJSONConfig(caps, xmlopt, config); + + if (vmdef && expectError) { + if (testSanitizeDef(vmdef) < 0) + goto fail; + + if (!(actualxml = virDomainDefFormat(vmdef, caps, 0))) + goto fail; + + if (virTestCompareToFile(actualxml, xmlfile) < 0) + goto fail; + } + + ret = 0; + + fail: + VIR_FREE(actualxml); + VIR_FREE(config); + virDomainDefFree(vmdef); + return ret; +} + +struct testInfo { + const char *name; + bool expectError; +}; + +static int +testCompareXMLToConfigHelper(const void *data) +{ + int result = -1; + const struct testInfo *info = data; + char *xml = NULL; + char *config = NULL; + + if (virAsprintf(&xml, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&config, "%s/lxcdocker2xmldata/lxcdocker2xmldata-%s.json", + abs_srcdir, info->name) < 0) + goto cleanup; + + result = testCompareXMLToConfigFiles(xml, config, info->expectError); + + cleanup: + VIR_FREE(xml); + VIR_FREE(config); + return result; +} + +static int +mymain(void) +{ + int ret = EXIT_SUCCESS; + + if (!(caps = testLXCCapsInit())) + return EXIT_FAILURE; + + if (!(xmlopt = lxcDomainXMLConfInit())) { + virObjectUnref(caps); + return EXIT_FAILURE; + } + + +# define DO_TEST(name, expectError) \ + do { \ + const struct testInfo info = { name, expectError }; \ + if (virTestRun("DOCKER JSON-2-XML " name, \ + testCompareXMLToConfigHelper, \ + &info) < 0) \ + ret = EXIT_FAILURE; \ + } while (0) + + DO_TEST("simple", true);
Is there a way to test DO_TEST("less_simple", false)
How comes we are expecting an error here?
I dunno - but having a parsing error because of no name or no uuid? Whatever XML is generated should be valid enough to supply to some define/start or create processing to have a functional domain. I cannot fully recall what I had in mind for less_simple though ;-) John
-- Cedric
John
+ + virObjectUnref(xmlopt); + virObjectUnref(caps); + + return ret; +} + +VIR_TEST_MAIN(mymain) + +#else + +int +main(void) +{ + return EXIT_AM_SKIP; +} + +#endif /* WITH_LXC */
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Docker Json command is split between entrypoint and command properties. Compute a command out of those to fit the LXC container XML configuration. --- src/lxc/lxc_docker.c | 65 ++++++++++++++++++++++ .../lxcdocker2xmldata-command.json | 47 ++++++++++++++++ .../lxcdocker2xmldata-command.xml | 17 ++++++ tests/lxcdocker2xmltest.c | 1 + 4 files changed, 130 insertions(+) create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-command.json create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-command.xml diff --git a/src/lxc/lxc_docker.c b/src/lxc/lxc_docker.c index dbb2a81..b3e25f7 100644 --- a/src/lxc/lxc_docker.c +++ b/src/lxc/lxc_docker.c @@ -70,12 +70,70 @@ static int virLXCDockerParseMem(virDomainDefPtr dom, return 0; } +struct virLXCDockerCmdArgsIteratorArgs { + virDomainDefPtr vmdef; + size_t ninitargs; +}; + +static int virLXCDockerCmdArgsIterator(size_t pos ATTRIBUTE_UNUSED, + virJSONValuePtr item, + void *opaque) +{ + struct virLXCDockerCmdArgsIteratorArgs *args = opaque; + const char *value = virJSONValueGetString(item); + + if (!args->vmdef->os.init) { + if (VIR_STRDUP(args->vmdef->os.init, value) < 0) + return -1; + else + return 1; + } + + if (VIR_EXPAND_N(args->vmdef->os.initargv, args->ninitargs, 1) < 0) + return -1; + + if (VIR_STRDUP(args->vmdef->os.initargv[args->ninitargs - 1], value) < 0) + return -1; + + return 1; +} + + +static int virLXCDockerBuildInitCmd(virDomainDefPtr vmdef, + virJSONValuePtr config) +{ + virJSONValuePtr entry_point = virJSONValueObjectGetArray(config, "Entrypoint"); + virJSONValuePtr command = virJSONValueObjectGetArray(config, "Cmd"); + struct virLXCDockerCmdArgsIteratorArgs iterator_args = { vmdef, 0 }; + + if (entry_point && virJSONValueArrayForeachSteal(entry_point, + &virLXCDockerCmdArgsIterator, + &iterator_args) < 0) + goto error; + + if (command && virJSONValueArrayForeachSteal(command, + &virLXCDockerCmdArgsIterator, + &iterator_args) < 0) + goto error; + + /* Append NULL element at the end */ + if (iterator_args.ninitargs > 0 && + VIR_EXPAND_N(vmdef->os.initargv, iterator_args.ninitargs, 1) < 0) + goto error; + + return 0; + + error: + return -1; +} + virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps ATTRIBUTE_UNUSED, virDomainXMLOptionPtr xmlopt, const char *config) { virJSONValuePtr json_obj; virJSONValuePtr host_config; + virJSONValuePtr docker_config; if (!(json_obj = virJSONValueFromString(config))) return NULL; @@ -101,6 +159,13 @@ virDomainDefPtr virLXCDockerParseJSONConfig(virCapsPtr caps ATTRIBUTE_UNUSED, } } + if ((docker_config = virJSONValueObjectGetObject(json_obj, "Config")) != NULL) { + if (virLXCDockerBuildInitCmd(def, docker_config) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse Command")); + goto error; + } + } + def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; def->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY; diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-command.json b/tests/lxcdocker2xmldata/lxcdocker2xmldata-command.json new file mode 100644 index 0000000..14e4d92 --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-command.json @@ -0,0 +1,47 @@ +{ + "Id": "618a2e773b84976ca2736e443a0c98eeeecdcfb2584e3dab1a3cafa313a936da", + "Created": "2017-06-07T00:46:07.149971669Z", + "Path": "/bin/ping", + "Args": [ + "localhost" + ], + "HostConfig": { + "LogConfig": { + "Type": "json-file", + "Config": {} + }, + "NetworkMode": "default", + "PortBindings": {}, + "RestartPolicy": { + "Name": "no", + "MaximumRetryCount": 0 + }, + "ShmSize": 67108864, + "CpuShares": 0, + "Memory": 0, + "MemorySwap": 0, + "MemorySwappiness": -1, + "CpuCount": 0, + "CpuPercent": 0, + "IOMaximumBandwidth": 0 + }, + "GraphDriver": { + "Name": "aufs", + "Data": null + }, + "Mounts": [], + "Config": { + "Hostname": "618a2e773b84", + "Domainname": "", + "User": "", + "Cmd": [ + "/bin/ping", + "localhost" + ], + "WorkingDir": "", + "Entrypoint": null, + "OnBuild": null, + "Labels": {} + } + +} diff --git a/tests/lxcdocker2xmldata/lxcdocker2xmldata-command.xml b/tests/lxcdocker2xmldata/lxcdocker2xmldata-command.xml new file mode 100644 index 0000000..52155d1 --- /dev/null +++ b/tests/lxcdocker2xmldata/lxcdocker2xmldata-command.xml @@ -0,0 +1,17 @@ +<domain type='lxc'> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>0</memory> + <currentMemory unit='KiB'>0</currentMemory> + <vcpu placement='static'>0</vcpu> + <os> + <type>exe</type> + <init>/bin/ping</init> + <initarg>localhost</initarg> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + </devices> +</domain> diff --git a/tests/lxcdocker2xmltest.c b/tests/lxcdocker2xmltest.c index ccac4c4..dc7778a 100644 --- a/tests/lxcdocker2xmltest.c +++ b/tests/lxcdocker2xmltest.c @@ -129,6 +129,7 @@ mymain(void) } while (0) DO_TEST("simple", true); + DO_TEST("command", true); virObjectUnref(xmlopt); virObjectUnref(caps); -- 2.7.4
participants (3)
-
Cedric Bosdonnat -
John Ferlan -
Venkat Datta N H