Skip to content
This repository has been archived by the owner on Apr 3, 2018. It is now read-only.

hypervisor: Add Default{VCPUs, MemSz} to HypervisorConfig #299

Merged
merged 1 commit into from
Jul 7, 2017
Merged

hypervisor: Add Default{VCPUs, MemSz} to HypervisorConfig #299

merged 1 commit into from
Jul 7, 2017

Conversation

dvoytik
Copy link
Contributor

@dvoytik dvoytik commented Jul 5, 2017 •
edited
Loading

A runtime implementation can use these fields to setup the default
vCPU number and default memory size for new VMs.
VMConfig can overwrite these settings.

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

@dvoytik
Copy link
Contributor Author

dvoytik commented Jul 5, 2017

Okay, I see the problem. I'm on it.

@dvoytik dvoytik force-pushed the add_default_vcpus_memsz branch from bef3c12 to a56b81f Compare July 5, 2017 14:25
@coveralls
Copy link

coveralls commented Jul 5, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 65.692% when pulling a56b81f on dvoytik:add_default_vcpus_memsz into fd5f0dd on containers:master.

@dvoytik
Copy link
Contributor Author

dvoytik commented Jul 5, 2017 •
edited
Loading

I see. I have the same problem on my host. It seems like QEMU has an issue with less than 512 MiB guest physical memory size.

@dvoytik dvoytik force-pushed the add_default_vcpus_memsz branch from a56b81f to 4c2d2e3 Compare July 5, 2017 14:50
@coveralls
Copy link

coveralls commented Jul 5, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 65.692% when pulling 4c2d2e3 on dvoytik:add_default_vcpus_memsz into fd5f0dd on containers:master.

@dvoytik dvoytik mentioned this pull request Jul 5, 2017
Merged
@jodh-intel
Copy link
Collaborator

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

Hi @dvoytik - thanks for raising.

lgtm.

@dlespiau - could you take a look?

Approved with PullApprove

@dvoytik
Copy link
Contributor Author

dvoytik commented Jul 6, 2017

Hi @dlespiau , are you OK with this patch?

@dlespiau
Copy link

dlespiau commented Jul 6, 2017

You have two things in this PR:

  • Introducing the memory and vcpu configuration knobs (great)
  • Changing the default from 2G to 512MB (debatable :)

I don't remember we have a great reason to default to 2G but I suspect quite a few images won't start with only 512MB of RAM. If you only introduce the first point in this PR (configuration), it can have my lgtm. Changing the default will need more work (we have a script to test the top 100 containers for instance, it probably has to go through it).

So maybe a good idea is to only have the configurability in this PR and merge it, with another PR to change the default if you so wish.

Of course, we can also specify 2G in the clear containers runtime, that should result in no effective change either.

@dvoytik
Copy link
Contributor Author

dvoytik commented Jul 6, 2017

@dlespiau , I'm OK to keep it 2G. I'll update the PR in a minute.

@dvoytik dvoytik force-pushed the add_default_vcpus_memsz branch from 4c2d2e3 to 98ff4b8 Compare July 6, 2017 14:28
@coveralls
Copy link

coveralls commented Jul 6, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 65.692% when pulling 98ff4b8 on dvoytik:add_default_vcpus_memsz into 65f177c on containers:master.

jodh-intel
jodh-intel reviewed Jul 7, 2017
View reviewed changes
hypervisor.go Outdated
}

if conf.DefaultMemSz == 0 {
conf.DefaultMemSz = 2048 // MiB
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd be tempted to do something like:

// 2G
const defaultMemSzMiB = 2048

    :
conf.DefaultMemSz = defaultMemSzMiB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Also did for conf.DefaultVCPUs

hypervisor_test.go Outdated
ImagePath: fmt.Sprintf("%s/%s", testDir, testImage),
HypervisorPath: "",
DefaultVCPUs: 1,
DefaultMemSz: 2048,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, since this value is used here and in qemu_test.go, I'd define the following at the top of virtcontainers_test.go since that is where we place "shared" test variables:

const testDefaultMemSzMiB = 2048

hypervisor: Add Default{VCPUs, MemSz} to HypervisorConfig
ceb467e 
A runtime implementation can use these fields to setup the default
vCPU number and default memory size for new VMs.
VMConfig can overwrite these settings.

Signed-off-by: Dmitry Voytik <[email protected]>
@dvoytik dvoytik force-pushed the add_default_vcpus_memsz branch from 98ff4b8 to ceb467e Compare July 7, 2017 09:19
@dlespiau
Copy link

dlespiau commented Jul 7, 2017 •
edited
Loading

lgtm

Approved with PullApprove

@jodh-intel
Copy link
Collaborator

lgtm

@coveralls
Copy link

coveralls commented Jul 7, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 65.692% when pulling ceb467e on dvoytik:add_default_vcpus_memsz into 65f177c on containers:master.

@dlespiau
Copy link

dlespiau commented Jul 7, 2017

Alright, the owners are all on holidays, so merging this with two review. Note we've adding one more owner in #301 to deal with that situation. Could be a good idea to add james as well, potentially.

For now, and until #301 is merged, using superpowers to bypass pullapprove.

@dlespiau dlespiau merged commit e01ebd0 into containers:master Jul 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jodh-intel jodh-intel jodh-intel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dvoytik @coveralls @jodh-intel @dlespiau