Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

Create the hypervisor package#1208

Closed
sameo wants to merge 7 commits into
kata-containers:masterfrom
sameo:topic/pkg-hypervisor
Closed

Create the hypervisor package#1208
sameo wants to merge 7 commits into
kata-containers:masterfrom
sameo:topic/pkg-hypervisor

Conversation

@sameo
Copy link
Copy Markdown

@sameo sameo commented Feb 1, 2019 •
edited
Loading

With the store package now merged in, we can finally create a standalone hypervisor package for all thing hypervisor/vmm related. We still want to decouple the Endpoint interface from the hypervisor package but that can be done as a follow up step.

Fixes #1229

This was referenced Feb 1, 2019
Merged
Closed
@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Feb 5, 2019

@sameo looks like a refactor. any updates? still working through the tests? Thx!

@sameo sameo force-pushed the topic/pkg-hypervisor branch 2 times, most recently from 92308a8 to 54fab52 Compare February 12, 2019 10:22
@sameo sameo changed the title [RFC] Hypervisor package Create the hypervisor package Feb 12, 2019
Samuel Ortiz added 4 commits February 12, 2019 12:05
virtcontainers: hypervisor: Minimal package
96d1876 
This is the minimal package to help us define the hypervisor interface
into its own package.

Fixes: kata-containers#1229

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
virtcontainers: hypervisor: Move endpoints under hypervisor
1eb9eff 
There is currently a cyclic logical dependency between network, endpoint
and hypervisor structures, types and APIs. Hypervisor interface
implementations call into the Endpoint interface, which itself calls
into the Network* APIs. Since we want to move all hypervisor implementations
under the hypervisor package, we need to:

- Split the network implementation into 3 pieces: Types definitions,
  core namespace API and Endpoints.
- Move the Endpoint handling under the hypervisor package. This could be
  splitted further if we manage to decouple the Hypervisor interface
  from the Endpoint one, and vice-versa.

Fixes: kata-containers#1229

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
virtcontainers: Move kataVSOCK to types
c394517 
This is a generic type, shared across the agent and hypervisor
implementations.

Fixes: kata-containers#1229

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
virtcontainers: hypervisor: Remove the QEMU specific constant
28ec210 
Max VCPUs is a QEMU specific configuration. In order to even decouple
further the hypervisor interface from downward dependencies (e.g. qemu),
we leave the hypervisor max VCPUs configuration handling down to the
hypervisor implementation itself.

Fixes: kata-containers#1229

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
@sameo sameo force-pushed the topic/pkg-hypervisor branch from 54fab52 to 66a6d7e Compare February 12, 2019 11:12
@ttx ttx mentioned this pull request Feb 12, 2019
Closed
Samuel Ortiz added 3 commits February 12, 2019 12:25
virtcontainers: hypervisor: Add qemu implementation
a88c1a7 
We can now move the qemu implementation and test under the hypervisor
package.

Fixes: kata-containers#1229

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
virtcontainers: hypervisor: Add mock implementation
5e24af4 
We can now move the mock implementation and test under the hypervisor
package.

Fixes: kata-containers#1229

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
virtcontainers: hypervisor: Add firecracker implementation
c9fd159 
We can now move the firecracker implementation and test under the hypervisor
package.

Fixes: kata-containers#1229

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
@sameo sameo force-pushed the topic/pkg-hypervisor branch from 66a6d7e to c9fd159 Compare February 12, 2019 11:25
@sameo
Copy link
Copy Markdown
Author

sameo commented Feb 12, 2019

/test

sboeuf
sboeuf reviewed Feb 12, 2019
View reviewed changes
Copy link
Copy Markdown

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

@sameo
The whole PR looks globally good, but it feels really not natural to move endpoints under the hypervisor package as you did in the commit virtcontainers: hypervisor: Move endpoints under hypervisor.
You explained the cyclic dependency, and I understand the issue behind this, but aren't we making a mistake by moving Endpoints to this new hypervisor package just for the sake of getting the code to compile?

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Feb 12, 2019

@sameo The PR looks good overall. But I agree with @sboeuf, moving the network endpoints under hypervisor package, although solves the cyclic issue, seems a bit counter-intuitive.
I dont have any solution for this either, maybe have some kind of intermediate representation of just the required network properties in the hypervisor package. Not sure if that will over-complicate things.

@sameo
Copy link
Copy Markdown
Author

sameo commented Feb 13, 2019

@amshinde @sboeuf I fully agree that it's not entirely natural to put Endpoint under the hypervisor package but the reality is that our current endpoint interface is hypervisor bound for the device attachment part.
I'm ok with putting this PR on hold until we manage to break that dependency. There are a couple ways to do that and I'm going to open an issue to track that.

@sameo
Copy link
Copy Markdown
Author

sameo commented Feb 13, 2019

Adding donotmerge back, let's see if we can have a network/endpoint package first.

@sameo
Copy link
Copy Markdown
Author

sameo commented Feb 13, 2019

I'm ok with putting this PR on hold until we manage to break that dependency. There are a couple ways to do that and I'm going to open an issue to track that.

I already did that a few days ago...See #1209

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Feb 27, 2019

Needs a rebase though ;)

@jodh-intel jodh-intel mentioned this pull request Feb 28, 2019
Merged
@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 11, 2019

@sameo - this still needs a rebase.

@kata-containers/runtime - are you there? :)

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Mar 11, 2019

I guess this PR depend on #1233 being merged first.

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Mar 25, 2019

@sameo ping, any updates? Thx!

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Apr 18, 2019

@sameo nudge

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented May 24, 2019

@sameo any updates?

@egernst egernst added needs-rebase PR contains conflicts which need resolving needs-help Request for extra help (technical, resource, etc) enhancement Improvement to an existing feature labels Jun 11, 2019
@egernst
Copy link
Copy Markdown
Member

egernst commented Jun 11, 2019

need help for driving this forward. Added 'needs-help' label

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Jun 27, 2019

Any takers?

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Aug 2, 2019

Pinging from your weekly herder, any takers? 😄 🙏

@Zyqsempai
Copy link
Copy Markdown

Zyqsempai commented Sep 25, 2019

Hey guys, I want to take care of this, but it looks like this PR it's hard to reanimate, since there is to much conflicts, it's almost impossible to resolve them, believe me, I spent 4 hours trying to do that.
Do you have any idea where better to start, or maybe it will be easier to create a new PR?

@caoruidong
Copy link
Copy Markdown
Member

caoruidong commented Sep 25, 2019

@Zyqsempai Yes, you can create a new one.

@Zyqsempai
Copy link
Copy Markdown

Zyqsempai commented Sep 25, 2019

Also, I see that there is an open question about Endpoints, where they should be moved.
Do you have decision related to this topic?

@Zyqsempai
Copy link
Copy Markdown

Zyqsempai commented Oct 1, 2019

@raravena80 @caoruidong @egernst So, i am almost finished, the only one question is still open, is Endpoints where and how should we move them?
There is one already opened PR #1233 by same as this is stock.

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Nov 15, 2019

@Zyqsempai Any update on this PR? Thx

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Dec 19, 2019

@Zyqsempai any updates?

Your weekly Kata herder.

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Jan 24, 2020

@Zyqsempai ping 😄

@Zyqsempai
Copy link
Copy Markdown

Zyqsempai commented Feb 3, 2020

Sorry guys, didn't had enough time to work on it, will try to look into it on this week.

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Apr 4, 2020

@Zyqsempai friendly ping 😄

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Jul 21, 2020

@Zyqsempai this is old, should we close this and move the work to 2.0?

@c3d
Copy link
Copy Markdown
Member

c3d commented Sep 29, 2020

@Zyqsempai Two friendly pings, the last one in June, and in bad need of a rebase.
Closing for now. Please feel free to re-open when you feel it's back in the "ready to merge" state.
Thanks!

@c3d c3d closed this Sep 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bergwolf bergwolf Awaiting requested review from bergwolf

@amshinde amshinde Awaiting requested review from amshinde

2 more reviewers

@sboeuf sboeuf sboeuf left review comments

@jodh-intel jodh-intel jodh-intel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement Improvement to an existing feature needs-help Request for extra help (technical, resource, etc) needs-rebase PR contains conflicts which need resolving

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add hypervisor package

9 participants

@sameo @raravena80 @amshinde @jodh-intel @egernst @Zyqsempai @caoruidong @c3d @sboeuf