Configure pod network for launcher from virt-handler #2837
Conversation
kubevirt-bot
added
release-note
|
/hold This is NOT ready. This is merely a start, but it allows to boot a VMI using masquerade pod networking and get it booted (even if without DHCP allocated address, yet). More patches are coming. |
kubevirt-bot
added
the
do-not-merge/hold
kubevirt-bot
added
the
needs-rebase
booxter
force-pushed
the
net-admin-in-handler
branch
from
e2eef6e
to
28b0e12
Compare
kubevirt-bot
added
dco-signoff: yes
booxter
force-pushed
the
net-admin-in-handler
branch
from
28b0e12
to
2e211a5
Compare
kubevirt-bot
added
dco-signoff: no
|
The latest upload fixes remaining issues with Overall, this version is now at a point where VMIs can be booted and can reach outside. |
booxter
force-pushed
the
net-admin-in-handler
branch
from
c5c801c
to
8cf3f19
Compare
kubevirt-bot
added
dco-signoff: yes
booxter
force-pushed
the
net-admin-in-handler
branch
2 times, most recently
from
661a9ce
to
dd79a52
Compare
|
@phoracek @qinqon this PR got close enough for me to not be completely embarrassed by its shape. If you could find some time to check it, I would appreciate it. Let me know what you think about where this is going. Note this is not a complete solution to remove NET_ADMIN from launcher pods, just one piece. Other pieces are: move DHCP server out of launcher; validate latest RHEL libvirt with backports allows to preconfigure tap device for qemu; finally, remove NET_ADMIN from launcher. This is just a first step that moves most network operations, but not everything, into handler. |
|
/hold Need to fix generated files (apparently my |
booxter
force-pushed
the
net-admin-in-handler
branch
from
dd79a52
to
fba9519
Compare
|
@booxter first thanks for tackling this difficult topic.
Regarding to DHCP, this is an interesting sub-problem. I am thinking here about situations where virt-handler is not present, because it is e.g. updated. We would then probably not have a dhcp server available for all VMIs at that time. |
|
One other thing to consider re: this PR is that right now all launchers will share storage with VIF / interface cache files, which is information leak. Perhaps the template for launcher should have a separate and specific mount point for a directory shared with handler that would contain only the files that belong to this particular launcher. But, if we finally merge phase2 into phase1 and leave the whole network configuration to handler, then there is no need for a shared directory at all, and this becomes a moot point. |
booxter
force-pushed
the
net-admin-in-handler
branch
from
82face7
to
4196d7a
Compare
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
I'm good with this once we get the os thread locking thing fixed. Like i said in a previous comment, i'm okay with merging the unoptimized solution and following up with a optimization afterwards. This PR is already so complex i don't see any reason to block it any further.
|
Is there some new issue with Travis? It failed on both platforms, one failure seems completely unrelated; another is in unit tests though I couldn't reproduce it. Is there a way to re-trigger the job? I don't have creds to do it on travis myself. |
Not sure why did it fail. I've restarted the job. |
|
@booxter: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
i re-triggered it. |
|
/lgtm |
|
/override ci/prow/e2e that one should not be reported. |
|
@rmohr: Overrode contexts on behalf of rmohr: ci/prow/e2e In response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rmohr 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 |
kubevirt-bot
added
the
approved
|
/retest pull-kubevirt-e2e-k8s-1.15.1-ceph |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
@booxter: The following tests failed, say
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. I understand the commands that are listed here. |
There was a problem hiding this comment.
/lgtm
super happy to see this going in. great work @booxter !
|
/test pull-kubevirt-e2e-k8s-1.15.1-ceph |
|
/retest |
|
Yay! 👏 |
|
Thanks to all reviewers that helped with pushing this. I appreciate your patience with multiple revisions of the PR. With your help, it's a lot less rough than I originally proposed. |
What this PR does / why we need it: this PR moves networking configuration phase from virt-launcher to virt-handler. This should eventually allow to remove NET_ADMIN from launcher pods, after libvirtd is capable of operating in unprivileged mode.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Special notes for your reviewer:
Release note:
TODO:
1. Make bridge, masquerade, slirp, sriov interfaces to boot with all network configuration (except DHCP) happening in virt-handler.2. Fix unit test failures.3. Fix functional test failures.Not in scope of this PR: moving DHCP server to handler (has its own complexities better to consider separately - clean up, management of the thread etc.); removing NET_ADMIN from virt-launcher. (Requires an updated libvirt but should be easy overall.)