Proposal: Add vsock support to KubeVirt #191

rmohr · 2022-10-14T10:35:06Z

Proposal to add vsock support to KubeVirt.

gdoc which was used for initial discussions: https://docs.google.com/document/d/1KfFsO6jKvie9EGMl9FOMtfqAYAMUFOTl2nIup14besA/edit

Signed-off-by: Roman Mohr <rmohr@google.com>

rmohr · 2022-10-14T10:35:47Z

/cc @rthallisey
/cc @vladikr

Signed-off-by: Roman Mohr <rmohr@google.com>

rthallisey · 2022-10-14T14:42:07Z

/lgtm
Reviewed in the gdoc. Proposal lgtm.

davidvossel · 2022-10-14T15:04:18Z

design-proposals/vsock/vsock.md

+
+#### API changes
+
+A `autoattachVsock` flag will be added to the VMI spec at `spec.domain.devices` which defaults to `false` to stay backward compatible. Also a `VsockCID` field will be added to the VMI status as `status.vsockCID` for CID allocation and lookup.


is there any chance that we'll need more fields associated with vsock other that "true|false" for attachment? if so, maybe we shouldn't use a boolean.

libvirt has three fields, one for auto-cid selection, one for the virtio model and one for manual cid assignment. Model has to be in sync with other virtio devices and auto-assignment of the CID does not make sense in our design. We could not come up with additional use-cases where more fields would have to be on the VMI. For e.g. confidential computing I would expect that vsock as a whole has to be disabled, but that's about it ...

Can you think about something?

Could that be that we will need more than one vsock per guest at some point?

Hm, it is technically possible to attach multiple vsock devices, but guest drivers don't have a concept of multiple vsock devices. See for instance https://bugzilla.redhat.com/show_bug.cgi?id=1455015.

Ah, okay. I thought I read about multiple transports with vsock, but I might be wrong.
I guess we won't be able to have both the qemu guest agent and another agent using vsocks.

The vsock device has ports like TCP/IP. We can have a lot of agents over the single device. That is probably what you read about.

vladikr · 2022-10-14T15:52:29Z

/lgtm

davidvossel · 2022-10-14T17:37:20Z

/approve
/hold

The community has already been discussing this design (in google doc form) for a few days before this pr was created, and no strong objections have been raised. I put a hold on the PR simply to give more people time to comment on this before it's merged. If there are no new open discussions by next Tuesday (Oct 18th), then I think it's reasonable to remove the hold.

kubevirt-bot · 2022-10-14T17:37:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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~~ [davidvossel]

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

xpivarc · 2022-10-17T10:24:07Z

design-proposals/vsock/vsock.md

