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

feature: add run flag ulimit #1179

Merged
merged 1 commit into from
Apr 26, 2018
Merged

feature: add run flag ulimit #1179

merged 1 commit into from
Apr 26, 2018

Conversation

Ace-Tang
Copy link
Contributor

Signed-off-by: Ace-Tang [email protected]

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Apr 22, 2018

Codecov Report

Merging #1179 into master will decrease coverage by 0.1%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1179      +/-   ##
=========================================
- Coverage    15.7%   15.6%   -0.11%     
=========================================
  Files         171     172       +1     
  Lines       10398   10433      +35     
=========================================
- Hits         1633    1628       -5     
- Misses       8645    8685      +40     
  Partials      120     120
Impacted Files Coverage Δ
cli/common_flags.go 0% <0%> (ø) ⬆️
cli/container.go 0% <0%> (ø) ⬆️
daemon/mgr/spec_process.go 0% <0%> (ø) ⬆️
cli/ulimit.go 0% <0%> (ø)
daemon/mgr/container.go 0% <0%> (ø) ⬆️
pkg/bytefmt/bytefmt.go 88.88% <0%> (-0.95%) ⬇️

@@ -978,3 +978,24 @@ func (suite *PouchRunSuite) TestRunWithVolumesFrom(c *check.C) {

c.Assert(volumeFound, check.Equals, true)
}

// TestRunWithUlimit tests running container with --ulimit flag.
func (suite *PouchRunSuite) TestRunWithUlimit(c *check.C) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a test which is to to test if the value is in the container meta data. Is there a way to check whether this configuration takes effect in the running container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allencloud , I think you are not take a careful look at test , it test the ulimit set output,.

out := res.Stdout()
 c.Assert(out, check.Equals, "256\n")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha, my fault.

cli/ulimit.go Outdated
}

// value return ulimit values as type ResourcesUlimitsItems0
func (u *Ulimit) value() []*types.ResourcesUlimitsItems0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ResourcesUlimitsItems0 is not in the correct way of api types. @fuweid This is in API side. Please help to take a look.

In addition, if this file is reasonable to locate here. How about package opts? @HusterWan

Copy link
Contributor Author

@Ace-Tang Ace-Tang Apr 23, 2018

Choose a reason for hiding this comment

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

ResourcesUlimitsItems0 is generated before, I also do not like it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@allencloud I have synced with @Ace-Tang offline. we can define ulimit out of the resource so that we can avoid the default composed name.

Copy link
Collaborator

@allencloud allencloud Apr 23, 2018

Choose a reason for hiding this comment

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

That is good. If we have figured out the way, then let us kick the ball. @Ace-Tang @fuweid

@@ -30,6 +30,7 @@ var setupFunc = []SetupFunc{
setupCap,
setupNoNewPrivileges,
setupOOMScoreAdj,
setupUlimits,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish that we could merge #1154 first. This refactoring is quite important I think. I suggest promoting that pr's priority.

Otherwise conflict is there.

support ulimit in run and exec process

Signed-off-by: Ace-Tang <[email protected]>
@pouchrobot pouchrobot added size/XL and removed size/L labels Apr 25, 2018
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 26, 2018
@allencloud allencloud merged commit 6d7aa83 into AliyunContainerService:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants