refactor: make CRI Stream Server share http server with pouchd #2214

YaoZengzeng · 2018-09-07T08:34:26Z

Signed-off-by: YaoZengzeng yaozengzeng@zju.edu.cn

Ⅰ. Describe what this PR did

If the CRI enabled, pouch will launch two http server, one for pouchd and another for stream server of CRI, so we will need two different port

However port is scarce resources in company which has large scale of data center.

So maybe it is a good idea that we reuse the same http server for pouchd and stream server which only one port is needed

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

No

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io · 2018-09-07T09:39:59Z

Codecov Report

Merging #2214 into master will decrease coverage by 0.21%.
The diff coverage is 56.66%.

@@            Coverage Diff             @@
##           master    #2214      +/-   ##
==========================================
- Coverage   66.57%   66.36%   -0.22%     
==========================================
  Files         208      208              
  Lines       16744    16837      +93     
==========================================
+ Hits        11148    11174      +26     
- Misses       4273     4321      +48     
- Partials     1323     1342      +19

Flag	Coverage Δ
#criv1alpha1test	`32.63% <26%> (-0.35%)`	⬇️
#criv1alpha2test	`33.28% <26.66%> (-0.15%)`	⬇️
#integrationtest	`39.71% <12%> (-0.26%)`	⬇️
#nodee2etest	`33.59% <25.33%> (-0.12%)`	⬇️
#unittest	`23.88% <16.52%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
apis/server/server.go	`52.5% <ø> (ø)`	⬆️
apis/server/router.go	`85.44% <0%> (-6.4%)`	⬇️
cri/v1alpha1/cri_wrapper.go	`55.3% <0%> (-0.5%)`	⬇️
cri/v1alpha2/cri_wrapper.go	`61.98% <0%> (-3.71%)`	⬇️
daemon/daemon.go	`58.37% <100%> (+0.68%)`	⬆️
cri/v1alpha2/server.go	`79.2% <100%> (-0.65%)`	⬇️
cri/v1alpha1/server.go	`79.2% <100%> (-0.65%)`	⬇️
cri/v1alpha1/cri.go	`61.46% <40%> (-1.11%)`	⬇️
cri/v1alpha2/cri.go	`71.33% <40%> (-0.87%)`	⬇️
cri/criservice.go	`55.95% <55.17%> (-8.76%)`	⬇️
... and 14 more

allencloud · 2018-09-09T01:48:33Z

I think this feature is quite urgent for PouchContainer. PTAL ASAP. Thanks. @fuweid @starnop

allencloud · 2018-09-09T01:54:16Z

main.go

@@ -69,7 +69,7 @@ func setupFlags(cmd *cobra.Command) {
 	flagSet := cmd.Flags()

 	flagSet.StringVar(&cfg.HomeDir, "home-dir", "/var/lib/pouch", "Specify root dir of pouchd")
-	flagSet.StringArrayVarP(&cfg.Listen, "listen", "l", []string{"unix:///var/run/pouchd.sock"}, "Specify listening addresses of Pouchd")
+	flagSet.StringArrayVarP(&cfg.Listen, "listen", "l", []string{"unix:///var/run/pouchd.sock", "tcp://0.0.0.0:4243"}, "Specify listening addresses of Pouchd")


By default, tcp://0.0.0.0:4243 is set in pouchd? I do not think it is a good way.
Shall we still set the default value to be unix socket, and if user needs to expand the CRI within the one port, he needs to add a tcp address when startup of pouchd. Here I am afraid we need to add more explanation of the comment Specify listening addresses of Pouchd, and it is too general and hard for others to understand the real functionality of it.

In addition, I am just here to double check if we still support to set the two different port in pouchd for configuration, one for container api port, the other for cri stream server port?

fuweid · 2018-09-10T03:00:02Z

cri/v1alpha2/cri.go

@@ -57,7 +58,7 @@ const (
 	// Address and port of stream server.
 	// TODO: specify them in the parameters of pouchd.
 	streamServerAddress = ""
-	streamServerPort    = "10011"
+	streamServerPort    = "4243"


could we share same port with pouch daemon or create other option like what @allencloud says?

fuweid · 2018-09-10T03:01:07Z

cri/criservice.go

-		if err := <-errChan; err != nil {
-			return err
-		}
+	if err := <-errChan; err != nil {


are we need the 2 in make(chan error, 2) here?

YaoZengzeng · 2018-09-10T03:31:20Z

I just refactored this PR.

Now we could specify two stream server related options with pouchd.

stream-server-port: specify the port of stream server, default is 10010

stream-server-reuse-port: only this option is enabled, the stream server will share the port with pouchd.

NOTE: in this condition, we should specify a tcp socket in pouchd and the port should be same with stream-server-port, for example:

pouchd --listen tcp://0.0.0.0:4243 --stream-server-reuse-port --stream-server-port 4243

The --stream-server-reuse-port feature should only be used internally and not be make public, otherwise it will be complicated and hard to be compatible.

@allencloud @fuweid WDYT?

YaoZengzeng · 2018-09-11T02:57:48Z

I adjust the mechanism again.

When stream-server-reuse-port is disabled, stream sever will listen on its own port and you can use stream-server-port of pouchd to tune it.
If stream-server-reuse-port is enabled, the stream-server-port will not have any effect. Stream sever will reuse the pouchd's port and extract it from the listen option of pouchd.

fuweid · 2018-09-11T01:08:29Z

cri/v1alpha1/cri.go

@@ -57,7 +58,7 @@ const (
 	// Address and port of stream server.
 	// TODO: specify them in the parameters of pouchd.
 	streamServerAddress = ""
-	streamServerPort    = "10010"
+	streamServerPort    = "4243"


I think we should keep 10010 as default here.

fuweid · 2018-09-14T03:09:30Z

cri/v1alpha1/cri.go

+	streamServerPort := config.CriConfig.StreamServerPort
+	// If stream server reuse the pouchd's port, extract the port from pouchd's listening addresses.
+	if config.CriConfig.StreamServerReusePort {
+		streamServerAddress, streamServerPort = extractIPAndPortFromAddresses(config.Listen)


since the user maybe pass multiple tcp addresses, could we show the address and port which CRI stream uses?

If the streamServerAddress we get is 0.0.0.0, we will reacquire the concrete IP that the stream server will use in newStreamServer, so I output the log there.

oops, my mistake.

fuweid · 2018-09-14T03:09:58Z

cri/v1alpha1/cri_stream_test.go

+			args:     []string{"unix:///var/run/pouchd.sock", "tcp://0.0.0.0:4345"},
+			wantIP:   "0.0.0.0",
+			wantPort: "4345",
+		},


please add two tcp addresses case here

Signed-off-by: YaoZengzeng <yaozengzeng@zju.edu.cn>

fuweid

LGTM

pouchrobot added kind/refactor size/XL labels Sep 7, 2018

YaoZengzeng changed the title ~~refactor: make CRI Stream Server share port(http server) with pouchd~~ [WIP]refactor: make CRI Stream Server share port(http server) with pouchd Sep 7, 2018

YaoZengzeng force-pushed the stream-port branch 3 times, most recently from 39743fc to d6ab819 Compare September 7, 2018 08:48

YaoZengzeng changed the title ~~[WIP]refactor: make CRI Stream Server share port(http server) with pouchd~~ refactor: make CRI Stream Server share port(http server) with pouchd Sep 7, 2018

YaoZengzeng changed the title ~~refactor: make CRI Stream Server share port(http server) with pouchd~~ refactor: make CRI Stream Server share http server with pouchd Sep 7, 2018

allencloud assigned fuweid and starnop Sep 9, 2018

allencloud reviewed Sep 9, 2018

View reviewed changes

allencloud added the areas/kubernetes label Sep 9, 2018

fuweid reviewed Sep 10, 2018

View reviewed changes

YaoZengzeng force-pushed the stream-port branch 2 times, most recently from f9c95c2 to 429f381 Compare September 10, 2018 03:17

YaoZengzeng force-pushed the stream-port branch 9 times, most recently from 7ac7b2a to fb10eab Compare September 11, 2018 02:50

YaoZengzeng force-pushed the stream-port branch 2 times, most recently from 1a71c73 to d39c8e1 Compare September 11, 2018 05:56

YaoZengzeng force-pushed the stream-port branch 4 times, most recently from 2e7c784 to 3069db2 Compare September 11, 2018 06:37

fuweid reviewed Sep 14, 2018

View reviewed changes

YaoZengzeng force-pushed the stream-port branch from 3069db2 to 6260902 Compare September 14, 2018 05:23

refactor: make CRI Stream Server share port(http server) with pouchd

e5914d5

Signed-off-by: YaoZengzeng <yaozengzeng@zju.edu.cn>

YaoZengzeng force-pushed the stream-port branch from 6260902 to e5914d5 Compare September 14, 2018 05:26

fuweid approved these changes Sep 17, 2018

View reviewed changes

fuweid merged commit 1596dd7 into AliyunContainerService:master Sep 17, 2018

pouchrobot mentioned this pull request Sep 22, 2018

Weekly Report of PouchContainer from 2018-09-15 to 2018-09-22 #2272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: make CRI Stream Server share http server with pouchd #2214

refactor: make CRI Stream Server share http server with pouchd #2214

YaoZengzeng commented Sep 7, 2018 •

edited by allencloud

Loading

codecov-io commented Sep 7, 2018 •

edited

Loading

allencloud commented Sep 9, 2018

allencloud Sep 9, 2018

fuweid Sep 10, 2018

fuweid Sep 10, 2018

YaoZengzeng commented Sep 10, 2018

YaoZengzeng commented Sep 11, 2018

fuweid Sep 11, 2018

fuweid Sep 14, 2018

YaoZengzeng Sep 14, 2018

fuweid Sep 17, 2018

fuweid Sep 14, 2018

fuweid left a comment

refactor: make CRI Stream Server share http server with pouchd #2214

refactor: make CRI Stream Server share http server with pouchd #2214

Conversation

YaoZengzeng commented Sep 7, 2018 • edited by allencloud Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Sep 7, 2018 • edited Loading

Codecov Report

allencloud commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaoZengzeng commented Sep 10, 2018

YaoZengzeng commented Sep 11, 2018

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

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

YaoZengzeng commented Sep 7, 2018 •

edited by allencloud

Loading

codecov-io commented Sep 7, 2018 •

edited

Loading