feat(xo-server): set new XAPI platform flag for nested virtualization #8395

MelissaFrncJrg · 2025-03-03T09:45:20Z

Description

Short explanation of this PR (feel free to re-use commit message)

Checklist

Commit
- Title follows commit conventions
- Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
- If bug fix, add Introduced by
Changelog
- If visible by XOA users, add changelog entry
- Update "Packages to release" in CHANGELOG.unreleased.md
PR
- If UI changes, add screenshots
- If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

develop skills of junior reviewers

limit the workload for senior reviewers

limit the number of unnecessary changes by the author

The author creates a PR.
Review process:
1. The author assigns the junior reviewer.
2. The junior reviewer conducts their review:
  - Resolves their comments if they are addressed.
  - Adds comments if necessary or approves the PR.
3. The junior reviewer assigns the senior reviewer.
4. The senior reviewer conducts their review:
  - If there are no unresolved comments on the PR → merge.
  - Otherwise, we continue with 3.
The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

The author can request a review at any time, even if the PR is still a Draft.
In theory, there should not be more than one reviewer at a time.
The author should not make any changes:
- When a reviewer is assigned.
- Between the junior and senior reviews.

MathieuRA · 2025-03-03T13:10:08Z

packages/xo-server/src/xapi-object-to-xo.mjs

+      expNestedHvm: semver.satisfies(obj.hardware_platform_version, '>=3.4')
+        ? obj.platform['nested-virt'] === 'true'
+        : obj.platform['exp-nested-hvm'] === true,


IMHO, expNestedHvm should only return obj.platform['exp-nested-hvm'] === ' true'' to be consistent with the XAPI and be flagged as deprecated with a small comment above explaining why, and what to use instead.
And maybe expose one property like:

isNestedVirtEnable: (isXcp83OrHigher && platformVersion >= 3.4 ? obj.platform['nested-virt'] : obj.platform['exp-nested-hvm']) === 'true'

If you follow my suggestion, you should update the BaseXoVm type in @vates/types/xo.mts to add the new property and flag expNestedHvm as deprecated

MathieuRA · 2025-03-03T13:12:49Z

packages/xo-server/src/xapi/mixins/vm.mjs

+      set: (expNestedHvm, vm) => {
+        const platformVersion = semver.satisfies(vm.hardware_platform_version, '>=3.4')
+          ? 'nested-virt'
+          : 'exp-nested-hvm'
+        vm.update_platform(platformVersion, expNestedHvm ? 'true' : null)
+      },


Same here, I found that weird having a set expNestedHvm that in reality will updated nested-virt

would it be ok to update both ?
also this could help handling VM that are migrated from 8.2 to 8.3 hosts

MathieuRA · 2025-03-03T13:16:10Z

packages/xo-server/src/xapi/mixins/vm.mjs

@@ -491,7 +492,12 @@ const methods = {
    hasVendorDevice: true,

    expNestedHvm: {
-      set: (expNestedHvm, vm) => vm.update_platform('exp-nested-hvm', expNestedHvm ? 'true' : null),
+      set: (expNestedHvm, vm) => {
+        const platformVersion = semver.satisfies(vm.hardware_platform_version, '>=3.4')


can platform_version be >= 3.4 in case of XCP-ng 8.2?

No it will always be 3.2.1

3.4 is only XCP-ng 8.3 and XS 8.4

MathieuRA · 2025-03-03T13:24:32Z

CHANGELOG.unreleased.md

+- [VM] Updated Nested Virtualization handling to use `platform:nested-virt` for XCP-ng 8.3+ (PR [#8395](https://github.com/vatesfr/xen-orchestra/pull/8395))
+


This is not really user-oriented, but I don't have a better suggestion.
Maybe you can ask the XCP team what the advantage of using nested-virt is, and for example in the changelog:
use platform:nested-virt to determine if nested virtualization is enabled, because ...

Nested virt has never supported by XCP-ng but it kinda worked until 8.3 where some stuff in Xen broke it.
Some changes were made to gt it back to where it were before - some not supported but kinda working and considered experimental.

So for the phrasing I'd say "add back nested virt experimental features for XCP-ng >=8.3" or something like that.

Bear in mind the fix is not yet available for users though so we might want to sync for the release.

fbeauchamp

would it be ok to update both value on edit and only read the right value for the host handling the VM when checking it ?

this could help handling VM that are migrated from 8.2 to 8.3 hosts

or does the VM with the old flags need to add the new flag to handle the (future) nested virtualization ? ( @benjamreis )

benjamreis · 2025-03-06T08:42:37Z

would it be ok to update both value on edit and only read the right value for the host handling the VM when checking it ?

this could help handling VM that are migrated from 8.2 to 8.3 hosts

or does the VM with the old flags need to add the new flag to handle the (future) nested virtualization ? ( @benjamreis )

I'm not sure - ping @ydirson @stormi .
I think having both flags all the time means the new one will be ignored on 8.2 and the old one will be ignored in 8.3 - but the case of 8.2 -> 8.3 upgrade is indeed a good reason to have both on 8.2 at least. What do you guys think?

ydirson · 2025-03-06T09:18:37Z

I do think the easy way is to set both flags unconditionally until 8.2 gets out of support, but concern was voiced that it would make the old flag live for too long (potentially forever).
OTOH setting both flags would not handle all cases either, like a VM on a 8.2 pool that had nested-virt previously activated with the old flag only, and that gets upgraded to 8.3. That kind of mismatch could possibly be detected by XO to offer the use the ability to "fix the flag", but then the same mechanism could also be used if we decide to stick with the >= 3.4 check to keep a single flag.

feat(xo-server): set new XAPI platform flag for nested virtualization

40d4b6c

MelissaFrncJrg requested a review from MathieuRA March 3, 2025 09:45

MelissaFrncJrg added 2 commits March 3, 2025 10:49

changelog entry

2e76d7b

prettier

6966fa0

MelissaFrncJrg marked this pull request as ready for review March 3, 2025 13:01

MathieuRA requested changes Mar 3, 2025

View reviewed changes

MathieuRA requested a review from fbeauchamp March 3, 2025 13:24

fbeauchamp reviewed Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(xo-server): set new XAPI platform flag for nested virtualization #8395

feat(xo-server): set new XAPI platform flag for nested virtualization #8395

MelissaFrncJrg commented Mar 3, 2025

MathieuRA Mar 3, 2025

MathieuRA Mar 3, 2025

fbeauchamp Mar 6, 2025

MathieuRA Mar 3, 2025

benjamreis Mar 3, 2025

ydirson Mar 6, 2025

MathieuRA Mar 3, 2025

benjamreis Mar 3, 2025

fbeauchamp left a comment

benjamreis commented Mar 6, 2025

ydirson commented Mar 6, 2025

		- [VM] Updated Nested Virtualization handling to use `platform:nested-virt` for XCP-ng 8.3+ (PR [#8395](https://github.com/vatesfr/xen-orchestra/pull/8395))

feat(xo-server): set new XAPI platform flag for nested virtualization #8395

Are you sure you want to change the base?

feat(xo-server): set new XAPI platform flag for nested virtualization #8395

Conversation

MelissaFrncJrg commented Mar 3, 2025

Description

Checklist

Review process

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbeauchamp left a comment

Choose a reason for hiding this comment

benjamreis commented Mar 6, 2025

ydirson commented Mar 6, 2025