koordlet:move audit log to resource executor after resource update #1055

Re-Grh · 2023-02-27T09:20:47Z

Ⅰ. Describe what this PR does

to solve the problem of duplicate audit logs by moving it to the executor module after resource update

Ⅱ. Does this pull request fix one issue?

fixes #1018
The audit log redundancy issue has been addressed by leveraging the executor's built-in cache, which has changed the recording frequency from 10 seconds to 60 seconds. Meanwhile, the adjustbecpuset policy execution shows frequent CPU rebinding , both klog and audit log will print.

and klog shows here

and i will start a new issue since it looks like we need to modify the function applyCPUSetWithNonePolicy(beCPUSet, oldCPUSet) to improve the stability of CPUs for the BE Group's calculation policy.

Ⅲ. 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-02-27T09:41:40Z

Codecov Report

Patch coverage: 44.18% and no project coverage change.

Comparison is base (7fafbe3) 66.98% compared to head (8329606) 66.98%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1055   +/-   ##
=======================================
  Coverage   66.98%   66.98%           
=======================================
  Files         263      263           
  Lines       28975    29036   +61     
=======================================
+ Hits        19409    19450   +41     
- Misses       8206     8226   +20     
  Partials     1360     1360

Flag	Coverage Δ
unittests	`66.98% <44.18%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...oordlet/runtimehooks/protocol/container_context.go	`5.33% <0.00%> (-0.04%)`	⬇️
.../koordlet/runtimehooks/protocol/kubeqos_context.go	`0.00% <0.00%> (ø)`
pkg/koordlet/runtimehooks/protocol/pod_context.go	`6.95% <0.00%> (ø)`
pkg/koordlet/runtimehooks/protocol/protocol.go	`3.57% <0.00%> (ø)`
pkg/koordlet/util/system/common.go	`58.92% <0.00%> (-3.34%)`	⬇️
pkg/util/cpuset/cpuset.go	`70.90% <0.00%> (-2.34%)`	⬇️
pkg/koordlet/resourceexecutor/resctrl_updater.go	`44.09% <11.11%> (-1.81%)`	⬇️
pkg/koordlet/resourceexecutor/cgroup.go	`60.44% <46.15%> (-2.84%)`	⬇️
.../koordlet/runtimehooks/hooks/groupidentity/rule.go	`81.30% <57.14%> (-0.30%)`	⬇️
pkg/koordlet/resourceexecutor/updater.go	`45.03% <60.00%> (+1.60%)`	⬆️
... and 4 more

... and 3 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.

pkg/koordlet/resmanager/cpu_suppress.go

pkg/koordlet/resourceexecutor/updater.go

zwzhang0107 · 2023-03-03T03:58:59Z

pkg/koordlet/resourceexecutor/updater.go

@@ -227,6 +256,7 @@ type NewResourceUpdaterFunc func(resourceType sysutil.ResourceType, parentDir st
 type ResourceUpdaterFactory interface {
 	Register(g NewResourceUpdaterFunc, resourceTypes ...sysutil.ResourceType)
 	New(resourceType sysutil.ResourceType, parentDir string, value string) (ResourceUpdater, error)
+	NewWithAudit(resourceType sysutil.ResourceType, parentDir string, value string, e *audit.EventHelper) (ResourceUpdater, error)


the origin New() can be abandoned and rename NewWithAudit with New
Is there anyone still using New()?

pkg/koordlet/resmanager/cpu_suppress.go

zwzhang0107 · 2023-03-03T04:04:49Z

pkg/koordlet/resourceexecutor/updater.go

 func CommonCgroupUpdateFunc(resource ResourceUpdater) error {
 	c := resource.(*CgroupResourceUpdater)
-	_ = audit.V(5).Reason(ReasonUpdateCgroups).Message("update %v to %v", resource.Path(), resource.Value()).Do()
-	return sysutil.CgroupFileWriteIfDifferent(c.parentDir, c.file, c.value)
+	dirty, err := cgroupFileWriteIfDifferent(c.parentDir, c.file, c.value)


dirty->updated

zwzhang0107 · 2023-03-03T04:07:33Z

pkg/koordlet/resourceexecutor/updater.go

@@ -227,6 +256,7 @@ type NewResourceUpdaterFunc func(resourceType sysutil.ResourceType, parentDir st
 type ResourceUpdaterFactory interface {
 	Register(g NewResourceUpdaterFunc, resourceTypes ...sysutil.ResourceType)
 	New(resourceType sysutil.ResourceType, parentDir string, value string) (ResourceUpdater, error)
+	NewWithAudit(resourceType sysutil.ResourceType, parentDir string, value string, e *audit.EventHelper) (ResourceUpdater, error)


there is still some V5 logs in other pkg, which need to be modified.

zwzhang0107 · 2023-03-03T10:23:48Z

problem about applyCPUSetWithNonePolicy?

zwzhang0107 · 2023-03-03T10:25:39Z

pkg/koordlet/resourceexecutor/resctrl_updater.go

@@ -151,7 +151,7 @@ func UpdateResctrlTasksFunc(resource ResourceUpdater) error {
 	// 122
 	// 123
 	// 124
-	_ = audit.V(5).Reason(ReasonUpdateResctrl).Message("update %v to %v", resource.Key(), resource.Value()).Do()
+	_ = audit.V(3).Reason(ReasonUpdateResctrl).Message("update %v to %v", resource.Key(), resource.Value()).Do()


zwzhang0107 · 2023-03-06T08:38:14Z

pkg/koordlet/resmanager/cpu_suppress.go

-	if err != nil {
-		klog.Warningf("suppressBECPU failed to apply be cpu suppress policy, err: %s", err)
-		return
+	if !compareIntSlice(beCPUSet, oldCPUSet) {


remove this

zwzhang0107 · 2023-03-08T11:28:24Z

pkg/koordlet/resourceexecutor/updater.go

 	f.lock.RLock()
 	defer f.lock.RUnlock()
 	g, ok := f.registry[resourceType]
 	if !ok {
 		return nil, fmt.Errorf("resource type %s not registered", resourceType)
 	}
-	return g(resourceType, parentDir, value)
+	u, err := g(resourceType, parentDir, value)


pass EventHelper to g()

zwzhang0107 · 2023-03-08T11:30:34Z

pkg/koordlet/resourceexecutor/updater.go

+	} else if updated {
+		_ = audit.V(3).Reason(ReasonUpdateCgroups).Message("update %v to %v", resource.Path(), resource.Value()).Do()
+	}
+	return nil
 }

 func CommonDefaultUpdateFunc(resource ResourceUpdater) error {


NewCommonDefaultUpdater

zwzhang0107 · 2023-03-09T04:04:59Z

pkg/koordlet/resmanager/system_config.go

@@ -81,7 +81,7 @@ func caculateMemoryConfig(strategy *slov1alpha1.SystemStrategy, nodeMemory int64
 		if sysutil.ValidateResourceValue(&minFreeKbytes, "", sysutil.MinFreeKbytes) {
 			valueStr := strconv.FormatInt(minFreeKbytes, 10)
 			file := sysutil.MinFreeKbytes.Path("")
-			resource, err := resourceexecutor.NewCommonDefaultUpdater(file, file, valueStr)
+			resource, err := resourceexecutor.NewCommonDefaultUpdater(file, file, valueStr, nil)


should pass eventHelper, not nil

zwzhang0107 · 2023-03-09T04:07:40Z

pkg/koordlet/resourceexecutor/updater.go

 	r, err := sysutil.GetCgroupResource(resourceType)
 	if err != nil {
 		return nil, err
 	}
+	if e == nil {


why need this

zwzhang0107 · 2023-03-09T04:09:13Z

pkg/koordlet/resourceexecutor/updater.go

-func NewCommonDefaultUpdaterWithUpdateFunc(key string, file string, value string, updateFunc UpdateFunc) (ResourceUpdater, error) {
+func NewCommonDefaultUpdaterWithUpdateFunc(key string, file string, value string, updateFunc UpdateFunc, e *audit.EventHelper) (ResourceUpdater, error) {
+	if e == nil {
+		e = &audit.EventHelper{}


zwzhang0107 · 2023-03-09T04:09:59Z

pkg/koordlet/resourceexecutor/updater.go

+	if err != nil {
+		return err
+	}
+	if updated && c.eventHelper.Event.Reason != "" {


zwzhang0107 · 2023-03-09T09:50:20Z

check the final result

Signed-off-by: Re-Grh <1271013391@qq.com>

zwzhang0107 · 2023-03-10T03:45:42Z

/approve
/lgtm

zwzhang0107 · 2023-03-10T03:46:00Z

/assign @FillZpp

hormes · 2023-03-13T01:21:32Z

/approve

koordinator-bot · 2023-03-13T01:21:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hormes, 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:

~~pkg/koordlet/OWNERS~~ [hormes,zwzhang0107]
~~pkg/util/OWNERS~~ [hormes]

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

koordinator-bot bot requested review from saintube and zwzhang0107 February 27, 2023 09:20

koordinator-bot bot added the size/L label Feb 27, 2023

Re-Grh changed the title ~~move audit log to resource executor after resource update~~ koordlet:move audit log to resource executor after resource update Feb 27, 2023

saintube reviewed Feb 27, 2023

View reviewed changes

zwzhang0107 reviewed Mar 3, 2023

View reviewed changes

Re-Grh force-pushed the audit branch from 2acefe8 to 8b50d0d Compare March 3, 2023 09:45

zwzhang0107 reviewed Mar 3, 2023

View reviewed changes

zwzhang0107 mentioned this pull request Mar 6, 2023

koordlet: add event cache for audit log #941

Closed

3 tasks

zwzhang0107 reviewed Mar 6, 2023

View reviewed changes

Re-Grh force-pushed the audit branch 4 times, most recently from 02cb0c6 to 8fec93d Compare March 8, 2023 08:44

Re-Grh closed this Mar 8, 2023

Re-Grh reopened this Mar 8, 2023

Re-Grh closed this Mar 8, 2023

Re-Grh force-pushed the audit branch from 8fec93d to 616b2c3 Compare March 8, 2023 09:19

koordinator-bot bot added size/XS and removed size/L labels Mar 8, 2023

Re-Grh reopened this Mar 8, 2023

koordinator-bot bot added size/L and removed size/XS labels Mar 8, 2023

Re-Grh force-pushed the audit branch from 6cd9098 to e69bfe1 Compare March 8, 2023 10:44

zwzhang0107 reviewed Mar 8, 2023

View reviewed changes

zwzhang0107 reviewed Mar 9, 2023

View reviewed changes

Re-Grh force-pushed the audit branch 2 times, most recently from 2734589 to 663cf6d Compare March 9, 2023 09:31

audit-fix-2

e0dcdcc

Signed-off-by: Re-Grh <1271013391@qq.com>

Re-Grh force-pushed the audit branch from c41889f to 4669b9c Compare March 9, 2023 13:31

audit-fix-3

cd8585c

Signed-off-by: Re-Grh <1271013391@qq.com>

Re-Grh force-pushed the audit branch from 4669b9c to cd8585c Compare March 9, 2023 13:36

audit-fix-f

8891e62

Signed-off-by: Re-Grh <1271013391@qq.com>

Re-Grh force-pushed the audit branch from 02da136 to 8891e62 Compare March 10, 2023 03:02

Merge branch 'main' into audit

8329606

koordinator-bot bot assigned zwzhang0107 Mar 10, 2023

koordinator-bot bot added the lgtm label Mar 10, 2023

koordinator-bot bot assigned FillZpp Mar 10, 2023

koordinator-bot bot added the approved label Mar 13, 2023

koordinator-bot bot merged commit 4c2d190 into koordinator-sh:main Mar 13, 2023

zwzhang0107 added this to the v1.2 milestone Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

koordlet:move audit log to resource executor after resource update #1055

koordlet:move audit log to resource executor after resource update #1055

Re-Grh commented Feb 27, 2023 •

edited

codecov bot commented Feb 27, 2023 •

edited

zwzhang0107 Mar 3, 2023

zwzhang0107 Mar 3, 2023

zwzhang0107 Mar 3, 2023

zwzhang0107 commented Mar 3, 2023

zwzhang0107 Mar 3, 2023

zwzhang0107 Mar 6, 2023

zwzhang0107 Mar 8, 2023

zwzhang0107 Mar 8, 2023

zwzhang0107 Mar 9, 2023

zwzhang0107 Mar 9, 2023

zwzhang0107 Mar 9, 2023

zwzhang0107 Mar 9, 2023

zwzhang0107 commented Mar 9, 2023

zwzhang0107 commented Mar 10, 2023

zwzhang0107 commented Mar 10, 2023

hormes commented Mar 13, 2023

koordinator-bot bot commented Mar 13, 2023

koordlet:move audit log to resource executor after resource update #1055

koordlet:move audit log to resource executor after resource update #1055

Conversation

Re-Grh commented Feb 27, 2023 • edited

Ⅰ. 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 Feb 27, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwzhang0107 commented Mar 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwzhang0107 commented Mar 9, 2023

zwzhang0107 commented Mar 10, 2023

zwzhang0107 commented Mar 10, 2023

hormes commented Mar 13, 2023

koordinator-bot bot commented Mar 13, 2023

Re-Grh commented Feb 27, 2023 •

edited

codecov bot commented Feb 27, 2023 •

edited