-
Notifications
You must be signed in to change notification settings - Fork 389
T7783: T7786: VPP clarify error messages for CPU requirements #4696
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
Conversation
👍 |
7d6b494
to
2cf3360
Compare
2cf3360
to
6e448a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR clarifies VPP CPU requirement error messages by providing more detailed information about system CPU configuration and availability. The changes improve user experience by showing total system cores, required cores, and available cores with clear formatting.
- Consolidates CPU verification logic to only run minimum CPU checks when workers are not explicitly configured
- Introduces a new helper function to generate detailed CPU error messages with system information
- Updates all CPU-related error messages to use consistent formatting and provide comprehensive details
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/conf_mode/vpp.py | Modified CPU verification logic to conditionally check minimum requirements |
python/vyos/vpp/config_verify.py | Added detailed CPU error message generation and updated all CPU error messages |
python/vyos/vpp/config_resource_checks/cpu.py | Added system CPU information retrieval function using lscpu |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
6e448a3
to
62968d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
return all_core_numbers | ||
|
||
|
||
def get_system_cpu_info(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this function needed? There's already vyos.utils.cpu.get_core_count()
that returns the number of physical cores, and vyos.utils.cpu.get_cpus()
for detailed information.
else '' | ||
) | ||
|
||
message = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this code use the tabulate
module perhaps, like we do in op mode a lot?
62968d6
to
647fa67
Compare
647fa67
to
8890dd1
Compare
CI integration 👍 passed! Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My change request are addressed, let's merge it.
Change summary
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Checklist: