[CRIU] [PATCH v3 5/7] config: check for CRIU_CONFIG_FILE environment variable

[Adrian Reber]

On Mon, Jul 09, 2018 at 12:36:30PM -0700, Andrei Vagin wrote:
> On Mon, Jul 09, 2018 at 08:29:51AM +0200, Adrian Reber wrote:
> > On Sun, Jul 08, 2018 at 11:24:40PM -0700, Andrei Vagin wrote:
> > > On Sun, Jul 08, 2018 at 05:28:04PM +0100, Radostin Stoyanov wrote:
> > > > On 08/07/18 08:05, Adrian Reber wrote:
> > > > > On Sat, Jul 07, 2018 at 07:22:33PM -0700, Andrei Vagin wrote:
> > > > >> Hi Adrian,
> > > > >>
> > > > >> Last time, we discussed that we need a way how to override criu options
> > > > >> which are set by Docker, runc, LXC or other tools.
> > > > >>
> > > > >> https://marc.info/?l=openvz-criu&m=152844187925264&w=2
> > > > > Yes and no. Now it gets complicated because Pavel and you are expecting
> > > > > different things ;-). This initial idea was the configuration file
> > > > > options are defaults and can be overridden by CLI/RPC settings.
> > > > >
> > > > >>> My idea was to introduce two config files for criu. The first one is exsting one, that
> > > > >>> criu reads from known global place (/etc/...). Then criu parses CLI/RPC options and 
> > > > >>> overrides values read from config. Then should go the 2nd config, which is formatted
> > > > >>> the same way as the 1st one, but it sits not in some other place path to which is
> > > > >>> somehow (I don't know how to pass this knowledge "through" docker/lxd) told to criu
> > > > >>> by the caller. This 2nd config file will override options set via CLI/RPC. Also, since
> > > > >>> the path to this 2nd config file is specified by the caller, if some other criu invocation
> > > > >>> happens in parallel it will not erroneously pick this file's options.
> > > > >>>
> > > > >> But CRIU_CONFIG_FILE doesn't work this way, does it?
> > > > > It does as described above. CLI/RPC overrides configuration file.
> > > > >
> > > > >> # cat /tmp/criu.cfg 
> > > > >> log-file /tmp/criu.log
> > > > >> # export CRIU_CONFIG_FILE="/tmp/criu.cfg"
> > > > >> # ./criu/criu dump -t 8888888 -v4
> > > > >> # cat /tmp/criu.log | grep Error
> > > > >> (00.001984) Error (criu/util.c:407): Can't open 8888888: No such file or directory
> > > > >> (00.002021) Error (compel/src/lib/infect.c:341): Unable to detach from 8888888: No such process
> > > > >> (00.002056) Error (criu/cr-dump.c:1834): Dumping FAILED.
> > > > >>
> > > > >> # unlink /tmp/criu.log 
> > > > >> # ./criu/criu dump -t 8888888 -v4 --log-file dump.log
> > > > >> # cat /tmp/criu.log | grep Error
> > > > >> cat: /tmp/criu.log: No such file or directory
> > > > > Exactly what is happening here. We have a 'default' setting in
> > > > > '/tmp/criu.log', but once we say '--log-file' via CLI the configuration
> > > > > file value is no longer valid.
> > > > >
> > > > > The initial idea of the configuration file was to be able to set default
> > > > > values which can be changed by CLI/RPC. So instead of saying -v4
> > > > > every time on the CLI I set it once in the configuration file and it
> > > > > always works. For the one case where I want -v0 I can still set it via
> > > > > CLI.
> > > > >
> > > > > So we really need to figure out what we want. Do we want to set
> > > > > defaults and be able to override them via CLI/RPC? Or do we want to set
> > > > > CRIU options which can be never overridden by CLI/RPC?
> > > > >
> > > > > My current patch set supports both setups for RPC, if the RPC client
> > > > > (runc) sets opts.prefer_config_file=True, the configuration files have
> > > > > override any settings done via RPC. If opts.prefer_config_file is set to
> > > > > False, CRIU behaves just like in CLI mode and any configuration file
> > > > > value which is explicitly set via RPC is ignored. Both modes of
> > > > > opts.prefer_config_file have their advantages and drawbacks. If it is
> > > > > set to True, the user can break the RPC client by setting incompatible
> > > > > values in the configuration file. If opts.prefer_config_file is set to
> > > > > false some things cannot be changed because the RPC client already sets
> > > > > them.
> > > > An alternative approach, to support both functionalities, could be to
> > > > add a prefix
> > > > (e.g. default-log-file) for options in the config file that will be
> > > > overridden by CLI/RPC.
> > > > Options which do not have this prefix (e.g. log-file) will override CLI/RPC.
> > > > 
> > > > In other words, in the config file we can use a "log-file" option to
> > > > specify a value that
> > > > will overwrite CLI/RPC, and "default-log-file" for a value that will be
> > > > overridden by CLI/RPC.
> > > > 
> > > > What do you think?
> > > 
> > > I think it is a good idea. My example is a real use-case where I'm going
> > > to use this functionality. I don't know where Docker or LXC save criu log
> > > files, for me it will be easier to specify a log file in a config file.
> > 
> > What does this mean for the current patchset? Can it be merged and I add
> > the override values later? Or do you want it all together?
> 
> I'm going to review this series and merge it into the criu-dev branch.
> 
> Could you send this patches as a new series? The current version was not
> tested by Jenkins and it is applied to the criu-dev with conflicts.

I rebased it and travis seems to be happy with it. Resending soon.

> But I would prefer to have an ability to override values before merging
> config files into the master branch. For me this functionality looks
> much more usefull in a real life than changing default values...

For the CLI use case it works most of the time because you will probably
not specify options you have in config file. But yes, I will provide
appropriate patches on top of it.

		Adrian