Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

BlackHole1
Copy link
Contributor

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

Copy link

openshift-ci bot commented Dec 26, 2023

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gbraad
Copy link

gbraad commented Dec 27, 2023

/ok-to-test

Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

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!)

pkg/vf/vsock.go Outdated Show resolved Hide resolved
pkg/vf/vsock.go Outdated Show resolved Hide resolved
@BlackHole1
Copy link
Contributor Author

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
Copy link
Collaborator

cfergeau commented Jan 22, 2024

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 #24

Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

One more comment regarding the closes list which would simplify the code if it works for you.
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.
Apart from this, this looks good to me!

cmd/vfkit/main.go Show resolved Hide resolved
When other projects use the `vf.ExposeVsock` method, there will be unexpected issues due to the absence of a close proxy.

Signed-off-by: Kevin Cui <[email protected]>
@BlackHole1
Copy link
Contributor Author

BlackHole1 commented Jan 23, 2024

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
Copy link
Collaborator

/lgtm
/approve

Copy link

openshift-ci bot commented Jan 24, 2024

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 7e5ba4c into crc-org:main Jan 24, 2024
6 checks passed
@BlackHole1 BlackHole1 deleted the close-proxy branch January 24, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants