design-proposal: Support isolateEmulatorThread with SMT enabled Nodes #247

RamLavi · 2023-10-16T08:12:59Z

This design proposal is proposing a fix to BZ#2228103.

kubevirt-bot · 2023-10-16T08:13:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

RamLavi · 2023-10-17T06:40:20Z

/cc @vladikr @iholder101
could you take a look?

RamLavi · 2023-10-17T07:35:16Z

/cc @xpivarc
could you also take a look?

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md

orelmisan · 2023-10-17T12:43:16Z

/cc @iholder101

orelmisan

Thank you for the PR @RamLavi
Please see the inline comments.

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md

RamLavi · 2023-10-29T12:38:47Z

Change: Changed annotation to @vladikr's suggestion: kubevirt.io/CPUManagerPolicyBetaOptions:full-pcpus-only

enp0s3

@RamLavi Hi! Thank you raising this issue, I have a few questions below.

enp0s3 · 2023-10-29T13:41:57Z

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md

+* We’re adding yet another semi alpha knob to an already complicated set of knobs.
+
+# Fix Alternatives
+## Alternative #1 Adding `full-pcpus-only` option as a global configuration


@RamLavi I am not sure whether we can consider it as alternative, because it depends on the main proposal. It can rather be an option in the implementation details, to enhance the kubevirt CR with the option to annotate every VM with the suggested annotation.

@RamLavi I am not sure whether we can consider it as alternative, because it depends on the main proposal. It can rather be an option in the implementation details, to enhance the kubevirt CR with the option to annotate every VM with the suggested annotation.

I don't mean that this global option will annotate every VM once it is enabled.
I mean that this will be a global behavior, in a similar way that defaultRuntimeClass sets the runtimeClass without changing the VM manifest (metadata or spec API) at all.
It is in that sense - that it is a different alternative.

enp0s3 · 2023-10-29T13:42:58Z

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md

+Cons:
+* The suggested annotation is waisting a CPU when used on nodes with hyperthreading disabled. A cluster-wide configuration may be wasteful for heterogeneous clusters, where node-level granularity is needed.
+
+## Alternative #2 Configure both pod and Guest


@RamLavi This is also doesn't look like an alternative solution, but a suggestion to how to implement the addition of the core so that it will be even.

I kinda agree with you here.
If this alternative is chosen then the Action item is no-op in kubevirt's perspective. I just wanted this option to be heard, as it has its advantages.

Thank you for putting it writing, as this is my favorite alternative.

enp0s3 · 2023-10-29T13:47:27Z

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md

+* Similar to the way default runtimeclass is configured on Kubevirt.
+
+Cons:
+* The suggested annotation is waisting a CPU when used on nodes with hyperthreading disabled. A cluster-wide configuration may be wasteful for heterogeneous clusters, where node-level granularity is needed.


@RamLavi IIUC the main proposal also has this disadvantage. I can annotate the VM and it can still be scheduled on a non-SMT node in case the cluster is heterogeneous

yes but notice the scope - iIn the main proposal the scope is per VM, hence minimal (only when necessary), whereas on this alternative the impact is every VM on the cluster (!).
Considering that a standard Node has a few dozens of CPUs, this change of scope is what makes this Con - a very big and waistfull one (CPU wise).

Currently the VMI is blocked if the `kubevirt.io/CPUManagerPolicyBetaOptions` annotation set on the VMI and the CPUManagerPolicyBetaOptions feature-gate is not enabled. Removing this logic from the webhook in order to align with the design proposal [0] The meaning of this change is that if the annotation is set on the VMI manually - then the CPUManagerPolicyBetaOptions:full-pcpu-only behavior will take place in spite of the feature gate being disabled. [0] kubevirt/community#247 Signed-off-by: Ram Lavi <ralavi@redhat.com>

The AlignCPUs feature gate is proposed [0] and introduced [1][2][3][4] on Kubevirt in order to allow support in SMT enabled nodes while using IsolateEmulatorThread on the VMI [5]. The configuration is set cluster-wide, using the kubevirt CR (annotation + kubevirt feature gate). This commit is enabling the feature-gate on the kubevirt CR when the new HCO AlignCPUs is set to true. [0] kubevirt/community#247 [1] kubevirt/kubevirt#10593 [2] kubevirt/kubevirt#10783 [3] kubevirt/kubevirt#10839 [4] kubevirt/kubevirt#10872 [5] https://bugzilla.redhat.com/show_bug.cgi?id=2228103 Signed-off-by: Ram Lavi <ralavi@redhat.com>

… annotation The alpha.kubevirt.io/EmulatorThreadCompleteToEvenParity annotation is proposed [0] and introduced [1][2][3][4] on Kubevirt in order to allow support in SMT enabled nodes while using IsolateEmulatorThread on the VMI [5]. The configuration is set cluster-wide, using the kubevirt CR (annotation + kubevirt feature gate set in previous commit). This commit is setting the annotation on the kubevirt CR if the HCO AlignCPUs feature gate is set to true. [0] kubevirt/community#247 [1] kubevirt/kubevirt#10593 [2] kubevirt/kubevirt#10783 [3] kubevirt/kubevirt#10839 [4] kubevirt/kubevirt#10872 [5] https://bugzilla.redhat.com/show_bug.cgi?id=2228103 Signed-off-by: Ram Lavi <ralavi@redhat.com>

orelmisan · 2023-12-31T10:12:37Z

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md

