Skip to content

cr: get pid from criu notify when restore#1944

Merged
crosbymichael merged 1 commit into
opencontainers:masterfrom
Ace-Tang:criu_notify_pid
Dec 3, 2018
Merged

cr: get pid from criu notify when restore#1944
crosbymichael merged 1 commit into
opencontainers:masterfrom
Ace-Tang:criu_notify_pid

Conversation

@Ace-Tang
Copy link
Copy Markdown
Contributor

@Ace-Tang Ace-Tang commented Dec 3, 2018

when restore container from a checkpoint directory, we should get
pid from criu notify, since c.initProcess has not been created.

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

why I do this

I fail to update runc version in pouch project, AliyunContainerService/pouch#2516, I check the restore code, in criuNotifications function, currentOCIState() can not get pid since c.initProcess has not been created.

    case notify.GetScript() == "setup-namespaces":
        if c.config.Hooks != nil {
            s, err := c.currentOCIState()
            if err != nil {
                return nil
            }

I do not figure out how criu notify can get pid, I want invite @avagin and @adrianreber help to check whether the fix in pr is correct.

when restore container from a checkpoint directory, we should get
pid from criu notify, since c.initProcess has not been created.

Signed-off-by: Ace-Tang <aceapril@126.com>
@adrianreber
Copy link
Copy Markdown
Contributor

So it seems #1741 is the reason for the problem. That PR basically removes, by calling out to a new function, that s.Pid is set.

I also see in #1741 that at a few places s.Pid is set just like it is done in this PR.

So it seems that this PR is doing the right thing. 👍

@Ace-Tang
Copy link
Copy Markdown
Contributor Author

Ace-Tang commented Dec 3, 2018

Thanks @adrianreber

@cyphar
Copy link
Copy Markdown
Member

cyphar commented Dec 3, 2018

LGTM, seems reasonable.

Approved with PullApprove

@cyphar cyphar added this to the 1.0.0 milestone Dec 3, 2018
@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Dec 3, 2018

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit ff38d6e into opencontainers:master Dec 3, 2018
@Ace-Tang Ace-Tang deleted the criu_notify_pid branch December 4, 2018 01:39
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.

4 participants