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.

Sign up for GitHub

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

Jump to bottom

feature: cli supports --memory-reservation #2860

Merged
merged 1 commit into from
May 30, 2019
Merged

feature: cli supports --memory-reservation #2860

merged 1 commit into from
May 30, 2019

Conversation

KevinBetterQ
Copy link
Contributor

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

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
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
test/environment/env.go Outdated
@@ -80,6 +80,11 @@ var (
return cgroupInfo.Memory.MemoryLimit
}

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

Choose a reason for hiding this comment

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

avaible -> avaiable?

Copy link
Contributor Author

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
Contributor

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
Contributor Author

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
@pouchrobot pouchrobot added size/XL and removed size/L labels May 28, 2019
HusterWan
HusterWan reviewed May 28, 2019
View reviewed changes
apis/opts/memory_reservation.go Outdated
if memoryReservation == "" {
return 0, nil
}
result, err := units.RAMInBytes(memoryReservation)
Copy link
Contributor

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
Contributor Author

Choose a reason for hiding this comment

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

modified

Copy link
Contributor Author

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
fuweid
fuweid reviewed May 29, 2019
View reviewed changes
test/cli_run_memory_test.go Outdated
// test run with invalid memory reservation
cname := "TestRunWithMemoryReservationInvalid"
res := command.PouchRun("run", "-d",
"-m", "500m",
Copy link
Contributor

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>
fuweid
fuweid reviewed May 30, 2019
View reviewed changes
Copy link
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

@fuweid fuweid fuweid left review comments

@HusterWan HusterWan HusterWan left review comments

@ZYecho ZYecho ZYecho left review comments

@CodeJuan CodeJuan CodeJuan left review comments

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