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

[MRG] Update base image used for memory limit checks #677

Merged
merged 6 commits into from May 8, 2019

Conversation

@betatim
Copy link
Member

betatim commented May 7, 2019

Locally this still fails :-/

@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented May 7, 2019

It seems like (locally) the limit doesn't actually get applied/used :-/ I'll need a bit of time to investigate what is going on.

@Xarthisius

This comment has been minimized.

Copy link
Contributor

Xarthisius commented May 7, 2019

Locally this still fails :-/

@betatim I vaguely remember that I had problem imposing mem limits for docker using python API. It accepted the size notation but it wasn't setting the limit properly. I'd try to use number of bytes explicitly.

@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented May 7, 2019

When we finally actually call the docker client the size is an integer without a postfix :-/

@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented May 8, 2019

Mystery solved. The docker-py documentation says that a value of -1 will disable swap, which we were using. However https://docs.docker.com/engine/reference/commandline/build/ specifies that -1 means "unlimited swap". In the docker-py code I can't find anywhere where they do processing of the values we passed in.

Updated our code to reflect this new knowledge. Though I am confused why all tests have passed on travis (for 5adc4b2) when they fail locally. Speculation: there is no swap for the whole machine on travis?

@betatim betatim force-pushed the betatim:fix-memlimit-image branch from 8588fe2 to 032baf6 May 8, 2019
@betatim betatim changed the title [WIP] Update base image used for memory limit checks [MRG] Update base image used for memory limit checks May 8, 2019
@yuvipanda

This comment has been minimized.

Copy link
Collaborator

yuvipanda commented May 8, 2019

I dug around a bit more, and found docker/docker.github.io#4335. Is there a reason we're setting memory swap to be one byte more, instead of exactly the same?

@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented May 8, 2019

When I had it set to 1 (a guess for "if we can't disable then let's use 1byte") I got an error the said "swap has to be bigger than memory limit", I didn't check if they actually meant "equal or larger". Checking now.

@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented May 8, 2019

Looks like setting it to the same amount works! That is a nicer solution :D Thanks for asking.

@yuvipanda

This comment has been minimized.

Copy link
Collaborator

yuvipanda commented May 8, 2019

@betatim awesome! I changed the comment around it a little, otherwise it LGTM.

@yuvipanda yuvipanda merged commit 2086b7a into jupyter:master May 8, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 100% of diff hit (target 20%)
Details
codecov/project 90.93% (+0.03%) compared to d674ece
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@yuvipanda

This comment has been minimized.

Copy link
Collaborator

yuvipanda commented May 8, 2019

Merged with @betatim's blessing. <3

@betatim betatim deleted the betatim:fix-memlimit-image branch May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.