Fix guest-agent not starting with HyperV Enlightenments enabled #3592
Conversation
Guest agent doesn't start because if HyperV Enlightenments are enabled, the virtualization gets detected incorrectly. Backport Systemd patch that fixes the detection, allowing the guest-agent service to meet its dependencies. This patch should be no longer needed after update of Systemd to v256, or in case the patch gets eventually backported to the v254 stable branch. Fixes #3565
sairon
added
board/ova
WalkthroughWalkthroughThe patch modifies the virtualization detection logic in the systemd codebase, specifically in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
buildroot-external/patches/systemd/0004-detect-virt-detect-hyperv-enlightened-qemu-as-qemu-n.patch (2)
33-39: Enhance the code comment for clarityConsider rephrasing the comment to make it clearer and more precise.
Suggested rephrased comment:
-/* QEMU sets the CPUID string to hyperv's, in case it provides hyperv enlightenments. Let's hence not return Microsoft here but just use the other mechanisms first to make a better decision. */ +/* When QEMU provides Hyper-V enlightenments, it sets the CPUID string to Hyper-V's signature. Therefore, we should not immediately return Microsoft as the virtualization type but use other detection mechanisms first to make a better decision. */
24-24: Add a comment explaining the newhypervflagAdding a brief comment about the purpose of the
hypervflag would improve code readability.For example:
+ bool other = false, hyperv = false; /* Flags to indicate detection of other VMs and Hyper-V */
| CPUID reporting hyperv should be taken with a grain of salt, and we | ||
| should prefer other mechanisms then. |
There was a problem hiding this comment.
Correct the grammatical error in the commit message
In the commit message, the word "then" should be "instead" to improve clarity.
Apply this diff to fix the typo:
-CPUID reporting hyperv should be taken with a grain of salt, and we should prefer other mechanisms then.
+CPUID reporting hyperv should be taken with a grain of salt, and we should prefer other mechanisms instead.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CPUID reporting hyperv should be taken with a grain of salt, and we | |
| should prefer other mechanisms then. | |
| CPUID reporting hyperv should be taken with a grain of salt, and we | |
| should prefer other mechanisms instead. |
| finish: | ||
| - if (v == VIRTUALIZATION_NONE && other) | ||
| - v = VIRTUALIZATION_VM_OTHER; | ||
| + /* None of the checks above gave us a clear answer, hence let's now use fallback logic: if hyperv | ||
| + * enlightenments are available but the VMM wasn't recognized as anything yet, it's probably | ||
| + * Microsoft. */ | ||
| + if (v == VIRTUALIZATION_NONE) { | ||
| + if (hyperv) | ||
| + v = VIRTUALIZATION_MICROSOFT; | ||
| + else if (other) | ||
| + v = VIRTUALIZATION_VM_OTHER; | ||
| + } |
There was a problem hiding this comment.
💡 Codebase verification
Issues Found in Virtualization Detection Logic
VIRTUALIZATION_MICROSOFTis assigned outside the fallback logic inbuildroot-external/patches/systemd/0004-detect-virt-detect-hyperv-enlightened-qemu-as-qemu-n.patchat lines 45 and 56.
Analysis chain
Confirm the correctness of the updated fallback logic
The updated fallback logic appears to correctly handle scenarios where no virtualization type is conclusively detected. If hyperv is true, it returns VIRTUALIZATION_MICROSOFT, otherwise, it checks for other.
Run the following script to verify that the detection behaves as expected in different virtualization environments:
Please ensure to perform tests in the actual environments to confirm the detection logic works as intended.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify virtualization detection in various environments.
# Test: Check the virtualization type reported by systemd-detect-virt.
# Expected Results: Correct virtualization type is reported for each environment.
# Test environments (simulate or document the environments you want to test):
environments=("QEMU with Hyper-V enlightenments" "Native Hyper-V" "Other VMs" "No virtualization")
# Placeholder for loop to execute in each environment (pseudo-code):
for env in "${environments[@]}"; do
echo "Testing in environment: $env"
# Command to run systemd-detect-virt in the specified environment
# Since we cannot simulate environments here, document the expected behavior
# e.g., systemd-detect-virt
# Ensure the output matches the expected virtualization type
done
Length of output: 616
Script:
#!/bin/bash
# Description: Verify the updated fallback logic in virtualization detection.
# Search for assignments to VIRTUALIZATION_MICROSOFT to ensure they are correctly tied to the hyperv condition.
echo "Checking assignments to VIRTUALIZATION_MICROSOFT..."
ast-grep --lang patch --pattern $'if ($_ == "VIRTUALIZATION_NONE") { $$$ if ($_) { $$$ VIRTUALIZATION_MICROSOFT; $$$ } $$$ else if ($_) { $$$ VIRTUALIZATION_VM_OTHER; $$$ } $$$ }' > verification_result.txt
# Display the results
cat verification_result.txt
# Check for any unintended assignments to VIRTUALIZATION_MICROSOFT outside the fallback logic
echo "Ensuring VIRTUALIZATION_MICROSOFT is only assigned within the fallback logic..."
rg 'VIRTUALIZATION_MICROSOFT' | grep -v 'if (v == VIRTUALIZATION_NONE)' > unintended_assignments.txt
# Display unintended assignments if any
cat unintended_assignments.txt
Length of output: 1005
|
Thank you! |
|
@IngmarStein No problem! If you want to test it yourself, you can update to the latest dev using |
|
Ok, cool. I'll wait for the rc build and report back. |
|
I can confirm that the patch is effective. |
Guest agent doesn't start because if HyperV Enlightenments are enabled, the virtualization gets detected incorrectly. Backport Systemd patch that fixes the detection, allowing the guest-agent service to meet its dependencies.
This patch should be no longer needed after update of Systemd to v256, or in case the patch gets eventually backported to the v254 stable branch.
Fixes #3565
Summary by CodeRabbit
New Features
Bug Fixes