pkg/oci: honour CPU period and quota

[devimc]

the number of vCPUs must be calculated using CPU period and quota
doesn't matter if a memory limit was specified or not

fixes #540

Signed-off-by: Julio Montes [email protected]

[sboeuf]

Thanks for fixing the semantic here !
LGTM

[sboeuf]

@jodh-intel @sameo please merge if you're fine with that.

[jodh-intel]

Good catch. The code looks good, but we clearly need some tests for this.

Could you add some to TestVmConfig()? We might also (separately) want a .bats test for asserting the Docker args over in https://github.com/clearcontainers/tests too.

[devimc]

@jodh-intel yes, I'm going to add a test in https://github.com/clearcontainers/tests

[jodh-intel]

@devimc - thanks. I think we do need a unit test too though as TestVmConfig() was updated for quota/period but clearly is missing some cases as we would have expected that test to fail without your fix on this PR.

[devimc]

@jodh-intel sure, working on that

[devimc]

@jodh-intel done, thanks

[jodh-intel]

Thanks @devimc - I've tested this and with your new test and the old code, TestVmConfig() fails as we'd expect.

lgtm

[sameo]

LGTM

[sameo]

coveralls shows a 3.5% drop in coverage with this PR. @devimc Could you please look at it?

[devimc]

Hi @sameo , I can't see coveralls results

coverage/coveralls — Waiting for status to be reported 

[jodh-intel]

Hi @devimc - something odd is up with coveralls today. If you look at the 16.04 log file, you can see https://coveralls.io/jobs/32558455, but like #539 I think it's probably lying about the drop in coverage (you could test that locally I guess).

[devimc]

@jodh-intel yep, I think the same (coverall lied)
@sameo I don't see any drop in the coverage

cd $GOPATH/src/github.com/containers/virtcontainers/pkg/oci
go test -run TestVmConfig -cover -coverprofile cover.txt
go tool cover -html=cover.txt -o coverage.html

[sameo]

LGTM then

[grahamwhaley]

Just noting - we discussed in a call that we should ensure we understood what this was doing iirc (in particular when it was written in the original/expanded form with real values - like 10000 + (20000 - 1) / 20000. I had a stare and think, and yes, this does the expected and works out how many 'whole cpus' the given quota gets us given the period - but the above round up to 1 CPU comment is slightly misleading - it is really rounding up to ensure we have at least 1 CPU aiui. Just for the record.

[sboeuf]

Thanks for the analysis @grahamwhaley. I think you should submit a PR to fix the comment and maybe be more verbose about the reason why this is the right way to compute the number of CPUs.

[devimc]

Actually that comment was add in #344
/cc @wcwxyz