feature: support docker proxy #128

ZYecho · 2022-05-09T07:42:09Z

Signed-off-by: Yue Zhang huaihuan.zy@alibaba-inc.com

Ⅰ. Describe what this PR does

to #64

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

honpey · 2022-05-12T02:31:11Z

pkg/runtime-manager/server/docker/server.go

+			http.Error(wr, "Failed to checkpoint resource", http.StatusInternalServerError)
+			return
+		}
+		d.dispatcher.Dispatch(ctx, runtimeHookPath, types.PostHook, resourceExecutor.GenerateHookRequest())


dispatch posthook should be called ahead of copying info io.Copy(wr, bytes.NewReader(respBytes))

honpey · 2022-05-12T02:45:04Z

pkg/runtime-manager/server/docker/server.go

+}
+
+func (d *RuntimeManagerDockerServer) failOver() error {
+	dockerClient, err := client.NewClientWithOpts(client.WithHost("unix://"+utils.DefaultDockerSocketPath), client.WithVersion(""))


no need to get a new docker client here? before failover, the backend client should have been initialized, use this backend client instead?

for the client in failover, i import the client define in docker repo, so we not need to care about the protocol of listcontainers..

jasonliu747

please resolve conflicts.

codecov-commenter · 2022-05-24T10:02:54Z

Codecov Report

Merging #128 (4696fdd) into main (dc28485) will decrease coverage by 2.70%.
The diff coverage is 3.24%.

❗ Current head 4696fdd differs from pull request most recent head 1c84f64. Consider uploading reports for the commit 1c84f64 to get more accurate results

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   61.23%   58.52%   -2.71%     
==========================================
  Files          97      100       +3     
  Lines        8791     9222     +431     
==========================================
+ Hits         5383     5397      +14     
- Misses       2958     3374     +416     
- Partials      450      451       +1

Flag	Coverage Δ
unittests	`58.52% <3.24%> (-2.71%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/runtimeproxy/server/docker/handler.go	`0.00% <0.00%> (ø)`
pkg/runtimeproxy/server/docker/server.go	`0.00% <0.00%> (ø)`
pkg/runtimeproxy/server/docker/utils.go	`17.50% <17.50%> (ø)`

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 dc28485...1c84f64. Read the comment docs.

pkg/runtime-manager/server/docker/server.go

hormes · 2022-05-25T07:03:08Z

pkg/runtime-manager/server/docker/server.go

+
+	ctx := context.TODO()
+	reqCopy := req.Clone(ctx)
+	if strings.Contains(req.URL.Path, "create") {


please refactor to

if contains create {
handleCreateRequest();
} else if contains xx {
handleXXRequest();
}

hormes · 2022-05-25T07:11:41Z

pkg/runtime-manager/server/docker/server.go

+
+func (d *RuntimeManagerDockerServer) ServeHTTP(wr http.ResponseWriter, req *http.Request) {
+	if !strings.Contains(req.URL.Path, "containers") {
+		d.Direct(wr, req)


missing return ？

hormes · 2022-05-25T08:04:48Z

pkg/runtime-manager/server/docker/server.go

+			http.Error(wr, "Failed to Unmarshal req body to docker config ", http.StatusInternalServerError)
+			return
+		}
+		config.ContainerName = req.URL.Query()["name"][0]


Query()["name"][0] Check for correctness?

jasonliu747

Please fix thoses CI errors.

pkg/runtime-manager/resource-executor/container.go

pkg/runtime-manager/resource-executor/pod.go

eahydra · 2022-05-27T02:57:23Z

pkg/runtime-manager/resource-executor/container.go

@@ -14,16 +14,18 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package cri
+package resource_executor


We should unify the style of the package name and remove the middle bar include the package pkg/runtime-manager

eahydra · 2022-05-27T03:02:31Z

pkg/runtime-manager/server/docker/handler.go

+	// 3. send req to pre-hook
+	runtimeHookPath := types.NoneRuntimeHookPath
+	if response, err := d.dispatcher.Dispatch(ctx, runtimeHookPath, types.PreHook, resourceExecutor.GenerateHookRequest()); err != nil {
+		klog.Errorf("fail to call hook server %v", err)


missing return error response?

please add UT for these files.

eahydra · 2022-05-27T03:15:39Z

pkg/runtime-manager/server/docker/handler.go

+	}
+
+	if response, err := d.dispatcher.Dispatch(ctx, runtimeHookPath, types.PreHook, resourceExecutor.GenerateHookRequest()); err != nil {
+		klog.Errorf("fail to call hook server %v", err)


missing return error?

eahydra · 2022-05-27T03:16:52Z

pkg/runtime-manager/server/docker/server.go

+		dispatcher: dispatcher.NewRuntimeDispatcher(),
+	}
+	criInterceptor.router = map[*regexp.Regexp]func(context.Context, http.ResponseWriter, *http.Request){
+		regexp.MustCompile("^/(v\\d\\.\\d+/)?containers(/\\w+)?/update$"): criInterceptor.HandleUpdateContainer,


Can we replace this custom regular implementation with a more mature route package?

jasonliu747

jasonliu747 · 2022-05-30T14:11:32Z

pkg/runtime-manager/server/docker/server.go

+}
+
+func (d *RuntimeManagerDockerServer) Run() error {
+


Redundant line

jasonliu747 · 2022-05-30T14:11:51Z

pkg/runtime-manager/server/docker/utils_test.go

+)
+
+func Test_calculateContentLength(t *testing.T) {
+	type tetCase struct {


tetCase -> testCase

jasonliu747 · 2022-05-30T14:13:33Z

cmd/runtime-manager/main.go

@@ -40,16 +43,22 @@ func main() {
 	pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
 	pflag.Parse()

+	dir, _ := filepath.Split(options.RuntimeManagerEndpoint)
+	_ = os.Mkdir(dir, 0777)


is it guaranteed to succeed?

Handle error;

os.MkdirAll may be better;

hormes · 2022-05-31T02:43:35Z

pkg/runtime-manager/server/docker/handler.go

+
+package docker
+
+import (


please create an issue to add tests for server and handler

stormgbs · 2022-05-31T02:28:24Z

cmd/runtime-manager/main.go

@@ -40,16 +43,22 @@ func main() {
 	pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
 	pflag.Parse()

+	dir, _ := filepath.Split(options.RuntimeManagerEndpoint)
+	_ = os.Mkdir(dir, 0777)


Handle error;

os.MkdirAll may be better;

stormgbs · 2022-05-31T02:35:25Z

pkg/runtime-manager/server/docker/handler.go

+
+	// no need to care about the resp
+	if _, err := d.dispatcher.Dispatch(ctx, runtimeHookPath, config.PreHook, hookReq); err != nil {
+		klog.Errorf("fail to call pre start container hook server %v", err)


fail -> Failed

stormgbs · 2022-05-31T02:35:37Z

pkg/runtime-manager/server/docker/handler.go

+	d.Direct(wr, req)
+
+	if _, err := d.dispatcher.Dispatch(ctx, runtimeHookPath, config.PostHook, hookReq); err != nil {
+		klog.Errorf("fail to call post start container hook server %v", err)


fail -> Failed

stormgbs · 2022-05-31T02:36:27Z

pkg/runtime-manager/server/docker/handler.go

+
+	response, err := d.dispatcher.Dispatch(ctx, runtimeHookPath, config.PreHook, hookReq)
+	if err != nil {
+		klog.Errorf("fail to call pre update hook server %v", err)


stormgbs · 2022-05-31T02:40:39Z

pkg/runtime-manager/server/docker/server.go

+
+	lis, err := net.Listen("unix", options.RuntimeManagerEndpoint)
+	if err != nil {
+		klog.Fatal("fail to create the lis %v", err)


Signed-off-by: Yue Zhang <huaihuan.zy@alibaba-inc.com>

hormes · 2022-05-31T09:19:12Z

/lgtm
/approve

koordinator-bot · 2022-05-31T09:19:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hormes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hormes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jasonliu747 · 2022-05-31T10:04:41Z

/hold cancel

koordinator-bot bot requested review from FillZpp and hormes May 9, 2022 07:42

koordinator-bot bot added size/XXL do-not-merge/hold labels May 9, 2022

honpey reviewed May 12, 2022

View reviewed changes

jasonliu747 assigned stormgbs and honpey May 23, 2022

ZYecho force-pushed the docker-proxy branch from 8adbdba to fc61496 Compare May 23, 2022 11:19

jasonliu747 requested changes May 24, 2022

View reviewed changes

jasonliu747 linked an issue May 24, 2022 that may be closed by this pull request

[feature request] runtime-manager docker proxy #64

Closed

jasonliu747 requested changes May 24, 2022

View reviewed changes

pkg/runtime-manager/server/docker/server.go Outdated Show resolved Hide resolved

hormes reviewed May 25, 2022

View reviewed changes

ZYecho force-pushed the docker-proxy branch 2 times, most recently from 8c21d7f to 347e2c4 Compare May 26, 2022 13:47

jasonliu747 requested changes May 26, 2022

View reviewed changes

pkg/runtime-manager/resource-executor/container.go Outdated Show resolved Hide resolved

pkg/runtime-manager/resource-executor/pod.go Outdated Show resolved Hide resolved

eahydra requested changes May 27, 2022

View reviewed changes

ZYecho force-pushed the docker-proxy branch 5 times, most recently from b6bbff7 to 224d3b3 Compare May 27, 2022 07:04

ZYecho requested review from jasonliu747, eahydra, honpey and hormes May 27, 2022 07:21

jasonliu747 requested changes May 27, 2022

View reviewed changes

ZYecho force-pushed the docker-proxy branch 2 times, most recently from e7daa54 to bff134a Compare May 28, 2022 02:07

ZYecho force-pushed the docker-proxy branch 6 times, most recently from 0db4e70 to 6dad85c Compare May 30, 2022 13:11

koordinator-bot bot added size/XL and removed size/XXL labels May 30, 2022

jasonliu747 reviewed May 30, 2022

View reviewed changes

hormes reviewed May 31, 2022

View reviewed changes

stormgbs reviewed May 31, 2022

View reviewed changes

ZYecho force-pushed the docker-proxy branch 4 times, most recently from 1efc6a6 to 4696fdd Compare May 31, 2022 08:55

feature: support docker proxy

1c84f64

Signed-off-by: Yue Zhang <huaihuan.zy@alibaba-inc.com>

ZYecho force-pushed the docker-proxy branch from 4696fdd to 1c84f64 Compare May 31, 2022 09:15

koordinator-bot bot assigned hormes May 31, 2022

koordinator-bot bot added the lgtm label May 31, 2022

koordinator-bot bot added the approved label May 31, 2022

koordinator-bot bot removed the do-not-merge/hold label May 31, 2022

koordinator-bot bot merged commit a66749d into koordinator-sh:main May 31, 2022

ZYecho deleted the docker-proxy branch May 31, 2022 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: support docker proxy #128

feature: support docker proxy #128

ZYecho commented May 9, 2022

honpey May 12, 2022

honpey May 12, 2022

ZYecho May 23, 2022

jasonliu747 left a comment •

edited

Loading

codecov-commenter commented May 24, 2022 •

edited

Loading

hormes May 25, 2022

ZYecho May 26, 2022

hormes May 25, 2022

ZYecho May 26, 2022

hormes May 25, 2022

jasonliu747 left a comment •

edited

Loading

eahydra May 27, 2022

ZYecho May 27, 2022

eahydra May 27, 2022

eahydra May 27, 2022

eahydra May 27, 2022

eahydra May 27, 2022

jasonliu747 left a comment

jasonliu747 May 30, 2022

jasonliu747 May 30, 2022

jasonliu747 May 30, 2022

stormgbs May 31, 2022

hormes May 31, 2022

ZYecho May 31, 2022

stormgbs May 31, 2022

stormgbs May 31, 2022

stormgbs May 31, 2022

stormgbs May 31, 2022

stormgbs May 31, 2022

hormes commented May 31, 2022

koordinator-bot bot commented May 31, 2022

jasonliu747 commented May 31, 2022

feature: support docker proxy #128

feature: support docker proxy #128

Conversation

ZYecho commented May 9, 2022

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonliu747 left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented May 24, 2022 • edited Loading

Codecov Report

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

jasonliu747 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonliu747 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hormes commented May 31, 2022

koordinator-bot bot commented May 31, 2022

jasonliu747 commented May 31, 2022

jasonliu747 left a comment •

edited

Loading

codecov-commenter commented May 24, 2022 •

edited

Loading

jasonliu747 left a comment •

edited

Loading