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: expose balloons and optionally containers with affinity in NRT #469

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

askervin
Copy link
Collaborator

No description provided.

@klihub klihub requested review from klihub, kad and fmuyassarov January 23, 2025 17:26
Copy link
Collaborator

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

Great work @askervin !
One question, I just ran a test from your branch and in my bare minimum cluster where there isn't any real workload with hundreds of Pods, the size of the NRT object was close to 600 lines. I'm just wondering if you have verified that if there is any limit from the api-machinery for the size of the CRD. Because in case of production level cluster, there might be hundreds of pods running and size of the NRT will grow quite fast as every container has its own zone attribute.

cmd/plugins/balloons/policy/balloons-policy.go Outdated Show resolved Hide resolved
@fmuyassarov
Copy link
Collaborator

Would you mind to share why did you mean (optionally) on the PR title?

@askervin askervin changed the title RFC: expose balloons and (optionally) allowed CPUs and mems of every container in NRT balloons: expose balloons and optionally containers with affinity in NRT Jan 24, 2025
@askervin askervin marked this pull request as ready for review January 24, 2025 13:09
@askervin
Copy link
Collaborator Author

Would you mind to share why did you mean (optionally) on the PR title?

showContainersInNrt policy and balloon option are now added. They didn't exist in the draft yet. They default to false, that is, only balloon instances are shown by default.

When enabled, the balloons policy exposes containers in balloons as
node resource topology subzones. This enables inspecting, for
instance, CPU and memory affinity of any ballooned containers via node
resource topology customer resources.

Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
Copy link
Collaborator

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

Thanks Antti. Looks better I think.
IINW Changing the showContainersInNrt field has no effect until the next container event. If true, do you think it is worth of mentioning it in the document? Otherwise, LGTM.

@kad
Copy link
Collaborator

kad commented Feb 3, 2025

LGTM. It might be good to have NRT capitalisation instead of Nrt, for readability, but that's not critical.

@klihub klihub merged commit 9f7ba0d into containers:main Feb 4, 2025
9 checks passed
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