Skip to content

tests/int/checkpoint: fds and pids cleanup#2509

Merged
cyphar merged 2 commits into
opencontainers:masterfrom
kolyshkin:cpt-tests-cleanups
Jul 20, 2020
Merged

tests/int/checkpoint: fds and pids cleanup#2509
cyphar merged 2 commits into
opencontainers:masterfrom
kolyshkin:cpt-tests-cleanups

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Jul 7, 2020

  1. Do not use hardcoded fd numbers, instead relying on bash feature of
    assigning an fd to a variable.

    This looks very weird, but the rule of thumb here is:

    • if this is exec, use {var} (i.e. no $);
    • otherwise, use as normal ($var or ${var}).
  2. Add killing the background processes and closing the fds to teardown.
    This is helpful in case of a test failure, in order to not affect the
    subsequent tests. Found in libct/cgroups/GetCgroupRoot: make it faster #2507 (comment)

  3. Drop removing readonly flag for lazy cpt tests, it is no longer required
    (see get_clean_mount: demote an error to a warning checkpoint-restore/criu#1033, runc lazy-pages test case needs "readonly": false (Error (criu/mount.c:1119): mnt: Can't create a temporary directory: Read-only file system) checkpoint-restore/criu#575)

1. Do not use hardcoded fd numbers, instead relying on bash feature of
   assigning an fd to a variable.

   This looks very weird, but the rule of thumb here is:
   - if this is in exec, use {var} (i.e. no $);
   - otherwise, use as normal ($var or ${var}).

2. Add killing the background processes and closing the fds to teardown.
   This is helpful in case of a test failure, in order to not affect the
   subsequent tests.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Copy Markdown
Contributor Author

@tianon @adrianreber @avagin PTAL

Copy link
Copy Markdown
Contributor

@adrianreber adrianreber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks weird, indeed. If it works, good. Looks more flexible than hardcoding.

This should not longer be necessary (in theory, at least),
let's see how it goes.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Copy Markdown
Contributor Author

The {fd} feature requires bash-4.1. Documentation: https://www.gnu.org/software/bash/manual/html_node/Redirections.html

@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Jul 9, 2020

I've also found added a commit dropping the readonly flag removal for cpt tests. Indeed it looks like checkpoint-restore/criu#575 is somehow fixed.

@kolyshkin
Copy link
Copy Markdown
Contributor Author

@AkihiroSuda @mrunalp PTAL (this should make our CI cleaner a bit)

@kolyshkin
Copy link
Copy Markdown
Contributor Author

Indeed it looks like checkpoint-restore/criu#575 is somehow fixed.

Clarification on "somehow". The removal of "read-only" was preventing the following like from appearing in the logs:

(00.069337) Error (criu/mount.c:1119): mnt: Can't create a temporary directory: Read-only file system

The thing is, this was not an error but actually warning, since the code was working around the read-only state, so clearing the read-only state was not needed. Later, checkpoint-restore/criu#1033 demoted that error to a warning, so since criu 3.14 it is no longer appearing as an error. But even without this patch read-only could safely be removed.

Copy link
Copy Markdown
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants