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

refactor: eliminate containerMeta in daemon manager #1300

Merged

Conversation

allencloud
Copy link
Collaborator

@allencloud allencloud commented May 10, 2018

Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com

Ⅰ. Describe what this PR did

In the ContainerMgr of original code, we have two structs which describe container instance, one is Container, and the other is ContainerMeta. While we have the following relationship between them:

type ContainerMeta struct {
    sync.Mutex
    *Container
    Detachkeys string
}

So, we can find that ContainerMeta has a very similar construction of Container. So we could only make use of Container to eliminate one to make code and data structure much more simplified.

Ⅱ. Does this pull request fix one issue?

none

Ⅲ. Describe how you did it

none

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

@allencloud allencloud force-pushed the deliminate-containerMeta branch 3 times, most recently from 2a4014b to e9426cb Compare May 10, 2018 17:09
@allencloud
Copy link
Collaborator Author

With this PR, I think api image deletion test fails:

FAIL: /go/src/github.com/alibaba/pouch/test/api_image_delete_test.go:31: APIImageDeleteSuite.TestDeleteUsingImage
/go/src/github.com/alibaba/pouch/test/api_image_delete_test.go:38:
    CheckRespStatus(c, resp, 500)
/go/src/github.com/alibaba/pouch/test/util_api.go:21:
    c.Assert(resp.StatusCode, check.Equals, status, check.Commentf("Error:%s", got.Message))
... obtained int = 204
... expected int = 500
... Error:

I am wondering if this is related to the reference struct. Could you help to take a look at the failure? @fuweid Thanks a lot.

@fuweid
Copy link
Contributor

fuweid commented May 11, 2018

Got it @allencloud

@allencloud allencloud force-pushed the deliminate-containerMeta branch 3 times, most recently from 066cbfc to 7c6363c Compare May 11, 2018 02:53
@allencloud
Copy link
Collaborator Author

It also reports panic error like:

PANIC: /go/src/github.com/alibaba/pouch/test/z_cli_daemon_test.go:229: PouchDaemonSuite.TestDaemonRestart
... Panic: runtime error: index out of range (PC=0x45AF0A)
/usr/local/go/src/runtime/panic.go:491
  in gopanic
/usr/local/go/src/runtime/panic.go:28
  in panicindex
/go/src/github.com/alibaba/pouch/test/z_cli_daemon_test.go:268
  in PouchDaemonSuite.TestDaemonRestart
/usr/local/go/src/reflect/value.go:302
  in Value.Call
/go/src/github.com/alibaba/pouch/vendor/github.com/go-check/check/check.go:772
  in suiteRunner.forkTest.func1
/go/src/github.com/alibaba/pouch/vendor/github.com/go-check/check/check.go:666
  in suiteRunner.forkCall.func1
/usr/local/go/src/runtime/asm_amd64.s:2337
  in goexit
----------------------------------------------------------------------
PANIC: /go/src/github.com/alibaba/pouch/test/z_cli_daemon_test.go:272: PouchDaemonSuite.TestDaemonRestartWithPausedContainer
... Panic: runtime error: index out of range (PC=0x45AF0A)
/usr/local/go/src/runtime/panic.go:491
  in gopanic
/usr/local/go/src/runtime/panic.go:28
  in panicindex
/go/src/github.com/alibaba/pouch/test/z_cli_daemon_test.go:316
  in PouchDaemonSuite.TestDaemonRestartWithPausedContainer
/usr/local/go/src/reflect/value.go:302
  in Value.Call
/go/src/github.com/alibaba/pouch/vendor/github.com/go-check/check/check.go:772
  in suiteRunner.forkTest.func1
/go/src/github.com/alibaba/pouch/vendor/github.com/go-check/check/check.go:666
  in suiteRunner.forkCall.func1
/usr/local/go/src/runtime/asm_amd64.s:2337
  in goexit

@allencloud allencloud force-pushed the deliminate-containerMeta branch from 7c6363c to 17d4408 Compare May 11, 2018 03:44
@codecov-io
Copy link

codecov-io commented May 11, 2018

Codecov Report

Merging #1300 into master will increase coverage by 0.03%.
The diff coverage is 4.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1300      +/-   ##
==========================================
+ Coverage   16.42%   16.46%   +0.03%     
==========================================
  Files         182      182              
  Lines       11303    11277      -26     
==========================================
  Hits         1857     1857              
+ Misses       9310     9284      -26     
  Partials      136      136
Impacted Files Coverage Δ
daemon/mgr/spec_linux.go 0% <0%> (ø) ⬆️
daemon/mgr/spec_process.go 0% <0%> (ø) ⬆️
daemon/mgr/cri.go 0% <0%> (ø) ⬆️
daemon/mgr/container.go 0% <0%> (ø) ⬆️
daemon/mgr/container_exec.go 0% <0%> (ø) ⬆️
daemon/mgr/spec_annotations.go 0% <0%> (ø) ⬆️
daemon/mgr/system.go 0% <0%> (ø) ⬆️
apis/server/image_bridge.go 15% <0%> (ø) ⬆️
daemon/mgr/spec_hook.go 0% <0%> (ø) ⬆️
apis/server/container_bridge.go 0% <0%> (ø) ⬆️
... and 9 more

@allencloud allencloud changed the title refactor: eliminate containerMeta refactor: eliminate containerMeta in daemon manager May 11, 2018
container.NetworkSettings = &types.ContainerNetworkSettings{
Networks: m.NetworkSettings.Networks,
}
container := types.Container{
Copy link
Contributor

Choose a reason for hiding this comment

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

types.Container and ContainerMgr.Container may be confused, can we named different to distinct this two containers struct ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, PTAL @HusterWan

Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
@allencloud allencloud force-pushed the deliminate-containerMeta branch from 17d4408 to 6920221 Compare May 11, 2018 05:46
@HusterWan
Copy link
Contributor

I have reviewed part of pouchd, LGTM . cri part need @YaoZengzeng to review

@YaoZengzeng
Copy link
Contributor

Only name changing (ContainerMeta -> Container) in CRI side and the CI passed as well.

So LGTM.

@allencloud
Copy link
Collaborator Author

Thanks a lot for your review. @HusterWan @YaoZengzeng 🍻

@allencloud allencloud merged commit c83b0cb into AliyunContainerService:master May 11, 2018
@allencloud allencloud deleted the deliminate-containerMeta branch May 11, 2018 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants