Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

shimv2: remove use containerd ns as netns#1722

Merged
devimc merged 1 commit intokata-containers:masterfrom
Ace-Tang:rm-ns
May 22, 2019
Merged

shimv2: remove use containerd ns as netns#1722
devimc merged 1 commit intokata-containers:masterfrom
Ace-Tang:rm-ns

Conversation

@Ace-Tang
Copy link
Copy Markdown
Contributor

//the network namespace created by cni plugin
netns, err = namespaces.NamespaceRequired(ctx)
if err != nil {
        return nil, errors.Wrap(err, "create namespace")
}

the netns is a containerd namespace concept, it not netns, event a cni
set netns for this, this is a tricky way, so remove the logic.

Fixes: #1692

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

}

if n.Path == "" {
n.Path = netns
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just say,this code is wrong, n is a temporary value, so n.Path is not changed

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good catch @Ace-Tang !!

@lifupan
Copy link
Copy Markdown
Member

lifupan commented May 22, 2019

@Ace-Tang Thanks for the fix, I'm fine with this change once you fixed the travis failure.

```
//the network namespace created by cni plugin
netns, err = namespaces.NamespaceRequired(ctx)
if err != nil {
        return nil, errors.Wrap(err, "create namespace")
}
```

the netns is a containerd namespace concept, it not netns, event a cni
set netns for this, this is a tricky way, so remove the logic.

Fixes: #1692

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

Updated, make not take effect since containerd-shim-kata-v2 binary exist

@lifupan
Copy link
Copy Markdown
Member

lifupan commented May 22, 2019

/test

}

if n.Path == "" {
n.Path = netns
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good catch @Ace-Tang !!

@devimc devimc merged commit 618ae4d into kata-containers:master May 22, 2019
@Ace-Tang Ace-Tang deleted the rm-ns branch May 23, 2019 06:52
This was referenced Jun 3, 2019
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.

Miss use containerd namespace as netns ?

5 participants