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

oVirt / vdsm

The Virtual Desktop Server Manager

GNU General Public License v2.0

160 stars 201 forks source link

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

Closed shubhaOracle closed 5 months ago

shubhaOracle commented 5 months ago

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 Shubha.kulkarni@oracle.com

aesteve-rh commented 5 months ago

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 commented 5 months ago

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

shubhaOracle commented 5 months ago

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

tinez commented 5 months ago

@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 commented 5 months ago

Posted a short guide here

shubhaOracle commented 5 months ago

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 37fe02548 and 8f12263bc. Pl suggest alternative.

tinez commented 5 months ago

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

shubhaOracle commented 5 months ago

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

tinez commented 5 months ago

yw :)

shubhaOracle commented 5 months ago

Made fix to address the failing assertion.

mz-pdm commented 5 months ago

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.

shubhaOracle commented 5 months ago

Updated the commit message.