Skip to content
This repository has been archived by the owner on Sep 24, 2021. It is now read-only.

Cni integration #62

Merged
merged 5 commits into from
Jan 19, 2017
Merged

Cni integration #62

merged 5 commits into from
Jan 19, 2017

Conversation

YaoZengzeng
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 6, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@YaoZengzeng YaoZengzeng mentioned this pull request Jan 6, 2017
Closed
@YaoZengzeng YaoZengzeng force-pushed the cni-integration branch 6 times, most recently from 39081a9 to f3f3c38 Compare January 6, 2017 11:57
feiskyer
feiskyer reviewed Jan 10, 2017
View reviewed changes
pkg/hyper/hyper.go
@@ -23,6 +23,7 @@ import (
"github.com/golang/glog"
"github.com/golang/protobuf/proto"

"github.com/rajatchopra/ocicni"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also moves this to a local packages? It will be moved into cri-o, refer here.

@feiskyer feiskyer self-assigned this Jan 10, 2017
@YaoZengzeng YaoZengzeng force-pushed the cni-integration branch 2 times, most recently from bf9fecd to 193bde2 Compare January 10, 2017 09:10
feiskyer
feiskyer reviewed Jan 10, 2017
View reviewed changes
pkg/hyper/ocicni/types.go
// SetUpPod is the method called after the infra container of
// the pod has been created but before the other containers of the
// pod are launched.
SetUpPod(netnsPath string, namespace string, name string, containerID string) error
Copy link
Contributor

Choose a reason for hiding this comment

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

what is namespace for? I don't think it is required.

@feiskyer
Copy link
Contributor

feiskyer commented Jan 10, 2017 •
edited
Loading

We need also checkpoint network info same with dockershim, refer proposal here. It could be done by another PR.

Also updated follow-up todos at #58.

@resouer resouer mentioned this pull request Jan 10, 2017
Closed
5 tasks
feiskyer
feiskyer reviewed Jan 10, 2017
View reviewed changes
pkg/hyper/hyper.go
@@ -60,7 +62,18 @@ func NewHyperRuntime(hyperEndpoint string, streamingConfig *streaming.Config) (*
}
}

return &Runtime{client: hyperClient, streamingServer: streamingServer}, streamingServer, nil
netPlugin, err := ocicni.InitCNI("")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make pluginDir and cniDir configurable via frakti flags.

feiskyer
feiskyer suggested changes Jan 10, 2017
View reviewed changes
pkg/hyper/sandbox.go
ifaces = append(ifaces, &types.UserInterface{
Ifname: fmt.Sprintf("eth%d", seq),
Bridge: bridge,
Ip: iface.Ip,
Copy link
Contributor

Choose a reason for hiding this comment

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

missing mac and gateway here.

@YaoZengzeng YaoZengzeng force-pushed the cni-integration branch 3 times, most recently from 969814f to d0df8d4 Compare January 13, 2017 12:05
@feiskyer
Copy link
Contributor

feiskyer commented Jan 13, 2017 •
edited
Loading

@YaoZengzeng Why includes commits from master? rebase error?

@YaoZengzeng
Copy link
Contributor Author

Already fix rebase error @feiskyer

feiskyer
feiskyer suggested changes Jan 14, 2017
View reviewed changes
pkg/hyper/network.go
Ifaces []*InterfaceInfo
}

type InterfaceInfo struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use full name instead of short one

pkg/hyper/network.go

mac := link.Attrs().HardwareAddr.String()

addrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a TODO comment for supporting v6 in the future

pkg/hyper/network.go
mac := link.Attrs().HardwareAddr.String()

addrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Log error here for easy tracking problems

pkg/hyper/network.go

gw := ""
routes, err := netlink.RouteList(link, netlink.FAMILY_V4)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, log error message

pkg/hyper/network.go
continue
}
for _, route := range routes {
if route.Gw != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comments of losing routes here, we may add them back in the future

pkg/hyper/ocicni/ocicni.go
}

func (plugin *cniNetworkPlugin) syncNetworkConfig() {
network, err := getDefaultCNINetwork(plugin.netDir, plugin.pluginDirs, plugin.vendorCNIDirPrefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we support multiple networks?

pkg/hyper/ocicni/ocicni.go
if err != nil {
glog.Errorf("Error while adding to cni lo network: %s", err)
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a info log of CNI response

pkg/hyper/ocicni/ocicni.go
if err != nil {
glog.Errorf("Error while adding to cni network: %s", err)
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a info log of CNI response

pkg/hyper/ocicni/ocicni.go
glog.V(4).Infof("About to run with conf.Network.Type=%v", netconf.Network.Type)
res, err := cninet.AddNetwork(netconf, rt)
if err != nil {
glog.Errorf("Error adding network: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

also add podID and netns in error logs (and also other places)

pkg/hyper/sandbox.go
@@ -51,6 +79,28 @@ func (h *Runtime) RunPodSandbox(config *kubeapi.PodSandboxConfig) (string, error
return podID, nil
}

func addNetNsInfos2UserPod(userpod *types.UserPod, info *NetNsInfos) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only bridge plugin is supported in this PR, add a TODO of supporting other plugins

YaoZengzeng and others added 4 commits January 18, 2017 16:32
@YaoZengzeng
integration with cni, add network enabled
9f206e3 
Signed-off-by: Yao Zengzeng <yaozengzeng@zju.edu.cn>
@YaoZengzeng
integration with cni, del network enabled
d67c5da 
Signed-off-by: Yao Zengzeng <yaozengzeng@zju.edu.cn>
@YaoZengzeng
add our own ocicni library
2ce139b 
Signed-off-by: Yao Zengzeng <yaozengzeng@zju.edu.cn>
@YaoZengzeng
update vendor
8c84250 
Signed-off-by: YaoZengzeng <zengzengyao@harmonycloud.cn>
@YaoZengzeng
set flag for cniNetDir and cniPluginDir
34d82e2 
Signed-off-by: Yao Zengzeng <yaozengzeng@zju.edu.cn>
@YaoZengzeng
Copy link
Contributor Author

Ready for review again @feiskyer

@feiskyer feiskyer mentioned this pull request Jan 18, 2017
Closed
@feiskyer
Copy link
Contributor

LGTM. Thanks.

@feiskyer feiskyer merged commit 2d6533d into kubernetes-retired:master Jan 19, 2017
@resouer resouer mentioned this pull request Feb 13, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@feiskyer feiskyer feiskyer approved these changes

Assignees

@feiskyer feiskyer

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@YaoZengzeng @k8s-reviewable @feiskyer @k8s-ci-robot