feat(orm): support outofband-resource-manager #406

WangZzzhe · 2023-12-14T02:50:55Z

What type of PR is this?

Features

What this PR does / why we need it:

orm(outofband-resource-manager) supports dynamically allocating resources to pods with different QoS asynchronously through a bypass approach, without relying on intrusive changes to kubelet.
The current version supports shared_cores and reclaimed_cores pool.

Which issue(s) this PR fixes:

#430

codecov · 2023-12-14T03:27:49Z

Codecov Report

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

Comparison is base (f0f2218) 53.41% compared to head (2e93922) 54.41%.
Report is 14 commits behind head on main.

Files	Patch %	Lines
pkg/agent/resourcemanager/outofband/manager.go	56.62%	134 Missing and 33 partials ⚠️
...g/agent/resourcemanager/outofband/pluginhandler.go	7.40%	99 Missing and 1 partial ⚠️
...manager/outofband/endpoint/resource_plugin_stub.go	36.61%	84 Missing and 6 partials ⚠️
pkg/util/cgroup/manager/fake_manager.go	11.11%	32 Missing ⚠️
...kg/agent/resourcemanager/outofband/pod_resource.go	77.44%	25 Missing and 5 partials ⚠️
...ent/resourcemanager/outofband/endpoint/endpoint.go	64.86%	21 Missing and 5 partials ⚠️
...ent/resourcemanager/outofband/executor/executor.go	67.14%	21 Missing and 2 partials ⚠️
...t/resourcemanager/outofband/metamanager/manager.go	84.84%	14 Missing and 1 partial ⚠️
cmd/katalyst-agent/app/options/orm/orm_base.go	50.00%	12 Missing ⚠️
cmd/katalyst-agent/app/agent/orm.go	0.00%	10 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   53.41%   54.41%   +0.99%     
==========================================
  Files         448      484      +36     
  Lines       50138    53159    +3021     
==========================================
+ Hits        26782    28924    +2142     
- Misses      20298    21053     +755     
- Partials     3058     3182     +124

Flag	Coverage Δ
unittest	`54.41% <54.88%> (+0.99%)`	⬆️

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.

waynepeking348 · 2023-12-18T05:01:43Z

pkg/agent/resourcemanager/outofband/checkpoint_test.go

