Skip to content

feature: cli supports --memory-reservation#2860

Merged
fuweid merged 1 commit into
AliyunContainerService:masterfrom
KevinBetterQ:add_memory-reservation
May 30, 2019
Merged

feature: cli supports --memory-reservation#2860
fuweid merged 1 commit into
AliyunContainerService:masterfrom
KevinBetterQ:add_memory-reservation

Conversation

@KevinBetterQ
Copy link
Copy Markdown
Contributor

@KevinBetterQ KevinBetterQ commented May 23, 2019

Signed-off-by: KevinBetterQ 1093850932@qq.com

Ⅰ. Describe what this PR did

pouch command supports --memory-reservation

Ⅱ. Does this pull request fix one issue?

fixes #2701

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

added

Ⅳ. Describe how to verify it

# pouch run -d --memory-reservation 4M busybox top
58369a469f0579234538717575d0338eb969b4009c6136ef32b678c432d8c072

# pouch inspect -f {{.HostConfig.MemoryReservation}} 5836
4194304

# cat /sys/fs/cgroup/memory/default/58369a469f0579234538717575d0338eb969b4009c6136ef32b678c432d8c072/memory.soft_limit_in_bytes
4194304

Ⅴ. Special notes for reviews

@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2019 •
edited
Loading

Codecov Report

Merging #2860 into master will decrease coverage by 0.02%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2860      +/-   ##
==========================================
- Coverage    69.1%   69.07%   -0.03%     
==========================================
  Files         285      286       +1     
  Lines       17887    17903      +16     
==========================================
+ Hits        12360    12366       +6     
- Misses       4120     4131      +11     
+ Partials     1407     1406       -1
Flag Coverage Δ
#criv1alpha2_test 39.03% <25%> (-0.05%) ⬇️
#integration_test_0 36.69% <25%> (+0.03%) ⬆️
#integration_test_1 35.38% <25%> (-0.41%) ⬇️
#integration_test_2 36.69% <60%> (+0.13%) ⬆️
#integration_test_3 35.42% <25%> (-0.1%) ⬇️
#node_e2e_test 34.79% <25%> (-0.08%) ⬇️
#unittest 28.56% <40%> (+0.11%) ⬆️
Impacted Files Coverage Δ
daemon/mgr/container_types.go 71.32% <ø> (ø) ⬆️
pkg/system/cgroup.go 93.65% <100%> (+0.1%) ⬆️
daemon/mgr/spec_linux.go 79.37% <100%> (+0.21%) ⬆️
apis/opts/memory_reservation.go 100% <100%> (ø)
daemon/mgr/container_validation.go 42.79% <50%> (+0.26%) ⬆️
pkg/streams/utils.go 82.14% <0%> (-7.15%) ⬇️
pkg/meta/store.go 67.44% <0%> (-1.56%) ⬇️
daemon/mgr/container_utils.go 76.76% <0%> (-1.02%) ⬇️
ctrd/container.go 54.68% <0%> (+0.76%) ⬆️

@KevinBetterQ KevinBetterQ force-pushed the add_memory-reservation branch 2 times, most recently from b810783 to 268a54a Compare May 23, 2019 11:13
@CodeJuan
Copy link
Copy Markdown
Contributor

CodeJuan commented May 24, 2019 •
edited
Loading

@KevinBetterQ

Is it necessary to validate memory-reservation in fuction validateResource? I'm not sure there is a limitation of kernel that memory-reservation should be less than memory?
WDYT? @HusterWan @Ace-Tang

ZYecho
ZYecho reviewed May 24, 2019
View reviewed changes
Comment thread test/environment/env.go Outdated
return cgroupInfo.Memory.MemoryLimit
}

// IsMemoryReservationSupport checks if memory reservation cgroup is avaible
Copy link
Copy Markdown
Contributor

@ZYecho ZYecho May 24, 2019

Choose a reason for hiding this comment

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

