Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

KVM: Hypervisor support for KVM flavor #2684

Merged
merged 4 commits into from
Jul 18, 2016
Merged

Conversation

jjlakis
Copy link
Contributor

@jjlakis jjlakis commented May 24, 2016

Here's the first PR in our plan to put some focus on qemu as a KVM hypervisor. Whole code is based on Michał Stachowski's (@mstachowski) work in #2592 ,and it's taken by me, due to the other activities inside our company that Michał must handle. We're also closing #2592.
This code provides a generic mechanism to use different kvm hypervisors (such as lkvm, qemu-kvm), to set it by passing proper option to ./configure (New configure option --with-stage1-kvm-hypervisor). This PR covers only implementation of KvmHypervisor type and a small refactor of kvm's part of init.go to match the new KvmHypervisor interface (lkvm is the only option that could be passed to --with-stage1-kvm-hypervisor and it's also default)
I fixed most of the review suggestions from #2592

@jjlakis jjlakis changed the title Hypervisor support for KVM flavor KVM: Hypervisor support for KVM flavor May 24, 2016
@jjlakis
Copy link
Contributor Author

jjlakis commented May 24, 2016

The reason of failing Travis-CI check is:

CC @alban

@lucab
Copy link
Member

lucab commented May 24, 2016

Validator glitch, coreos/git-validation#5 should fix it. I just restarted this job on travis.

tmrts added a commit that referenced this pull request May 24, 2016
Complains when the commit is a merge commit, it shouldn't!

Discovered in  #2684

cc @lucab
@tmrts
Copy link
Contributor

tmrts commented May 24, 2016

I kickstarted the Travis build, should be good now

@jellonek
Copy link
Contributor

Travis is still failing.

@tmrts
Copy link
Contributor

tmrts commented May 24, 2016

@jellonek It's related to git validation, but Semaphore is green, so it should be safe to merge after a review

tmrts added a commit to tmrts/rkt that referenced this pull request May 25, 2016
Complains when the commit is a merge commit, it shouldn't!

Discovered in  rkt#2684

cc @lucab
@jonboulle jonboulle added this to the v1.9.0 milestone May 27, 2016
@hectorj2f
Copy link

@jjlakis I'd like to test this PR, I started playing with #2592. But I cannot access to this branch.

@jellonek
Copy link
Contributor

What stops you? Did you added https://github.com/intelsdi-x/rkt.git as separate git remote?

@hectorj2f
Copy link

@jellonek I was expecting to directly use this branch intelsdi-x:jlakis-hypervisor :). But I'd now use the rkt fork of intelsdi.

@jellonek
Copy link
Contributor

Mentioned branch is based on master from this coreos/rkt repository. There is no difference in both forks other than it's easier to start working on rkt by new guy at Intel, before one would get privilege to push into coreos/rkt.
So there is no reason to be sad about this - this is usual situation in collaboration on github ;)

@jjlakis jjlakis force-pushed the jlakis-hypervisor branch 2 times, most recently from 93798b0 to 4c52578 Compare May 30, 2016 08:55
)

func StartCmd(wdPath, name, kernelPath string, nds []kvm.NetDescriber, cpu, mem int64, debug bool) ([]string, error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

superfluous newline

@jjlakis jjlakis force-pushed the jlakis-hypervisor branch 3 times, most recently from 8508957 to 40fc66f Compare July 18, 2016 13:00
@jjlakis
Copy link
Contributor Author

jjlakis commented Jul 18, 2016

All tests passing

[AS_HELP_STRING([--with-stage1-kvm-hypervisor],
[hypervisor used by 'kvm' flavor; default and only option for now: 'lkvm'])],
[RKT_STAGE1_KVM_HV="${withval}"],
[RKT_STAGE1_KVM_HV='lkvm'])
Copy link
Member

Choose a reason for hiding this comment

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

there're tabs on these lines, please use only spaces

@iaguis
Copy link
Member

iaguis commented Jul 18, 2016

LGTM after the nits

KernelParams []string
}

// InitKernelParams sets debug and common parameters passed to kernel and debug parameters
Copy link
Member

Choose a reason for hiding this comment

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

Last thing: please remove the trailing " and debug parameters" and s/to kernel/to the kernel/.

@iaguis
Copy link
Member

iaguis commented Jul 18, 2016

Tests passed before and the changes afterwards were just cosmetic. Merging.

@iaguis iaguis merged commit 1336ab9 into rkt:master Jul 18, 2016
@squall0gd
Copy link
Contributor

It's good to see this in master :)

@jjlakis jjlakis mentioned this pull request Jul 20, 2016
@jonboulle
Copy link
Contributor

\o/

On 20 July 2016 at 13:10, Michał Stachowski notifications@github.com
wrote:

It's good to see this in master :)


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2684 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACewN988kH8v22DaMNBUqY1iiaHyr_KCks5qXgIigaJpZM4IloPM
.

@mzylowski mzylowski deleted the jlakis-hypervisor branch August 1, 2016 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.