Skip to content

Add cpu policy helper and test#504

Open
TSnake41 wants to merge 1 commit into
masterfrom
teddy/cpu-policy
Open

Add cpu policy helper and test#504
TSnake41 wants to merge 1 commit into
masterfrom
teddy/cpu-policy

Conversation

@TSnake41
Copy link
Copy Markdown
Member

Add a CPU policy tooling to be able to check for certain CPU feature.
The test_cpu_policy also logs hosts CPU policies for debugging purposes (especially around live migrations).

Required by #393

@TSnake41 TSnake41 requested a review from a team as a code owner April 27, 2026 14:21
Copy link
Copy Markdown
Member

@glehmann glehmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test doesn't seem actually to validate anything. Am I missing something?

Comment thread tests/misc/test_cpu_policy.py Outdated
@TSnake41
Copy link
Copy Markdown
Member Author

The test doesn't seem actually to validate anything. Am I missing something?

There's nothing to validate aside that it doesn't errors.

Comment thread lib/cpu_policy.py Outdated
Comment thread lib/cpu_policy.py Outdated
Comment thread jobs.py Outdated
Comment thread lib/cpu_policy.py Outdated
Comment thread lib/cpu_policy.py Outdated
# msr -> val
msr: dict[int, int]

def __init__(self, cpuid: dict[(int, int), int], msr: dict[int, int]):
Copy link
Copy Markdown
Member

@glehmann glehmann Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __init__(self, cpuid: dict[(int, int), int], msr: dict[int, int]):
def __init__(self, cpuid: dict[tuple[int, int], tuple[int, int, int, int]], msr: dict[int, int]):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glehmann, is the current recommendation to use native types for typing or the ones from the typing module?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tuple[int, int, int, int] might be better represented with

@dataclass(frozen=True)
class CpuidRegisters:
    eax: int
    ebx: int
    ecx: int
    edx: int

and used as

def __init__(self, cpuid: dict[tuple[int, int], CpuidRegisters], msr: dict[int, int]):
                current_policy.cpuid[key] = CpuidRegisters(
                    int(eax, 16),
                    int(ebx, 16),
                    int(ecx, 16),
                    int(edx, 16)
                )

Each element can then be accessed by name:

current_policy.cpuid[key].eax

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glehmann, is the current recommendation to use native types for typing or the ones from the typing module?

No, we had diverging opinions on that last time we discussed it.
I'm proposing to go with builtin types in #460

Comment thread lib/cpu_policy.py Outdated
Comment thread lib/cpu_policy.py Outdated
@TSnake41 TSnake41 requested a review from a team as a code owner April 28, 2026 11:47
Copy link
Copy Markdown
Member

@dinhngtu dinhngtu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the checkers first.

@TSnake41 TSnake41 force-pushed the teddy/cpu-policy branch 2 times, most recently from accf2e0 to a31a080 Compare May 11, 2026 15:08
Add a CPU policy tooling to be able to check for certain CPU feature.
The test_cpu_policy also logs hosts CPU policies for debugging purposes.

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
@TSnake41 TSnake41 force-pushed the teddy/cpu-policy branch from a31a080 to 8842d6b Compare May 11, 2026 15:09
@stormi
Copy link
Copy Markdown
Member

stormi commented May 11, 2026

@TSnake41 After a force push, if your PR is ready for re-review, always ask the reviewers who commented for a re-review.

from lib.host import Host

class TestCpuPolicy:
def test_cpu_policy(self, host: Host) -> None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a test that doesn't test anything. I understand from the commit message that its goal is to log data about the CPUs, but I have no idea when and why this data is supposed to be gathered. Well, no, by following the link to another PR from this PR's description I understand that it's related to #393, but this explains why the lib was added (which I would probably have done in the other PR), not the test itself.

I assume that you rely on the fact that you put the test in the xen directory, and that, for the time being, it will get selected by the current selection criteria for the xen job in jobs.py, but this seems fragile to me.

I don't think we should have tests whose sole objective is to log data without testing anything in particular. But if we do, they it should be made very clear in the test description.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does log thing, but it also does tests, as if we can't gather this information (this is mostly going to happen in a change in xen-cpuid) the test would now fail, instead of making all the tests that depends on cpu policy fail.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests that depend on it would also fail, if executed first.

I guess we can keep it as it is, but it would be good to add explanations as a comment, and rename the test to indicate what it really tests: we don't test the CPU policies, here, we test CPU policies collection.


from lib.cpu_policy import NO_SUBLEAF, HostCpuPolicy
from lib.host import Host

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a "requirements" comment here, in test files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants