Implemented memory and CPU limits for LCOW. #37296
Conversation
|
You need to run Also, could you add unit tests? |
|
I use VSCode and it automatically applies those tools on save. I cannot see any formatting or importing issues. If there is, can yo point me to it? For unit testing, I can add test to https://github.com/moby/moby/blob/master/integration-cli/docker_cli_run_test.go, however, the docs says it is deprecated. I couldn't find a way to launch LCOW test from the new integration test suite. By the way #37294 is very similar to this one and it is getting merged without any tests. |
|
Yeah, LCOW doesn't currently run in CI here (I think that's being worked on) @yusuf-gunaydin looks like this needs a rebase (it shows there's a merge conflict); can you update the PR? |
|
Please sign your commits following these rules: $ git clone -b "lcow_limits" git@github.com:yusuf-gunaydin/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354292296
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
414577b to
c98b26a
Compare
|
@thaJeztah Resolved the conflict. However, I am not sure why Janky build fails. The failing test is TestSwarmClusterRotateUnlockKey and it doesn't seem to be related to this PR. |
|
May I enquire the status of this? I tested @yusuf-gunaydin 's branch and it indeed resolved the memory starvation we had by allowing more memory to be assigned via command line. |
|
ping @johnstep @jhowardmsft PTAL @yusuf-gunaydin looks like there's a merge-commit in this PR; could you rebase your branch, and squash the commits down to a single commit? Let me know if you need help doing so |
|
FYI, the memory limit actually works, but CPU doesn't. the configuration is valid and goes to daemon as expected, but Windows doesn't yet handle the parameter properly. So when --cpu 20 is used, container always only sees 1. |
4e28575 to
74c57a3
Compare
Codecov Report
@@ Coverage Diff @@
## master #37296 +/- ##
=========================================
Coverage ? 36.6%
=========================================
Files ? 610
Lines ? 45237
Branches ? 0
=========================================
Hits ? 16561
Misses ? 26394
Partials ? 2282 |
74c57a3 to
062ac48
Compare
|
@thaJeztah I have applied the rebase. I assume the failing builds are unrelated as other pull requests fail with the same errors. @lahma I tested with this command and result is correct: This PR changes the limits on the created Hyper-V machine. I don't think there is an issue with Windows not supporting multiple CPUs with Hyper-V. |
|
@yusuf-gunaydin sorry, I left out the fact that my use case is with Windows Server 2019 Preview (build 17733 if I remember right). So it has lacking support for the CPU parameter at least per my testing (/proc/cpuinfo). Bleeding edge with quirks... |
|
@beweedon just in case. As mentioned offline |
|
In last docker edge version, the issue still exist. |
|
@orest-gulman Yes, that will be the case until this PR has been merged into moby/moby and docker-ce/ee takes the patch :/ |
|
@johnstep & @thaJeztah If i can add my 2c, can this go in? |
|
@yusuf-gunaydin I can only apologise on behalf of other maintainers that this seems to keep being ignored. Unfortunately it needs another rebase. @thaJeztah @johnstep @cpuguy83 what I can I do to convince other maintainers that this is required (for both local inprocess and the containerd work)? There’s no shortage of people who have indicated it’s a blocking issue for them, and it’s been ongoing for many many months now. |
|
Thanks @cpuguy83. @yusuf-gunaydin If you can rebase, we should be able to get this merged. Thanks! |
Signed-off-by: Yusuf Tarık Günaydın <yusuf_tarik@hotmail.com>
7744f44 to
86bd2e9
Compare
|
@jhowardmsft Rebased again. Experimental build failed with a swarm test saying that context deadline exceeded. I think it's not related to this one, but I am not sure. |
|
Thank you @vdemeester! (and @yusuf-gunaydin!) |
@jhowardmsft Now we need to wait for an update docker ce?, thanks |
|
Do you know if there's anything we can track on Docker's side to know they took the merge? |
|
Not that I know of. It will be In master.dockerproject.org binaries now. CE and EE will probably take a cut in March for their 19.03 release. I doubt this would be backported to 18.09 or earlier as LCOW remains an experimental feature. @thaJeztah may know a better way to track than I do, but I think what I said is pretty accurate. |
|
docker-ce repository has an docker-ce is pulling in commit docker-archive/docker-ce@c385bd1 as of 2/6/2019, which, according to the commit, represents commit f8e29fd from this repo f8e29fd includes the merge of this PR (#37296), so the nightly builds from master at https://master.dockerproject.com/windows/x86_64/docker.zip should now contain this feature (validating now) For those interested in running / configuring nightlies, there are some instructions for installing this build (and configuring LCOW) that I've documented here: UPDATE: Downloaded master nightly builds from Feb 5 2019, and it represents commit https://github.com/docker/engine/commits/f091a8d which includes this change. |
It would be really nice if you actually would consider backporting, as LCOW in 19.03 seems to require Windows build 1809 (due to pull request #39108, with further explanation here), and some of us will be stuck with Windows build 1803 for some time (corporate managed computer). @thaJeztah have submitted a lot of backport pull requests on docker/engine recently, I would love to see this one included as well! |
Hi, a little late to the party, is this currently possible in compose? What tags to use? |
Signed-off-by: Yusuf Tarık Günaydın yusuf_tarik@hotmail.com
- What I did
Added implementation of --memory and --cpu switches for LCOW.
- How I did it
Filled the Resources object in spec.Windows for both Windows and Linux by separating it into a new method. Also separated the code that fills the CPU and memory part of the hcsshim configuration and called it for both Windows and Linux.
- How to verify it
docker run -it --memory=2G --cpus=8 alpine sh -c "nproc && cat /proc/meminfo"
- Description for the changelog
Added memory and CPU limits for LCOW.
- A picture of a cute animal (not mandatory but encouraged)