Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: basic framework of stream server for cri manager #797

Merged

Conversation

YaoZengzeng
Copy link
Contributor

Signed-off-by: YaoZengzeng [email protected]

Ⅰ. Describe what this PR did

This PR will implement the stream server and Exec method of CRI manager.

The other two stream server related method Attach and PortForward will be implemented in other PRs

Ⅱ. Does this pull request fix one issue?

fixes part of #420

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Mar 2, 2018

Codecov Report

Merging #797 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
- Coverage    13.7%   13.66%   -0.04%     
==========================================
  Files         108      109       +1     
  Lines        6492     6511      +19     
==========================================
  Hits          890      890              
- Misses       5543     5562      +19     
  Partials       59       59
Impacted Files Coverage Δ
daemon/mgr/cri_stream.go 0% <0%> (ø)
daemon/mgr/cri.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16fb071...3a578c0. Read the comment docs.

@YaoZengzeng
Copy link
Contributor Author

@allencloud PTAL.

@YaoZengzeng YaoZengzeng force-pushed the streaming-server branch 2 times, most recently from aa8d440 to 5804280 Compare March 2, 2018 07:59
}

// WriteError translates a CRI streaming error into an appropriate HTTP response.
func WriteError(err error, w http.ResponseWriter) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no idea that how many grpc code grpc server would return. However, if there are lots of its, maybe we can refer to https://github.com/moby/moby/blob/master/api/server/httputils/errors.go#L98-L129

w.Header().Add(httpstream.HeaderUpgrade, HeaderSpdy31)
w.WriteHeader(http.StatusSwitchingProtocols)

// 获取底层的连接
Copy link
Collaborator

Choose a reason for hiding this comment

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

? Could you make this in English? 😄

if err != nil {
return "", err
}
// 将cache entry加入list
Copy link
Collaborator

Choose a reason for hiding this comment

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

? 😄


// Insert the given request into the cache and returns the token used for fetching it out.
func (c *requestCache) Insert(req request) (token string, err error) {
c.lock.Lock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if this lock is locking two much actions, not only data. Just add a comment, since no testing data no bb. Not blocking.

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Mar 5, 2018
@allencloud allencloud merged commit 905dcfe into AliyunContainerService:master Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants