Fix: Align Reenlightenment flows between converter.go and template.go #8740

iholder101 · 2022-11-08T20:07:01Z

What this PR does / why we need it:
This PR aims to fix an inconsistency regarding re-enlightenment VMs and TSC frequency.

While the converter.go's logic checked whether we have topology hints defined and that tsc frequency is required (which is the right thing to do), template.go logic did not check the later. The both flows are now aligned, with IsManualTSCFrequencyRequired() being the sole source of truth.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2139896

Release note:

Fix: Align Reenlightenment flows between converter.go and template.go

iholder101 · 2022-11-08T20:07:22Z

/cc @vladikr

vladikr · 2022-11-08T20:14:40Z

pkg/virt-controller/services/template_test.go

 				Entry("and consider graphics overhead if it is not set on arm64", "arm64", nil, 469),
-				Entry("and consider graphics overhead if it is set to true on arm64", "arm64", True(), 469),
-				Entry("and not consider graphics overhead if it is set to false on arm64", "arm64", False(), 453),
+				Entry("and consider graphics overhead if it is set to true on arm64", "arm64", pointer.Bool(true), 469),


vladikr · 2022-11-08T20:15:59Z

Great!
Thanks @iholder-redhat.
Looks good to me.

/approve

kubevirt-bot · 2022-11-08T20:16:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vladikr

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

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

vladikr · 2022-11-08T21:35:56Z

/cc @acardace

vladikr · 2022-11-10T15:11:56Z

/hold

It looks like any frequency can be supported on a CPU that has an invtsc flag.
In that case, we could make the scheduling requirement more robust.

vladikr · 2022-11-10T17:27:03Z

/hold

It looks like any frequency can be supported on a CPU that has an invtsc flag. In that case, we could make the scheduling requirement more robust.

Everything looks fine :) The low freq. is being added as a schedulable label on nodes with scalable tsc. [1]
[1]

kubevirt/pkg/virt-controller/watch/topology/tsc.go

Line 88 in 24ec5ba

    
           func CalculateTSCLabelDiff(frequenciesInUse []int64, frequenciesOnNode []int64, nodeFrequency int64, scalable bool) (toAdd []int64, toRemove []int64) {

vladikr · 2022-11-10T17:27:13Z

/hold cancel

acardace · 2022-11-14T09:14:25Z

/restest-required

acardace · 2022-11-15T10:25:57Z

/test pull-kubevirt-e2e-k8s-1.24-sig-compute-root

acardace · 2022-11-15T10:26:17Z

/cherrypick release-0.58

kubevirt-bot · 2022-11-15T10:26:24Z

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

In response to this:

/cherrypick release-0.58

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.

While the converter.go logic checked whether we have topology hints defined and that tsc frequency is required (which is the right thing to do), template.go logic did not check the later. In addition, improve IsManualTSCFrequencyRequired() to also check whether topology hints are defined. Signed-off-by: Itamar Holder <iholder@redhat.com>

Signed-off-by: Itamar Holder <iholder@redhat.com>

…e updated Signed-off-by: Itamar Holder <iholder@redhat.com>

That's how we can distinguish between us waiting for a TSC frequency and knowing there is no TSC frequency information, hence we can stop waiting for it. This is necessary, since the vmi controller executes "sync()" and "updateStatus()" in every control loop. If TSC frequency is required, sync() would wait until topology hints would be set up by updateStatus(). Today, updateStatus() doesn't update the topology hints at all if no TSC frequency information exists, therefore sync will continue waiting forever and the VM wouldn't be able to start. Signed-off-by: Itamar Holder <iholder@redhat.com>

…gy hints are updated Signed-off-by: Itamar Holder <iholder@redhat.com>

…ined Signed-off-by: Itamar Holder <iholder@redhat.com>

acardace · 2022-11-15T16:45:13Z

/lgtm

kubevirt-bot · 2022-11-15T22:52:58Z

@acardace: #8740 failed to apply on top of branch "release-0.58":

Applying: Fix: Align Reenlightenment flows between converter.go and template.go
Applying: Unit test: ensure tsc label is set when needed
Using index info to reconstruct a base tree...
M	pkg/virt-controller/services/BUILD.bazel
M	pkg/virt-controller/services/template_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/services/template_test.go
Auto-merging pkg/virt-controller/services/BUILD.bazel
Applying: Unit test: Refactor
Applying: make sure vmi controller does not create pod before topology hints are updated
Applying: Set frequency as 0 if TSC frequency is available on the cluster
Applying: unit test: make sure vmi controller does not create pod before topology hints are updated
Using index info to reconstruct a base tree...
M	pkg/virt-controller/watch/vmi_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/watch/vmi_test.go
Applying: Functest: Ensure reenlightenment VMs have tsc label and frequency defined
Using index info to reconstruct a base tree...
A	tests/hyperv_test.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): tests/hyperv_test.go deleted in HEAD and modified in Functest: Ensure reenlightenment VMs have tsc label and frequency defined. Version Functest: Ensure reenlightenment VMs have tsc label and frequency defined of tests/hyperv_test.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0007 Functest: Ensure reenlightenment VMs have tsc label and frequency defined
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-0.58

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.

iholder101 · 2022-12-25T09:55:14Z

/cherrypick release-0.53

kubevirt-bot · 2022-12-25T09:55:51Z

@iholder101: #8740 failed to apply on top of branch "release-0.53":

Applying: Fix: Align Reenlightenment flows between converter.go and template.go
Using index info to reconstruct a base tree...
M	pkg/virt-controller/services/template.go
M	pkg/virt-launcher/virtwrap/converter/converter.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-launcher/virtwrap/converter/converter.go
Auto-merging pkg/virt-controller/services/template.go
CONFLICT (content): Merge conflict in pkg/virt-controller/services/template.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: Align Reenlightenment flows between converter.go and template.go
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-0.53

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 Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Nov 8, 2022

kubevirt-bot requested review from kbidarkar and lyarwood November 8, 2022 20:07

kubevirt-bot requested a review from vladikr November 8, 2022 20:07

vladikr reviewed Nov 8, 2022

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022

kubevirt-bot requested a review from acardace November 8, 2022 21:35

iholder101 force-pushed the fix/reenlightenment-align-flows branch from 035a72a to 9a723e8 Compare November 10, 2022 10:26

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2022

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2022

acardace approved these changes Nov 15, 2022

View reviewed changes

kubevirt-bot assigned acardace Nov 15, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2022

iholder101 added 4 commits November 15, 2022 18:13

Unit test: ensure tsc label is set when needed

b6f9e70

Signed-off-by: Itamar Holder <iholder@redhat.com>

Unit test: Refactor

7e8a404

Signed-off-by: Itamar Holder <iholder@redhat.com>

make sure vmi controller does not create pod before topology hints ar…

5f7c6e8

…e updated Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 added 3 commits November 15, 2022 18:13

unit test: make sure vmi controller does not create pod before topolo…

b82ce72

…gy hints are updated Signed-off-by: Itamar Holder <iholder@redhat.com>

Functest: Ensure reenlightenment VMs have tsc label and frequency def…

c9a8668

…ined Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 force-pushed the fix/reenlightenment-align-flows branch from 9a723e8 to c9a8668 Compare November 15, 2022 16:34

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2022

kubevirt-bot merged commit 40b4770 into kubevirt:main Nov 15, 2022
40 of 41 checks passed

iholder101 mentioned this pull request Nov 16, 2022

[release-0.58]: Fix: Align Reenlightenment flows between converter.go and template.go #8798

Merged

Fix: Align Reenlightenment flows between converter.go and template.go #8740

Fix: Align Reenlightenment flows between converter.go and template.go #8740

iholder101 commented Nov 8, 2022 •

edited

iholder101 commented Nov 8, 2022

vladikr Nov 8, 2022

vladikr commented Nov 8, 2022

kubevirt-bot commented Nov 8, 2022

vladikr commented Nov 8, 2022

vladikr commented Nov 10, 2022

vladikr commented Nov 10, 2022

vladikr commented Nov 10, 2022

acardace commented Nov 14, 2022

acardace commented Nov 15, 2022

acardace commented Nov 15, 2022

kubevirt-bot commented Nov 15, 2022

acardace commented Nov 15, 2022

kubevirt-bot commented Nov 15, 2022

iholder101 commented Dec 25, 2022

kubevirt-bot commented Dec 25, 2022

Fix: Align Reenlightenment flows between converter.go and template.go #8740

Fix: Align Reenlightenment flows between converter.go and template.go #8740

Conversation

iholder101 commented Nov 8, 2022 • edited

iholder101 commented Nov 8, 2022

vladikr Nov 8, 2022

Choose a reason for hiding this comment

vladikr commented Nov 8, 2022

kubevirt-bot commented Nov 8, 2022

vladikr commented Nov 8, 2022

vladikr commented Nov 10, 2022

vladikr commented Nov 10, 2022

vladikr commented Nov 10, 2022

acardace commented Nov 14, 2022

acardace commented Nov 15, 2022

acardace commented Nov 15, 2022

kubevirt-bot commented Nov 15, 2022

acardace commented Nov 15, 2022

kubevirt-bot commented Nov 15, 2022

iholder101 commented Dec 25, 2022

kubevirt-bot commented Dec 25, 2022

iholder101 commented Nov 8, 2022 •

edited