Add more rich network functions #533

WeiZhang555 · 2017-07-19T09:58:53Z

Implement more rich network functions

* Support add ip addr to a specific network interface
* Support delete ip address from a network interface
* Support modify MTU
* Support add any routes into container

* Support add ip addr to a specific network interface * Support delete ip address from a network interface * Support modify MTU Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

Enhance network functions: Support add any routes into container Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

Fix bug that container panics when doing the network interface deletion work. Also modify nic delete logic a little bit, now we can delete single IP and delete link separately. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

gao-feng · 2017-08-02T10:15:45Z

hypervisor/network.go

 }

+func (nc *NetworkContext) AddIPAddr(id, ip string) error {
+	nc.slotLock.RLock()


Should not be read lock here.

You're right.

gao-feng · 2017-08-02T10:16:02Z

hypervisor/network.go

+
+func (nc *NetworkContext) DeleteIPAddr(id, ip string) error {
+	nc.slotLock.RLock()
+	defer nc.slotLock.RUnlock()


gao-feng · 2017-08-02T10:19:15Z

hypervisor/network.go

+
+	for i, v := range inf.IpAddr {
+		if v == ip {
+			inf.IpAddr = append(inf.IpAddr[:i], inf.IpAddr[i+1:]...)


what happen when the last element in ipaddr array match the ip, is the inf.IpAddr[i+1] will access the array out of range?

It should be fine, golang will handle it. See https://play.golang.org/p/5SWxZB3p2r

gao-feng · 2017-08-02T10:19:39Z

hypervisor/network.go

+
+	for i, v := range inf.Desc.Ip {
+		if v == ip {
+			inf.IpAddr = append(inf.Desc.Ip[:i], inf.Desc.Ip[i+1:]...)


gao-feng · 2017-08-02T10:20:34Z

hypervisor/network.go

+			break
+		}
+	}
+	return nil


return error or printout warning info if there is no matching ip?

gao-feng · 2017-08-02T10:23:21Z

hypervisor/network.go

+
+func (nc *NetworkContext) UpdateMtu(id string, mtu uint64) error {
+	nc.slotLock.RLock()
+	defer nc.slotLock.RUnlock()


Seems like write lock is more appropriate here

gao-feng · 2017-08-02T10:26:34Z

hypervisor/network/network.go

 const (
 	DefaultBridgeIface = "hyper0"
 	DefaultBridgeIP    = "192.168.123.0/24"
+	DefaultMtu         = 1450


why 1450 by default? it should not be 1500?

I remember that vxlan interface has a mtu of 1450, once I met a problem that container network can't visit outside via vxlan, after setting its mtu to 1450, everything is good again.

But I'll keep mtu empty to let os handle it as your other comments suggest.

gao-feng · 2017-08-02T10:29:45Z

hypervisor/network/network_linux.go

-		Automatic:   true,
+		Mac:       mac,
+		IPAddress: []string{ip.String()},
+		Mtu:       DefaultMtu,


I think by default we should not care about mtu, vm/os will setup the appropriate mtu for us. lets just set it null.

gao-feng · 2017-08-02T10:45:07Z

hypervisor/vm_states.go

+	if err := ctx.hyperstart.AddRoute(r); err != nil {
+		return err
+	}
+	return nil


return ctx.hyperstart.AddRoute(r)?

gao-feng · 2017-08-02T11:01:46Z

supervisor/proxy/nslistener.go

+	// delete all the routes from outside
+	// The reason that we need to delete all the routes is
+	// that if we don't do this, we will get two active network interfaces,
+	// and ping the IP will get duplicate ICMP replies.


The nic in netns of docker container(netns of nslistener) should be down, is it still has this problem?

I think we can still enter the netns and make the nic up? This should be a quite easy step, so we can't guarantee CNI plugin won't set nic up later.

Delete all route should be another protection for duplicate network flows.

I'm wonder if this deleting of router of netns will trigger route-update netlink event, though we don't care about this event right now.

Maybe removing the veth pair on host which joins the docker bridge is a good way, but the code will be complicated...

Maybe removing the veth pair on host which joins the docker bridge is a good way

This is also another option, maybe we should do this? For example, after tap created and inserted, removed veth from bridge?

@gao-feng I just met a extremely weird problem, starting two container in one VM(POD), first one has proper route rules, but second container has no route rules. How could this happen? I thought they were in same network namespace and shares all routes. Do you have any idea?

@WeiZhang555
Can you check the ipaddress of nic? is it same on two container? they should stay in the same netns.

Yes, the nic ip is the same, only routes are different, quite weird. I'll try to do more investigation.

gnawux · 2017-08-03T09:00:06Z

@WeiZhang555 could you help do a small rebase? The #537 introduced some conflicts

WeiZhang555 · 2017-08-03T09:06:13Z

@gnawux I'll do it soon 😉

WeiZhang555 · 2017-09-12T09:25:21Z

Closing this. We have a new one #597 carrying this 😄

Implement more rich network functions

24e75eb

* Support add ip addr to a specific network interface * Support delete ip address from a network interface * Support modify MTU Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

WeiZhang555 force-pushed the add-network-functions-up branch from 1e15b14 to 6c61422 Compare July 19, 2017 11:22

Support add any route into container

ffb5656

Enhance network functions: Support add any routes into container Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

WeiZhang555 force-pushed the add-network-functions-up branch from 6c61422 to ffb5656 Compare July 19, 2017 11:39

Bugfix: fix panic when delete nic

c78f19f

Fix bug that container panics when doing the network interface deletion work. Also modify nic delete logic a little bit, now we can delete single IP and delete link separately. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

gao-feng reviewed Aug 2, 2017

View reviewed changes

gnawux added this to the v1.0.0 milestone Aug 3, 2017

WeiZhang555 mentioned this pull request Sep 12, 2017

Enhance network functions #597

Merged

WeiZhang555 closed this Sep 12, 2017

Add more rich network functions #533

Add more rich network functions #533

Uh oh!

Conversation

WeiZhang555 commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnawux commented Aug 3, 2017

Uh oh!

WeiZhang555 commented Aug 3, 2017

Uh oh!

WeiZhang555 commented Sep 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants