fix(ksm): set ksm-enabled label to true independently from node pressure #11306

fossedihelm · 2024-02-20T12:20:52Z

What this PR does

Before this PR:
Currently, KubeVirt adds the kubevirt.io/ksm-enabled label to those nodes that match the labelSelector defined in kvConfiguration. The value of that label will be true only if that node is under pressure, false otherwise.

After this PR:
This is wrong since we need to provide users the information that a particular node is configured to use ksm, so that it can be used to schedule vms there.

Fixes #
jira-ticket: https://issues.redhat.com/browse/CNV-38568

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

~~Design: A design document was considered and is present (link) or not required~~
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
~~Upgrade: Impact of this change on upgrade flows was considered and addressed if required~~
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
~~Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.~~
~~Community: Announcement to kubevirt-dev was considered~~

Release note

fix(ksm): set the `kubevirt.io/ksm-enabled` node label to true if the ksm is managed by KubeVirt, instead of reflect the actual ksm value.

kubevirt-bot · 2024-02-20T12:20:54Z

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

fossedihelm · 2024-02-20T12:21:09Z

/cc @jean-edouard

fabiand · 2024-02-20T12:24:28Z

particular node is "protected" by ksm

protected might be misleading. Specifically because ksm has a security to it.
Maybe simply "particular node is configured to use ksm
?

fossedihelm · 2024-02-20T12:27:06Z

particular node is "protected" by ksm

protected might be misleading. Specifically because ksm has a security to it. Maybe simply "particular node is configured to use ksm ?

Thanks @fabiand! Looks much better! Adjusted PR description and commit message as well!

Barakmor1 · 2024-02-22T07:58:48Z

/cc

pkg/virt-handler/heartbeat/ksm.go

Barakmor1 · 2024-02-22T10:55:26Z

pkg/virt-handler/heartbeat/ksm.go

@@ -249,32 +249,33 @@ func getIntParam(node *v1.Node, param string, defaultValue, lowerBound, upperBou
 // will set the outcome value to the n.KSM struct
 // If the node labels match the selector terms, the ksm will be enabled.
 // Empty Selector will enable ksm for every node
-func handleKSM(node *v1.Node, clusterConfig *virtconfig.ClusterConfig) (bool, bool) {
-	available, running := loadKSM()
+func handleKSM(node *v1.Node, clusterConfig *virtconfig.ClusterConfig) (ksmHandlerEnabled, ksmEnabledByUs bool) {


I think that a better name would be shouldSetKsmAsEnabled i was confused by the these names

I know that this part is not trivial at all, but IMHO this function will not just enable KSM, but disable it too.
I will leave the rename discussion for a separate/refactoring PR. WDYT? Thanks

I was speaking about the return value ksmHandlerEnabled personally the name really confused me maybe we can fnid a better name than shouldSetKsmAsEnabled But IIUC this var only point is to set the value of this label

so maybe something like ksmLabelValue would make more sense?

Got you! I like the ksmLabelValue.

Barakmor1 · 2024-02-22T11:09:06Z

Don't we need to remove or change line 151 in ksm.go to

			if ksm.pages < nPagesMin {
				ksm.pages = nPagesMin
				ksm.running = true
			}

or maybe change line 187 in writeKsmValuesToFiles to:

	err := os.WriteFile(ksmRunPath, []byte([]byte("1\n")), 0644)

To make the changes in this PR description?

fossedihelm · 2024-02-22T11:36:56Z

@Barakmor1 Thanks for the review! I pushed almost all the changes with the suggested code. PTAL
For the other things, I will reply inline. Thank you!

fossedihelm · 2024-02-22T11:47:42Z

Don't we need to remove or change line 151 in ksm.go to
			if ksm.pages < nPagesMin {
				ksm.pages = nPagesMin
				ksm.running = true
			}
or maybe change line 187 in writeKsmValuesToFiles to:
	err := os.WriteFile(ksmRunPath, []byte([]byte("1\n")), 0644)
To make the changes in this PR description?

This PR is just to change the node labelling behavior, not the ksm activation logic.
This piece of code is regarding the activation of ksm, that will occur only on node pressure.

Barakmor1 · 2024-02-22T11:50:20Z

Don't we need to remove or change line 151 in ksm.go to
			if ksm.pages < nPagesMin {
				ksm.pages = nPagesMin
				ksm.running = true
			}
or maybe change line 187 in writeKsmValuesToFiles to:
	err := os.WriteFile(ksmRunPath, []byte([]byte("1\n")), 0644)
To make the changes in this PR description?
This PR is just to change the node labelling behavior, not the ksm activation logic. This piece of code is regarding the activation of ksm, that will occur only on node pressure.

If this is the case why do we need this PR?
as i see it it make sense to set kubevirt.io/ksm-enabled: false if ksm is disabled don't you think?

Maybe we should change the name of this label i find it super confusing...

Maybe something like kubevirt.io/ksm-handling: true

fossedihelm · 2024-02-22T11:57:54Z

Don't we need to remove or change line 151 in ksm.go to
			if ksm.pages < nPagesMin {
				ksm.pages = nPagesMin
				ksm.running = true
			}
or maybe change line 187 in writeKsmValuesToFiles to:
	err := os.WriteFile(ksmRunPath, []byte([]byte("1\n")), 0644)
To make the changes in this PR description?
This PR is just to change the node labelling behavior, not the ksm activation logic. This piece of code is regarding the activation of ksm, that will occur only on node pressure.
If this is the case why do we need this PR? as i see it it make sense to set kubevirt.io/ksm-enabled: false if ksm is disabled don't you think?

Because we need to provide users the information that a particular node is configured to use ksm, so that it can be used to schedule vms there.
Basically, we need to label those nodes where kubevirt, in case of node pressure, will activate ksm to reclaim some memory.
Having the kubevirt.io/ksm-enabled to true only when ksm is actually enabled is how it works right now; but this information is a mis-information, because it let people think that that node is a "safer" place to schedule VMs, but actually is the opposite, since it is currently under pressure. Do you agree? Thank you!

Barakmor1 · 2024-02-22T12:01:03Z

Don't we need to remove or change line 151 in ksm.go to
			if ksm.pages < nPagesMin {
				ksm.pages = nPagesMin
				ksm.running = true
			}
or maybe change line 187 in writeKsmValuesToFiles to:
	err := os.WriteFile(ksmRunPath, []byte([]byte("1\n")), 0644)
To make the changes in this PR description?
This PR is just to change the node labelling behavior, not the ksm activation logic. This piece of code is regarding the activation of ksm, that will occur only on node pressure.
If this is the case why do we need this PR? as i see it it make sense to set kubevirt.io/ksm-enabled: false if ksm is disabled don't you think?
Because we need to provide users the information that a particular node is configured to use ksm, so that it can be used to schedule vms there. Basically, we need to label those nodes where kubevirt, in case of node pressure, will activate ksm to reclaim some memory. Having the kubevirt.io/ksm-enabled to true only when ksm is actually enabled is how it works right now; but this information is a mis-information, because it let people think that that node is a "safer" place to schedule VMs, but actually is the opposite, since it is currently under pressure. Do you agree? Thank you?

I think that the name of the label is very confusing and we should avoid setting label called kubevirt.io/ksm-enabled to false if ksm is enabled that doesn't make sense IMO

Barakmor1 · 2024-02-22T12:50:48Z

pkg/virt-handler/heartbeat/ksm.go

+	if ksmConfig == nil && enabled {
+		disableKSM(node)
+		return false, false
 	}

 	selector, err := metav1.LabelSelectorAsSelector(ksmConfig.NodeLabelSelector)
 	if err != nil {
 		log.DefaultLogger().Errorf("An error occurred while converting the ksm selector: %s", err)
-		return running, false
+		return false, false
 	}

-	if !selector.Matches(labels.Set(node.ObjectMeta.Labels)) {
-		if disableKSM(node, running) {
-			return false, false
-		} else {
-			return running, false
-		}
+	if !selector.Matches(labels.Set(node.ObjectMeta.Labels)) && enabled {
+		disableKSM(node)
+		return false, false


why do we need the && enabled ?
I think that we should return from this func even if it is not enabled

You are right, but we also don't want to write always the same value, when ksm is disabled.
Will adjust! Thank you

jean-edouard · 2024-02-22T16:13:49Z

@Barakmor1 Yes, that name right now is misleading @acardace @xpivarc @jean-edouard What do you think?

Adding a new label kubevirt.io/ksm-handling alongside the already existing one?

Rename the kubevirt.io/ksm-enbled to kubevirt.io/ksm-handling?
Thank you!

Neither IMO. We need to restore KSM behavior to what's documented in user-guide, which this PR does, thank you! :)
As far as conveying the information of "is KSM actually running or not?" (i.e. KSM is enabled + there's memory pressure), we could add a label for that if we think it's useful, like ksm-running or ksm-active. I guess it would make sense for some workloads to have an anti-affinity with such a label.
Thanks again for fixing the logic!

Barakmor1 · 2024-02-22T21:10:30Z

@Barakmor1 Yes, that name right now is misleading @acardace @xpivarc @jean-edouard What do you think?

Adding a new label kubevirt.io/ksm-handling alongside the already existing one?

Rename the kubevirt.io/ksm-enbled to kubevirt.io/ksm-handling?
Thank you!

Neither IMO. We need to restore KSM behavior to what's documented in user-guide, which this PR does, thank you! :) As far as conveying the information of "is KSM actually running or not?" (i.e. KSM is enabled + there's memory pressure), we could add a label for that if we think it's useful, like ksm-running or ksm-active. I guess it would make sense for some workloads to have an anti-affinity with such a label. Thanks again for fixing the logic!

Sounds good to me, maybe we can think of something in a follow up PR

Barakmor1 · 2024-02-22T21:11:12Z

/lgtm

@fossedihelm Thank you
Great work

fossedihelm · 2024-02-22T23:11:59Z

Thank you both @jean-edouard @Barakmor1

Currently, KubeVirt adds the `kubevirt.io/ksm-enabled` label to those nodes that match the labelSelector defined in kvConfiguration. The value of that label will be true only if that node is under pressure, false otherwise. This is wrong since we need to provide users the information that a particular node is configured to use ksm, so that it can be used to schedule vms there. Signed-off-by: fossedihelm <ffossemo@redhat.com>

fossedihelm · 2024-02-26T08:03:43Z

@Barakmor1 Changes rebase

Barakmor1 · 2024-02-26T11:08:37Z

/lgtm

jean-edouard

/approve

kubevirt-bot · 2024-02-26T13:50:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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~~ [jean-edouard]

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

kubevirt-commenter-bot · 2024-02-26T13:50:10Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-commenter-bot · 2024-02-27T06:28:39Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-02-27T13:28:40Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fossedihelm · 2024-02-27T20:20:05Z

/cherrypick release-1.2

kubevirt-bot · 2024-02-27T20:20:07Z

@fossedihelm: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.2

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.

kubevirt-commenter-bot · 2024-02-28T03:28:41Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fossedihelm · 2024-02-28T08:30:38Z

/retest

kubevirt-bot · 2024-02-28T10:43:33Z

@fossedihelm: #11306 failed to apply on top of branch "release-1.2":

Applying: fix(ksm): set ksm-enabled label to true independently from node pressure
Using index info to reconstruct a base tree...
M	pkg/virt-handler/heartbeat/ksm.go
M	staging/src/kubevirt.io/api/core/v1/types.go
Falling back to patching base and 3-way merge...
Auto-merging staging/src/kubevirt.io/api/core/v1/types.go
Auto-merging pkg/virt-handler/heartbeat/ksm.go
CONFLICT (content): Merge conflict in pkg/virt-handler/heartbeat/ksm.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix(ksm): set ksm-enabled label to true independently from node pressure
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.2

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.

kubevirt-bot added release-note do-not-merge/work-in-progress dco-signoff: yes kind/api-change size/M labels Feb 20, 2024

kubevirt-bot requested review from dhiller and victortoso February 20, 2024 12:21

kubevirt-bot requested a review from jean-edouard February 20, 2024 12:21

fossedihelm force-pushed the fix_ksm_label branch from 1166f3b to 0e229c0 Compare February 20, 2024 12:26

fossedihelm marked this pull request as ready for review February 21, 2024 08:34

kubevirt-bot removed the do-not-merge/work-in-progress label Feb 21, 2024

kubevirt-bot requested a review from acardace February 21, 2024 08:34

kubevirt-bot requested a review from Barakmor1 February 22, 2024 07:58

Barakmor1 reviewed Feb 22, 2024

View reviewed changes

pkg/virt-handler/heartbeat/ksm.go Outdated Show resolved Hide resolved

pkg/virt-handler/heartbeat/ksm.go Outdated Show resolved Hide resolved

pkg/virt-handler/heartbeat/ksm.go Outdated Show resolved Hide resolved

pkg/virt-handler/heartbeat/ksm.go Outdated Show resolved Hide resolved

Barakmor1 reviewed Feb 22, 2024

View reviewed changes

fossedihelm force-pushed the fix_ksm_label branch from 0e229c0 to df1a875 Compare February 22, 2024 11:35

kubevirt-bot added size/L and removed size/M labels Feb 22, 2024

fossedihelm force-pushed the fix_ksm_label branch from df1a875 to 70c41f5 Compare February 22, 2024 11:50

fossedihelm force-pushed the fix_ksm_label branch from 70c41f5 to 383c8e6 Compare February 22, 2024 12:40

Barakmor1 reviewed Feb 22, 2024

View reviewed changes

fossedihelm force-pushed the fix_ksm_label branch from 383c8e6 to 21ad5ae Compare February 22, 2024 12:55

kubevirt-bot assigned Barakmor1 Feb 22, 2024

kubevirt-bot added the lgtm label Feb 22, 2024

kubevirt-bot added the needs-rebase label Feb 24, 2024

fossedihelm force-pushed the fix_ksm_label branch from 21ad5ae to cc674ee Compare February 26, 2024 08:01

kubevirt-bot removed lgtm needs-rebase labels Feb 26, 2024

kubevirt-bot added the lgtm label Feb 26, 2024

jean-edouard reviewed Feb 26, 2024

View reviewed changes

kubevirt-bot added the approved label Feb 26, 2024

kubevirt-bot merged commit 3499434 into kubevirt:main Feb 28, 2024

fossedihelm mentioned this pull request Feb 28, 2024

[release-1.2] fix(ksm): set ksm-enabled label to true independently from node pressure #11378

Merged

8 tasks

fix(ksm): set ksm-enabled label to true independently from node pressure #11306

fix(ksm): set ksm-enabled label to true independently from node pressure #11306

Conversation

fossedihelm commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

Uh oh!

kubevirt-bot commented Feb 20, 2024

Uh oh!

fossedihelm commented Feb 20, 2024

Uh oh!

fabiand commented Feb 20, 2024

Uh oh!

fossedihelm commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Barakmor1 commented Feb 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Barakmor1 Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

fossedihelm Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

Barakmor1 Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fossedihelm Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

Barakmor1 commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fossedihelm commented Feb 22, 2024

Uh oh!

fossedihelm commented Feb 22, 2024

Uh oh!

Barakmor1 commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fossedihelm commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Barakmor1 commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Barakmor1 Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

fossedihelm Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

jean-edouard commented Feb 22, 2024

Uh oh!

Barakmor1 commented Feb 22, 2024

Uh oh!

Barakmor1 commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fossedihelm commented Feb 22, 2024

Uh oh!

fossedihelm commented Feb 26, 2024

Uh oh!

Barakmor1 commented Feb 26, 2024

Uh oh!

jean-edouard left a comment

Choose a reason for hiding this comment

Uh oh!

kubevirt-bot commented Feb 26, 2024

Uh oh!

kubevirt-commenter-bot commented Feb 26, 2024

fossedihelm commented Feb 20, 2024 •

edited

Loading

fossedihelm commented Feb 20, 2024 •

edited

Loading

Barakmor1 Feb 22, 2024 •

edited

Loading

Barakmor1 commented Feb 22, 2024 •

edited

Loading

Barakmor1 commented Feb 22, 2024 •

edited

Loading

fossedihelm commented Feb 22, 2024 •

edited

Loading

Barakmor1 commented Feb 22, 2024 •

edited

Loading

Barakmor1 commented Feb 22, 2024 •

edited

Loading