refactor: Extract stream manager as the public part #1725

starnop · 2018-07-12T09:04:35Z

Signed-off-by: Starnop [email protected]

Ⅰ. Describe what this PR did

For now，The organization of the code about stream Manager is unreasonable，we should reorganize the file structure: extract stream manager as the public part.

Ⅱ. Does this pull request fix one issue?

fixes part of #1524

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io · 2018-07-12T09:53:27Z

Codecov Report

Merging #1725 into master will decrease coverage by 2.29%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #1725     +/-   ##
=========================================
- Coverage   41.76%   39.46%   -2.3%     
=========================================
  Files         278      277      -1     
  Lines       18244    19258   +1014     
=========================================
- Hits         7619     7601     -18     
- Misses       9699    10719   +1020     
- Partials      926      938     +12

Impacted Files	Coverage Δ
cri/stream/remotecommand/exec.go	`0% <0%> (ø)`	⬆️
cri/v1alpha2/server.go	`0% <0%> (ø)`	⬆️
cri/stream/runtime.go	`0% <0%> (ø)`
cri/v1alpha1/cri_stream.go	`0% <0%> (ø)`	⬆️
cri/stream/portforward/portforward.go	`0% <0%> (ø)`	⬆️
cri/v1alpha1/server.go	`0% <0%> (ø)`	⬆️
cri/stream/portforward/httpstream.go	`0% <0%> (ø)`	⬆️
cri/v1alpha2/cri_stream.go	`0% <0%> (ø)`	⬆️
cri/stream/remotecommand/attach.go	`0% <0%> (ø)`	⬆️
apis/opts/oom_score.go	`0% <0%> (-100%)`	⬇️
... and 20 more

allencloud · 2018-07-12T11:20:06Z

Sounds good to me, while I still wish @YaoZengzeng @fuweid could take a review for this.

fuweid

The change is acceptable and reasonable. But there needs little change in this commit. Please take a look.

fuweid · 2018-07-12T14:59:00Z

cri/stream/config.go

+package stream
+
+import (
+	"github.com/alibaba/pouch/cri/stream/constant"


split packages into two groups~

fuweid · 2018-07-12T15:00:22Z

cri/stream/config.go

+	// Address is the addr:port address the server will listen on.
+	Address string
+
+	// BaseURL is the optional base URL for constructing streaming URLs. If empty, the baseURL will be constructed from the serve address.


Could we fold the comment into two lines, since this line is too long? 😄

fuweid · 2018-07-12T15:01:11Z

cri/stream/config.go

+
+// DefaultConfig provides default values for server Config.
+var DefaultConfig = Config{
+	StreamIdleTimeout:               4 * time.Hour,


use DefaultStreamIdleTimeout instead of 4*time.Hour ?

fuweid · 2018-07-12T15:08:07Z

cri/stream/runtime.go

+}
+
+// Exec executes a command inside the container.
+func (s *streamRuntime) Exec(containerID string, cmd []string, streamOpts *remotecommand.Options, streams *remotecommand.Streams) (uint32, error) {


can we add the context as first argument? If we have the ctx, we can handle the progress by the upstream.

fuweid

ping @YaoZengzeng

fuweid · 2018-07-16T05:35:39Z

cri/stream/portforward/httpstream.go

 package portforward

 import (
+	con "context"


I think we should use the context here to make the word readable, because the con might be the connection.

fuweid · 2018-07-16T05:37:11Z

cri/stream/portforward/httpstream.go

 }

-func handleHTTPStreams(w http.ResponseWriter, req *http.Request, portForwarder PortForwarder, podName string, idleTimeout, streamCreationTimeout time.Duration, supportedPortForwardProtocols []string) error {
+func handleHTTPStreams(cont con.Context, w http.ResponseWriter, req *http.Request, portForwarder PortForwarder, podName string, idleTimeout, streamCreationTimeout time.Duration, supportedPortForwardProtocols []string) error {


the var of context.Context can be named by ctx.

fuweid · 2018-07-16T05:39:04Z

cri/stream/portforward/portforward.go

 // connections; i.e., multiple `curl http://localhost:8888/` requests will be
 // handled by a single invocation of ServePortForward.
-func ServePortForward(w http.ResponseWriter, req *http.Request, portForwarder PortForwarder, podName string, idleTimeout time.Duration, streamCreationTimeout time.Duration, supportedProtocols []string) {
+func ServePortForward(cont con.Context, w http.ResponseWriter, req *http.Request, portForwarder PortForwarder, podName string, idleTimeout time.Duration, streamCreationTimeout time.Duration, supportedProtocols []string) {


for the name rule, please check the last comment

All mentioned above is because there's a structure called context, we could renamed it in the next PR.

fuweid · 2018-07-16T05:41:06Z

cri/stream/runtime.go

+		return fmt.Errorf("failed to attach to container %q: %v", containerID, err)
+	}
+
+	<-streams.StreamCh


The channel is just used for the notify event?

cc @YaoZengzeng

@fuweid Yes, it is used to notify the container io has been closed and then the Attach function could return, the http connection could be closed.

fuweid · 2018-07-16T05:46:58Z

cri/v1alpha1/server.go

 }

 func (s *server) serveExec(w http.ResponseWriter, r *http.Request) {
+	ctx := context.Background()


we can use r.Context() instead of context.Background().

Signed-off-by: Starnop <[email protected]>

fuweid

LGTM @YaoZengzeng please help us to check again.

YaoZengzeng · 2018-07-16T07:47:54Z

LGTM

pouchrobot added kind/refactor size/XXL labels Jul 12, 2018

starnop force-pushed the cri-stream-manager branch 2 times, most recently from 6ac64ba to fc71fc7 Compare July 12, 2018 09:13

allencloud assigned YaoZengzeng and fuweid Jul 12, 2018

fuweid reviewed Jul 12, 2018

View reviewed changes

starnop force-pushed the cri-stream-manager branch 4 times, most recently from f622d61 to bddb6e6 Compare July 16, 2018 02:14

fuweid reviewed Jul 16, 2018

View reviewed changes

starnop force-pushed the cri-stream-manager branch from bddb6e6 to ac2ce18 Compare July 16, 2018 06:17

refactor:Extract stream manager as the public part

206c597

Signed-off-by: Starnop <[email protected]>

starnop force-pushed the cri-stream-manager branch from ac2ce18 to 206c597 Compare July 16, 2018 06:21

fuweid reviewed Jul 16, 2018

View reviewed changes

allencloud merged commit adb8ce1 into AliyunContainerService:master Jul 16, 2018

pouchrobot mentioned this pull request Jul 19, 2018

Weekly Report of PouchContainer from 2018-07-12 to 2018-07-19 #1752

Closed

refactor: Extract stream manager as the public part #1725

refactor: Extract stream manager as the public part #1725

Uh oh!

Conversation

starnop commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

codecov-io commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

allencloud commented Jul 12, 2018

Uh oh!

fuweid left a comment

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!

fuweid left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng commented Jul 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

starnop commented Jul 12, 2018 •

edited

Loading

codecov-io commented Jul 12, 2018 •

edited

Loading