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

Don't append the container id to custom directory checkpoints. #35694

Merged
merged 1 commit into from Jan 3, 2018

Conversation

Projects
None yet
7 participants
@boucher
Contributor

boucher commented Dec 4, 2017

- What I did

Issue #34601 documents a change in the way the --checkpoint-dir flag works, breaking the behavior for people using it. This fixes that issue.

- How I did it

Simply don't append the container id to the checkpoint path.

- How to verify it

You can verify by building and creating a custom dir checkpoint, and seeing that the resulting folder isn't nested inside one with the container's id.

Unfortunately, implementing this fix highlighted a new problem:
ddae20c#diff-3cb140026df40998ea29c5bcb6bb292eR118

As of the above commit, restoring from a custom directory is no longer supported anyway. So, I'm not really sure what to do. If a subsequent release of pre containerd 1.0 docker is going to be created it would be nice to get this fix in I think.

Don't append the container id to custom directory checkpoints. Fixes #…
…34601.

Signed-off-by: Ross Boucher <rboucher@gmail.com>
@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Dec 5, 2017

Contributor

Two obvious options are:

  • drop --checkpoint-dir entirely
  • fix the support in containerd (not sure what needs to be done)
Contributor

kolyshkin commented Dec 5, 2017

Two obvious options are:

  • drop --checkpoint-dir entirely
  • fix the support in containerd (not sure what needs to be done)
@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Dec 5, 2017

Contributor

power failure seems unrelated but I'm looking at it

Contributor

kolyshkin commented Dec 5, 2017

power failure seems unrelated but I'm looking at it

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Dec 20, 2017

Contributor

Oups, it's actually doable if a little convoluted to support from external directory. One would need to create the needed snapshot/manifest into containerd then provide that to the runtime when restoring.

I thought I had done so, the commit must have been lost during one of my rebase 😓

Contributor

mlaventure commented Dec 20, 2017

Oups, it's actually doable if a little convoluted to support from external directory. One would need to create the needed snapshot/manifest into containerd then provide that to the runtime when restoring.

I thought I had done so, the commit must have been lost during one of my rebase 😓

@mlaventure

LGTM

As for restoring from that directory, Me or another volunteer would have to handle the TODO I left in the code. When I get enough cycle I'll tackle it, but I have a lot less of them to dedicate to docker nowadays 😅

@tswift242

This comment has been minimized.

Show comment
Hide comment
@tswift242

tswift242 Dec 20, 2017

Contributor

@mlaventure I might have some time coming up to help fix the C/R from a custom directory use case. How large in scope is the work to address the TODO you mentioned? I have not yet contributed to docker/containerd, but I could take a crack at it if I got some pointers.

Contributor

tswift242 commented Dec 20, 2017

@mlaventure I might have some time coming up to help fix the C/R from a custom directory use case. How large in scope is the work to address the TODO you mentioned? I have not yet contributed to docker/containerd, but I could take a crack at it if I got some pointers.

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Dec 21, 2017

Contributor

@tswift242 should be pretty straightforward, the code to do so is here: https://github.com/containerd/containerd/blob/967caeeaccac497c6708d84f6c3c0c8205bda481/services/tasks/service.go#L439-L456 (minus the call to checkpoint).

It creates a snapshot in containerd and the resulting reference can be used as a checkpoint

Contributor

mlaventure commented Dec 21, 2017

@tswift242 should be pretty straightforward, the code to do so is here: https://github.com/containerd/containerd/blob/967caeeaccac497c6708d84f6c3c0c8205bda481/services/tasks/service.go#L439-L456 (minus the call to checkpoint).

It creates a snapshot in containerd and the resulting reference can be used as a checkpoint

@tswift242

This comment has been minimized.

Show comment
Hide comment
@tswift242

tswift242 Jan 3, 2018

Contributor

Can we get this PR merged? Is anything blocking? Also, is there any chance this could be backported to 17.09.x? I believe that's before the containerd 1.0 integration, and so shouldn't have the new bug @boucher mentioned.

I haven't had time yet to take a look at the containerd code related to the other issue. @boucher do you have time to look at the other issue? And should we create a separate github issue for tracking?

Contributor

tswift242 commented Jan 3, 2018

Can we get this PR merged? Is anything blocking? Also, is there any chance this could be backported to 17.09.x? I believe that's before the containerd 1.0 integration, and so shouldn't have the new bug @boucher mentioned.

I haven't had time yet to take a look at the containerd code related to the other issue. @boucher do you have time to look at the other issue? And should we create a separate github issue for tracking?

@cpuguy83

LGTM

@cpuguy83 cpuguy83 merged commit f72b180 into moby:master Jan 3, 2018

5 of 6 checks passed

powerpc Jenkins build Docker-PRs-powerpc 7658 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38172 has succeeded
Details
janky Jenkins build Docker-PRs 46889 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18438 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7113 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment