You need to run gofmt and goimport on the modified files.

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)

ping @johnstep @AntaresS ?

@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:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "lcow_limits" [email protected]: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 -f

Amending updates the existing PR. You DO NOT need to open a new one.

@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.

Codecov Report

❗ No coverage uploaded for pull request base (master@50e63ad). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #37296   +/-   ##
=========================================
  Coverage          ?    36.6%           
=========================================
  Files             ?      610           
  Lines             ?    45237           
  Branches          ?        0           
=========================================
  Hits              ?    16561           
  Misses            ?    26394           
  Partials          ?     2282

@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:

docker run --rm -it --cpus=8 alpine sh -c "nproc"
8

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.
PS C:\scripts> docker run -it --memory=2G alpine sh
/ # free -m
total used free shared buffers cached
Mem: 962 104 858 18 0 18
-/+ buffers/cache: 85 876
Swap: 0 0 0

@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!

@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!)

@orest-gulman Yes, that will be the case until this PR has been merged into moby/moby and docker-ce/ee takes the patch :/

@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 engine directory at https://github.com/docker/docker-ce/tree/master/components/engine which contains the fork of the moby project that lives in https://github.com/docker/engine

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:
https://github.com/puppetlabs/pupperware/blob/master/README-windows.md#install-the-docker-nightly-build

UPDATE: Downloaded master nightly builds from Feb 5 2019, and it represents commit https://github.com/docker/engine/commits/f091a8d which includes this change.

Client:
 Version:           master-dockerproject-2019-02-05
 API version:       1.40
 Go version:        go1.11.5
 Git commit:        896ff57b
 Built:             Tue Feb  5 23:51:43 2019
 OS/Arch:           windows/amd64
 Experimental:      false

Server:
 Engine:
  Version:          master-dockerproject-2019-02-05
  API version:      1.40 (minimum version 1.24)
  Go version:       go1.11.5
  Git commit:       f091a8d
  Built:            Tue Feb  5 23:59:30 2019
  OS/Arch:          windows/amd64
  Experimental:     true

I doubt this would be backported to 18.09 or earlier as LCOW remains an experimental feature.

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!

As far as I can see after compiling moby from source, I am able to get memory limits working even in Docker compose. Think we are just waiting on this to be merged.

Hi, a little late to the party, is this currently possible in compose? What tags to use?