feature: add network disconnect interface #1172

HusterWan · 2018-04-20T09:06:50Z

Signed-off-by: Michael Wan zirenwan@gmail.com

Ⅰ. Describe what this PR did

support network disconnect restful api

Ⅱ. Does this pull request fix one issue?

fixes part of #868

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

show container network information

root@osboxes:pouch (zr/network-disconnect) -> pouch inspect -f {{.NetworkSettings.Networks.bridge}} test
{[] map[] bfdbbdd9fb28258c270eed0a93b90da4172eee62336b35ddcf9f58e1375237f7 172.17.0.1  0 <nil> 172.17.0.2 24  [] 02:42:ac:11:00:02 89f1c37aac4c6cd86848290c54a85f9af37b4c7ce18b64358cc4dc49d4c89f74}

check network config inside container:

root@osboxes:pouch (zr/network-disconnect) -> nsenter -t 14414 -p -n -m ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue qlen 1
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host
       valid_lft forever preferred_lft forever
9: eth0@if10: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue
    link/ether 02:42:ac:11:00:02 brd ff:ff:ff:ff:ff:ff
    inet 172.17.0.2/24 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::42:acff:fe11:2/64 scope link
       valid_lft forever preferred_lft forever

check network config on host:

root@osboxes:pouch (zr/network-disconnect) -> brctl show
bridge name	bridge id		STP enabled	interfaces
docker0		8000.0242fbc4e8bd	no
p0		8000.1ad80237a756	no		veth9489e6d
virbr0		8000.525400edda3f	yes		virbr0-nic

disconnect container from network

root@osboxes:pouch (zr/network-disconnect) -> pouch network disconnect bridge test

check network config again:

root@osboxes:pouch (zr/network-disconnect) -> brctl show
bridge name	bridge id		STP enabled	interfaces
docker0		8000.0242fbc4e8bd	no
p0		8000.000000000000	no
virbr0		8000.525400edda3f	yes		virbr0-nic

endpoint has been removed

Ⅴ. Special notes for reviews

codecov-io · 2018-04-20T09:27:27Z

Codecov Report

Merging #1172 into master will decrease coverage by 0.03%.
The diff coverage is 10.14%.

@@            Coverage Diff             @@
##           master    #1172      +/-   ##
==========================================
- Coverage   16.01%   15.97%   -0.04%     
==========================================
  Files         175      176       +1     
  Lines       10062    10130      +68     
==========================================
+ Hits         1611     1618       +7     
- Misses       8333     8394      +61     
  Partials      118      118

Impacted Files	Coverage Δ
apis/server/network_bridge.go	`0% <0%> (ø)`	⬆️
ctrd/container.go	`0% <0%> (ø)`	⬆️
daemon/mgr/container.go	`0% <0%> (ø)`	⬆️
cli/network.go	`0% <0%> (ø)`	⬆️
apis/server/router.go	`0% <0%> (ø)`	⬆️
client/network_disconnect.go	`100% <100%> (ø)`

allencloud · 2018-04-20T13:17:29Z

apis/server/network_bridge.go

+		return httputils.NewHTTPError(err, http.StatusBadRequest)
+	}
+
+	name := mux.Vars(req)["name"]


I sugget using network with the naming, since it will be some kind of clear. 😃 Not blocking

allencloud · 2018-04-20T13:18:30Z

apis/swagger.yml

+      responses:
+        200:
+          description: "No error"
+        404:


You returned return httputils.NewHTTPError(err, http.StatusBadRequest). So I am afraid we need to add status code of 400.

allencloud · 2018-04-20T13:21:12Z

cli/network.go