+
+*   The whole `vsock` features will live behind a feature gate
+*   By default the first 1024 ports of a vsock device are privileged. Services trying  to connect to those require `CAP_NET_BIND_SERVICE`.
+*   We aim to block the `AF_VSOCK` `socket` syscall in containerd (https://github.com/containerd/containerd/issues/7442), however if that will happen is not sure. It is therefore right now the responsibility of the vendor to apply the required seccomp change to all unprivileged ports, or to verify that the `CAP_NET_BIND_SERVICE` privilege is handed out carefully enough


Why did you choose containerd? I think containers/common would also benefit and might even reach a broader spectrum of users. (Kubervirt itself doesn't use containerd for some time...)

Yes KubeVirt uses cri-o in CI, but we use containerd and KubeVirt is not designed to only work with a specific CRI. This is an independent effort of this proposal to secure it for our purpose. I think it makes a lot of sense to also push this to other popular CRIs, but it is out of scope of this proposal :). But I explicitly wanted to mention this so that it can easily be picked up for other CRIs if there is interest.

I see. I read the proposal as a community - meaning we == community, and in that sense, it would make more sense to aim for changes in crio(therefore containers/common). I can pick this up.

Ah, I see the confusion now regarding to the usage of we. 👍

xpivarc · 2022-10-17T10:31:13Z

design-proposals/vsock/vsock.md

+### Possible next steps
+
+*   Allow running `qemu-guest-agent` over virtio-vsock too
+*   Introduce a general purpose mechanism via gRPC in virt-handler which allows efficient and scalable collection of version and readiness of registered third-party agents without having to alter virt-handler, or having to run additional node-local services. Also move the `qemu-guest-agent` polling behind this mechanism.


Nit qemu-quest-agent use case is mentioned above.

Could you explain what you mean? Running qemu-guest-agent on vsock is an explicit non-goal, but I wanted to outline how a possible future integration in a more general guest-agent mechanism could look like.

There are two points:

Allow running qemu-guest-agent over virtio-vsock too

end of the point "Also move the qemu-guest-agent polling behind this mechanism.
" <- feels redundant

Hm, they are two different points though:

This proposal does not change code-paths in virt-handler to allow running the qemu-guest-agent via vsock, that is the first point.

Another point would be introducing a common polling mechanism, where no extra handling for the qemu-guest-agent would be there, it would use a common extensible mechanism.

That are just some rough ideas to highlight that while qemu-guest-agent is not tackled in this proposal, it is not in the way of doing so later.

Now I got you. I still feel like the last part of the second point is redundant. Your comment explained it very well, maybe we can incorporate it into the points?

* Remove ambiguity of who is meant with "We" in the security section * Better explain how future steps beyond this proposal could look like Signed-off-by: Roman Mohr <rmohr@google.com>

rmohr · 2022-10-18T09:58:06Z

/unhold

Proposal: Add vsock support to KubeVirt

eebc9f9

Signed-off-by: Roman Mohr <rmohr@google.com>

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Oct 14, 2022

kubevirt-bot requested review from cwilkers and jean-edouard October 14, 2022 10:35

kubevirt-bot added the size/M label Oct 14, 2022

kubevirt-bot requested review from rthallisey and vladikr October 14, 2022 10:35

Add a paragraph describing scale considerations for phase 2

c17a5b8

Signed-off-by: Roman Mohr <rmohr@google.com>

kubevirt-bot assigned rthallisey Oct 14, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2022

davidvossel reviewed Oct 14, 2022

View reviewed changes

kubevirt-bot assigned vladikr Oct 14, 2022

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2022

xpivarc reviewed Oct 17, 2022

View reviewed changes

Clarify a few unclear sentences

c317986

* Remove ambiguity of who is meant with "We" in the security section * Better explain how future steps beyond this proposal could look like Signed-off-by: Roman Mohr <rmohr@google.com>

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022

xpivarc approved these changes Oct 18, 2022

View reviewed changes

kubevirt-bot assigned xpivarc Oct 18, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022

kubevirt-bot merged commit 45220b7 into kubevirt:main Oct 18, 2022

rmohr mentioned this pull request Oct 18, 2022

Add Vsock support to KubeVirt kubevirt/kubevirt#8546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Add vsock support to KubeVirt #191

Proposal: Add vsock support to KubeVirt #191

rmohr commented Oct 14, 2022 •

edited

rmohr commented Oct 14, 2022

rthallisey commented Oct 14, 2022

davidvossel Oct 14, 2022

rmohr Oct 14, 2022 •

edited

rmohr Oct 14, 2022

vladikr Oct 14, 2022

rmohr Oct 14, 2022 •

edited

vladikr Oct 14, 2022

rmohr Oct 14, 2022

vladikr commented Oct 14, 2022

davidvossel commented Oct 14, 2022

kubevirt-bot commented Oct 14, 2022

xpivarc Oct 17, 2022

rmohr Oct 17, 2022 •

edited

xpivarc Oct 18, 2022

rmohr Oct 18, 2022

xpivarc Oct 17, 2022

rmohr Oct 17, 2022

xpivarc Oct 17, 2022

rmohr Oct 17, 2022 •

edited

xpivarc Oct 18, 2022

rmohr commented Oct 18, 2022


		#### API changes

		A `autoattachVsock` flag will be added to the VMI spec at `spec.domain.devices` which defaults to `false` to stay backward compatible. Also a `VsockCID` field will be added to the VMI status as `status.vsockCID` for CID allocation and lookup.

Proposal: Add vsock support to KubeVirt #191

Proposal: Add vsock support to KubeVirt #191

Conversation

rmohr commented Oct 14, 2022 • edited

rmohr commented Oct 14, 2022

rthallisey commented Oct 14, 2022

Choose a reason for hiding this comment

rmohr Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikr commented Oct 14, 2022

davidvossel commented Oct 14, 2022

kubevirt-bot commented Oct 14, 2022

Choose a reason for hiding this comment

rmohr Oct 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr Oct 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr commented Oct 18, 2022

rmohr commented Oct 14, 2022 •

edited

rmohr Oct 14, 2022 •

edited

rmohr Oct 14, 2022 •

edited

rmohr Oct 17, 2022 •

edited

rmohr Oct 17, 2022 •

edited