+	"github.com/kubewharf/katalyst-core/pkg/agent/resourcemanager/outofband/endpoint"
+)
+
+func TestCheckpoint(t *testing.T) {


t.Parallel()

waynepeking348 · 2023-12-18T05:52:18Z

cmd/katalyst-agent/app/agent/orm.go

+)
+
+func InitORM(agentCtx *GenericContext, conf *config.Configuration, _ interface{}, _ string) (bool, Component, error) {
+	m, err := outofband.NewManager(conf.PluginRegistrationDir+"/kubelet.sock", agentCtx.EmitterPool.GetDefaultMetricsEmitter(), agentCtx.MetaServer, conf)


will this sock be conflicted with already-existing ones? such as, qrm, agent, sysadvisor?

no, "/var/lib/katalyst/plugin-socks/kubelet.sock" is only used by ORM now.
cpuPlugin: qrm_cpu_plugin_dynamic.sock
memoryPlugin: qrm_memory_plugin_dynamic.sock
headroom reporter: headroom-reporter-plugin.sock
kubelet QRM: /var/lib/kubelet

waynepeking348 · 2023-12-18T05:55:34Z

cmd/katalyst-agent/app/options/orm/orm_base.go

+
+	fs.DurationVar(&o.ORMRconcilePeriod, "orm-reconcile-period",
+		o.ORMRconcilePeriod, "orm resource reconcile period")
+	fs.Var(cliflag.NewMapStringString(&o.ORMResourceNamesMap), "orm-resource-names-map", "A set of ResourceName=ResourceQuantity (e.g. best-effort-cpu=cpu,best-effort-memory=memory,...) pairs that map resource name \"best-effort-cpu\" to resource name \"cpu\" during QoS Resource Manager allocation period.")


pls add more comments for this flag (for why we need this), since it is kind of wired for upstream

waynepeking348 · 2023-12-21T09:56:42Z

lgtm for me now; you may still need to fix the conflict and then get approve from code owners

waynepeking348 · 2023-12-25T02:27:19Z

@luomingmeng @csfldf

luomingmeng · 2024-01-03T08:59:30Z

cmd/katalyst-agent/app/options/orm/orm_base.go

+
+	fs.DurationVar(&o.ORMRconcilePeriod, "orm-reconcile-period",
+		o.ORMRconcilePeriod, "orm resource reconcile period")
+	fs.Var(cliflag.NewMapStringString(&o.ORMResourceNamesMap), "orm-resource-names-map",


why not use StringToStringVar directly?

luomingmeng · 2024-01-03T09:09:32Z

pkg/agent/resourcemanager/outofband/manager.go

+	return containerType == pluginapi.ContainerType_INIT
+}
+
+func isPodKatalystQoSLevelSystemCores(pod *v1.Pod) bool {


why not use qos conf instead of annotation directly?

luomingmeng · 2024-01-03T09:28:13Z

pkg/util/native/resources.go

@@ -306,3 +306,7 @@ func MultiplyQuantity(quantity resource.Quantity, y float64) resource.Quantity {
 	value = int64(float64(value) * y)
 	return *resource.NewQuantity(value, quantity.Format)
 }
+
+func ParseQuantityToFloat64(quantity resource.Quantity) float64 {


why not use AsApproximateFloat64 directly?

why not use AsApproximateFloat64 directly?

All comments have been resolved.

caohe · 2024-01-03T09:41:30Z

pkg/agent/resourcemanager/outofband/types.go

+
+	// errUnsupportedVersion is the error raised when the resource plugin uses an API version not
+	// supported by the Kubelet registry
+	errUnsupportedVersion = "requested API version %q is not supported by kubelet. Supported version is %q"


caohe · 2024-01-03T09:44:14Z

pkg/agent/resourcemanager/outofband/types.go

+	MetricAddPodTimeout    = "ORM_add_pod_timeout"
+	MetricDeletePodTImeout = "ORM_delete_pod_timeout"
+
+	MainContainerNameAnnotationKey = "kubernetes.io/main-container-name"


Do we need to support the main container annotation in ORM?

Do we need to support the main container annotation for ORM?

Not necessary, first container in container list will be defaulted as the main container, but we can keep it as a feature. What do you think?

In vanilla K8s, all containers are treated as main containers. If we keep this manner, will there be any problems in the colocation scenario?

To remain consistent with QRM, we can retain this usage. In the long term, when the K8s version is higher, we can use the native sidecar container feature provided by K8s. https://kubernetes.io/blog/2023/08/25/native-sidecar-containers/

caohe · 2024-01-03T09:55:50Z

pkg/agent/resourcemanager/outofband/manager.go

+			ContainerName:  container.Name,
+			ContainerType:  containerType,
+			ContainerIndex: containerIndex,
+			// customize for tce, PodRole and PodType should be identified by more general annotations


Maybe we can remove "customize for tce" ?

caohe · 2024-01-03T12:32:57Z

pkg/agent/resourcemanager/outofband/types.go

+	errBadSocket = "bad socketPath, must be an absolute path:"
+
+	// errUnsupportedVersion is the error raised when the resource plugin uses an API version not
+	// supported by the Kubelet registry


Could you also modify this comment?

caohe · 2024-01-03T12:52:38Z

cmd/katalyst-agent/app/options/orm/orm_base.go

+	fs.DurationVar(&o.ORMRconcilePeriod, "orm-reconcile-period",
+		o.ORMRconcilePeriod, "orm resource reconcile period")
+	fs.StringToStringVar(&o.ORMResourceNamesMap, "orm-resource-names-map", o.ORMResourceNamesMap,
+		"A set of ResourceName=ResourceQuantity pairs that map resource name during QoS Resource Manager allocation period. "+


Would it be more accurate to use Out-of-band Resource Manager instead of QoS Resource Manager in the message?

waynepeking348 assigned WangZzzhe Dec 14, 2023

waynepeking348 added the enhancement New feature or request label Dec 14, 2023

waynepeking348 requested review from csfldf, waynepeking348 and caohe December 14, 2023 02:58

WangZzzhe force-pushed the dev/outofband-resource-manager branch from e371bb1 to e79e43a Compare December 14, 2023 03:19

WangZzzhe force-pushed the dev/outofband-resource-manager branch from e79e43a to c983a3b Compare December 14, 2023 03:56

waynepeking348 requested a review from nightmeng December 14, 2023 11:18

waynepeking348 reviewed Dec 18, 2023

View reviewed changes

waynepeking348 added the workflow/need-review review: test succeeded, need to review label Dec 18, 2023

waynepeking348 added the workflow/merge-ready merge-ready: code is ready and can be merged label Dec 21, 2023

WangZzzhe added 4 commits December 21, 2023 20:16

feat(ORM):add outofband resource manager

ea8504b

bugfix: generate podlevel cgroup path with pod prefix

06c70ea

trigger

3763554

add comment for ORMResourceNamesMap flag

ca29648

WangZzzhe force-pushed the dev/outofband-resource-manager branch from a26b792 to ca29648 Compare December 21, 2023 12:17

WangZzzhe requested a review from luomingmeng as a code owner December 21, 2023 12:17

csfldf previously approved these changes Jan 3, 2024

View reviewed changes

waynepeking348 previously approved these changes Jan 3, 2024

View reviewed changes

caohe mentioned this pull request Jan 3, 2024

[Umbrella] Decouple QoS Resource Manager (QRM) from kubelet #430

Open

5 tasks

luomingmeng reviewed Jan 3, 2024

View reviewed changes

caohe reviewed Jan 3, 2024

View reviewed changes

waynepeking348 self-requested a review January 3, 2024 09:44

caohe reviewed Jan 3, 2024

View reviewed changes

check qoslevel by qos config in orm

0bfafcb

WangZzzhe dismissed waynepeking348’s stale review via 0bfafcb January 3, 2024 11:53

WangZzzhe dismissed csfldf’s stale review via 0bfafcb January 3, 2024 11:53

caohe reviewed Jan 3, 2024

View reviewed changes

WangZzzhe force-pushed the dev/outofband-resource-manager branch from 5194e72 to bdebb96 Compare January 3, 2024 12:49

caohe reviewed Jan 3, 2024

View reviewed changes

init qosconfig in ut

2e93922

WangZzzhe force-pushed the dev/outofband-resource-manager branch from bdebb96 to 2e93922 Compare January 3, 2024 13:02

caohe approved these changes Jan 3, 2024

View reviewed changes

caohe requested review from luomingmeng and csfldf January 3, 2024 13:04

luomingmeng approved these changes Jan 3, 2024

View reviewed changes

waynepeking348 approved these changes Jan 4, 2024

View reviewed changes

csfldf approved these changes Jan 4, 2024

View reviewed changes

waynepeking348 merged commit e86c2b7 into kubewharf:main Jan 4, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(orm): support outofband-resource-manager #406

feat(orm): support outofband-resource-manager #406

WangZzzhe commented Dec 14, 2023 •

edited by caohe

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

waynepeking348 Dec 18, 2023

WangZzzhe Dec 18, 2023

waynepeking348 Dec 18, 2023

WangZzzhe Dec 18, 2023

waynepeking348 Dec 18, 2023

WangZzzhe Dec 18, 2023

waynepeking348 commented Dec 21, 2023

waynepeking348 commented Dec 25, 2023

luomingmeng Jan 3, 2024

luomingmeng Jan 3, 2024

luomingmeng Jan 3, 2024

WangZzzhe Jan 3, 2024

caohe Jan 3, 2024

caohe Jan 3, 2024 •

edited

Loading

WangZzzhe Jan 3, 2024

caohe Jan 3, 2024

caohe Jan 3, 2024 •

edited

Loading

caohe Jan 3, 2024

caohe Jan 3, 2024

caohe Jan 3, 2024

feat(orm): support outofband-resource-manager #406

feat(orm): support outofband-resource-manager #406

Conversation

WangZzzhe commented Dec 14, 2023 • edited by caohe Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waynepeking348 commented Dec 21, 2023

waynepeking348 commented Dec 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caohe Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caohe Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WangZzzhe commented Dec 14, 2023 •

edited by caohe

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

caohe Jan 3, 2024 •

edited

Loading

caohe Jan 3, 2024 •

edited

Loading