crc-org / vfkit

Apache License 2.0
119 stars 23 forks source link

refactor(vsock): call proxy.Close when vm stop #74

Closed BlackHole1 closed 7 months ago

BlackHole1 commented 8 months ago

When other projects use the vf.ExposeVsock method, there will be unexpected issues due to the absence of a close proxy.

openshift-ci[bot] commented 8 months ago

Hi @BlackHole1. Thanks for your PR.

I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
gbraad commented 8 months ago

/ok-to-test

BlackHole1 commented 8 months ago

Can you share more details about the code base you are using this in? pkg/vf is mostly internal to vfkit, so I'm both surprised and curious to see there are external users of it :) (but that's a good thing!)

Here: https://github.com/oomol-lab/ovm/blob/main/pkg/vfkit/vfkit.go

In our product, we need to initiate containers doing some work, and in order to prevent users from manually installing podman / docker, we have to integrate podman or docker within. After consideration, we choose podman. But if we directly use podman, several issues would appear:

  1. If users have already installed podman, conflicts would occur.
  2. Currently, podman is still experimental with Apple HV.
  3. The Fedora CoreOS virtual machine image required by podman is too large (exceeding 1G). If built in, our application is going to exceed 2G.
  4. In podman, gvproxy and vfkit are resident processes, whereas in our requirements, we need to quit the whole virtual machine after application closure.
  5. Configuration files of podman are scattered in various directories across system, not suitable for centralized management.

Following these requirements, we generated a minimum Linux image ovm-core (around 100M) that can run podman, then run gvproxy and vfkit via ovm-js. However, in the course of development, we found out that it’s incredibly inconvenient to manage gvproxy and vfkit processes uniformly through ovm-js, because:

  1. gvproxy has to be launched first and the socket file needs to be created and monitored.
  2. Launching vfkit depends on the socket file created by gvproxy. At the moment, we can only figure out whether gvproxy is prepared by polling the existence of the socket file.
  3. Once any process exits, the other has to exit too (they share the same life cycle).
  4. It’s impossible to print logs with a finer granularity.

So considering this, I created the ovm project integrating vfkit and gvproxy code, which can address the above-mentioned issues. The project is currently under development, so there is no README.md file yet.

Finally, I’d like to extend my gratitude to you, your team and RedHat for creating vfkit/podman/gvproxy. Without you guys, our product wouldn’t be possible. The ovm code will all be open source. Ultimately, we plan to roll out a product similar to orbstack (free and open-source), and use podman as a built-in container management program instead of docker.

At present, we’re preparing to sponsor Code-Hex, the writer of vz (expected next month as our company is dealing with bank card-related issues).

cfergeau commented 7 months ago

So considering this, I created the ovm project integrating vfkit and gvproxy code, which can address the above-mentioned issues

For what it's worth, merging gvproxy directly into vfkit is something I've had in mind for a while, but did not have time to do yet. I'd definitely review patches aiming at doing this.

This is https://github.com/crc-org/vfkit/issues/24

BlackHole1 commented 7 months ago

I also wanted to ask, regarding the Signed-off-by, can you use your real name in it? I don't think pseudonyms works for that.

Done

cfergeau commented 7 months ago

/lgtm /approve

openshift-ci[bot] commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/crc-org/vfkit/blob/main/OWNERS)~~ [cfergeau] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment