Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature(artifact_test): testing the output of perftune.py #8788

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fruch
Copy link
Contributor

@fruch fruch commented Sep 19, 2024

perftune.py is a script within seastar (which is a part of scylla_setup) that configures several system parameters, like the amount of CPU cores that are designated to handle IRQ, and which are not.

Ref: https:/scylladb/scylla-enterprise/issues/2909

Replacement of #6192

Testing

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

Copy link

mergify bot commented Sep 19, 2024

⚠️ The sha of the head commit of this PR conflicts with #6192. Mergify cannot evaluate rules on this PR. ⚠️

perftune.py is a script within seastar (which is a part of scylla_setup)
that configures several system parameters, like the amount of CPU cores that
are designated to handle IRQ, and which are not.

I have recorded the expected results as of master 20230717.567b4536892f
in defaults/perftune_results.json, and the test will compare them to
the output of the script.

Ref: scylladb/scylla-enterprise#2909
@fruch fruch marked this pull request as ready for review September 24, 2024 17:10
self.architecture = architecture
self.expected_compute_bits: int = 0

match number_of_cpu_cores:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roydahan

here's the new logic, it's just maps to how many irq cpus we expect.

self.compare_cpu_mask()
self.compare_irq_cpu_mask()
if not self.node.is_nonroot_install:
# The following checks are not applicable for non-root installations,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if it's intended or not, but non-root installs doesn't call perftune at all.

@fruch fruch added backport/2024.1 Need backport to 2024.1 backport/2024.2 Need backport to 2024.2 labels Sep 24, 2024
@fruch
Copy link
Contributor Author

fruch commented Sep 25, 2024

@roydahan have you seen this one ?

self.expected_irq_bits: int = 2
case 16:
if self.architecture == "aarch64":
self.expected_irq_bits: int = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@vladzcloudius isn't this a bug?
Why would we set only 1 core for irq in arm?

@fruch fruch requested a review from a team October 1, 2024 15:09
Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

Maybe it's a good opportunity to verify defaults shipped with AMI? Could be in followup too if we want it.

sdcm/utils/perftune_validator.py Show resolved Hide resolved
try:
self.compare_cpu_mask()
self.compare_irq_cpu_mask()
if not self.node.is_nonroot_install:
Copy link
Contributor

Choose a reason for hiding this comment

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

could return here to drop nesting.

@fruch
Copy link
Contributor Author

fruch commented Oct 6, 2024

Maybe it's a good opportunity to verify defaults shipped with AMI? Could be in followup too if we want it.

defaults you mean the content of io_properites.yaml ?

@soyacz
Copy link
Contributor

soyacz commented Oct 6, 2024

Maybe it's a good opportunity to verify defaults shipped with AMI? Could be in followup too if we want it.

defaults you mean the content of io_properites.yaml ?

yes, how much we differ from that - shouldn't be much (what is much I don't know yet)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/6.2 backport/2024.1 Need backport to 2024.1 backport/2024.2 Need backport to 2024.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants