Skip to content

define MemoryUnlimited const for users #1422

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
Closed
sjenning wants to merge 1 commit into from
Closed

define MemoryUnlimited const for users #1422

sjenning wants to merge 1 commit into from

Conversation

@sjenning
Copy link
Contributor

@sjenning sjenning commented Apr 26, 2017

Fixes #1421

Currently, a user wanting to set MemorySwap to unlimited has to do a messy trick:

unlimited := -1
res.MemorySwap = uint64(unlimited)

as uint64(-1) doesn't compile.

This PR defines a MemoryUnlimited constant to make this cleaner.

@derekwaynecarr @mrunalp

@derekwaynecarr
Copy link
Contributor

derekwaynecarr commented Apr 26, 2017

ideally we could have this in the rc and not just master...

@mrunalp
Copy link
Contributor

mrunalp commented Apr 26, 2017

@sjenning Could you sign the commit?

wking
wking reviewed Apr 26, 2017
View reviewed changes
libcontainer/cgroups/fs/memory.go Outdated
if cgroup.Resources.Memory == uint64(ulimited) {
// If the memory update is set to MemoryUnlimited we should also
// set swap to MemoryUnlimited.
if cgroup.Resources.Memory == uint64(configs.MemoryUnlimited) {
Copy link
Contributor

@wking wking Apr 26, 2017

Choose a reason for hiding this comment

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

With this commit, MemoryUnlimited is defined as a unit64, so I think you can drop the redundant cast from this line (and the later lines which also have uint64(configs.MemoryUnlimited).

@sjenning sjenning force-pushed the clean-up-unlimited branch from 31c2e0f to 0d64996 Compare April 26, 2017 20:28
@sjenning
Copy link
Contributor Author

sjenning commented Apr 26, 2017

@mrunalp @wking signed the commit and removed the unneeded type cast

@cyphar
Copy link
Member

cyphar commented Apr 26, 2017

@sjenning "Sign" refers the Signed-off-by line in your commit message. You can add it with the -s flag to git commit --amend (it signals that you can attest the Developer Certificate of Origin).

@sjenning sjenning force-pushed the clean-up-unlimited branch from 0d64996 to 369f710 Compare April 26, 2017 20:35
@sjenning
Copy link
Contributor Author

sjenning commented Apr 26, 2017

@cyphar oops! fixed.

@mrunalp
Copy link
Contributor

mrunalp commented Apr 27, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

@hqhq
Copy link
Contributor

hqhq commented May 3, 2017

@sjenning Sorry for the confusion. uint64(-1) is not some random trick, I use it because -1 is what cgroup api defines for unlimited usage, and we also export it to runc update and use this trick in https://github.com/opencontainers/runc/blob/v1.0.0-rc3/update.go#L228 , so what memory.go does just pairs that.

I agree this can be more explicit, but I don't see how ^uint64(0) could be less messy, why not just use math.MaxUint64? And if you do this change, you may also need to fix what we did in update.go.

Thanks for doing this.

@sjenning sjenning force-pushed the clean-up-unlimited branch from 369f710 to 2a0c037 Compare May 5, 2017 02:58
@sjenning
Copy link
Contributor Author

sjenning commented May 5, 2017

@hqhq thanks for the review. I've made the requested changes.

dqminh
dqminh reviewed May 7, 2017
View reviewed changes
libcontainer/cgroups/fs/memory.go
// Only set swap if it's enbled in kernel
if cgroups.PathExists(filepath.Join(path, cgroupMemorySwapLimit)) {
cgroup.Resources.MemorySwap = uint64(ulimited)
cgroup.Resources.MemorySwap = uint64(configs.MemoryUnlimited)
Copy link
Contributor

@dqminh dqminh May 7, 2017

Choose a reason for hiding this comment

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

i think you dont need to cast this to uint64 anymore

libcontainer/cgroups/fs/memory.go
// for memory and swap memory, so it won't fail because the new
// value and the old value don't fit kernel's validation.
if cgroup.Resources.MemorySwap == uint64(ulimited) || memoryUsage.Limit < cgroup.Resources.MemorySwap {
if cgroup.Resources.MemorySwap == uint64(configs.MemoryUnlimited) || memoryUsage.Limit < cgroup.Resources.MemorySwap {
Copy link
Contributor

@dqminh dqminh May 7, 2017

Choose a reason for hiding this comment

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

same as the above, casting is not necessary.

@sjenning sjenning force-pushed the clean-up-unlimited branch from 2a0c037 to 5663b26 Compare May 7, 2017 03:03
justincormack
justincormack reviewed Jun 23, 2017
View reviewed changes
libcontainer/configs/cgroup_unix.go
*Resources
}

const MemoryUnlimited = math.MaxUint64
Copy link
Contributor

@justincormack justincormack Jun 23, 2017

Choose a reason for hiding this comment

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

This is not the value that is actually used by the kernel, which is 9223372036854771712. Some older kernels will set any value larger than this to 0, and then crash, eg RHEL 7.2.

Copy link
Contributor

@justincormack justincormack Jun 23, 2017

Choose a reason for hiding this comment

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

Also this value may well be totally incorrect on 32 bit machines, not sure what happens there.

@tiborvass tiborvass mentioned this pull request Jun 23, 2017
Closed
@hqhq
Copy link
Contributor

hqhq commented Jun 23, 2017

I've carried this PR in #1494 , thanks for your contribution @sjenning .

@hqhq hqhq closed this Jun 23, 2017
justincormack added a commit to justincormack/runc that referenced this pull request Jun 24, 2017
@justincormack
Update memory specs to use int64 not uint64
48f04f8 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
@justincormack justincormack mentioned this pull request Jun 24, 2017
Merged
justincormack added a commit to justincormack/runc that referenced this pull request Jun 27, 2017
@justincormack
Update memory specs to use int64 not uint64
3d9074e 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
tiborvass pushed a commit to tiborvass/runc that referenced this pull request Jul 6, 2017
@justincormack
Update memory specs to use int64 not uint64
654477c 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
(cherry picked from commit 3d9074e)
Signed-off-by: Tibor Vass <[email protected]>
tiborvass pushed a commit to tiborvass/runc that referenced this pull request Jul 6, 2017
@justincormack
Update memory specs to use int64 not uint64
2a13a01 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
(cherry picked from commit 3d9074e)
Signed-off-by: Tibor Vass <[email protected]>
adrianreber pushed a commit to adrianreber/runc that referenced this pull request Jul 27, 2017
@justincormack @adrianreber
Update memory specs to use int64 not uint64
eec6814 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Oct 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dqminh dqminh dqminh left review comments

+2 more reviewers

@wking wking wking left review comments

@justincormack justincormack justincormack left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@sjenning @derekwaynecarr @mrunalp @cyphar @hqhq @dqminh @wking @justincormack