Add CRIU configuration file support

[adrianreber]

CRIU 3.11 introduces configuration files:

https://criu.org/Configuration_files
https://lisas.de/~adrian/posts/2018-Nov-08-criu-configuration-files.html

This enables the user to influence CRIU's behaviour without code changes if using new CRIU features or if the user wants to enable certain CRIU behaviour without always specifying certain options.

With this it is possible to write 'tcp-established' to the configuration file:

$ echo tcp-established > /etc/criu/runc.conf

and from now on all checkpoints will preserve the state of established TCP connections. This removes the need to always use

$ runc checkpoint --tcp-stablished

If the goal is to always checkpoint with '--tcp-established'

It also adds the possibility for unexpected CRIU behaviour if the user created a configuration file at some point in time and forgets about it.

[adrianreber]

@kolyshkin

[adrianreber]

Any reviewers? We would like to make CRIU's configuration file functionality available in runc to make the usage easier for programs relying on runc.

[rst0git]

LGTM

[Ace-Tang]

Hi, @adrianreber , from your word

CRIU 3.11 introduces configuration files:
https://criu.org/Configuration_files
https://lisas.de/~adrian/posts/2018-Nov-08-criu-configuration-files.html

this feature is not suit for criu under version 3.11 ? we are not using so new version now

[adrianreber]

Hi, @adrianreber , from your word

CRIU 3.11 introduces configuration files:
https://criu.org/Configuration_files
https://lisas.de/~adrian/posts/2018-Nov-08-criu-configuration-files.html

this feature is not suit for criu under version 3.11 ? we are not using so new version now

Older versions of CRIU do not support it, yes. But older CRIU versions just ignore if the option is set. With this change CRIU will either use it if it is new enough or ignore it. Nothing will break or change for users of older versions of CRIU.

[Ace-Tang]

It is good

[crosbymichael]

What happens with the hard coded path when that file or directory does not exist?

[adrianreber]

What happens with the hard coded path when that file or directory does not exist?

CRIU just skips it. CRIU 3.11 will always try to read /etc/criu/default.conf if it exists and skip it if it does not exist. Same is true for $HOME/.criu/default.conf.

We did not want to overwrite settings via RPC like runc does it per default. Therefore we require that the RPC client (runc in this case) explicitly sets a configuration file and only then the values from that configuration file overwrites RPC settings.

If the file does not exist, nothing changes. If it exists CRIU will use it.

[avagin]

Hi Adrian,

Can we declare an "annotation" parameter to specify a criu config? In this case, the user will be able to specify a criu config per container.
https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L20

[adrianreber]

@avagin does that mean the use could set it in config.json? Sounds like a good idea.

Would that overwrite runc's default CRIU configuration file? Or would runc only tell CRIU about a configuration file if it is in the spec? Just trying to understand what should runc do if there is nothing in the spec.

[avagin]

@avagin does that mean the use could set it in config.json? Sounds like a good idea.
yes

Would that overwrite runc's default CRIU configuration file? Or would runc only tell CRIU about a configuration file if it is in the spec? Just trying to understand what should runc do if there is nothing in the spec.

I'm not sure whether we need runc's default CRIU config file or not. But I definitely want to have an ability to specify a per-container config file. The global config file can be changed only if you have root privileges, but a per-container config file can be set by a regular user which can C/R a container.

[cyphar]

I agree with using an annotation. Maybe org.criu.config or something.

[adrianreber]

Any ideas if we want to set a default like this PR does? Or do we only want to managed it via annotations?

[avagin]

@adrianreber we can have both, but my opinion is that per-container config files are more useful.

[adrianreber]

@avagin @cyphar I changed this PR to also work with an annotation if it exists.

What is the right place to document this new annotation?

[avagin]

LGTM

It would be good to create a test. It should be very easy. You can create a test config which sets a path to a log file, then do C/R and check that the log file has been created.

[adrianreber]

I added the test. @avagin , your LGTM has been reset.

[adrianreber]

Test fails, works locally. Adding commits why it fails.

[adrianreber]

Fixed the tests. They were failing because CRIU was not able to keep kdat cache on non-tempfs.

[avagin]

@adrianreber I don't like the idea to check that a log file contains less than 45 lines. Let's overwrite a log file path, it is easier and more reliable:

avagin@cba8d33

[adrianreber]

@avagin Tried it, but I think you found a bug in the output file handling of the RPC code. So right now it is not possible to change the log file to something else. All the other settings are overwritable. My test has always been tcp-established, that's why have not hit this bug yet.

So we need a fix in CRIU first. I will submit something tomorrow.

[adrianreber]

The following patch is an approach to fix this in CRIU. Once this is merged I will continue here.

https://lists.openvz.org/pipermail/criu/2018-December/043104.html

[adrianreber]

This failure is expected as the mentioned CRIU patch is missing.

[adrianreber]

Finally the tests (including the new test) are passing. All failures from yesterday seem to have been Travis related.

From my point of view this is now ready for review and merge. Waiting for a CRIU 3.12 release would also be possible but as we have handled CRIU features before like this (adding upstream patches to make tests pass) I think this could be merged. The CRIU patch for testing is also only necessary if someone wants to change CRIU's log file. For all other CRIU related settings the patch on top of CRIU 3.11 is not needed.

[rst0git]

What is the right place to document this new annotation?

I believe the right place to document a new annotation would be in runtime-spec/config.md.

I can't find any place where annotations have been previously documented.

cc @crosbymichael

[avagin]

LGTM

I can't find any place where annotations have been previously documented.

I think it is a first annotation which is used in runc. I think we can add docs/checkpoint.md and describe there how this functionality works and how it can be customized. I would suggest to do this in another pr.

[adrianreber]

I will open a PR with the appropriate documentation once this is merged.

[adrianreber]

@cyphar @crosbymichael anything missing from your point of view? Could I get another approval to get this merged?

[adrianreber]

Could any other reviewer please have a look?

[crosbymichael]

LGTM