feature: add network connect for container #1187

rudyfly · 2018-04-23T07:42:48Z

Ⅰ. Describe what this PR did

Add network connect for container.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

see test case: TestNetworkConnect

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang [email protected]
Signed-off-by: Zou Rui [email protected]

allencloud · 2018-04-23T08:59:23Z

apis/server/network_bridge.go

+func (s *Server) connectToNetwork(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
+	networkIDOrName := mux.Vars(req)["id"]
+
+	net, err := s.NetworkMgr.Get(ctx, networkIDOrName)


How about moving this part after request body validation. Maybe the order below will be better:

validation thing

inner logic within pouchd's manager

WDYT?

allencloud · 2018-04-23T09:00:19Z

apis/swagger.yml

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


Please add 400, since you have code return httputils.NewHTTPError(err, http.StatusBadRequest)

allencloud · 2018-04-23T09:04:42Z

cli/network.go

+
+// networkConnectExample shows examples in network connect command, and is used in auto-generated cli docs.
+func networkConnectExample() string {
+	return `$ pouch network connect`


Please add the output message here when success:

fmt.Printf("container %s is connected to network %s\n", container, network)

allencloud · 2018-04-23T09:05:11Z

daemon/mgr/container.go

 	"github.com/alibaba/pouch/pkg/reference"
+	"github.com/alibaba/pouch/pkg/stringid"
 	"github.com/alibaba/pouch/pkg/utils"
 	"github.com/alibaba/pouch/storage/quota"


Please add a blank line here.

allencloud · 2018-04-23T09:07:53Z

daemon/mgr/container.go

+// Connect is used to connect a container to a network.
+func (mgr *ContainerManager) Connect(ctx context.Context, name string, networkIDOrName string, epConfig *types.EndpointSettings) error {
+	c, err := mgr.container(name)
+	if err != nil || c == nil {


I do not think we should combine err != nil and c==nil. Because we did not output the error message in log and err.

allencloud · 2018-04-23T09:08:25Z

daemon/mgr/container.go

+	if epConfig == nil {
+		epConfig = &types.EndpointSettings{}
+	}
+	c.Lock()


Is this lock locked to much logic?

allencloud · 2018-04-23T09:10:54Z

daemon/mgr/container.go

 }
+
+// hasUserDefinedIPAddress returns whether the passed endpoint configuration contains IP address configuration
+func hasUserDefinedIPAddress(epConfig *types.EndpointSettings) bool {


This kind of function should be encapsulated in network utils or something like that. Here in container manager is quite unreasonable.

allencloud · 2018-04-23T09:13:09Z

pkg/stringid/stringid.go

@@ -0,0 +1,19 @@
+package stringid


please add a unit test of this file. Thanks a lot.

allencloud · 2018-04-23T13:14:45Z

CI fails:

FAIL: /go/src/github.com/alibaba/pouch/test/cli_network_test.go:348: PouchNetworkSuite.TestNetworkConnect
/go/src/github.com/alibaba/pouch/test/cli_network_test.go:354:
    // create bridge device
    icmd.RunCommand("brctl", "addbr", bridgeName).Assert(c, icmd.Success)
/go/src/github.com/alibaba/pouch/vendor/github.com/gotestyourself/gotestyourself/icmd/command.go:61:
    t.Fatalf("at %s:%d - %s\n", filepath.Base(file), line, err.Error())
... Error: at cli_network_test.go:354 - 
Command:  brctl addbr p1
ExitCode: 127
Error:    exec: "brctl": executable file not found in $PATH
Stdout:   
Stderr:   
Failures:
ExitCode was 127 expected 0
Expected no error

I am wondering if it is proper to use a third-party binary to make it.
In the future , we will keep this in netlink. How about that?
@rudyfly

codecov-io · 2018-04-24T13:18:11Z

Codecov Report

Merging #1187 into master will decrease coverage by 0.26%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1187      +/-   ##
==========================================
- Coverage   16.03%   15.76%   -0.27%     
==========================================
  Files         176      177       +1     
  Lines       10155    10327     +172     
==========================================
  Hits         1628     1628              
- Misses       8407     8579     +172     
  Partials      120      120

Impacted Files	Coverage Δ
pkg/utils/utils.go	`77.03% <0%> (+1.12%)`	⬆️
daemon/mgr/network_utils.go	`0% <0%> (ø)`	⬆️
apis/server/router.go	`0% <0%> (ø)`	⬆️
cli/network.go	`0% <0%> (ø)`	⬆️
client/network_connect.go	`0% <0%> (ø)`
apis/server/network_bridge.go	`0% <0%> (ø)`	⬆️
daemon/mgr/container.go	`0% <0%> (ø)`	⬆️

HusterWan · 2018-04-25T01:17:51Z

daemon/mgr/container.go

 	Restart(ctx context.Context, name string, timeout int64) error

+	// Connect is used to connect a container to a network.
+	Connect(ctx context.Context, name string, networkIDOrName string, epConfig *types.EndpointSettings) error


Connect or ConnectContainerToNetwork ?

Disconnect or DisconnectContainerFromNetwork ?

I think short function name is more better, write the detail information in comments is ok.

Add network connect for container. Signed-off-by: Rudy Zhang <[email protected]> Signed-off-by: Zou Rui <[email protected]>

HusterWan · 2018-04-25T03:30:39Z

LGTM

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

rudyfly force-pushed the network-connect branch 3 times, most recently from b05f9a2 to 10fbafe Compare April 23, 2018 08:55

allencloud reviewed Apr 23, 2018

View reviewed changes

pkg/stringid/stringid.go Outdated

@@ -0,0 +1,19 @@

package stringid

Copy link

Collaborator

allencloud Apr 23, 2018

Choose a reason for hiding this comment

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

please add a unit test of this file. Thanks a lot.

rudyfly force-pushed the network-connect branch 4 times, most recently from f7a544e to 7157116 Compare April 24, 2018 12:57

HusterWan reviewed Apr 25, 2018

View reviewed changes

rudyfly force-pushed the network-connect branch from 7157116 to e3885ef Compare April 25, 2018 02:03

feature: add network connect for container

ecba8a4

Add network connect for container. Signed-off-by: Rudy Zhang <[email protected]> Signed-off-by: Zou Rui <[email protected]>

rudyfly force-pushed the network-connect branch from e3885ef to ecba8a4 Compare April 25, 2018 02:32

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

HusterWan merged commit 0713c83 into AliyunContainerService:master Apr 25, 2018

rudyfly mentioned this pull request Apr 25, 2018

[feature required] need network APIs #868

Closed

2 tasks

pouchrobot mentioned this pull request Apr 26, 2018

Weekly Report of Pouch from 2018-04-19 to 2018-04-26 #1223

Closed

rudyfly deleted the network-connect branch October 29, 2018 09:17

feature: add network connect for container #1187

feature: add network connect for container #1187

Uh oh!

Conversation

rudyfly commented Apr 23, 2018

Ⅰ. 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

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!

allencloud commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 24, 2018

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HusterWan commented Apr 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

allencloud commented Apr 23, 2018 •

edited

Loading