feature: support checkpoint #1740

Ace-Tang · 2018-07-16T07:41:48Z

support checkpoint create,list,delete

Signed-off-by: Ace-Tang aceapril@126.com

Ⅰ. Describe what this PR did

support funtion

pouch checkpoint create
create a checkpoint from a running container using criu.
pouch checkpoint ls
list a container all checkpoints.
pouch checkpoint rm
delete a container checkpoint.
pouch start --checkpoint
restore a container from a checkpoint image.

support checkpoint in #1723.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

chuanchang · 2018-07-16T09:34:36Z

----------------------------------------------------------------------
FAIL: /go/src/github.com/alibaba/pouch/test/api_checkpoint_test.go:35: APIContainerCheckpointSuite.TestCheckpointCreateAPI
/go/src/github.com/alibaba/pouch/test/api_checkpoint_test.go:48:
    c.Assert(err, check.IsNil)
... value *url.Error = &url.Error{Op:"Post", URL:"http://d/v1.24/containers/TestCheckpointCreateAPI/checkpoints", Err:(*errors.errorString)(0xc42004a060)} ("Post http://d/v1.24/containers/TestCheckpointCreateAPI/checkpoints: EOF")
----------------------------------------------------------------------
FAIL: /go/src/github.com/alibaba/pouch/test/api_checkpoint_test.go:95: APIContainerCheckpointSuite.TestCheckpointDelAPI
/go/src/github.com/alibaba/pouch/test/api_checkpoint_test.go:107:
    c.Assert(err, check.IsNil)
... value *url.Error = &url.Error{Op:"Post", URL:"http://d/v1.24/containers/TestCheckpointDelAPI/checkpoints", Err:(*errors.errorString)(0xc42004a060)} ("Post http://d/v1.24/containers/TestCheckpointDelAPI/checkpoints: EOF")
----------------------------------------------------------------------

Ace-Tang · 2018-07-16T10:42:01Z

It's weird that all failed tests were passed on my machine.

Ace-Tang · 2018-07-16T10:43:02Z

Oh, I Got it, it miss criu on test machine, it should be test first.

codecov-io · 2018-07-16T11:33:56Z

Codecov Report

Merging #1740 into master will increase coverage by 4.79%.
The diff coverage is 31.65%.

@@            Coverage Diff             @@
##           master    #1740      +/-   ##
==========================================
+ Coverage   58.64%   63.44%   +4.79%     
==========================================
  Files         206      207       +1     
  Lines       15840    16035     +195     
==========================================
+ Hits         9289    10173     +884     
+ Misses       5413     4588     -825     
- Partials     1138     1274     +136

Flag	Coverage Δ
#criv1alpha1test	`32.88% <11%> (-0.32%)`	⬇️
#criv1alpha2test	`33.41% <11%> (?)`
#integrationtest	`37.74% <25.22%> (-0.2%)`	⬇️
#unittest	`23.46% <4.58%> (-0.27%)`	⬇️

Impacted Files	Coverage Δ
cri/v1alpha1/cri.go	`65.36% <100%> (ø)`	⬆️
apis/server/router.go	`91.6% <100%> (+0.17%)`	⬆️
cri/v1alpha2/cri.go	`66.02% <100%> (+66.02%)`	⬆️
ctrd/container.go	`43.5% <18.07%> (-7.23%)`	⬇️
daemon/mgr/container_checkpoint.go	`22.78% <22.78%> (ø)`
daemon/mgr/container.go	`55.28% <53.33%> (-0.24%)`	⬇️
apis/server/container_bridge.go	`78.84% <61.76%> (-2.74%)`	⬇️
apis/server/utils.go	`61.9% <0%> (-4.77%)`	⬇️
daemon/logger/jsonfile/utils.go	`71.54% <0%> (-1.63%)`	⬇️
... and 12 more

allencloud · 2018-07-16T13:19:19Z

Please add a proposal in the issue list first. This is a big big feature in PouchContainer, we must to explain the feature demand much more in issue #1723. Thanks a lot.

