Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libcriu: add API for join-ns option #1617

Merged
merged 5 commits into from
Oct 5, 2021

Conversation

rst0git
Copy link
Member

@rst0git rst0git commented Sep 21, 2021

We use the join-ns RPC API in runc to enable checkpoint/restore of containers with shared namespaces. Shared namespaces are often used when containers run inside Kubernetes Pod. However, crun is using libcriu to interface with CRIU and libcriu currently doesn't provide an API for join-ns. This pull request adds the libcriu API necessary to enable checkpoint/restore of containers with shared namespaces with crun.

@adrianreber
Copy link
Member

I was a bit confused about the interface. Path names instead of file descriptors. Looking at the RPC and CLI interface I think your implementation is correct as RPC and CLI also use path names and not file descriptors. Which is confusing because inherit-fd uses file descriptors.

Everything looks good. Unfortunately the arm tests are currently not running. I will wait for the results from those tests and then it looks ready to be merged.

@rst0git
Copy link
Member Author

rst0git commented Sep 21, 2021

Thank you Adrian for the code review. I was hoping to ask for feedback for the test case of join-ns. It would currently check that checkpoint/restore has succeeded. While this is sufficient to check if the join-ns option has been passed to CRIU (example below), we don't currently have tests to verify that --join-ns actually works. Perhaps we could add the tests in runc/crun?

sudo make -C test/others/libcriu/ run
sudo cat test/others/libcriu/wdir/i/test_join_ns/restore.log | grep 'join namespace'
(00.000244) Added ipc:/proc/66302/ns/ipc join namespace
(00.000264) Added uts:/proc/66302/ns/uts join namespace
(00.000281) Added time:/proc/66302/ns/time join namespace

@avagin do you have any advice?

@adrianreber
Copy link
Member

Thank you Adrian for the code review. I was hoping to ask for feedback for the test case of join-ns. It would currently check that checkpoint/restore has succeeded. While this is sufficient to check if the join-ns option has been passed to CRIU (example below), we don't currently have tests to verify that --join-ns actually works. Perhaps we could add the tests in runc/crun?

sudo make -C test/others/libcriu/ run
sudo cat test/others/libcriu/wdir/i/test_join_ns/restore.log | grep 'join namespace'
(00.000244) Added ipc:/proc/66302/ns/ipc join namespace
(00.000264) Added uts:/proc/66302/ns/uts join namespace
(00.000281) Added time:/proc/66302/ns/time join namespace

@avagin do you have any advice?

I would compare /proc/<PID>/ns/<namespace> just as we do in other tests.

In runc we use the join-ns RPC API to enable checkpoint/restore of
containers with shared namespaces. Shared namespaces are often used
when containers run inside Kubernetes Pod.

In crun we use libcriu to interface with CRIU, however it currently
doesn't provide an API for join-ns. This patch adds the necessary
libcriu API to enable checkpoint/restore of containers with shared
namespaces with crun.

Signed-off-by: Radostin Stoyanov <[email protected]>
Replace magic numbers used to set log level in libcriu
with constants.

Signed-off-by: Radostin Stoyanov <[email protected]>
@rst0git
Copy link
Member Author

rst0git commented Sep 24, 2021

I would compare /proc/<PID>/ns/<namespace> just as we do in other tests.

Thanks, I've updated the test case.

@rst0git rst0git force-pushed the libcriu-join-ns branch 3 times, most recently from c787bae to 5b49fe4 Compare September 24, 2021 11:39
@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2021

Codecov Report

Merging #1617 (29459f9) into criu-dev (afbfaca) will decrease coverage by 0.03%.
The diff coverage is 46.66%.

❗ Current head 29459f9 differs from pull request most recent head 2aa4af9. Consider uploading reports for the commit 2aa4af9 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           criu-dev    #1617      +/-   ##
============================================
- Coverage     68.84%   68.80%   -0.04%     
============================================
  Files           137      137              
  Lines         33299    33359      +60     
============================================
+ Hits          22924    22953      +29     
- Misses        10375    10406      +31     
Impacted Files Coverage Δ
lib/c/criu.c 34.55% <46.66%> (+0.76%) ⬆️
criu/uffd.c 76.94% <0.00%> (-0.99%) ⬇️
criu/cr-restore.c 66.62% <0.00%> (-0.20%) ⬇️
criu/cr-service.c 63.33% <0.00%> (+0.19%) ⬆️
criu/namespaces.c 69.59% <0.00%> (+0.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afbfaca...2aa4af9. Read the comment docs.

@rst0git rst0git force-pushed the libcriu-join-ns branch 3 times, most recently from 5a0c770 to 2aa4af9 Compare September 25, 2021 11:44
This test case aims to verify that CRIU correctly
restores a process in IPC, UTS and Time namespaces
with criu_join_ns_add() libcriu API.

Signed-off-by: Radostin Stoyanov <[email protected]>
@rst0git rst0git force-pushed the libcriu-join-ns branch 3 times, most recently from 15a4662 to 4dae9df Compare September 25, 2021 13:58
run_test was trying to read criu logs on build failure
instead of runtime error.

This patch also removes the unnecessary subfolder with name "i"
and resolves some of issues reported by shellcheck.

Signed-off-by: Radostin Stoyanov <[email protected]>
@adrianreber
Copy link
Member

I see that you changed run.sh. Could you add it to the shellcheck test targets?

@adrianreber adrianreber merged commit 8cf99be into checkpoint-restore:criu-dev Oct 5, 2021
@rst0git rst0git deleted the libcriu-join-ns branch December 5, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants