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

Fix the cpu pinning parsing logic to parse the format appropriately and exclude cpu from pinning list #409

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

shubhaOracle
Copy link
Contributor

Issue: The CPU pinning tool tip says that to exclude a cpu from pinning, it needs to provided in the following format.
0#10-13_1#25-27,^26

With this format CPU 26 will be excluded from pinning. However, vdsm code fails to parse the above string resulting in VM start failure.

Fix: Fix the cpu pinning parsing logic to parse the format appropriately and exclude cpu from pinning list.

Signed-off-by: ShubhaOracle [email protected]

lib/vdsm/taskset.py Outdated Show resolved Hide resolved
@aesteve-rh
Copy link
Member

Please squash both commits into a single one. In general, changes addressing reviews shall be made into the relevant commit, not adding new commits, to keep the git history clean.

Thanks for your contribution!

@tinez
Copy link
Member

tinez commented Mar 12, 2024

The code LGTM. +1 to what @aesteve-rh said - please rebase the branch and squash the commits into a single one.

@shubhaOracle
Copy link
Contributor Author

Updated the branch. Can we squash commits when merging the changes?

@tinez
Copy link
Member

tinez commented Mar 14, 2024

@shubhaOracle you've used a merge commit instead of rebasing. You can read about the differences here. Please rewrite the branch so that we have 1 commit that includes all the changes and no merge commits.

I just noticed that we don't have a section in our development guide on git commits, I'll make a PR for that.

@tinez
Copy link
Member

tinez commented Mar 14, 2024

Posted a short guide here

@shubhaOracle
Copy link
Contributor Author

I tried to follow the instructions to rebase and squash. While the rebase part seems to have worked, the squash commit can only work with adjacent commit. Pl see the screenshot. The commits need to squash are 37fe025 and 8f12263. Pl suggest alternative.
image

@tinez
Copy link
Member

tinez commented Mar 15, 2024

I've made a nice tutorial for you, enjoy :) https://asciinema.org/a/9QlEBnXyUlEBIJ2q1putVOoZC

@shubhaOracle
Copy link
Contributor Author

@tinez Thank you so much for the tutorial!!! I have made the changes now.

@tinez
Copy link
Member

tinez commented Mar 18, 2024

yw :)

Copy link
Member

@tinez tinez left a comment

Choose a reason for hiding this comment

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

We still need to address the failing assertion.

@shubhaOracle
Copy link
Contributor Author

Made fix to address the failing assertion.

@mz-pdm
Copy link
Member

mz-pdm commented Mar 19, 2024

The patch itself looks good to me, but I'd suggest:

  • Improving the commit message. It's important to have good and descriptive commit messages because they are the primary source of information about the changes applied when examining them in future. In this case, I would change the commit title to something like virt: Add support for excluding CPUs in CPU list parsing, which fits into a single line easily. And would add the reasoning, basically this PR description, to the commit message body.
  • Using negative ranges, e.g. 1-10,^4-6, is not supported, which is fine but is worth to mention in the commit message.
  • It may be worth to mention the exclusion syntax in cpulist_parse docstring.

Issue: The CPU pinning tool tip says that to exclude a cpu from pinning,
it needs to provided in the following format.
0#10-13_1#25-27,^26

With this format CPU 26 will be excluded from pinning. However, vdsm
code fails to parse the above string resulting in VM start failure.

Fix: Fix the cpu pinning parsing logic to parse the format appropriately
and exclude cpu from pinning list.

Signed-off-by: ShubhaOracle <[email protected]>
@shubhaOracle
Copy link
Contributor Author

Updated the commit message.

@tinez tinez merged commit c641c1f into oVirt:master Mar 20, 2024
17 checks passed
@sandrobonazzola sandrobonazzola added this to the ovirt-4.5.7 milestone Oct 18, 2024
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.

5 participants