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

feat(ORM): outofband topology manager #435

Merged

Conversation

WangZzzhe
Copy link
Collaborator

What type of PR is this?

Enhancements

What this PR does / why we need it:

Add topology manager with policies(none, best-effort, restrict, single-numa-node, numeric) for ORM.
Pods added by ORM should GetTopologyHints from QRM plugins and generated a best hint by topology manager policy before allocate resource. Dedicated-cores pods with numaBinding can be allocated resource by this way in ORM.

@WangZzzhe WangZzzhe requested a review from caohe January 8, 2024 12:52
@WangZzzhe WangZzzhe self-assigned this Jan 8, 2024
@WangZzzhe WangZzzhe added the enhancement New feature or request label Jan 8, 2024
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 139 lines in your changes are missing coverage. Please review.

Comparison is base (628d01f) 54.54% compared to head (3b4434b) 55.01%.
Report is 2 commits behind head on main.

Files Patch % Lines
pkg/agent/orm/manager.go 52.90% 59 Missing and 14 partials ⚠️
pkg/agent/orm/topology/policy_numeric.go 86.09% 15 Missing and 6 partials ⚠️
pkg/agent/orm/topology/topologyhint.go 21.05% 14 Missing and 1 partial ⚠️
pkg/agent/orm/endpoint/endpoint.go 0.00% 9 Missing ⚠️
pkg/agent/orm/topology/manager.go 91.58% 7 Missing and 2 partials ⚠️
cmd/katalyst-agent/app/options/orm/orm_base.go 63.63% 4 Missing ⚠️
pkg/agent/orm/pod_resource.go 73.33% 3 Missing and 1 partial ⚠️
pkg/util/bitmask/bitmask.go 96.96% 2 Missing and 1 partial ⚠️
pkg/agent/orm/pluginhandler.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
+ Coverage   54.54%   55.01%   +0.47%     
==========================================
  Files         493      502       +9     
  Lines       53793    54580     +787     
==========================================
+ Hits        29340    30028     +688     
- Misses      21242    21312      +70     
- Partials     3211     3240      +29     
Flag Coverage Δ
unittest 55.01% <82.58%> (+0.47%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WangZzzhe WangZzzhe force-pushed the dev/outofband-topology-manager branch from 1d874f4 to 44ba1c2 Compare January 8, 2024 13:03
@WangZzzhe WangZzzhe added the workflow/need-review review: test succeeded, need to review label Jan 8, 2024
waynepeking348
waynepeking348 previously approved these changes Jan 8, 2024
waynepeking348
waynepeking348 previously approved these changes Jan 11, 2024
@waynepeking348
Copy link
Collaborator

@caohe pls have a look for this pr

// this function for each hint provider, and merges the hints to produce
// a consensus "best" hint. The hint providers may subsequently query the
// topology manager to influence actual resource assignment.
GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]TopologyHint
Copy link
Member

Choose a reason for hiding this comment

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

Do we also need GetPodTopologyHints?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we also need GetPodTopologyHints?

yes, if we want to allocate resources for dedicated cores pods on specific NUMA nodes, this is the only way to get available hints from QRMPlugins.

Copy link
Member

Choose a reason for hiding this comment

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

It seems GetPodTopologyHints has not been defined and implemented yet. Could you also add it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems GetPodTopologyHints has not been defined and implemented yet. Could you also add it?

sorry I misunderstood you question, GetPodTopologyHints is not implemented by QRM plugins so we don't need it.
I add GetPodTopologyHints for ORM manager and it only returns nil now.

Copy link
Member

Choose a reason for hiding this comment

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

According to offline discussions, we can leave an empty implementation now and implement it later. Maybe we can record a TODO in the comment. cc @WangZzzhe

@WangZzzhe WangZzzhe force-pushed the dev/outofband-topology-manager branch from 85ece23 to 3b4434b Compare January 15, 2024 09:41
@caohe caohe added the workflow/merge-ready merge-ready: code is ready and can be merged label Jan 15, 2024
@waynepeking348 waynepeking348 merged commit f0e37c9 into kubewharf:main Jan 17, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request workflow/merge-ready merge-ready: code is ready and can be merged workflow/need-review review: test succeeded, need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants