config: Add default_{vcpus, memory} configurations #297

dvoytik · 2017-07-04T12:28:33Z

Introduce two new configuration settings:

default_vcpus - defines default vCPU number for new created PODs
default_memory - defines default memory size for new created PODs

Fixes #164.

Signed-off-by: Dmitry Voytik [email protected]

jodh-intel · 2017-07-04T13:10:32Z

Hi @dvoytik - thanks for raising.

Travis is failing as you seem to have found a bug/limitation of our checkcommits tool - it currently requires the "Fixes #XXX" comment in column 0 (no indent :) as that is our convention (I'll raise a PR on the tool itself to allow atleast a pure whitespace indent...)

More fundamentally, semaphore is unable to launch a hypervisor with this PR. I'm not sure if you've used it, but you can actually ssh into the semaphore build env for this PR to debug why that is happening.

dvoytik · 2017-07-04T13:27:29Z

@jodh-intel thanks for the clarification. I'll try to fix all problems and update the PR.

devimc · 2017-07-04T13:35:16Z

lgtm

jodh-intel · 2017-07-04T13:59:01Z

Hi @dvoytik - the code looks good, but since you are changing virtcontainers code as well, the way we will need to land this is:

You raise a PR on https://github.com/containers/virtcontainers (for the changes you've made to files below vendor/* on this PR).
Once that PR has been merged into virtcontainers, we'll re-vendor the runtime to update the version of virtcontainers it uses.
We can then land the changes on this PR that make use of Default{VCPUs,MemSz} from the version of virtcontainers imported into the runtime.

dvoytik · 2017-07-04T15:55:21Z

Hi @jodh-intel. Got it. I'll do it.

jodh-intel · 2017-07-05T13:10:52Z

Follow-up to #297 (comment): I've raised the following PR on checkcommits to resolve the issue you got caught by where you had indented the "Fixes XXX" comment:

Checkcommits handle indented fixes tests#114
(specifically clearcontainers/tests@f4a502e)

dvoytik · 2017-07-05T15:18:22Z

Hi @jodh-intel, could you please merge containers/virtcontainers#299, so I can proceed. Thank you.

dvoytik · 2017-07-06T16:32:31Z

The PR was rebased and updated: changed default memory size from 512MiB to 2048MiB as @dlespiau suggested.
This PR still needs merging of containers/virtcontainers#299 and vendoring of virtcontainers.
Something is wrong with both CI. Please let me know if I should do something about it. Thanks.

jodh-intel · 2017-07-07T08:15:36Z

Adding tag to ensure we don't inadvertently merge this before containers/virtcontainers#299 lands...

dvoytik · 2017-07-07T11:29:45Z

I was thinking, how to let the sysadmin/ops easily configure the runtime to have by default as many vCPUs as physical cores on the host. What about making default_vcpus as signed integer and have special meaning for -1 - automatically inferring from physical core count. Also since QEMU supoorts no more than 255 cores, we should also limit this setting.

jodh-intel · 2017-07-07T12:53:28Z

@dvoytik - that sounds like a good idea.

dvoytik · 2017-07-07T13:07:08Z

@jodh-intel, Ok then. I'll update this PR after containers/virtcontainers#299 is being merged and vendored into runtime repo.

dvoytik · 2017-07-07T16:36:50Z

Update:

added vendoring of virtcontainers
now default_vcpus can be set to -1 to enable automatic physical cores infer
P.S.
It seems I wasn't the reason of CI failure this time 😄

grahamwhaley · 2017-07-11T18:56:22Z

Hi @dvoytik Indeed, that was not you breaking the CI - this was a problem we seemed to be seeing last week with Travis. I've not seen one yet this week, so it could have been transient. Something that often fixes it seems to be a rebase and push, or a minor change and push (to get a SHA change, to get a rebuild of the CI). See clearcontainers/tests#136 for a few more details, but we've not bottomed the issue yet.

dvoytik · 2017-07-12T12:35:22Z

Hi @grahamwhaley. OK, thanks for clarifying this. Just please let me know when I should repush this PR to trigger CI tests again, I don't want to spam this PR until. Thank you in advance.

coveralls · 2017-07-13T12:11:07Z

Coverage increased (+0.5%) to 54.039% when pulling 5682afc on dvoytik:add_cfg_vcpus_memory into e85026c on clearcontainers:master.

dvoytik · 2017-07-13T12:39:33Z

A clean re-base of the PR. It seems that now all CIs passed.

grahamwhaley · 2017-07-14T13:26:37Z

Hi @dvoytik Just to note, we are not ignoring you ;-) - I think this looks good now. Just waiting for @jodh-intel to come back online, do the review, and drop the DNM tag. That should be early next week I believe.
I'll note that containers/virtcontainers#299 has landed, so I believe we are all clear to get this moving again.

dvoytik · 2017-07-14T13:44:32Z

@grahamwhaley, thanks! :)

devimc · 2017-07-17T15:11:24Z

lgtm

devimc · 2017-07-17T15:13:51Z

@jodh-intel could you review it and remove the do-not-merge tag if you accept this pr?

jodh-intel · 2017-07-18T08:12:42Z

Makefile

@@ -92,6 +92,9 @@ PAUSEBINRELPATH := bin/pause

 GLOBALLOGPATH := $(PKGLIBDIR)/runtime/runtime.log

+DEFDEFVCPUS := 1


I'm not clear on the "double DEF" here. Couldn't this just be DEFAULTVCPUS and DEFAULTMEMSZ?

It's a default of default value :) If you insist, a can change it, no problem

jodh-intel · 2017-07-18T08:13:24Z

Makefile

@@ -92,6 +92,9 @@ PAUSEBINRELPATH := bin/pause

 GLOBALLOGPATH := $(PKGLIBDIR)/runtime/runtime.log

+DEFDEFVCPUS := 1
+DEFDEFMEMSZ := 2048


I'd add a comment to specify the units this variable is using.

jodh-intel · 2017-07-18T08:14:08Z

Makefile

@@ -178,6 +183,9 @@ const defaultRuntimeRun = "$(PKGRUNDIR)"
 const defaultShimPath = "$(SHIMPATH)"
 const pauseBinRelativePath = "$(PAUSEBINRELPATH)"

+const defaultDefaultVCPUs uint32 = $(DEFDEFVCPUS)


Again, defaultDefault... looks a little odd to me. Maybe just defaultVCPUs?

There is already a function called defaultVCPUs(). Wouldn't it be confusing having both with the same name? If not, then I can change as you propose.

Agreed. You could use a name like const defaultVCPUCount maybe.

Ok. Will do it.

Shortlog since last vendoring: ceb467e hypervisor: Add Default{VCPUs, MemSz} to HypervisorConfig e6862a1 1.0.0-rc.3 release The change is required to fix clearcontainers#164. Signed-off-by: Dmitry Voytik <[email protected]>

Introduce two new configuration settings: * default_vcpus - defines default vCPU number for newly created PODs * default_memory - defines default memory size for newly created PODs Fixes clearcontainers#164. Signed-off-by: Dmitry Voytik <[email protected]>

coveralls · 2017-07-18T15:23:47Z

Coverage increased (+0.5%) to 53.132% when pulling d14ce99 on dvoytik:add_cfg_vcpus_memory into 79abac4 on clearcontainers:master.

jodh-intel · 2017-07-18T15:33:27Z

Thanks @dvoytik.

lgtm.

jodh-intel · 2017-07-18T15:34:21Z

hmm, pullapprove is confused again. Let's try the label toggle again...

jodh-intel · 2017-07-18T15:34:48Z

Yep - convinced it.

If error occurs after sandbox network created successfully, we need to rollback to remove the created sandbox network Fixes: clearcontainers#297 Signed-off-by: flyflypeng <[email protected]>

If some errors occur after kata-proxy start, we need to rollback to kill kata-proxy process Fixes: clearcontainers#297 Signed-off-by: flyflypeng <[email protected]>

If some errors occur after qemu process start, then we need to rollback to kill qemu process Fixes: clearcontainers#297 Signed-off-by: flyflypeng <[email protected]>

If kata-agent doesn't start in VM, we need to do some rollback operations to release related resources. add grpc check() to check kata-agent is running or not Fixes: clearcontainers#297 Signed-off-by: flyflypeng <[email protected]>

dvoytik force-pushed the add_cfg_vcpus_memory branch from 5d17888 to 6172684 Compare July 6, 2017 15:49

jodh-intel added the do-not-merge label Jul 7, 2017

dvoytik force-pushed the add_cfg_vcpus_memory branch from 6172684 to cfa329b Compare July 7, 2017 16:21

grahamwhaley mentioned this pull request Jul 11, 2017

runtime: Make vCPUs and memory to be configurable for uncontrained co… #198

Closed

dvoytik force-pushed the add_cfg_vcpus_memory branch from cfa329b to 5682afc Compare July 13, 2017 12:06

egernst mentioned this pull request Jul 17, 2017

Add config fields for max vCPUs and memory for uncontrained containers #164

Closed

jodh-intel reviewed Jul 18, 2017

View reviewed changes

vendor: Update virtcontainers vendoring

fe1563e

Shortlog since last vendoring: ceb467e hypervisor: Add Default{VCPUs, MemSz} to HypervisorConfig e6862a1 1.0.0-rc.3 release The change is required to fix clearcontainers#164. Signed-off-by: Dmitry Voytik <[email protected]>

dvoytik force-pushed the add_cfg_vcpus_memory branch from 5682afc to 5fd86b4 Compare July 18, 2017 11:39

dvoytik force-pushed the add_cfg_vcpus_memory branch from 5fd86b4 to d14ce99 Compare July 18, 2017 15:19

jodh-intel removed the do-not-merge label Jul 18, 2017

jodh-intel added do-not-merge and removed do-not-merge labels Jul 18, 2017

jodh-intel merged commit 0b30b64 into clearcontainers:master Jul 18, 2017

egernst mentioned this pull request Jul 18, 2017

Add support for managing compute resources #341

Closed

egernst added the beta gating label Jul 18, 2017

grahamwhaley mentioned this pull request Aug 22, 2017

Debate what the default number of VCPUs should be containers/virtcontainers#335

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: Add default_{vcpus, memory} configurations #297

config: Add default_{vcpus, memory} configurations #297

dvoytik commented Jul 4, 2017

jodh-intel commented Jul 4, 2017

dvoytik commented Jul 4, 2017 •

edited

Loading

devimc commented Jul 4, 2017 •

edited by dlespiau

Loading

jodh-intel commented Jul 4, 2017

dvoytik commented Jul 4, 2017

jodh-intel commented Jul 5, 2017

dvoytik commented Jul 5, 2017

dvoytik commented Jul 6, 2017

jodh-intel commented Jul 7, 2017

dvoytik commented Jul 7, 2017

jodh-intel commented Jul 7, 2017

dvoytik commented Jul 7, 2017

dvoytik commented Jul 7, 2017

grahamwhaley commented Jul 11, 2017

dvoytik commented Jul 12, 2017

coveralls commented Jul 13, 2017 •

edited

Loading

dvoytik commented Jul 13, 2017

grahamwhaley commented Jul 14, 2017

dvoytik commented Jul 14, 2017

devimc commented Jul 17, 2017

devimc commented Jul 17, 2017

jodh-intel Jul 18, 2017

dvoytik Jul 18, 2017

jodh-intel Jul 18, 2017

dvoytik Jul 18, 2017

jodh-intel Jul 18, 2017

dvoytik Jul 18, 2017

jodh-intel Jul 18, 2017

dvoytik Jul 18, 2017

coveralls commented Jul 18, 2017 •

edited

Loading

jodh-intel commented Jul 18, 2017 •

edited by dlespiau

Loading

jodh-intel commented Jul 18, 2017

jodh-intel commented Jul 18, 2017

		@@ -92,6 +92,9 @@ PAUSEBINRELPATH := bin/pause

		GLOBALLOGPATH := $(PKGLIBDIR)/runtime/runtime.log

		DEFDEFVCPUS := 1

config: Add default_{vcpus, memory} configurations #297

config: Add default_{vcpus, memory} configurations #297

Conversation

dvoytik commented Jul 4, 2017

jodh-intel commented Jul 4, 2017

dvoytik commented Jul 4, 2017 • edited Loading

devimc commented Jul 4, 2017 • edited by dlespiau Loading

jodh-intel commented Jul 4, 2017

dvoytik commented Jul 4, 2017

jodh-intel commented Jul 5, 2017

dvoytik commented Jul 5, 2017

dvoytik commented Jul 6, 2017

jodh-intel commented Jul 7, 2017

dvoytik commented Jul 7, 2017

jodh-intel commented Jul 7, 2017

dvoytik commented Jul 7, 2017

dvoytik commented Jul 7, 2017

grahamwhaley commented Jul 11, 2017

dvoytik commented Jul 12, 2017

coveralls commented Jul 13, 2017 • edited Loading

dvoytik commented Jul 13, 2017

grahamwhaley commented Jul 14, 2017

dvoytik commented Jul 14, 2017

devimc commented Jul 17, 2017

devimc commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 18, 2017 • edited Loading

jodh-intel commented Jul 18, 2017 • edited by dlespiau Loading

jodh-intel commented Jul 18, 2017

jodh-intel commented Jul 18, 2017

dvoytik commented Jul 4, 2017 •

edited

Loading

devimc commented Jul 4, 2017 •

edited by dlespiau

Loading

coveralls commented Jul 13, 2017 •

edited

Loading

coveralls commented Jul 18, 2017 •

edited

Loading

jodh-intel commented Jul 18, 2017 •

edited by dlespiau

Loading