After maintainers review you proposal well, we start the code review part. Thanks for your cooperation. @Ace-Tang 🌹 ❤️

xiechengsheng · 2018-07-16T14:25:45Z

apis/server/router.go

@@ -33,6 +33,9 @@ func initRoute(s *Server) http.Handler {
 	s.addRoute(r, http.MethodPost, "/daemon/update", s.updateDaemon)

 	// container
+	s.addRoute(r, http.MethodPost, "/containers/{name:.*}/checkpoints", s.createContainerCheckpoint)


we could add the withCancelHandler to wrap s.createContainerCheckpoint, just in case that client closes connection without patience~

xiechengsheng · 2018-07-16T14:26:48Z

cli/checkpoint.go

+	apiClient := cc.cli.Client()
+
+	if err := apiClient.ContainerCheckpointCreate(ctx, args[0], &types.CheckpointCreateOptions{
+		CheckpointID: args[1],


should we add checkpoint dir here so that client could config checkpoint direction themselves? Because client may want to migrate container to another machine, they would like to send whole checkpoint direction to another machine~

Yes, we should consider it, but It will be done later.

xiechengsheng · 2018-07-16T14:26:57Z

cli/checkpoint.go

+func (cc *CheckpointDelCommand) Init(c *Cli) {
+	cc.cli = c
+	cc.cmd = &cobra.Command{
+		Use:   "delete [OPTIONS] CONTAINER CHECKPOINT",


Could we use pouch checkpoint rm instead of pouch checkpoint delete here to stay the same with removing containers/images?

I think checkpint is a independent function, why we should keep same with a command name.

maybe the rm can save time.

I am doing a bash complement for all pouch command, do it save your time

could you share your bash complement script? 😄

doing... not done, it will pushed as soon as it finished.

xiechengsheng · 2018-07-16T14:27:09Z

daemon/mgr/container.go

+	if err := mgr.Client.CreateCheckpoint(ctx, c.ID, dir, options.Exit); err != nil {
+		return err
+	}
+


After checkpointing containers successfully, we should set this container's status to checkpointed. And this status is different from paused.

chekpoint is not a status.

Ace-Tang · 2018-07-17T01:40:00Z

@allencloud , of course, we should do proposal first, consider for my time, I will finish this as soon as possible.

allencloud · 2018-07-23T05:58:55Z

Do we need to install additional packages related to criu when experiencing this feature? @Ace-Tang

We need to consider more about the doc parts.

allencloud · 2018-07-31T03:17:48Z

Do we need to add more details about the document part on how to install CRIU? @Ace-Tang

Ace-Tang · 2018-07-31T15:07:29Z

I do not think the CRIU installtion doc should stop the pr from reviewing, by the way, you can suggest me to add a doc @allencloud . Since the pr is hold so long, can someone do the review for the code? thanks, @alibaba/pouch

allencloud · 2018-08-01T01:57:21Z

apis/swagger.yml

@@ -3582,6 +3582,17 @@ definitions:
          items:
            type: "string"

+  CheckpointCreateOptions:


We need to add more API side details for all these APIs.

like:

/images/create: post: summary: "Create an image by pulling from a registry or importing from an existing source file" consumes:

Already done, @allencloud

allencloud

Need more API routes description in swagger for all these APIs.

xiechengsheng · 2018-08-01T07:20:51Z

daemon/mgr/container_checkpoint.go

+		return err
+	}
+
+	if err := mgr.Client.CreateCheckpoint(ctx, c.ID, dir, options.Exit); err != nil {


If options.Exit is true, the container would be stopped by checkpoint process. Do we need to add a markStoppedAndRelease here to set stopped status of this container and release some resources?

Yes, I am not consider that.

@xiechengsheng , I re-think the problem, you may not correct for this, markStoppedAndRelease will called as soon as container task exit, it is running in a goroutine, called by received containerd exit channel.

I'm sorry but could you tell me which goroutine runs this markStoppedAndRelease function? 😄

https://github.com/alibaba/pouch/blob/master/ctrd/watch.go#L71

xiechengsheng · 2018-08-01T07:24:53Z

test/cli_checkpoint_test.go

+}
+
+// TestCreate tests create checkpoint
+func (suite *PouchCheckpointSuite) TestCheckpointCreate(c *check.C) {


Could we add one test case TestCheckpointCreateWithExitFlag to checkpoint and stop one container, then check the stopped status and the checkpoint of this stopped container?

already add.

xiechengsheng · 2018-08-01T07:27:22Z

test/api_checkpoint_test.go

+}
+
+// TestCheckpointCreateAPI tests checkpoint create api
+func (suite *APIContainerCheckpointSuite) TestCheckpointCreateAPI(c *check.C) {


Could we add one test case TestCheckpointCreateAPIWithExitFlag to checkpoint and stop one container, then check the stopped status and the checkpoint of this stopped container?

fuweid · 2018-07-31T15:47:10Z

cli/checkpoint.go

+func (cc *CheckpointDelCommand) Init(c *Cli) {
+	cc.cli = c
+	cc.cmd = &cobra.Command{
+		Use:   "delete [OPTIONS] CONTAINER CHECKPOINT",


maybe the rm can save time.

fuweid · 2018-08-01T07:01:42Z

client/checkpoint_create_test.go

+		if !strings.HasPrefix(req.URL.Path, expectedURL) {
+			return nil, fmt.Errorf("expected URL '%s', got '%s'", expectedURL, req.URL)
+		}
+		if req.Header.Get("Content-Type") == "application/json" {


if the Content-Type doesn't equal application/json, the case should fail, right?

I re-think this problem, maybe it also can accepted non-json type? @fuweid

fuweid · 2018-08-01T07:16:17Z

ctrd/container.go

 	"github.com/containerd/containerd/leases"
 	"github.com/containerd/containerd/linux/runctypes"
 	"github.com/containerd/containerd/oci"
 	"github.com/docker/docker/pkg/stdcopy"
+	"github.com/opencontainers/image-spec/specs-go/v1"


could we use imagespec instead of v1 ? v1 doesn't contain any useful information.

fuweid · 2018-08-01T07:20:43Z

ctrd/container.go

+		return fmt.Errorf("failed to checkpoint: %s", err)
+	}
+	defer client.ImageService().Delete(ctx, checkpoint.Name())
+


could we split it into one function? for example, the named getCheckpointImageDescriptor function handled cpDesc.

fuweid · 2018-08-01T07:22:01Z

daemon/mgr/container_checkpoint.go

+	if id == "" {
+		return "", fmt.Errorf("checkpoint can not be empty")
+	}
+	dir := filepath.Join(mgr.Config.HomeDir, "containerd/state", "io.containerd.runtime.v1.linux", mgr.Config.Namespace, container, "checkpoint", id)


could we make it as function?

yyb196 · 2018-08-08T14:06:58Z

LGTM, except that draft a simple example describing how to install criu utils and create/list/delete checkpoints, how to quickly restore a container using a checkpoint.

fuweid · 2018-08-10T02:04:15Z

LGTM @allencloud please take a look.

allencloud · 2018-08-10T02:14:34Z

apis/server/container_bridge.go

+		return err
+	}
+
+	rw.WriteHeader(http.StatusOK)


How about changing this to status code of StatusCreated(201)?

allencloud · 2018-08-10T02:15:55Z

apis/swagger.yml

@@ -3559,6 +3559,17 @@ definitions:
      Width:
        type: "integer"

+  ContainerStartOptions:


I have not seen any API routes definition. Maybe in a follow-up pull request?

allencloud · 2018-08-10T02:28:09Z

daemon/mgr/container_checkpoint.go

+		case err != nil && os.IsNotExist(err):
+			return checkpointDir, os.MkdirAll(checkpointDir, 0700)
+		case err != nil:
+			return "", fmt.Errorf("failed to create checkpoint %s", checkpointID)


Why not wrap the err into the returned error?

allencloud · 2018-08-10T02:30:01Z

daemon/mgr/container_checkpoint.go

+)
+
+// getCheckpointDir gets container checkpoint directory.
+func (mgr *ContainerManager) getCheckpointDir(container, prefixDir, checkpointID string, create bool) (string, error) {


In the implementation of this function, I am wondering if we need to add more dedicated error instance when failed. Since these errors in errtypes packages could be used to construct the status codes for API layers. WDYT?

allencloud · 2018-08-10T02:31:55Z

daemon/mgr/container_checkpoint.go

+	}
+
+	if c.State.Status != types.StatusRunning {
+		return fmt.Errorf("can not checkpoint from a %s container", c.State.Status)


return errors.Wrapf(errtypes.ErrConflict, "can not checkpoint from a %s container", c.State.Status)
Now we have no ErrConflict, maybe we need to add one.

implementing checkpoint functionality, it will checkpointing the address space and state of the entire process tree to a collection of “image” files. This functionality need a external software criu. support pouch checkpoint create, list and delete. implement checkpoint in #1723 Signed-off-by: Ace-Tang <aceapril@126.com>

Signed-off-by: Ace-Tang <aceapril@126.com>

done

allencloud · 2018-08-10T05:44:14Z

LGTM

allencloud · 2018-08-10T05:59:24Z

client/container_start.go

-	}
-
-	resp, err := client.post(ctx, "/containers/"+name+"/start", q, nil, nil)
+func (client *APIClient) ContainerStart(ctx context.Context, name string, options types.ContainerStartOptions) error {


I am afraid this change has modified the API, we need to revert this part.

pouchrobot added kind/feature size/XXL labels Jul 16, 2018

Ace-Tang requested review from allencloud, HusterWan, Letty5411 and yyb196 July 16, 2018 11:47

xiechengsheng reviewed Jul 16, 2018

View reviewed changes

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jul 19, 2018

allencloud reviewed Aug 1, 2018

View reviewed changes

allencloud previously requested changes Aug 1, 2018

View reviewed changes

xiechengsheng reviewed Aug 1, 2018

View reviewed changes

fuweid reviewed Aug 1, 2018

View reviewed changes

pouchrobot added LGTM one maintainer or community participant agrees to merge the pull reuqest. conflict/needs-rebase and removed LGTM one maintainer or community participant agrees to merge the pull reuqest. conflict/needs-rebase labels Aug 8, 2018

pouchrobot removed the conflict/needs-rebase label Aug 9, 2018

allencloud reviewed Aug 10, 2018

View reviewed changes

pouchrobot added the conflict/needs-rebase label Aug 10, 2018

Ace-Tang added 3 commits August 10, 2018 11:10

support start container from a checkpoint

1a3f79c

Signed-off-by: Ace-Tang <aceapril@126.com>

checkpoint: add config file in checkpoint image

d204216

Signed-off-by: Ace-Tang <aceapril@126.com>

pouchrobot removed the conflict/needs-rebase label Aug 10, 2018

allencloud merged commit 3d16d73 into AliyunContainerService:master Aug 10, 2018

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Aug 10, 2018

allencloud reviewed Aug 10, 2018

View reviewed changes

pouchrobot mentioned this pull request Aug 11, 2018

Weekly Report of PouchContainer from 2018-08-04 to 2018-08-11 #2076

Closed

feature: support checkpoint #1740

feature: support checkpoint #1740

Conversation

Ace-Tang commented Jul 16, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

chuanchang commented Jul 16, 2018 • edited Loading

Ace-Tang commented Jul 16, 2018

Ace-Tang commented Jul 16, 2018

codecov-io commented Jul 16, 2018 • edited Loading

Codecov Report

allencloud commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ace-Tang commented Jul 17, 2018

allencloud commented Jul 23, 2018

allencloud commented Jul 31, 2018

Ace-Tang commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyb196 commented Aug 8, 2018

fuweid commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Aug 10, 2018

Choose a reason for hiding this comment

Ace-Tang commented Jul 16, 2018 •

edited

Loading

chuanchang commented Jul 16, 2018 •

edited

Loading

codecov-io commented Jul 16, 2018 •

edited

Loading