avaible -> avaiable?

Copy link
Copy Markdown
Contributor Author

@KevinBetterQ KevinBetterQ May 24, 2019

Choose a reason for hiding this comment

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

yes, it's a typo

@CodeJuan
Copy link
Copy Markdown
Contributor

CodeJuan commented May 24, 2019

Hi @KevinBetterQ ,

I was wondering if you could help to develop a new feature update memory-reservation by cli at you convenience?

@KevinBetterQ
Copy link
Copy Markdown
Contributor Author

KevinBetterQ commented May 24, 2019

Hi @KevinBetterQ ,

I was wondering if you could help to develop a new feature update memory-reservation by cli at you convenience?

OK, I think it is a similar call in the code

@weekly-digest weekly-digest Bot mentioned this pull request May 26, 2019
Closed
@KevinBetterQ KevinBetterQ force-pushed the add_memory-reservation branch from 268a54a to 6d46cd1 Compare May 28, 2019 07:23
@pouchrobot pouchrobot added size/XL and removed size/L labels May 28, 2019
HusterWan
HusterWan reviewed May 28, 2019
View reviewed changes
Comment thread apis/opts/memory_reservation.go Outdated
if memoryReservation == "" {
return 0, nil
}
result, err := units.RAMInBytes(memoryReservation)
Copy link
Copy Markdown
Contributor

@HusterWan HusterWan May 28, 2019

Choose a reason for hiding this comment

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

how about direct return units.RAMInBytes(memoryReservation)

Copy link
Copy Markdown
Contributor Author

@KevinBetterQ KevinBetterQ May 29, 2019

Choose a reason for hiding this comment

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

modified

Copy link
Copy Markdown
Contributor Author

@KevinBetterQ KevinBetterQ May 29, 2019

Choose a reason for hiding this comment

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

if there is an error, the upper function will always return, will not care about the result. and will return directly to the run execution failure, so there is no need to set this value.
Is that right?

@KevinBetterQ KevinBetterQ force-pushed the add_memory-reservation branch 2 times, most recently from 55c3bc2 to ed5b3d3 Compare May 29, 2019 05:02
CodeJuan
CodeJuan reviewed May 29, 2019
View reviewed changes
Comment thread test/cli_run_memory_test.go
fuweid
fuweid reviewed May 29, 2019
View reviewed changes
Comment thread test/cli_run_memory_test.go Outdated
// test run with invalid memory reservation
cname := "TestRunWithMemoryReservationInvalid"
res := command.PouchRun("run", "-d",
"-m", "500m",
Copy link
Copy Markdown
Contributor

@fuweid fuweid May 29, 2019

Choose a reason for hiding this comment

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

could we use less memory here? According to the testing machine, I am not sure that it will impact the case.

@KevinBetterQ KevinBetterQ force-pushed the add_memory-reservation branch 3 times, most recently from 06e5afc to 41c9d02 Compare May 29, 2019 09:29
@KevinBetterQ
feature: cli supports --memory-reservation
fff42a7 
Signed-off-by: KevinBetterQ <1093850932@qq.com>
@KevinBetterQ KevinBetterQ force-pushed the add_memory-reservation branch from 41c9d02 to fff42a7 Compare May 29, 2019 11:09
fuweid
fuweid reviewed May 30, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 7a98154 into AliyunContainerService:master May 30, 2019
@pouchrobot pouchrobot mentioned this pull request May 31, 2019
Closed
@weekly-digest weekly-digest Bot mentioned this pull request Jun 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@CodeJuan CodeJuan CodeJuan left review comments

@ZYecho ZYecho ZYecho left review comments

@HusterWan HusterWan HusterWan left review comments

@fuweid fuweid fuweid left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind/feature size/XL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[feature request] pouch cli supports --memory-reservation

6 participants

@KevinBetterQ @CodeJuan @ZYecho @HusterWan @fuweid @pouchrobot