feature: container start with network #660
Conversation
Codecov Report
@@ Coverage Diff @@
## master #660 +/- ##
==========================================
- Coverage 10.28% 10.16% -0.12%
==========================================
Files 71 71
Lines 3384 3422 +38
==========================================
Hits 348 348
- Misses 2997 3035 +38
Partials 39 39
Continue to review full report at Codecov.
|
|
fix #604 |
container start with default bridge network. Signed-off-by: Rudy Zhang <[email protected]>
|
ping~PTAL @allencloud |
|
LGTM. tested well on my local machine. |
pouchrobot
added
the
LGTM
| switch len(arr) { | ||
| case 1: | ||
| if ipaddr := net.ParseIP(arr[0]); ipaddr != nil { | ||
| ip = arr[0] |
There was a problem hiding this comment.
In this case, name is "", does networkingConfig.EndpointsConfig[""] make sense?
| @@ -35,6 +36,7 @@ type container struct { | |||
| pidMode string | |||
| utsMode string | |||
| sysctls []string | |||
| network []string | |||
There was a problem hiding this comment.
If the type is slice, "networks" is better.
| HostConfig: hostConfig, | ||
| ContainerConfig: config, | ||
| HostConfig: hostConfig, | ||
| NetworkingConfig: networkConfig, |
There was a problem hiding this comment.
NetworkingConfig or networkConfig? Why not make them consistent?
There was a problem hiding this comment.
Yeah, we got your point. Actually in Moby's it is NetworkingConfig, so we have not changed that yet.
| for name, endpointSetting := range c.meta.NetworkSettings.Networks { | ||
| _, err := mgr.NetworkMgr.EndpointCreate(ctx, c.ID(), name, c.meta.NetworkSettings, endpointSetting) | ||
| if err != nil { | ||
| logrus.Errorf("fail to create endpoint, err: %v", err) |
There was a problem hiding this comment.
logrus.Errorf("failed to create endpoint: %v", err)
| @@ -371,6 +380,17 @@ func (mgr *ContainerManager) Start(ctx context.Context, id, detachKeys string) ( | |||
| } | |||
| c.DetachKeys = detachKeys | |||
|
|
|||
| // initialise network endpoint | |||
| if c.meta.NetworkSettings.Networks != nil && len(c.meta.NetworkSettings.Networks) > 0 { | |||
There was a problem hiding this comment.
The judgement is redundant, it is ok to range map which is nil.
| // set network settings | ||
| meta.NetworkSettings = &types.NetworkSettings{} | ||
| if config.NetworkingConfig.EndpointsConfig != nil && | ||
| len(config.NetworkingConfig.EndpointsConfig) > 0 { |
There was a problem hiding this comment.
According to the code above, config.NetworkingConfig.EndpointsConfig will not be nil.
Maybe the judgement of len(...) > 0 is enough.
There was a problem hiding this comment.
Actually I found this issue as well. I will fix this.
| @@ -115,6 +117,28 @@ func (c *container) config() (*types.ContainerCreateConfig, error) { | |||
| }, | |||
| } | |||
|
|
|||
| if len(c.network) == 0 { | |||
| config.HostConfig.NetworkMode = "bridge" | |||
| } | |||
There was a problem hiding this comment.
What if the network mode is "none", "host" or "container"?
It seems weird to specify multiple networks in "pouch create" or "pouch run", what if the networks are conflict, one network is "container" and another is "none"? The judgement will be too complicated.
@allencloud @skoo87 WDYT?
1.Describe what this PR did
container start with network.
2.Does this pull request fix one issue?
fixes #604
3.Describe how you did it
4.Describe how to verify it
In container, you can get the network information. such as:
# ifconfig# ping 172.17.0.15.Special notes for reviews
Signed-off-by: Rudy Zhang [email protected]