feature: add container's network files #1403

shaloulcy · 2018-05-24T06:35:45Z

Signed-off-by: Eric Li lcy041536@gmail.com

Ⅰ. Describe what this PR did

Now when we start a container, it has no network files, including /etc/hosts, /etc/hostname and /etc/resolv.conf. In this PR, we will generate container's network files when container starts

Ⅱ. Does this pull request fix one issue?

fixes #1162

Ⅲ. Describe how you did it

The libnetwork will help us generate network files(/etc/hosts and /etc/resolv.conf). /etc/hostname will be generated by pouchd.

At last, we should mount it into container.

Ⅳ. Describe how to verify it

start a container, and list /etc

# pouch run -it --hostname helloPouch  reg.docker.alibaba-inc.com/busybox:latest sh


BusyBox v1.21.1 (Ubuntu 1:1.21.0-1ubuntu1) built-in shell (ash)
Enter 'help' for a list of built-in commands.

~ # ls /etc/
group          hosts          passwd
hostname       nsswitch.conf  resolv.conf

check network files

~ # cat /etc/hostname
helloPouch
~ # cat /etc/hosts
127.0.0.1	localhost
::1	localhost ip6-localhost ip6-loopback
fe00::0	ip6-localnet
ff00::0	ip6-mcastprefix
ff02::1	ip6-allnodes
ff02::2	ip6-allrouters
172.17.0.5	helloPouch
~ # ifconfig
eth0      Link encap:Ethernet  HWaddr 02:42:AC:11:00:05  
          inet addr:172.17.0.5  Bcast:0.0.0.0  Mask:255.255.255.0
          inet6 addr: fe80::42:acff:fe11:5/64 Scope:Link
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
          RX packets:8 errors:0 dropped:0 overruns:0 frame:0
          TX packets:8 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:0 
          RX bytes:648 (648.0 B)  TX bytes:648 (648.0 B)

lo        Link encap:Local Loopback  
          inet addr:127.0.0.1  Mask:255.0.0.0
          inet6 addr: ::1/128 Scope:Host
          UP LOOPBACK RUNNING  MTU:65536  Metric:1
          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
          TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1 
          RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)

~ # cat /etc/resolv.conf 
search DHCP
nameserver 127.0.0.11
options ndots:0

Ⅴ. Special notes for reviews

codecov-io · 2018-05-25T02:57:54Z

Codecov Report

Merging #1403 into master will increase coverage by 20.93%.
The diff coverage is 43.67%.

@@             Coverage Diff             @@
##           master    #1403       +/-   ##
===========================================
+ Coverage   16.26%   37.19%   +20.93%     
===========================================
  Files         206      252       +46     
  Lines       13759    17545     +3786     
===========================================
+ Hits         2238     6526     +4288     
+ Misses      11365    10185     -1180     
- Partials      156      834      +678

Impacted Files	Coverage Δ
cri/v1alpha2/cri_utils.go	`28.35% <0%> (-0.07%)`	⬇️
cli/common_flags.go	`0% <0%> (ø)`	⬆️
cli/container.go	`0% <0%> (ø)`	⬆️
cri/v1alpha1/cri_utils.go	`29.25% <0%> (-0.07%)`	⬇️
daemon/mgr/container.go	`35.12% <45.16%> (+35.12%)`	⬆️
daemon/mgr/spec_mount.go	`72.61% <63.15%> (+72.61%)`	⬆️
pkg/multierror/def.go	`0% <0%> (-100%)`	⬇️
cli/main.go	`0% <0%> (ø)`	⬆️
cli/pull.go	`0% <0%> (ø)`	⬆️
cli/upgrade.go	`0% <0%> (ø)`	⬆️
... and 129 more

allencloud · 2018-05-25T08:10:12Z

I would like to invite @HusterWan to take a review of the design. I think adding network files into the container will make the usage inside our environment incompatible. Right?

HusterWan · 2018-05-25T08:33:37Z

@allencloud Yes, we have discuss offline, i will review this PR

BTW, thanks a lot for your works @shaloulcy

HusterWan · 2018-05-29T08:00:22Z

daemon/mgr/spec_mount.go

+		return mounts
+	}
+
+	if c.HostnamePath != "" {


there are many duplicate codes here, maybe we can use

for _, bind := range []pathBinding{{Src: xxx, Dst: xxx}} { }

WDYT? :)

HusterWan · 2018-05-29T08:03:27Z

daemon/mgr/spec_mount.go

+
+// isSetupNetworkMount checks whether set network mount.
+func isSetupNetworkMount(mount *types.MountPoint, c *Container) bool {
+	if mount.Destination == "/etc/hostname" {


How about code below ?

if utils.StringInSlice([]string{"/etc/hostname", "/etc/hosts", "/etc/resolv.conf", mount.Destination}) xxx

the function just set one file, if user set all three files, should we also set them for c ?

the outer function will iterator all mountpoints

HusterWan · 2018-05-29T08:10:53Z

daemon/mgr/spec_mount.go

@@ -69,6 +78,10 @@ func setupMounts(ctx context.Context, c *Container, s *specs.Spec) error {
 			Options:     opts,
 		})
 	}
+
+	// generate network mounts.


should we put check switch code in here?

