[CRIU] [PATCH v2 2/4] Track which configuration options have been changed
Adrian Reber
areber at redhat.com
Thu May 10 13:25:49 MSK 2018
On Wed, May 09, 2018 at 11:53:51AM -0700, Andrei Vagin wrote:
> On Wed, May 09, 2018 at 05:16:48PM +0000, Adrian Reber wrote:
> > From: Adrian Reber <areber at redhat.com>
> >
> > For the upcoming RPC configuration file support, the RPC code path needs
> > to know which values have been changed from the default.
> >
> > The reason for this is, that we do not want that values specified via
> > RPC are overwriting user specified settings in the configuration file.
> >
> > For bool options is is not possible to tell if the value is the default
> > value as a result of the configuration file or as a result of just being
> > the default.
> >
> > For default values, which are not modified by configuration files we
> > want to use the RPC specified value. If a user changes a value to the
> > default in the configuration file we do not want to have it changed by
> > the RPC code path.
> >
> > This means that values from the configuration file have a higher
> > priority than values configured via RPC.
>
> Should it be the same for command line criu options?
>
> I have some doubts about such behaviour... Maybe we need to add a marker
> for a config options, which will say how it should be applied. Pavel,
> what do you think?
This is the real complicated thing about the configuration file. I
discussed this a lot with Veronika when she implemented it and it was
never really clear what is the right thing to do. The current
implementation is basically what I would expect how a tool handles it
(this must not be correct).
For CLI the configuration files are parsed first, but the user can
overwrite it via command-line. For certain cases this makes sense as the
user might want to change the default behavior of the configuration
files.
Example: The package manager or the system administrator sets
verbosity=4. I as user do not like it and can disable it via CLI by
setting --verbosity=0 (or a configuration file in my home directory).
First configuration files are parsed and then the CLI options are
evaluated and it the end I get --verbosity=0 -> good.
On the other hand for cases like LXC, where LXC builds a
command-line and uses CRIU via CLI it does not make sense as it makes it
impossible to change CRIU's behavior.
Example: LXC sets '-vvvvv' and as user I want to change it to
'verbosity=0'. As CRIU's command-line is part of LXC C code I cannot
change it quickly and therefore I try to set it via configuration file
(verbosity=0). This however does not work as the configuration file is
parsed first and the CLI overrides it. As a user I do not get what I
want -> bad.
So for CLI the current implementation in criu-dev can be correct (real
CLI) or wrong (CLI embedded in C code like LXC). One could argue that
LXC usage of CRIU is not how it is supposed to be and that RPC would be
better, but that is the current situation.
So CLI: first configuration file then CLI
For RPC, I would expect it to be the other way. As user I cannot change
how RPC is used without changing the code. Therefore I would expect that
the configuration file overrides the setting via RPC for me to be able
to change the behavior.
Example: runc also sets verbosity to 4. As a user I do not want big log
files and I change it to 'verbosity=0' via configuration file. Result:
CRIU uses verbosity=0 as the configuration file overrides the RPC
parameters. -> good, as a user I get what I expect.
It is, however, just the other way as CLI. Which is probably bad as it
might confuse the user.
So it really is difficult to do it correctly.
> > The reason for this is, that we want to enable the user to change CRIU's
> > behavior when embedded in a container runtime and used via RPC (e.g.
> > runc).
> >
> > If an option is specified in the configuration file the values for that
> > option are set to 1/true in the corresponding opt_changed_by_config.
> >
> > Now the RPC code path can check if an option has been changed via
> > configuration file and can then ignore the value specified via RPC.
>
> Will it be simpler just to run parsing of config options after handling rpc
> options?
Maybe, not sure. As we would need to run getopt() a second time. I
thought about it, but I was not sure. The problem is that we would need
to change the RPC parameters to a format which getopt() understands and
that also seemed unnecessary complicated.
The current configuration file code is nice that it just transforms the
configuration file to getopt() compatible input, on the other hand that
makes is complicated for the RPC case as we would need to create
getopt() compatible from the RPC options.
Adrian
> > Signed-off-by: Adrian Reber <areber at redhat.com>
> > ---
> > criu/crtools.c | 102 +++++++++++++++++++++++++++++++++-------------
> > criu/include/cr_options.h | 11 +++++
> > 2 files changed, 84 insertions(+), 29 deletions(-)
> >
> > diff --git a/criu/crtools.c b/criu/crtools.c
> > index 27e54e0..4d91e51 100644
> > --- a/criu/crtools.c
> > +++ b/criu/crtools.c
> > @@ -57,6 +57,7 @@
> > #include "img-remote.h"
> >
> > struct cr_options opts;
> > +struct cr_options opt_changed_by_config;
> > char **global_conf = NULL;
> > char **user_conf = NULL;
> >
> > @@ -273,8 +274,10 @@ int main(int argc, char *argv[], char *envp[])
> > char *imgs_dir = ".";
> >
> > #define BOOL_OPT(OPT_NAME, SAVE_TO) \
> > - {OPT_NAME, no_argument, SAVE_TO, true},\
> > - {"no-" OPT_NAME, no_argument, SAVE_TO, false}
> > + {OPT_NAME, no_argument, &opts.SAVE_TO, true},\
> > + {OPT_NAME, no_argument, &opt_changed_by_config.SAVE_TO, false},\
> > + {"no-" OPT_NAME, no_argument, &opts.SAVE_TO, false}, \
> > + {"no-" OPT_NAME, no_argument, &opt_changed_by_config.SAVE_TO, false}
> >
> > static const char short_opts[] = "dSsRf:F:t:p:hcD:o:v::x::Vr:jJ:lW:L:M:";
> > static struct option long_opts[] = {
> > @@ -282,9 +285,9 @@ int main(int argc, char *argv[], char *envp[])
> > { "pid", required_argument, 0, 'p' },
> > { "leave-stopped", no_argument, 0, 's' },
> > { "leave-running", no_argument, 0, 'R' },
> > - BOOL_OPT("restore-detached", &opts.restore_detach),
> > - BOOL_OPT("restore-sibling", &opts.restore_sibling),
> > - BOOL_OPT("daemon", &opts.restore_detach),
> > + BOOL_OPT("restore-detached", restore_detach),
> > + BOOL_OPT("restore-sibling", restore_sibling),
> > + BOOL_OPT("daemon", restore_detach),
> > { "contents", no_argument, 0, 'c' },
> > { "file", required_argument, 0, 'f' },
> > { "fields", required_argument, 0, 'F' },
> > @@ -295,27 +298,28 @@ int main(int argc, char *argv[], char *envp[])
> > { "root", required_argument, 0, 'r' },
> > { USK_EXT_PARAM, optional_argument, 0, 'x' },
> > { "help", no_argument, 0, 'h' },
> > - BOOL_OPT(SK_EST_PARAM, &opts.tcp_established_ok),
> > + BOOL_OPT(SK_EST_PARAM, tcp_established_ok),
> > + BOOL_OPT(SK_EST_PARAM, tcp_established_ok),
> > { "close", required_argument, 0, 1043 },
> > - BOOL_OPT("log-pid", &opts.log_file_per_pid),
> > + BOOL_OPT("log-pid", log_file_per_pid),
> > { "version", no_argument, 0, 'V' },
> > - BOOL_OPT("evasive-devices", &opts.evasive_devices),
> > + BOOL_OPT("evasive-devices", evasive_devices),
> > { "pidfile", required_argument, 0, 1046 },
> > { "veth-pair", required_argument, 0, 1047 },
> > { "action-script", required_argument, 0, 1049 },
> > - BOOL_OPT(LREMAP_PARAM, &opts.link_remap_ok),
> > - BOOL_OPT(OPT_SHELL_JOB, &opts.shell_job),
> > - BOOL_OPT(OPT_FILE_LOCKS, &opts.handle_file_locks),
> > - BOOL_OPT("page-server", &opts.use_page_server),
> > + BOOL_OPT(LREMAP_PARAM, link_remap_ok),
> > + BOOL_OPT(OPT_SHELL_JOB, shell_job),
> > + BOOL_OPT(OPT_FILE_LOCKS, handle_file_locks),
> > + BOOL_OPT("page-server", use_page_server),
> > { "address", required_argument, 0, 1051 },
> > { "port", required_argument, 0, 1052 },
> > { "prev-images-dir", required_argument, 0, 1053 },
> > { "ms", no_argument, 0, 1054 },
> > - BOOL_OPT("track-mem", &opts.track_mem),
> > - BOOL_OPT("auto-dedup", &opts.auto_dedup),
> > + BOOL_OPT("track-mem", track_mem),
> > + BOOL_OPT("auto-dedup", auto_dedup),
> > { "libdir", required_argument, 0, 'L' },
> > { "cpu-cap", optional_argument, 0, 1057 },
> > - BOOL_OPT("force-irmap", &opts.force_irmap),
> > + BOOL_OPT("force-irmap", force_irmap),
> > { "ext-mount-map", required_argument, 0, 'M' },
> > { "exec-cmd", no_argument, 0, 1059 },
> > { "manage-cgroups", optional_argument, 0, 1060 },
> > @@ -324,8 +328,8 @@ int main(int argc, char *argv[], char *envp[])
> > { "feature", required_argument, 0, 1063 },
> > { "skip-mnt", required_argument, 0, 1064 },
> > { "enable-fs", required_argument, 0, 1065 },
> > - { "enable-external-sharing", no_argument, &opts.enable_external_sharing, true },
> > - { "enable-external-masters", no_argument, &opts.enable_external_masters, true },
> > + BOOL_OPT("enable-external-sharing", enable_external_sharing),
> > + BOOL_OPT("enable-external-masters", enable_external_masters),
> > { "freeze-cgroup", required_argument, 0, 1068 },
> > { "ghost-limit", required_argument, 0, 1069 },
> > { "irmap-scan-path", required_argument, 0, 1070 },
> > @@ -334,21 +338,21 @@ int main(int argc, char *argv[], char *envp[])
> > { "external", required_argument, 0, 1073 },
> > { "empty-ns", required_argument, 0, 1074 },
> > { "lazy-pages", no_argument, 0, 1076 },
> > - BOOL_OPT("extra", &opts.check_extra_features),
> > - BOOL_OPT("experimental", &opts.check_experimental_features),
> > + BOOL_OPT("extra", check_extra_features),
> > + BOOL_OPT("experimental", check_experimental_features),
> > { "all", no_argument, 0, 1079 },
> > { "cgroup-props", required_argument, 0, 1080 },
> > { "cgroup-props-file", required_argument, 0, 1081 },
> > { "cgroup-dump-controller", required_argument, 0, 1082 },
> > - BOOL_OPT(SK_INFLIGHT_PARAM, &opts.tcp_skip_in_flight),
> > - BOOL_OPT("deprecated", &opts.deprecated_ok),
> > - BOOL_OPT("check-only", &opts.check_only),
> > - BOOL_OPT("display-stats", &opts.display_stats),
> > - BOOL_OPT("weak-sysctls", &opts.weak_sysctls),
> > + BOOL_OPT(SK_INFLIGHT_PARAM, tcp_skip_in_flight),
> > + BOOL_OPT("deprecated", deprecated_ok),
> > + BOOL_OPT("check-only", check_only),
> > + BOOL_OPT("display-stats", display_stats),
> > + BOOL_OPT("weak-sysctls", weak_sysctls),
> > { "status-fd", required_argument, 0, 1088 },
> > - BOOL_OPT(SK_CLOSE_PARAM, &opts.tcp_close),
> > + BOOL_OPT(SK_CLOSE_PARAM, tcp_close),
> > { "verbosity", optional_argument, 0, 'v' },
> > - BOOL_OPT("remote", &opts.remote),
> > + BOOL_OPT("remote", remote),
> > { "config", required_argument, 0, 1089},
> > { "no-default-config", no_argument, 0, 1090},
> > { },
> > @@ -416,20 +420,54 @@ int main(int argc, char *argv[], char *envp[])
> > break;
> > }
> > }
> > - if (!opt)
> > +
> > + /* opt == 0 means getopt is directly writing the value to the destination */
> > + if (opt == 0) {
> > + /*
> > + * Right now all bool options have a negated variant
> > + * created automatically:
> > + * --tcp-established and --no-tcp-established
> > + * For the RPC code path to detect if the default value
> > + * comes from the configuration file or because it is
> > + * the default value, there is a second structure which
> > + * is only used to check if an option has been changed via
> > + * the getopt parser: opt_changed_by_config
> > + * The following is the code to set those options in that
> > + * structure.
> > + */
> > +
> > + /*
> > + * The macro BOOL_OPT() always adds the corresponding
> > + * opt_changed_by_config at idx + 1. So by writing true
> > + * to long_opts[idx + 1].flag we are telling the RPC
> > + * code path that the user set a value (it might or might
> > + * not be different from the default).
> > + */
> > +
> > + /* Do no write beyond the long_opts array */
> > + if (idx + 1 > sizeof(long_opts)/sizeof(long_opts[0]))
> > + continue;
> > +
> > + *long_opts[idx + 1].flag = true;
> > +
> > + /* No need for further option analysis if opt == 0 */
> > continue;
> > + }
> >
> > switch (opt) {
> > case 's':
> > opts.final_state = TASK_STOPPED;
> > + opt_changed_by_config.final_state = 1;
> > break;
> > case 'R':
> > opts.final_state = TASK_ALIVE;
> > + opt_changed_by_config.final_state = 1;
> > break;
> > case 'x':
> > if (optarg && unix_sk_ids_parse(optarg) < 0)
> > return 1;
> > opts.ext_unix_sk = true;
> > + opt_changed_by_config.ext_unix_sk = true;
> > break;
> > case 'p':
> > pid = atoi(optarg);
> > @@ -458,6 +496,7 @@ int main(int argc, char *argv[], char *envp[])
> > break;
> > case 'S':
> > opts.restore_sibling = true;
> > + opt_changed_by_config.restore_sibling = true;
> > break;
> > case 'D':
> > imgs_dir = optarg;
> > @@ -481,6 +520,7 @@ int main(int argc, char *argv[], char *envp[])
> > opts.log_level = atoi(optarg);
> > } else
> > opts.log_level++;
> > + opt_changed_by_config.log_level = 1;
> > break;
> > case 1043: {
> > int fd;
> > @@ -546,6 +586,7 @@ int main(int argc, char *argv[], char *envp[])
> > case 1060:
> > if (parse_manage_cgroups(&opts, optarg))
> > goto usage;
> > + opt_changed_by_config.manage_cgroups = 1;
> > break;
> > case 1061:
> > {
> > @@ -589,6 +630,7 @@ int main(int argc, char *argv[], char *envp[])
> > break;
> > case 1069:
> > opts.ghost_limit = parse_size(optarg);
> > + opt_changed_by_config.ghost_limit = 1;
> > break;
> > case 1070:
> > if (irmap_scan_path_add(optarg))
> > @@ -627,9 +669,10 @@ int main(int argc, char *argv[], char *envp[])
> > return 1;
> > break;
> > case 1074:
> > - if (!strcmp("net", optarg))
> > + if (!strcmp("net", optarg)) {
> > opts.empty_ns |= CLONE_NEWNET;
> > - else {
> > + opt_changed_by_config.empty_ns = 1;
> > + } else {
> > pr_err("Unsupported empty namespace: %s\n",
> > optarg);
> > return 1;
> > @@ -654,6 +697,7 @@ int main(int argc, char *argv[], char *envp[])
> > pr_err("Unable to parse a value of --status-fd\n");
> > return 1;
> > }
> > + opt_changed_by_config.status_fd = 1;
> > break;
> > case 1089:
> > break;
> > diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
> > index 1365b2e..72778c7 100644
> > --- a/criu/include/cr_options.h
> > +++ b/criu/include/cr_options.h
> > @@ -128,6 +128,17 @@ struct cr_options {
> >
> > extern struct cr_options opts;
> >
> > +/*
> > + * This is used to track if options have been changed manually from the
> > + * default, or if it was manually set to the default value.
> > + *
> > + * In this copy of the actual used opts structure options changed by
> > + * the configuration file or command-line arguments are set to '1' or 'true'
> > + * so that the RPC code knows that this value should not be overwritten
> > + * by the RPC caller.
> > + */
> > +extern struct cr_options opt_changed_by_config;
> > +
> > extern void init_opts(void);
> >
> > #endif /* __CR_OPTIONS_H__ */
> > --
> > 1.8.3.1
> >
More information about the CRIU
mailing list