+<img src="smt-slignment-error-diagram.png">
+
+## Goals
+Fix [BZ#2228103](https://bugzilla.redhat.com/show_bug.cgi?id=2228103) on the Kubevirt scope.


nit: The BZ was migrated to https://issues.redhat.com/browse/CNV-31584.

orelmisan · 2023-12-31T10:14:30Z

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md

+When the following conditions are met:
+1. The feature gate is enabled on the Kubevirt CR
+2. Kubevirt CR has the annotation.
+3. VMI is configured with `isolateEmulatorThread: true` and `dedicatedCpuPlacement: true`


The actual implementation only took isolateEmulatorThread into account, since there is a validating webhook that runs after the mutating webhook to verity both exist.

orelmisan · 2024-02-14T15:24:55Z

@vladikr @xpivarc can we move forward with this PR?
The code changes were merged.

xpivarc · 2024-02-21T13:21:13Z

/cc @aburdenthehand

aburdenthehand · 2024-02-22T18:33:00Z

/approve

kubevirt-bot · 2024-02-22T18:33:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aburdenthehand

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

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

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2023

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

kubevirt-bot requested review from jean-edouard and jobbler October 16, 2023 08:13

kubevirt-bot added the size/L label Oct 16, 2023

RamLavi force-pushed the dpdk_isolate_emulator_short_fix branch 4 times, most recently from d24e552 to 3febc99 Compare October 16, 2023 08:23

kubevirt-bot requested review from itamarh and vladikr October 17, 2023 06:40

RamLavi marked this pull request as ready for review October 17, 2023 07:26

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2023

RamLavi force-pushed the dpdk_isolate_emulator_short_fix branch from 3febc99 to 0f35e2f Compare October 17, 2023 07:29

kubevirt-bot requested a review from xpivarc October 17, 2023 07:35

dankenigsberg suggested changes Oct 17, 2023

View reviewed changes

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md Outdated Show resolved Hide resolved

design-proposals/isolate-emulator-thread-with-smt-fix/isolate-emulator-thread-with-smt-fix.md Outdated Show resolved Hide resolved

kubevirt-bot assigned dankenigsberg Oct 17, 2023

RamLavi force-pushed the dpdk_isolate_emulator_short_fix branch from 0f35e2f to 013b628 Compare October 17, 2023 08:24

kubevirt-bot requested a review from iholder101 October 17, 2023 12:43

orelmisan mentioned this pull request Oct 19, 2023

[release-0.2] vmi spec: Add isolateEmulatorThread option kiagnose/kubevirt-dpdk-checkup#193

Closed

RamLavi force-pushed the dpdk_isolate_emulator_short_fix branch from 013b628 to ccd4cfd Compare October 19, 2023 13:01

orelmisan reviewed Oct 19, 2023

View reviewed changes

RamLavi mentioned this pull request Oct 22, 2023

isolateEmulatorThread: Add full-pcpu-only support kubevirt/kubevirt#10593

Merged

8 tasks

RamLavi force-pushed the dpdk_isolate_emulator_short_fix branch from ccd4cfd to 773c594 Compare October 29, 2023 12:37

enp0s3 reviewed Oct 29, 2023

View reviewed changes

RamLavi mentioned this pull request Dec 18, 2023

Add AlignCPUs behavior by default on kubevirt CR kubevirt/hyperconverged-cluster-operator#2685

Closed

11 tasks

RamLavi mentioned this pull request Dec 19, 2023

[release 1.1] isolateEmulatorThread: Add full-pcpu-only support kubevirt/kubevirt#10757

Merged

orelmisan approved these changes Dec 31, 2023

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 31, 2023

kubevirt-bot requested a review from aburdenthehand February 21, 2024 13:21

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2024

kubevirt-bot merged commit 82423b6 into kubevirt:main Feb 22, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

design-proposal: Support isolateEmulatorThread with SMT enabled Nodes #247

design-proposal: Support isolateEmulatorThread with SMT enabled Nodes #247

RamLavi commented Oct 16, 2023

kubevirt-bot commented Oct 16, 2023

RamLavi commented Oct 17, 2023 •

edited

RamLavi commented Oct 17, 2023

orelmisan commented Oct 17, 2023

orelmisan left a comment

RamLavi commented Oct 29, 2023 •

edited

enp0s3 left a comment

enp0s3 Oct 29, 2023

RamLavi Oct 30, 2023

enp0s3 Oct 29, 2023

RamLavi Oct 30, 2023

dankenigsberg Dec 10, 2023

enp0s3 Oct 29, 2023

RamLavi Oct 30, 2023

orelmisan Dec 31, 2023

orelmisan Dec 31, 2023 •

edited

orelmisan commented Feb 14, 2024

xpivarc commented Feb 21, 2024

aburdenthehand commented Feb 22, 2024

kubevirt-bot commented Feb 22, 2024

design-proposal: Support isolateEmulatorThread with SMT enabled Nodes #247

design-proposal: Support isolateEmulatorThread with SMT enabled Nodes #247

Conversation

RamLavi commented Oct 16, 2023

kubevirt-bot commented Oct 16, 2023

RamLavi commented Oct 17, 2023 • edited

RamLavi commented Oct 17, 2023

orelmisan commented Oct 17, 2023

orelmisan left a comment

Choose a reason for hiding this comment

RamLavi commented Oct 29, 2023 • edited

enp0s3 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orelmisan Dec 31, 2023 • edited

Choose a reason for hiding this comment

orelmisan commented Feb 14, 2024

xpivarc commented Feb 21, 2024

aburdenthehand commented Feb 22, 2024

kubevirt-bot commented Feb 22, 2024

RamLavi commented Oct 17, 2023 •

edited

RamLavi commented Oct 29, 2023 •

edited

orelmisan Dec 31, 2023 •

edited