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

Revert "Use update time to detect if kmem limits have been set" #961

Merged
merged 1 commit into from
Jul 21, 2016

Conversation

hqhq
Copy link
Contributor

@hqhq hqhq commented Jul 21, 2016

Revert: #935
Fixes: #946

I can reproduce #946 on some machines, the problem is on
some machines, it could be very fast that modify time
of memory.kmem.limit_in_bytes could be the same as
before it's modified.

And now we'll call SetKernelMemory twice on container
creation which cause the second time fail.

Revert this before we find a better solution.

Signed-off-by: Qiang Huang [email protected]

Revert: opencontainers#935
Fixes: opencontainers#946

I can reproduce opencontainers#946 on some machines, the problem is on
some machines, it could be very fast that modify time
of `memory.kmem.limit_in_bytes` could be the same as
before it's modified.

And now we'll call `SetKernelMemory` twice on container
creation which cause the second time failure.

Revert this before we find a better solution.

Signed-off-by: Qiang Huang <[email protected]>
@hqhq
Copy link
Contributor Author

hqhq commented Jul 21, 2016

We should reopen #841 if we merge this.

@hqhq
Copy link
Contributor Author

hqhq commented Jul 21, 2016

As the problem that we'll call SetKernelMemory twice on container creation, I can file a PR to fix this, but it didn't fix the problem, if modify time of memory.kmem.limit_in_bytes is still the same as memory.kmem.max_usage_in_bytes we'll fail if we want to update kernel memory of the container.

@cyphar
Copy link
Member

cyphar commented Jul 21, 2016

LGTM. I see this alot, and was one of my concerns with the original PR (using the modified time for attach logic). We should figure out a proper fix for this problem.

Approved with PullApprove

@dqminh
Copy link
Contributor

dqminh commented Jul 21, 2016

LGTM

Approved with PullApprove

@dqminh dqminh merged commit f0e17e9 into opencontainers:master Jul 21, 2016
@dqminh
Copy link
Contributor

dqminh commented Jul 21, 2016

@hqhq @cyphar @vishh i've reopened #841

@hqhq hqhq deleted the revert_935 branch July 22, 2016 01:09
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.

4 participants