koordlet: support block I/O QoS #1144

TheBeatles1994 · 2023-03-27T10:08:54Z

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

fixes #1003

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests
All checks passed in make test

codecov · 2023-03-27T10:30:02Z

Codecov Report

Patch coverage: 47.19% and project coverage change: -1.09 ⚠️

Comparison is base (c88a2d4) 66.10% compared to head (930b61d) 65.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1144      +/-   ##
==========================================
- Coverage   66.10%   65.02%   -1.09%     
==========================================
  Files         303      311       +8     
  Lines       31593    32719    +1126     
==========================================
+ Hits        20885    21274     +389     
- Misses       9179     9884     +705     
- Partials     1529     1561      +32

Flag	Coverage Δ
unittests	`65.02% <47.19%> (-1.09%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/koordlet/metriccache/metric_cache.go	`57.03% <0.00%> (-2.06%)`	⬇️
pkg/koordlet/metriccache/storage_tables.go	`57.14% <ø> (ø)`
pkg/koordlet/metrics/common.go	`61.22% <0.00%> (-15.70%)`	⬇️
pkg/koordlet/metricsadvisor/metrics_advisor.go	`86.15% <ø> (ø)`
pkg/koordlet/resmanager/resmanager.go	`54.54% <0.00%> (-0.84%)`	⬇️
pkg/koordlet/statesinformer/states_informer.go	`66.66% <0.00%> (-4.13%)`	⬇️
pkg/koordlet/util/cgroup_parent_utils.go	`93.75% <0.00%> (-6.25%)`	⬇️
pkg/koordlet/util/node.go	`5.06% <0.00%> (-0.35%)`	⬇️
pkg/koordlet/util/system/cgroup_resource.go	`80.68% <ø> (ø)`
pkg/koordlet/util/system/util_test_tool.go	`50.00% <ø> (ø)`
... and 8 more

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

apis/slo/v1alpha1/nodeslo_types.go

pkg/features/koordlet_features.go

zwzhang0107 · 2023-03-28T03:42:48Z

pkg/koordlet/resmanager/blkio_reconcile.go

+	resmanager   *resmanager
+	executor     resourceexecutor.ResourceUpdateExecutor
+	blkioScanner util.LocalStorageScanner
+	pvcInformer  cache.SharedIndexInformer


i remember we have a TODO here about "getting pod volume from local"

can we solve the problem now or leave a todo here

I have extended the interface of the states informer by adding the GetVolumeName function, which allows users to retrieve the relevant storage volume name through the PVC name.

zwzhang0107 · 2023-03-28T03:48:38Z

pkg/koordlet/resmanager/blkio_reconcile.go

+	strategy := nodeSLO.Spec.ResourceQOSStrategy
+	// lsr
+	if strategy.LSRClass != nil && strategy.LSRClass.BlkIOQOS != nil && *strategy.LSRClass.BlkIOQOS.Enable && len(strategy.LSRClass.BlkIOQOS.Blocks) != 0 {
+		klog.Warningf("%s: configuring blkio of LSRClass is not supported!", BlkIOReconcileName)


consider create an warning event to NodeSLO or Pod for bad format config

create warning event in reconcile may cause too many events?

we can add a flow control, like create event at least 10 minutes or more

I think we should add more information to the "NodeSLO" status field, such as warnings and error messages to provide better visibility into the status of the node.

yes, this is also an option. we can compare before update to avoid too many requests, and warning events can also be used for alarm

Currently, there is no definition for the status of NodeSLO. Shall we create a to-do for it?

pkg/koordlet/resmanager/blkio_reconcile.go

pkg/koordlet/util/system/cgroup_resource.go

saintube · 2023-03-30T04:17:14Z

pkg/koordlet/resourceexecutor/updater.go

+}
+
+// resourceType sysutil.ResourceType, parentDir string, value string, updateFunc UpdateFunc
+func NewBlkIOResourceUpdater(file sysutil.Resource, value string, parentDir string) *CgroupResourceUpdater {


Could we register the cgroup updater of the blkio resource in DefaultCgroupUpdaterFactory to support cgroups-v2 and maintain the blkio updater uniformly? Afterward, the callers can use the blkio updater via DefaultCgroupUpdaterFactory.New(resourceType, cgroupParent, value, auditor).

saintube · 2023-03-30T04:25:35Z

pkg/koordlet/util/blkio_scanner.go

+	}
+)
+
+func NewLocalStorageScanner(interval time.Duration) LocalStorageScanner {


could you move the scanner as a plugin of the metric advisor(collector) since it starts a goroutine to collect the node disk info periodically? or we may leave a TODO here?

saintube · 2023-03-30T04:28:42Z

pkg/koordlet/util/cgroup_parent_utils.go

+// GetPodCgroupDirWithKube gets the full pod cgroup parent dir with the podParentDir (excluding kubepods dir).
+// @podKubeRelativeDir kubepods-burstable.slice/kubepods-pod7712555c_ce62_454a_9e18_9ff0217b8941.slice/
+// @return kubepods.slice/kubepods-burstable.slice/kubepods-pod7712555c_ce62_454a_9e18_9ff0217b8941.slice/
+func GetPodCgroupDirWithKube(podParentDir string) string {


Please do not use GetPodCgroupDirWithKube anymore, as it is just removed recently. Instead, use the podMeta.podParentDir directly. See #1129.

saintube · 2023-04-03T10:20:27Z

pkg/koordlet/metriccache/api.go

+type NodeLocalStorageInfo struct {
+	util.LocalStorageInfo
+	// map is not thread safe. so "concurrent read/write of Map" can cause error.
+	RWLock sync.RWMutex


is it no longer in use?

It is used in BlkIOReconcile module (pkg/koordlet/resmanager/blkio_reconcile.go) for getting storage info.

perhaps the mutex lock is unnecessary

saintube · 2023-04-03T11:43:12Z

pkg/koordlet/metricsadvisor/collectors/nodeinfo/node_info_collector.go

+		klog.Errorf("insert node local storage info error: %v", err)
+	}
+
+	n.started.Store(true)


Some other modules rely on the status started of the CPU Info while sharing the status in collectNodeLocalStorageInfo may corrupt the condition.
e.g. The nodeTopologyInformer requires the CPU Info synced, so it can report the correct CPU topology.
How about moving the collection of the local storage into a new plugin?

Maybe we can rename NodeInfoCollector plugin to NodeCPUInfoCollector plugin, and add a new plugin named NodeStorageInfoCollector?

Or just remove n.started.Store(true)?

Yes, I prefer option one.

saintube · 2023-04-03T11:54:35Z

pkg/koordlet/resmanager/blkio_reconcile.go

+// deviceName: /dev/sdb
+// diskNumber: 253:16
+func (b *BlkIOReconcile) getDiskNumberFromDevice(deviceName string) (string, error) {
+	storageInfo, err := b.resmanager.metricCache.GetNodeLocalStorageInfo(&metriccache.QueryParam{})


perhaps we only need to invoke GetNodeLocalStorageInfo once in a reconcile

saintube · 2023-04-03T11:55:43Z

pkg/koordlet/util/node.go

+// GetCgroupRootBlkIOAbsoluteDir gets the root blkio directory
+// @output /sys/fs/cgroup/blkio
+func GetCgroupRootBlkIOAbsoluteDir() string {
+	return path.Join(system.Conf.CgroupRootDir, system.CgroupBlkioDir)


please use filepath.Join instead

saintube · 2023-04-14T02:37:35Z

pkg/koordlet/util/cgroup_parent_utils.go

+// @return like kubepods-burstable.slice/kubepods-pod7712555c_ce62_454a_9e18_9ff0217b8941.slice/
+// /sys/fs/cgroup/blkio/kubepods.slice/kubepods-burstable.slice/kubepods-pod7712555c_ce62_454a_9e18_9ff0217b8941.slice
+func GetPodCgroupBlkIOAbsolutePath(podParentDir string) string {
+	return path.Join(system.Conf.CgroupRootDir, system.CgroupBlkioDir, podParentDir)


A minor issue: Please use filepath.Join and check the other modifications where the path pkg is still adopted.

There are many other people's codes that also use the path.Join function, do I need to change them all at once?

Not required in this patch. Feel free to do the cleanup in another PR.

zwzhang0107 · 2023-04-18T03:40:34Z

consider using mount info to get the pvc path
https://github.com/kubernetes/cri-api/blob/master/pkg/apis/runtime/v1/api.proto#L1243

Signed-off-by: 惠志 <[email protected]>

zwzhang0107 · 2023-04-24T06:41:23Z

/lgtm

zwzhang0107 · 2023-04-24T06:41:33Z

/approve

zwzhang0107 · 2023-04-24T06:42:00Z

/assign @FillZpp @jasonliu747

FillZpp · 2023-04-24T09:56:48Z

/approve

jasonliu747

/lgtm

koordinator-bot · 2023-04-26T01:54:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FillZpp, jasonliu747, zwzhang0107

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~apis/OWNERS~~ [FillZpp]
~~config/OWNERS~~ [jasonliu747]
~~pkg/features/OWNERS~~ [FillZpp]
~~pkg/koordlet/OWNERS~~ [jasonliu747,zwzhang0107]
~~pkg/util/OWNERS~~ [FillZpp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

allwmh · 2023-04-27T02:59:13Z

pkg/koordlet/statesinformer/states_pvc.go

+	s.volumeNameMap[util.GetNamespacedName(pvc.Namespace, pvc.Name)] = pvc.Spec.VolumeName
+}
+
+func newPVCInformer(client clientset.Interface) cache.SharedIndexInformer {


the pvc informer should be restrict on the node, otherwise it may be raise the memory used and the kube-apiserver pressure.

koordinator-bot bot requested review from FillZpp, jasonliu747 and saintube March 27, 2023 10:09

koordinator-bot bot added the size/XXL label Mar 27, 2023

jasonliu747 changed the title ~~support io qos~~ koordlet: support block I/O QoS Mar 27, 2023

eahydra added area/koordlet enhancement New feature or request labels Mar 27, 2023

TheBeatles1994 force-pushed the support-io-qos branch 4 times, most recently from 5d2fe53 to 7ba6beb Compare March 28, 2023 03:22

zwzhang0107 reviewed Mar 28, 2023

View reviewed changes

TheBeatles1994 force-pushed the support-io-qos branch from 7ba6beb to 36193e4 Compare March 29, 2023 10:11

saintube reviewed Mar 30, 2023

View reviewed changes

TheBeatles1994 force-pushed the support-io-qos branch 3 times, most recently from 3efd6be to 6bf6915 Compare April 3, 2023 09:10

saintube reviewed Apr 3, 2023

View reviewed changes

TheBeatles1994 force-pushed the support-io-qos branch 2 times, most recently from a38f0ff to e1bb381 Compare April 14, 2023 01:57

saintube reviewed Apr 14, 2023

View reviewed changes

TheBeatles1994 force-pushed the support-io-qos branch 2 times, most recently from a530b0f to 63ab8ea Compare April 14, 2023 09:31

hormes added this to the v1.3 milestone Apr 18, 2023

TheBeatles1994 force-pushed the support-io-qos branch 4 times, most recently from 76a07b7 to 4b480ff Compare April 23, 2023 04:41

TheBeatles1994 force-pushed the support-io-qos branch 2 times, most recently from 46ff289 to 2264019 Compare April 24, 2023 03:10

koordlet: support block I/O QoS

930b61d

Signed-off-by: 惠志 <[email protected]>

TheBeatles1994 force-pushed the support-io-qos branch from 2264019 to 930b61d Compare April 24, 2023 04:51

koordinator-bot bot assigned zwzhang0107 Apr 24, 2023

koordinator-bot bot added the lgtm label Apr 24, 2023

koordinator-bot bot assigned FillZpp and jasonliu747 Apr 24, 2023

zwzhang0107 mentioned this pull request Apr 24, 2023

[proposal] improvement of BlkIO QoS: using Mount info in ContainerStatus instead of watching pvc from apiserver #1244

Closed

jasonliu747 approved these changes Apr 26, 2023

View reviewed changes

koordinator-bot bot added the approved label Apr 26, 2023

koordinator-bot bot merged commit 3f9fb9a into koordinator-sh:main Apr 26, 2023

allwmh reviewed Apr 27, 2023

View reviewed changes

koordlet: support block I/O QoS #1144

koordlet: support block I/O QoS #1144

Conversation

TheBeatles1994 commented Mar 27, 2023 • edited by jasonliu747 Loading

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

codecov bot commented Mar 27, 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

chzhj Mar 29, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintube Mar 30, 2023 • 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

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

TheBeatles1994 Apr 10, 2023 • 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

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

saintube Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwzhang0107 commented Apr 18, 2023

zwzhang0107 commented Apr 24, 2023

zwzhang0107 commented Apr 24, 2023

zwzhang0107 commented Apr 24, 2023

FillZpp commented Apr 24, 2023

jasonliu747 left a comment

Choose a reason for hiding this comment

koordinator-bot bot commented Apr 26, 2023

Choose a reason for hiding this comment

TheBeatles1994 commented Mar 27, 2023 •

edited by jasonliu747

Loading

codecov bot commented Mar 27, 2023 •

edited

Loading

chzhj Mar 29, 2023 •

edited

Loading

saintube Mar 30, 2023 •

edited

Loading

TheBeatles1994 Apr 10, 2023 •

edited

Loading

saintube Apr 14, 2023 •

edited

Loading