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

[Andrei Vagin]

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.

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...

> 
> 		Adrian