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

balloons: add support for isolated cpus. #344

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

fmuyassarov
Copy link
Collaborator

The isolcpus boot parameter allows you to isolate specific CPUs from
the general SMP balancing and scheduler algorithms. Currently, when a
balloon picks up CPUs, it does not differentiate between isolated and
non-isolated CPUs. This change improves the balloon's logic to:

  1. Distinguish between isolated and non-isolated CPUs.
  2. Allow users to preferably select isolated CPUs, though this is not guaranteed.
  3. Avoid using isolated CPUs unless specifically requested by the user via CR.

@klihub klihub changed the title Add support for isolated cpus in balloons balloons: add support for isolated cpus. Jun 26, 2024
@fmuyassarov fmuyassarov force-pushed the devel/isolcpus-balloons branch 2 times, most recently from b236ab8 to f54d190 Compare June 27, 2024 12:43
Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

Once all review comments are addressed I'm happy with this as soon as @askervin finds it good to go in.

Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

Nit: spotted a typo in documentation.

docs/resource-policy/policy/balloons.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

I have a number of new review comments and suggestions related to the e2e test framework additions and the test scriptlet itself.

test/e2e/run.sh Outdated Show resolved Hide resolved
@fmuyassarov fmuyassarov changed the title balloons: add support for isolated cpus. WIP: balloons: add support for isolated cpus. Jul 2, 2024
@fmuyassarov fmuyassarov force-pushed the devel/isolcpus-balloons branch 3 times, most recently from 9fb9999 to 64da112 Compare July 3, 2024 13:27
docs/resource-policy/policy/balloons.md Outdated Show resolved Hide resolved
deployment/helm/balloons/values.yaml Outdated Show resolved Hide resolved
deployment/helm/balloons/values.yaml Outdated Show resolved Hide resolved
docs/resource-policy/policy/balloons.md Outdated Show resolved Hide resolved
klihub
klihub previously approved these changes Jul 4, 2024
Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM.

@klihub klihub dismissed their stale review July 4, 2024 15:24

A few more nits...

Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

After passed test pod0 is left behind:

vagrant@n4c16-generic-ubuntu2204-containerd:~$ kubectl get pods -A
NAMESPACE     NAME                                                          READY   STATUS    RESTARTS      AGE
default       pod0                                                          2/2     Running   2 (92s ago)   2m10s
...

@klihub already suggested changes to cleanup() so that nothing is left behind if the test passed.

@fmuyassarov
Copy link
Collaborator Author

After passed test pod0 is left behind:

vagrant@n4c16-generic-ubuntu2204-containerd:~$ kubectl get pods -A
NAMESPACE     NAME                                                          READY   STATUS    RESTARTS      AGE
default       pod0                                                          2/2     Running   2 (92s ago)   2m10s
...

@klihub already suggested changes to cleanup() so that nothing is left behind if the test passed.

@askervin @klihub this should be addressed now. PTAL.

@fmuyassarov fmuyassarov force-pushed the devel/isolcpus-balloons branch 3 times, most recently from 0a38247 to 05dc6c8 Compare July 5, 2024 08:18
Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

There seems to be a risk of failing next tests if this passes.

Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this new feature, @fmuyassarov!

@klihub klihub merged commit 9704e94 into containers:main Jul 5, 2024
3 checks passed
@fmuyassarov fmuyassarov deleted the devel/isolcpus-balloons branch July 5, 2024 12:43
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