Skip to content
This repository has been archived by the owner on Apr 3, 2018. It is now read-only.

Run cc-shim in the network namespace of pod #618

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

miaoyq
Copy link
Contributor

@miaoyq miaoyq commented Feb 13, 2018

Now, the container pid that is get by docker inspect -f '{{.State.Pid}}' [container_id] is the pid of shim process, and the shim process is in the host netwok namespace.

In some case, we should get the netwok namespace of container via the container pid (e.g. dockershim),
This PR moves the shim process into the pod network namespace like VM procewss.

Fixes #615
Related to clearcontainers/runtime#987 (comment)

/cc @sboeuf @sameo @plutoinmii

Signed-off-by: Yanqiang Miao [email protected]

Copy link
Collaborator

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good globally but I have a few comments.
Also, could you improve your git commit by being more verbose, explaining why this change is needed and what it does. Same thing for the PR description.
The commit message is very important since I expect we can understand the reasons behind this commit without the need to go read the issue on Github.

shim.go Outdated
@@ -170,7 +170,18 @@ func prepareAndStartShim(pod *Pod, shim shim, cid, token, url string, cmd Cmd) (
Detach: cmd.Detach,
}

pid, err := shim.start(*pod, shimParams)
fs := &filesystem{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed since the pod structure holds the storage interface.

shim.go Outdated
@@ -170,7 +170,18 @@ func prepareAndStartShim(pod *Pod, shim shim, cid, token, url string, cmd Cmd) (
Detach: cmd.Detach,
}

pid, err := shim.start(*pod, shimParams)
fs := &filesystem{}
netNS, err := fs.fetchPodNetwork(pod.ID())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with:

netNS, err := pod.storage.fetchPodNetwork(pod.ID())

shim.go Outdated
var shimErr error
pid, shimErr = shim.start(*pod, shimParams)
return shimErr
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good but you could simplify it this way:

if err := pod.network.run(netNS.NetNsPath, func() (shimErr error) {
        pid, shimErr = shim.start(*pod, shimParams)
}); err != nil {
        return nil, err
}

@miaoyq
Copy link
Contributor Author

miaoyq commented Feb 13, 2018

@sboeuf Thanks for your review. Will address your comments.

@miaoyq
Copy link
Contributor Author

miaoyq commented Feb 13, 2018

@sboeuf Done. PTAL

@sameo
Copy link
Collaborator

sameo commented Feb 13, 2018

LGTM

Approved with PullApprove Approved with PullApprove

@sboeuf sboeuf merged commit 27e2964 into containers:master Feb 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants