-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add LXD VM Guest Agent Support #2145
Conversation
@@ -0,0 +1 @@ | |||
ACTION=="add", SYMLINK=="virtio-ports/org.linuxcontainers.lxd", TAG+="systemd", ACTION=="add", RUN+="/bin/systemctl start lxd-agent.service" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not use systemd unit VM detection like all other agent using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you recommend? A separate check for "/dev/lxd/sock" (indicating we are in a guest LXD) that is called by systemd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, systemd can have conditions in descriptive form. There is ConditionVirtualization=
which can be used to detect virtualization. So we can just set that in lxd-agent.service
, I think, but we should test it with and without LXC if things behave as expected.
https://www.freedesktop.org/software/systemd/man/systemd.unit.html#ConditionVirtualization=
https://www.freedesktop.org/software/systemd/man/systemd-detect-virt.html#
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now. Hmm, looking my test system there is not a good way to detect a LXD VM specifically, systemd-detect-virt shows just qemu. Apologies, I scraped this together from the debian package lxd-agent-loader. Will dig in to see if I can better detect this. Would it not be a good enough test to check for /dev/lxd/sock in this case if the existing systemd detection is not good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we may consider updating the virtualization checks for the qemu agent, as that tries and fails to load in this setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
systemd-detect-virt shows just qemu
Hm, I see there is no lxd
😢
Ideally we'd add lxd to systemd-detect-virt, then the Qemu problem would be solved as well 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked at the systemd source it looks like they are detecting /sys/class/dmi/id/sys_vendor which shows as qemu. I just looked and LXD shows up properly in /sys/class/dmi/id/board_name.
I can try and create a PR for systemd, but that is a tall order. Any chance we can implement a hack and I can follow up later with the correct systemd PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we have a PR with systemd already open before patching it on our side, so the process is at least kicked off 😄
Seems that worth a try submitting a PR which just reads /sys/class/dmi/id/board_name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can apply the PR also into our build system, so we not have to wait to get approved by systemd @bryanyork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working systemd patchset added. We are behind the latest systemd version and they refactored things. I'll work on a PR to systemd for HEAD now.
CONFIG_NET_9P=y | ||
CONFIG_NET_9P_VIRTIO=y | ||
CONFIG_9P_FS=y | ||
CONFIG_9P_FS_POSIX_ACL=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only comment from my side: It is really necessary to integrate all these 9P config option directly into a kernel or isn't it better to compile them as kernel modules instead? IMHO, compiling them as loadable kernel modules should be a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. CONFIG_PCI is still required as a built-in.
$(INSTALL) -D -m 644 $(LXD_GUEST_AGENT_PKGDIR)/99-lxd-agent.rules \ | ||
$(TARGET_DIR)/usr/lib/udev/rules.d/99-lxd-agent.rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, there also linter errors on those two lines, see:
https://github.com/home-assistant/operating-system/actions/runs/3079333274/jobs/4979283480
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed / Removed those rules as I have the systemd patch in place now.
e2ed7ad
to
b9553fa
Compare
@agners @pvizeli Updated this PR to address issues and submitted upstream PR to systemd: systemd/systemd#24802 |
@agners @pvizeli Updates here from the systemd PR: In later version of systemd past v250 series, LXD VM's are "properly" detected as KVM. I'm happy to keep the patch in, as v250 is still broken and detects qemu. I'll see if we can backport the fix to the v250 series. I would prefer to get something in so I can follow mainline for my installation. Thoughts on next steps to merge this PR? |
Updated. I think this is the best option to merge. Please let me know your thoughts. It seems I can't convince folks at systemd or LXD to return as such, so we'll need to target like this to support my use-case. Perhaps in the future we will see a better conditional that is backwards compatible. |
Thanks for merging @agners ! Do you know when this will hit a production release? |
Hiya again @agners ! Should I create a PR to move this to the rel-9 branch? Was hoping to get this into 9.4 so I can move my install to a production version. |
Fixes #676