+
+	ctx := context.Background()
+	apiClient := nd.cli.Client()
+	if err := apiClient.NetworkDisconnect(ctx, nd.network, nd.container, nd.force); err != nil {


If you do not wrapper the error in the return statement, you can use return apiClient.NetworkDisconnect(ctx, nd.network, nd.container, nd.force) directly.

allencloud · 2018-04-20T13:26:42Z

daemon/mgr/container.go

+	// Get network
+	network, err := mgr.NetworkMgr.Get(ctx, networkName)
+	if err != nil {
+		return fmt.Errorf("failed to disconnect container %s from %s: get network failed: %v", c.Name(), networkName, err)


fmt.Errorf("failed to get network %s when disconnecting container %s from it: %v", c.Name(), network, err) ?

allencloud · 2018-04-20T13:29:05Z

daemon/mgr/container.go

+		return fmt.Errorf("failed to disconnect container %s from %s: get network failed: %v", c.Name(), networkName, err)
+	}
+
+	if c.meta.NetworkSettings != nil {


I do not know if the logic is correct. While I insist the code could be simplified, like

if c.meta.NetworkSettings == nil { return nil } epConfig, ok := c.meta.NetworkSettings.Networks[network.Name] if !ok { return fmt.Errorf("failed to disconnect container from network: container %s not attach to %s", c.Name(), networkName) } ...........

Using return fast to reduce ident of code.

allencloud · 2018-04-20T15:00:09Z

daemon/mgr/container.go

+	}
+
+	networkSettings := c.meta.NetworkSettings
+	if networkSettings == nil || networkSettings.Networks == nil {


I tested that there is no need to check if networkSettings.Networks equals to nil, if we checked epConfig, ok := networkSettings.Networks[network.Name]. Since we could get a value by the following way if it is nil:

value, exist := networkSettings.Networks[network.Name]

even if networkSettings.Networks is nil.

HusterWan · 2018-04-21T09:21:38Z

@allencloud Thanks a lot for your review, Updated, PTAL

@rudyfly Please review this PR, thanks

Signed-off-by: Michael Wan <zirenwan@gmail.com>

allencloud · 2018-04-21T14:38:54Z

test/cli_network_test.go

+	inspectInfo := command.PouchRun("inspect", name).Stdout()
+	metaJSON := []types.ContainerJSON{}
+	if err := json.Unmarshal([]byte(inspectInfo), &metaJSON); err != nil {
+		c.Errorf("failed to decode inspect output: %v", err)


Should we use c.Assert here? I am not very sure. @Letty5411

allencloud · 2018-04-22T06:00:16Z

daemon/mgr/container.go

+	// remove old container from cache
+	mgr.cache.Remove(c.ID())
+	// add new container to cache
+	mgr.cache.Put(c.ID(), c)


Removing it and adding it again is our current way to update an instance.
I think it is kind of redundant. I wish that we could add an Update function in package satemap. Also it will be much better to encapsulate this two function into a single one.

Not block this. Just record this.

Continue to add a comment to this. I am afraid we do not need to function Update, because we can use Put function directly to achieve this. @HusterWan

Since the code is as following:

// Put stores a key-value pair into inner map safely. func (m *SafeMap) Put(k string, v interface{}) { m.Lock() defer m.Unlock() if m.inner == nil { return } m.inner[k] = v }

So no need to remove, just put is OK.

Yes, i will create a new pr to refactor this part of code.

allencloud · 2018-04-22T06:04:40Z

LGTM

rudyfly · 2018-04-23T03:46:14Z

apis/server/router.go

@@ -72,6 +72,7 @@ func initRoute(s *Server) http.Handler {
 	s.addRoute(r, http.MethodPost, "/networks/create", s.createNetwork)
 	s.addRoute(r, http.MethodGet, "/networks/{name:.*}", s.getNetwork)
 	s.addRoute(r, http.MethodDelete, "/networks/{name:.*}", s.deleteNetwork)
+	s.addRoute(r, http.MethodPost, "/networks/{name:.*}/disconnect", s.disconnectNetwork)


change "name" to "id", and make it consistent with moby api.

rudyfly · 2018-04-23T03:49:57Z

daemon/mgr/container.go

+
+	// DisconnectContainerFromNetwork disconnects the given container from
+	// given network
+	DisconnectContainerFromNetwork(ctx context.Context, containerName, networkName string, force bool) error


s/DisconnectContainerFromNetwork/Disconnect?
it looks complicated.

pouchrobot added areas/network kind/feature size/XL labels Apr 20, 2018

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Apr 20, 2018

HusterWan force-pushed the zr/network-disconnect branch from b1f889d to dcc6b41 Compare April 20, 2018 11:29

pouchrobot removed the conflict/needs-rebase label Apr 20, 2018

HusterWan changed the title ~~feature: add network disconnect interface~~ [WIP]feature: add network disconnect interface Apr 20, 2018

allencloud reviewed Apr 20, 2018

View reviewed changes

HusterWan force-pushed the zr/network-disconnect branch 3 times, most recently from 1f2c7ca to 026dd89 Compare April 20, 2018 14:33

allencloud reviewed Apr 20, 2018

View reviewed changes

HusterWan force-pushed the zr/network-disconnect branch from 026dd89 to adf0361 Compare April 21, 2018 09:18

HusterWan requested a review from rudyfly April 21, 2018 09:18

HusterWan changed the title ~~[WIP]feature: add network disconnect interface~~ feature: add network disconnect interface Apr 21, 2018

HusterWan assigned rudyfly Apr 21, 2018

feature: add network disconnect interface

fc27d0e

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan force-pushed the zr/network-disconnect branch from adf0361 to fc27d0e Compare April 21, 2018 13:21

allencloud reviewed Apr 21, 2018

View reviewed changes

allencloud reviewed Apr 22, 2018

View reviewed changes

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 22, 2018

allencloud merged commit 76bf1cf into AliyunContainerService:master Apr 22, 2018

Letty5411 mentioned this pull request Apr 22, 2018

[feature required] need network APIs #868

Closed

2 tasks

rudyfly reviewed Apr 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add network disconnect interface #1172

feature: add network disconnect interface #1172

HusterWan commented Apr 20, 2018 •

edited

Loading

codecov-io commented Apr 20, 2018 •

edited

Loading

allencloud Apr 20, 2018

allencloud Apr 20, 2018

allencloud Apr 20, 2018

allencloud Apr 20, 2018

allencloud Apr 20, 2018 •

edited

Loading

HusterWan Apr 20, 2018

allencloud Apr 20, 2018

HusterWan commented Apr 21, 2018

allencloud Apr 21, 2018

allencloud Apr 22, 2018 •

edited

Loading

allencloud Apr 22, 2018

HusterWan Apr 22, 2018

allencloud commented Apr 22, 2018

rudyfly Apr 23, 2018

rudyfly Apr 23, 2018

feature: add network disconnect interface #1172

feature: add network disconnect interface #1172

Conversation

HusterWan commented Apr 20, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Apr 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan commented Apr 21, 2018

Choose a reason for hiding this comment

allencloud Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan commented Apr 20, 2018 •

edited

Loading

codecov-io commented Apr 20, 2018 •

edited

Loading

allencloud Apr 20, 2018 •

edited

Loading

allencloud Apr 22, 2018 •

edited

Loading