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: refact the setup spec mount #2653

Merged
merged 1 commit into from
Jan 8, 2019

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Jan 7, 2019

Ⅰ. Describe what this PR did

refect the setup spec mount, make it a logical structure as below:

  1. override the default spec mounts by container's config;
  2. setup container's mounts into spec mounts;
  3. setup or modify extra configs into spec mounts;
  4. sort all spec mounts.

Ⅱ. Does this pull request fix one issue?

NONE

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

NO

Ⅳ. Describe how to verify it

NA

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com

@rudyfly rudyfly requested a review from Ace-Tang January 7, 2019 04:21
@codecov
Copy link

codecov bot commented Jan 7, 2019

Codecov Report

Merging #2653 into master will increase coverage by 0.22%.
The diff coverage is 82.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2653      +/-   ##
=========================================
+ Coverage   69.38%   69.6%   +0.22%     
=========================================
  Files         280     280              
  Lines       18845   18857      +12     
=========================================
+ Hits        13075   13125      +50     
+ Misses       4293    4266      -27     
+ Partials     1477    1466      -11
Flag Coverage Δ
#criv1alpha1test 31.45% <65.51%> (+0.19%) ⬆️
#criv1alpha2test 35.79% <65.51%> (+0.13%) ⬆️
#integrationtest 41.62% <82.75%> (+0.02%) ⬆️
#nodee2etest 32.77% <65.51%> (+0.07%) ⬆️
#unittest 26.93% <0%> (-0.02%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/spec_mount.go 85.34% <82.75%> (-0.24%) ⬇️
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
ctrd/image.go 75.9% <0%> (-2.28%) ⬇️
daemon/mgr/container.go 59.21% <0%> (+0.42%) ⬆️
cri/v1alpha2/cri.go 67.82% <0%> (+0.61%) ⬆️
cri/v1alpha1/cri.go 60.09% <0%> (+0.66%) ⬆️
pkg/meta/store.go 68.99% <0%> (+1.55%) ⬆️
ctrd/client.go 65.73% <0%> (+1.68%) ⬆️
daemon/mgr/container_utils.go 85.11% <0%> (+1.78%) ⬆️
daemon/mgr/events.go 100% <0%> (+3.7%) ⬆️
... and 4 more

@@ -42,7 +42,12 @@ func clearReadonly(m *specs.Mount) {

// setupMounts create mount spec.
func setupMounts(ctx context.Context, c *Container, s *specs.Spec) error {
if c.HostConfig == nil {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Return error here

Copy link
Contributor

Choose a reason for hiding this comment

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

delete this line

@@ -121,10 +116,19 @@ func setupMounts(ctx context.Context, c *Container, s *specs.Spec) error {
mounts = append(mounts, generateNetworkMounts(c)...)
}

s.Mounts = sortMounts(mounts)
// modify share memory size, and change rw mode for privileged mode.
for i := range s.Mounts {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/s.Mounts/mounts/, use combined mount

@rudyfly rudyfly force-pushed the refect_mount_spec branch from c09ff0b to 58b7248 Compare January 7, 2019 05:54
@pouchrobot pouchrobot added size/M and removed size/S labels Jan 7, 2019
@allencloud
Copy link
Collaborator

Could we separate the function setupMounts with more readable functions?
Every function contains the detailed implementation of the code block.

I think it is too long and a little bit difficult to understand. @rudyfly @Ace-Tang

@allencloud allencloud changed the title refector: refect the setup spec mount refactor: refact the setup spec mount Jan 7, 2019
@rudyfly rudyfly force-pushed the refect_mount_spec branch from 58b7248 to 0495722 Compare January 7, 2019 12:32
@pouchrobot pouchrobot added size/S and removed size/M labels Jan 7, 2019
@rudyfly rudyfly force-pushed the refect_mount_spec branch from 0495722 to 71c32e8 Compare January 7, 2019 12:56
@pouchrobot pouchrobot added size/M and removed size/S labels Jan 7, 2019
refact the setup spec mount, make it a logical structure as below:
1. override the default spec mounts by container's config;
2. setup container's mounts into spec mounts;
3. setup or modify extra configs into spec mounts;
4. sort all spec mounts.

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
@rudyfly rudyfly force-pushed the refect_mount_spec branch from 71c32e8 to 9f904bd Compare January 7, 2019 13:18
@rudyfly
Copy link
Collaborator Author

rudyfly commented Jan 8, 2019

PTAL @Ace-Tang @HusterWan @allencloud

// setupMounts create mount spec.
func setupMounts(ctx context.Context, c *Container, s *specs.Spec) error {
var (
mounts []specs.Mount
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to define this parameter, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it need.

func setupMounts(ctx context.Context, c *Container, s *specs.Spec) error {
var mounts []specs.Mount
// Override the default mounts which are duplicate with user defined ones.
func overrideDefaultMount(mounts []specs.Mount, c *Container, s *specs.Spec) ([]specs.Mount, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add some test cases for the new function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It isn't complicated logic, I think it no need to add unit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should add, but can be in another pr

@Ace-Tang
Copy link
Contributor

Ace-Tang commented Jan 8, 2019

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 8, 2019
@Ace-Tang Ace-Tang merged commit 22bfd53 into AliyunContainerService:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants