[libvirt] [PATCHv2 1/2] lxc: add support for docker-json Memory and VCPU conversion
John Ferlan
jferlan at redhat.com
Tue Jun 27 20:07:08 UTC 2017
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 at 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 at 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 at 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 */
>
More information about the libvir-list
mailing list