-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add support to migrate containers #2272
Add support to migrate containers #2272
Conversation
331cae2
to
4d3fa26
Compare
The restore of a looper example fails for me. Still investigating why... |
/retest |
Are you using |
Yes, I installed
I followed these steps to create, checkpoint, and restore a container:
Even though the restore has failed in the last step, a looper container is still created (with status |
@rst0git This is the same bug as in my runc pull request except that it is also true for non-bind mount mountpoints. It seems runc just creates all missing mountpoints even for read-only root filesystems. If you use a container image which includes all required mountpoints (in this case /run /proc /sys are missing) it should work. I will update my runc PR to handle also missing non-bind mount mountpoints. |
@mheon Thanks for the review. At first I was unsure if I can do all the necessary changes, but now, after I have thought about it, I think I can actually do everything you suggested and the result will be better. I hope. So thanks for pointing it out. |
/retest |
|
The checkpoint directory and the container definition (spec, config, network). No filesystem (yet). I was thinking about doing an automatic commit of the highest layer and including it also. But right now it is only the output of CRIU and some json files. |
74ab7b8
to
0ba0da7
Compare
@mheon I tried to rework my changes to the newContainer() function. There is now a RestoreContainer() function. Please have a look if this is now a better approach. |
All CI errors are the expected errors as long as the necessary runc patches have not been merged. |
I'll do a more thorough review tomorrow, but I'm generally in favor of the way NewContainer was split up. Still a little iffy on copying over the OCI config... I need to check, but there shouldn't be much in there that isn't deterministically generated, so it might not be necessary. We do need to make sure that the ContainerConfig we copied over makes sense before we use it - if the container is in a pod, we need a pod with the same ID present on the remote host, and the same holds for named volumes. We might want to make a generic sanity checker for ContainerConfig that we can use for both NewContainer and this. |
☔ The latest upstream changes (presumably #2252) made this pull request unmergeable. Please resolve the merge conflicts. |
0ba0da7
to
af784d5
Compare
Signed-off-by: Adrian Reber <[email protected]>
Signed-off-by: Adrian Reber <[email protected]>
Signed-off-by: Adrian Reber <[email protected]>
If restoring a container from a checkpoint it was necessary that the image the container is based was already available (podman pull). This commit adds the image download to podman container restore if it does not exist. Signed-off-by: Adrian Reber <[email protected]>
I think I was able to implememt all the changes from the latest review, let's see if the tests still pass. |
49b624d
to
0fc0218
Compare
The option to restore a container from an external checkpoint archive (podman container restore -i /tmp/checkpoint.tar.gz) restores a container with the same name and same ID as id had before checkpointing. This commit adds the option '--name,-n' to 'podman container restore'. With this option the restored container gets the name specified after '--name,-n' and a new ID. This way it is possible to restore one container multiple times. If a container is restored with a new name Podman will not try to request the same IP address for the container as it had during checkpointing. This implicitly assumes that if a container is restored from a checkpoint archive with a different name, that it will be restored multiple times and restoring a container multiple times with the same IP address will fail as each IP address can only be used once. Signed-off-by: Adrian Reber <[email protected]>
All tests green again (after a few retries). |
Any further comments regarding this PR? |
Sorry, we've been in a bit of a rush trying to get the recent CVE patched. I'm good to merge with one more LGTM |
/lgtm |
Thanks everyone for the reviews and the patience getting this merged. |
// a container from one host to another | ||
It("podman checkpoint container with export (migration)", func() { | ||
// CRIU does not work with seccomp correctly on RHEL7 | ||
session := podmanTest.Podman([]string{"run", "-it", "--security-opt", "seccomp=unconfined", "-d", ALPINE, "top"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @adrianreber, I was wondering what is the reason for seccomp=unconfined
, is there a GitHub issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rst0git : This is a RHEL7 - CRIU limitation. CRIU cannot handle seccomp with the RHEL7 kernel. I never really tried to understand why it does not work, but is does not work on the RHEL7 kernel. I was using RHEL7 as a development platform initially, that is why I worked around the seccomp limitations there. Not sure it is necessary to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation Adrian! I was curious because the seccomp support seems to work on Fedora.
Various small fixes to get BATS tests working again. Split from containers#2947 because that one keeps getting stalled, and I'm hoping these separate changes get approved. I consider these changes urgent because RHEL8 gating tests are failing, and will fail even more if/when containers#2272 gets picked up and packaged for RHEL8, and I consider it important to have clean passing tests for RHEL8. * info test: 'insecure registries' is gone. A recent commit (d1a7378) changed the format of 'podman info', removing the 'insecure registries' key. Deal with it. * info test: remove check for .host.{Conmon,OCIRuntime}.package; the value on f28 and f29 is 'Unknown' (instead of an NVR). We can live without this check. * 'load' test: skip when running in CI, because stdin is not a tty. * container restore: fix arg processing. containers#2272 broke argument processing: 'podman container restore', with no args, should exit with 'argument required' error. Root cause is that the new --import option takes the place of an argument, so the checkAllAndLatest() call had to be changed to not exit on error. Workaround is (sigh) to copy/paste the skipped checkAllAndLatest() code, with minor tweaks to accommodate --import. Signed-off-by: Ed Santiago <[email protected]>
This series adds container migration support to Podman.
The basic steps to migrate containers are:
For the newly added test to work an updated runc is required, which is still under review: opencontainers/runc#1968
Tries to solve: #1618
This PR includes the actual code, man-pages, bash-completion, tests, tutorial update.
Once this is merged I would also like to publish a related article on podman.io.