[WIP] revive config.json for docker checkpoints by lifubang · Pull Request #38028 · moby/moby

lifubang · 2018-10-12T10:21:54Z

Signed-off-by: Lifubang [email protected]

- What I did

root@dockerdemo:~/checkpoints# docker checkpoint ls redis2
Error response from daemon: open /var/lib/docker/containers/8b5ff6aa250b78dd8a672ee60ff239afda716e4bedc3f77acb4a70c4535b3eba/checkpoints/redis2-1/config.json: no such file or directory

- How I did it
After this commit (Update libcontainerd to use containerd 1.0): ddae20c#diff-d2fd8e9feab75f193a7d19493831c1f1
There is no "config.json" now.
So, when checkpoint ls, there will be an error.
Just create it, because all value in "config.json" is from daemon.

- How to verify it
After fix:

root@dockerdemo:~/checkpoints# docker checkpoint ls redis2
CHECKPOINT NAME
redis2-2

- Description for the changelog
Create config.json after checkpoint succ.

codecov · 2018-10-12T12:42:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@bc4c1c2). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38028   +/-   ##
=========================================
  Coverage          ?   36.11%           
=========================================
  Files             ?      610           
  Lines             ?    45225           
  Branches          ?        0           
=========================================
  Hits              ?    16334           
  Misses            ?    26650           
  Partials          ?     2241

AkihiroSuda · 2018-10-16T11:13:44Z

daemon/checkpoint.go

Please define new struct, because api/types is for REST API

For now, "docker checkpoint ls" just return "Checkpoint name", I think we need more useful information.
So, my original idea is that after this patch, "docker checkpoint ls " should print more information. such as:
CheckpintName, Create Time, Exit
So I reuse "types.Checkpoint".

Do you still think that we need to define a new struct?
If yes, I'll redefine it.

Ok, thank you for your review.
For one PR do one thing, I defined a new struct.

thaJeztah · 2018-10-19T10:30:30Z

daemon/checkpoint.go

If this type is (at least currently) only used locally, perhaps it should be un-exported.

Thank you.
I should be careful every time. Don't just copy & paste.

thaJeztah · 2018-10-23T13:50:50Z

daemon/checkpoint.go

It just occurred to me, but does this play nice with custom checkpoint dirs? (i.e.; should it remove the directory in that case?)

Yeah, I thought about it when I was writing this code. I don't know we should delete or just return the error. I just choose to delete it at that time. But now, with your opinion, I think we should remain that dir. I'll remove this line.

The alternative would be to check if we created the directory, but that would be dangerous (there could be a race-condition where we checked if it existed before, but meanwhile another session is creating a checkpoint (and the checkpoint dir))

how about add an option --save-whenfail?
Whether save any incomplete files or not when fail in checkpoint create process.
The default value is true, if set to false, remove the dir when fail.

Hm, no, I don't think we should add an option for this; also in light of future work for checkpoints is being worked on on containerd.

For https://github.com/containerd/containerd/blob/06b9cb35161009dcb7123345749fef02f7cea8e0/runtime/container.go#L367
If write config.json fail, it just return the error.
I think there is very very small opportunity to meet this error, so we just return the error without remove the dir?
If yes, I'll remove this line, and modify the checkpoint list func to ignore this type folder because there is no config.json in it(very very small opportunity).

Or move create config.json to the position before call daemon.containerd.CreateCheckpoint?

@thaJeztah I have moved the code position, and the checkpoint dir is always created by dockerd, not created by the user(

moby/daemon/checkpoint.go

Line 44 in 502b6e6

err2 = os.MkdirAll(checkpointAbsDir, 0700)

), so it will not delete user's existing dir.

Signed-off-by: Lifubang <[email protected]>

rst0git · 2018-12-17T12:05:58Z

daemon/checkpoint.go

 		return fmt.Errorf("cannot checkpoint container %s: %s", name, err)
 	}

+	checkpointInfo := checkPoint{


Could you add a space before the curly bracket, please?

If I add a space, gofmt will remove it.

And I prefer to add a space, it looks more pretty. But we need to upgrade gofmt.

rst0git · 2018-12-17T13:05:43Z

@lifubang Thank you for the PR! I can confirm that this commit solves the problem with docker checkpoint ls and the newly introduced structure checkPoint could be used to extend this functionality to display more useful information about the checkpoints in the future.

avagin · 2018-12-31T01:13:55Z

daemon/checkpoint.go

 )

+// checkPoint represents the details of a checkpoint
+type checkPoint struct {


Why do you not use types.Checkpoint?
https://github.com/moby/moby/blob/master/api/types/types.go#L502
https://github.com/moby/moby/blob/master/daemon/checkpoint.go#L135

Because we need to add some fields for types.Checkpoint. And AkihiroSuda tell me I should define a new struct, I accept his suggestion. Please see the first review msg at the top of this page. Thanks.

In this case, you have to fix CheckpointList too

kolyshkin · 2019-03-21T22:03:30Z

Since #38452 is merged, this is no longer needed to fix docker checkpoint ls, so it's more like an improvement. Adding WIP to the title -- feel free to remove once it's ready.

kolyshkin · 2019-03-21T22:07:21Z

To finish this, I guess CheckpointList need to be modified to show the new values you're adding to config.json, as pointed out by @avagin earlier.

Closing for now, feel free to reopen if you want to finish it.

GordonTheTurtle added the status/0-triage label Oct 12, 2018

yongtang requested a review from thaJeztah October 12, 2018 14:05

yongtang added status/2-code-review and removed status/0-triage labels Oct 12, 2018

AkihiroSuda reviewed Oct 16, 2018

View reviewed changes

lifubang force-pushed the checkpointconfig branch from c54b23e to 8b01e7f Compare October 17, 2018 03:45

thaJeztah reviewed Oct 19, 2018

View reviewed changes

lifubang force-pushed the checkpointconfig branch from 8b01e7f to ae58625 Compare October 19, 2018 11:31

thaJeztah reviewed Oct 23, 2018

View reviewed changes

lifubang force-pushed the checkpointconfig branch from ae58625 to 502b6e6 Compare October 25, 2018 06:59

checkpoint config.json lost

502b6e6

Signed-off-by: Lifubang <[email protected]>

GordonTheTurtle assigned tiborvass Oct 27, 2018

rst0git reviewed Dec 17, 2018

View reviewed changes

derek bot added the rebuild/* label Dec 19, 2018

GordonTheTurtle removed the rebuild/* label Dec 19, 2018

rst0git mentioned this pull request Dec 30, 2018

integration/container: add a base test for C/R #38452

Merged

avagin reviewed Dec 31, 2018

View reviewed changes

kolyshkin changed the title ~~fixes checkpoint config.json lost~~ [WIP] revive config.json for docker checkpoints Mar 21, 2019

kolyshkin closed this Mar 21, 2019

Conversation

lifubang commented Oct 12, 2018

Uh oh!

codecov bot commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifubang Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifubang Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rst0git commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 21, 2019

Uh oh!

kolyshkin commented Mar 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov bot commented Oct 12, 2018 •

edited

Loading

lifubang Oct 16, 2018 •

edited

Loading

lifubang Oct 25, 2018 •

edited

Loading

rst0git commented Dec 17, 2018 •

edited

Loading