if c.Config.DisableHostsfiles { mounts = append(mounts, generateNetworkMounts(c)...)

HusterWan · 2018-05-29T08:12:46Z

apis/swagger.yml

@@ -1787,6 +1787,11 @@ definitions:
        type: "boolean"
        x-nullable: false
        default: true
+      DisableHostsfiles:


Hostsfiles may not so good, do we have more properly name for /etc/hosts, /etc/hostname, /etc/resolve.conf files? 😆

cc @allencloud @rudyfly

I do not like the naming of DisableHostsfiles , either. How about DisableLocalFiles or DisableNetworkFiles?

DisableNetworkFiles +1

HusterWan · 2018-05-31T11:48:53Z

LGTM

BTW, can we add some test cases for this feature?

fuweid · 2018-05-31T11:51:59Z

@HusterWan I think we should add cases before merged. The test debt is rising right now... WDYT?

HusterWan · 2018-05-31T11:55:13Z

@fuweid Yes, of course we should add test cases before merged this PR, take it easy dude ! 😆 😆

fuweid · 2018-05-31T12:02:18Z

@HusterWan If the final can run into the Game7....yeah...of course!

Signed-off-by: Eric Li <lcy041536@gmail.com>

shaloulcy · 2018-06-01T01:44:02Z

@fuweid @HusterWan I have add some test cases

allencloud · 2018-06-01T01:51:56Z

daemon/mgr/container.go

 	}
 	container.NetworkSettings = new(types.NetworkSettings)
 	if len(config.NetworkingConfig.EndpointsConfig) > 0 {
 		container.NetworkSettings.Networks = config.NetworkingConfig.EndpointsConfig
 	}
-	if container.NetworkSettings.Networks == nil && networkMode != "" && !IsContainer(networkMode) {
+	if container.NetworkSettings.Networks == nil && !IsContainer(config.HostConfig.NetworkMode) {


I found that here we have so much blocks about network handling in this Create function. And this make the code much much longer and a little bit difficult to read. Is there any way to encapsulate these blocks into a single function? @shaloulcy
Maybe in another pull request.

HusterWan · 2018-06-01T03:01:16Z

@shaloulcy since the DisableNetworkFiles flag will affect all already running containers on host using PouchContainer in alibaba group, we must set DisableNetworkFiles to true before this feature releases online.

first, we stop pouchd
second, set all containers's DisableNetworkFiles to true by using script
then we start pouchd again

cc @yyb196

HusterWan · 2018-06-01T06:40:11Z

test/cli_run_test.go

+
+	res.Assert(c, icmd.Success)
+	output := res.Stdout()
+	if strings.Contains(output, "hostname") {


we may make sure the test image never contains this three files, just comment.

pouchrobot added areas/network kind/feature size/L labels May 24, 2018

shaloulcy force-pushed the add_network_files branch 3 times, most recently from edf5302 to f76448c Compare May 25, 2018 02:34

shaloulcy force-pushed the add_network_files branch from f76448c to e70ef26 Compare May 25, 2018 06:56

allencloud assigned HusterWan May 25, 2018

shaloulcy force-pushed the add_network_files branch from e70ef26 to 21470b4 Compare May 29, 2018 06:33

pouchrobot added size/XL and removed size/L labels May 29, 2018

HusterWan reviewed May 29, 2018

View reviewed changes

shaloulcy force-pushed the add_network_files branch 2 times, most recently from 8888389 to 1f50786 Compare May 31, 2018 09:57

shaloulcy requested a review from rudyfly May 31, 2018 10:04

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

shaloulcy force-pushed the add_network_files branch from 1f50786 to 4d1f3c4 Compare May 31, 2018 13:17

feature: add container's network files

db46eff

Signed-off-by: Eric Li <lcy041536@gmail.com>

shaloulcy force-pushed the add_network_files branch from 4d1f3c4 to db46eff Compare May 31, 2018 13:34

allencloud mentioned this pull request May 31, 2018

[feature request] add domain name resolve for pouch container #1162

Closed

allencloud reviewed Jun 1, 2018

View reviewed changes

HusterWan reviewed Jun 1, 2018

View reviewed changes

HusterWan merged commit d41da05 into AliyunContainerService:master Jun 1, 2018

pouchrobot mentioned this pull request Jun 7, 2018

Weekly Report of Pouch from 2018-05-31 to 2018-06-07 #1482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add container's network files #1403

feature: add container's network files #1403

shaloulcy commented May 24, 2018 •

edited by allencloud

Loading

codecov-io commented May 25, 2018 •

edited

Loading

allencloud commented May 25, 2018

HusterWan commented May 25, 2018

HusterWan May 29, 2018

shaloulcy May 31, 2018

HusterWan May 29, 2018 •

edited

Loading

shaloulcy May 31, 2018

HusterWan May 29, 2018 •

edited

Loading

shaloulcy May 31, 2018

HusterWan May 29, 2018 •

edited

Loading

allencloud May 31, 2018

HusterWan May 31, 2018

shaloulcy May 31, 2018

HusterWan commented May 31, 2018

fuweid commented May 31, 2018

HusterWan commented May 31, 2018

fuweid commented May 31, 2018

shaloulcy commented Jun 1, 2018

allencloud Jun 1, 2018

HusterWan commented Jun 1, 2018

HusterWan Jun 1, 2018

feature: add container's network files #1403

feature: add container's network files #1403

Conversation

shaloulcy commented May 24, 2018 • edited by allencloud 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 May 25, 2018 • edited Loading

Codecov Report

allencloud commented May 25, 2018

HusterWan commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan May 29, 2018 • edited Loading

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

HusterWan commented May 31, 2018

fuweid commented May 31, 2018

HusterWan commented May 31, 2018

fuweid commented May 31, 2018

shaloulcy commented Jun 1, 2018

Choose a reason for hiding this comment

HusterWan commented Jun 1, 2018

Choose a reason for hiding this comment

shaloulcy commented May 24, 2018 •

edited by allencloud

Loading

codecov-io commented May 25, 2018 •

edited

Loading

HusterWan May 29, 2018 •

edited

Loading

HusterWan May 29, 2018 •

edited

Loading

HusterWan May 29, 2018 •

edited

Loading