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

chore: fix wrong stressor init in validator #1844

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

rootfs
Copy link
Contributor

@rootfs rootfs commented Nov 14, 2024

this is to fix the previous PR that didn't get full tested. This change is verified with the following config:

log_level: warn # Logging level, defaults is warn

remote:
  host: my-vm
  username: root
  pkey: /tmp/vm_ssh_key

metal:
  vm:
    pid: 123456 # Process ID for the KVM process running on metal

prometheus:
  job:
    vm: vm  # Job name for virtual machine metrics, default is vm
    metal: metal  # Job name for metal metrics, default is metal

  url: http://localhost:9090 # Prometheus server URL
  rate_interval: 20s  # Rate interval for Promql, default is 20s, typically 4 x $scrape_interval
  steps: 3s  # Step duration for Prometheus range queries

stressor:
  total_runtime_seconds: 600  # Total runtime in seconds for the stress test
  curve_type: default  # Curve type for the stress test, default is linear

validations_file: ./validations.yaml  # Path to the validations file, default is ./validations.yaml

Copy link
Contributor

github-actions bot commented Nov 14, 2024

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This pull request enhances the stress testing functionality in the validator by introducing two new parameters, total_runtime_seconds and curve_type, to the remote.run_script method and the Stressor class initialization. The changes also update the validator configuration to include a "stressor" section with default values for these parameters.

Key Modifications:

  • Added total_runtime_seconds and curve_type parameters to remote.run_script method call and Stressor class initialization.
  • Updated validator configuration to include a "stressor" section with default values.

Impact: These changes facilitate more comprehensive stress testing and correct the stressor initialization in the validator.

Observations/Suggestions:

  • It would be beneficial to include additional tests to validate the correctness of the stress testing functionality with these new parameters.
  • Consider adding documentation or comments to explain the purpose and usage of the new parameters and the updated configuration section.
  • Review the default values for total_runtime_seconds and curve_type to ensure they are suitable for various use cases.

@rootfs rootfs force-pushed the stresser branch 2 times, most recently from c471dd8 to 9e07a8b Compare November 14, 2024 15:09
Copy link
Collaborator

@KaiyiLiu1234 KaiyiLiu1234 left a comment

Choose a reason for hiding this comment

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

lgtm

@KaiyiLiu1234 KaiyiLiu1234 merged commit 15ddff2 into sustainable-computing-io:main Nov 14, 2024
21 checks passed
# ruff: noqa: S108 (Suppressed hard-coded path because we want to intentionally copy stress.sh inside `/tmp` dir)
target_script = "/tmp/stress.sh"
cli_options = f"-t {self.total_runtime_seconds} -c {self.curve_type}"
cli_options = " ".join([f"-{k} {v}" for k, v in kwargs.items()]) if kwargs else ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

very nice!

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.

3 participants