You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/09/15 22:04:24 UTC
[GitHub] [incubator-yunikorn-k8shim] yangwwei opened a new pull request #303: [YUNIKORN-848] Scheduler occasionally crashed due to concurrent r/w map error.
yangwwei opened a new pull request #303:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/303
### What is this PR for?
It is found that occasionally the scheduler could crash due to concurrent map r/w error. The root cause is because during the evaluation of the predicates, we pass a nodesInfo map as a reference to the K8s side, and when concurrently there is a node being add/remove to the cluster, it could possibly cause the concurrent read/write issue. This PR is to fix that by passing a copy of nodes into.
### What type of PR is it?
* [x] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-848
### How should this be tested?
I've reproduced this locally with the following code (apply this to `predicator_test.go`):
```
func TestConcurrentAccess(t *testing.T) {
conf.GetSchedulerConf().SetTestMode(true)
events.SetRecorderForTest(events.NewMockedRecorder())
predictor := newPredictorInternal(&factory.PluginFactoryArgs{}, schedulerapi.Policy{
Predicates: []schedulerapi.PredicatePolicy{
{Name: predicates.MatchInterPodAffinityPred},
}})
pod1 := v1.Pod{
Spec: v1.PodSpec{
NodeName: "machine1",
Affinity: &v1.Affinity{
PodAffinity: &v1.PodAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: []v1.PodAffinityTerm{
{
LabelSelector: metav1.SetAsLabelSelector(labels.Set{
"app": "xyz",
}),
},
},
},
},
},
}
node1 := v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "machine1"}}
podsOnNode := []*v1.Pod{
&pod1,
}
nodeInfo := deschedulernode.NewNodeInfo(podsOnNode...)
_ = nodeInfo.SetNode(&node1)
nodeInfoMap := map[string]*deschedulernode.NodeInfo{
node1.Name: nodeInfo,
}
cache := &nodesCacheForTest{
nodesMap: nodeInfoMap,
}
go func() {
for {
meta := predictor.GetPredicateMeta(&pod1, cache.getNodesMap())
if err := predictor.Predicates(&pod1, meta, nodeInfo, true); err != nil {
//fmt.Println(err.Error())
}
}
}()
i := 0
go func() {
for {
time.Sleep(1 * time.Millisecond)
nodeName := fmt.Sprintf("machine%d", i)
t.Logf("adding node %s", nodeName)
newNodeInfo := deschedulernode.NewNodeInfo()
nodeN := v1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
_ = newNodeInfo.SetNode(&nodeN)
cache.addNode(nodeN.Name, newNodeInfo)
i++
}
}()
time.Sleep(10 * time.Second)
fmt.Println(len(nodeInfoMap))
}
type nodesCacheForTest struct {
nodesMap map[string]*deschedulernode.NodeInfo
sync.RWMutex
}
func (c *nodesCacheForTest) getNodesMap() map[string]*deschedulernode.NodeInfo {
return c.nodesMap
}
func (c *nodesCacheForTest) getNodesMapCopy() map[string]*deschedulernode.NodeInfo {
c.RLock()
defer c.RUnlock()
newMap := make(map[string]*deschedulernode.NodeInfo, len(c.nodesMap))
for k, v := range c.nodesMap {
newMap[k] = v.Clone()
}
return newMap
}
func (c *nodesCacheForTest) addNode(name string, nodeInfo *deschedulernode.NodeInfo) {
c.Lock()
defer c.Unlock()
c.nodesMap[name] = nodeInfo
}
```
Note, I set 1ms to update the map in order to trigger this in UT code. I got the stack trace:
```
fatal error: concurrent map read and map write
goroutine 1248 [running]:
runtime.throw(0x23ca940, 0x21)
/Users/wyang/go1.15/src/runtime/panic.go:1116 +0x72 fp=0xc000368c90 sp=0xc000368c60 pc=0x1038ed2
runtime.mapaccess1_faststr(0x21bf6a0, 0xc0005a22a0, 0x23aa919, 0x8, 0xc00030bb70)
/Users/wyang/go1.15/src/runtime/map_faststr.go:21 +0x465 fp=0xc000368d00 sp=0xc000368c90 pc=0x1015fc5
k8s.io/kubernetes/pkg/scheduler/algorithm/predicates.getTPMapMatchingIncomingAffinityAntiAffinity.func2(0x0)
/Users/wyang/gopath/pkg/mod/k8s.io/kubernetes@v1.16.13/pkg/scheduler/algorithm/predicates/metadata.go:715 +0xc5 fp=0xc000368f50 sp=0xc000368d00 pc=0x201ab45
k8s.io/client-go/util/workqueue.ParallelizeUntil.func1(0xc0007a2a60, 0xc0007cbb80, 0xc000311c10, 0xc0007e8750)
/Users/wyang/gopath/pkg/mod/k8s.io/client-go@v0.16.13/util/workqueue/parallelizer.go:57 +0x8d fp=0xc000368fc0 sp=0xc000368f50 pc=0x1eacccd
runtime.goexit()
/Users/wyang/go1.15/src/runtime/asm_amd64.s:1374 +0x1 fp=0xc000368fc8 sp=0xc000368fc0 pc=0x1070c21
created by k8s.io/client-go/util/workqueue.ParallelizeUntil
/Users/wyang/gopath/pkg/mod/k8s.io/client-go@v0.16.13/util/workqueue/parallelizer.go:49 +0x14a
goroutine 1 [chan receive]:
testing.(*T).Run(0xc000502900, 0x23b90c8, 0x14, 0x2452260, 0x1096401)
/Users/wyang/go1.15/src/testing/testing.go:1160 +0x3ad
testing.runTests.func1(0xc000582d80)
/Users/wyang/go1.15/src/testing/testing.go:1430 +0x78
testing.tRunner(0xc000582d80, 0xc000755de0)
/Users/wyang/go1.15/src/testing/testing.go:1108 +0xef
testing.runTests(0xc0000dc4a0, 0x2f499a0, 0xa, 0xa, 0x0, 0x0, 0x0, 0x100fc48)
/Users/wyang/go1.15/src/testing/testing.go:1428 +0x2e8
testing.(*M).Run(0xc00050e400, 0x0)
/Users/wyang/go1.15/src/testing/testing.go:1338 +0x245
main.main()
_testmain.go:61 +0x138
goroutine 19 [chan receive]:
k8s.io/klog.(*loggingT).flushDaemon(0x2f5f480)
/Users/wyang/gopath/pkg/mod/k8s.io/klog@v1.0.0/klog.go:1010 +0x8b
created by k8s.io/klog.init.0
/Users/wyang/gopath/pkg/mod/k8s.io/klog@v1.0.0/klog.go:411 +0xd8
goroutine 8 [chan receive]:
k8s.io/klog/v2.(*loggingT).flushDaemon(0x2f5f560)
/Users/wyang/gopath/pkg/mod/k8s.io/klog/v2@v2.0.0/klog.go:1107 +0x8b
created by k8s.io/klog/v2.init.0
/Users/wyang/gopath/pkg/mod/k8s.io/klog/v2@v2.0.0/klog.go:416 +0xd8
goroutine 54 [sleep]:
time.Sleep(0x2540be400)
/Users/wyang/go1.15/src/runtime/time.go:188 +0xbf
github.com/apache/incubator-yunikorn-k8shim/pkg/plugin/predicates.TestConcurrentAccess(0xc000502900)
/Users/wyang/workspace/apache/yunikorn/incubator-yunikorn-k8shim/pkg/plugin/predicates/predicator_test.go:2203 +0x54d
testing.tRunner(0xc000502900, 0x2452260)
/Users/wyang/go1.15/src/testing/testing.go:1108 +0xef
created by testing.(*T).Run
/Users/wyang/go1.15/src/testing/testing.go:1159 +0x386
goroutine 55 [semacquire]:
sync.runtime_Semacquire(0xc0007a2a68)
/Users/wyang/go1.15/src/runtime/sema.go:56 +0x45
sync.(*WaitGroup).Wait(0xc0007a2a60)
/Users/wyang/go1.15/src/sync/waitgroup.go:130 +0x65
k8s.io/client-go/util/workqueue.ParallelizeUntil(0x258a060, 0xc0007e2fc0, 0x4, 0x4, 0xc0007e8750)
/Users/wyang/gopath/pkg/mod/k8s.io/client-go@v0.16.13/util/workqueue/parallelizer.go:62 +0x16d
k8s.io/kubernetes/pkg/scheduler/algorithm/predicates.getTPMapMatchingIncomingAffinityAntiAffinity(0xc0001b0000, 0xc0005a22a0, 0xc0007e6670, 0x0, 0x0, 0xc0007e6650)
/Users/wyang/gopath/pkg/mod/k8s.io/kubernetes@v1.16.13/pkg/scheduler/algorithm/predicates/metadata.go:754 +0x6c5
k8s.io/kubernetes/pkg/scheduler/algorithm/predicates.(*PredicateMetadataFactory).GetMetadata(0xc000262370, 0xc0001b0000, 0xc0005a22a0, 0x2550320, 0xc0007e6650)
/Users/wyang/gopath/pkg/mod/k8s.io/kubernetes@v1.16.13/pkg/scheduler/algorithm/predicates/metadata.go:222 +0x1ee
github.com/apache/incubator-yunikorn-k8shim/pkg/plugin/predicates.(*Predictor).GetPredicateMeta(...)
/Users/wyang/workspace/apache/yunikorn/incubator-yunikorn-k8shim/pkg/plugin/predicates/predictor.go:245
github.com/apache/incubator-yunikorn-k8shim/pkg/plugin/predicates.TestConcurrentAccess.func1(0xc0002c44d0, 0xc0001b0000, 0xc00000e100, 0xc00030ba00)
/Users/wyang/workspace/apache/yunikorn/incubator-yunikorn-k8shim/pkg/plugin/predicates/predicator_test.go:2182 +0x43
created by github.com/apache/incubator-yunikorn-k8shim/pkg/plugin/predicates.TestConcurrentAccess
/Users/wyang/workspace/apache/yunikorn/incubator-yunikorn-k8shim/pkg/plugin/predicates/predicator_test.go:2180 +0x4ee
goroutine 56 [sleep]:
time.Sleep(0xf4240)
/Users/wyang/go1.15/src/runtime/time.go:188 +0xbf
github.com/apache/incubator-yunikorn-k8shim/pkg/plugin/predicates.TestConcurrentAccess.func2(0xc00051a088, 0xc000502900, 0xc00000e100)
/Users/wyang/workspace/apache/yunikorn/incubator-yunikorn-k8shim/pkg/plugin/predicates/predicator_test.go:2192 +0x86
created by github.com/apache/incubator-yunikorn-k8shim/pkg/plugin/predicates.TestConcurrentAccess
/Users/wyang/workspace/apache/yunikorn/incubator-yunikorn-k8shim/pkg/plugin/predicates/predicator_test.go:2190 +0x53a
Process finished with the exit code 1
```
to fix this, the simplest, most straightforward solution is to make a copy of the map. See `getNodesMapCopy()`. after using this method, the error is gone. This will bring some performance decrease when we do this, simulated nodes copy from 1000 to 10000, the perf decrease is somewhere near:
- 1000: 600+ µs
- 10000: 6+ ms
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #303: [YUNIKORN-848] Scheduler occasionally crashed due to concurrent r/w map error.
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #303:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/303#issuecomment-920425486
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#303](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f58885a) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `3.32%`.
> The diff coverage is `76.45%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #303 +/- ##
==========================================
+ Coverage 59.75% 63.08% +3.32%
==========================================
Files 35 37 +2
Lines 3133 3467 +334
==========================================
+ Hits 1872 2187 +315
- Misses 1180 1192 +12
- Partials 81 88 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2NvbnRleHRfcmVjb3ZlcnkuZ28=) | `45.23% <0.00%> (-1.11%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL25vZGVzLmdv) | `79.80% <ø> (ø)` | |
| [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/common/events/states.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvc3RhdGVzLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/shim/main.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NoaW0vbWFpbi5nbw==) | `0.00% <ø> (ø)` | |
| [pkg/shim/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NoaW0vc2NoZWR1bGVyLmdv) | `78.41% <20.00%> (-1.15%)` | :arrow_down: |
| ... and [27 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [83e23bf...f58885a](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/303?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei merged pull request #303: [YUNIKORN-848] Scheduler occasionally crashed due to concurrent r/w map error.
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #303:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/303
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #303: [YUNIKORN-848] Scheduler occasionally crashed due to concurrent r/w map error.
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #303:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/303#discussion_r710909399
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -65,6 +65,16 @@ func (cache *SchedulerCache) GetNodesInfoMap() map[string]*schedulernode.NodeInf
return cache.nodesMap
}
+func (cache *SchedulerCache) GetNodesInfoMapCopy() map[string]*schedulernode.NodeInfo {
+ cache.lock.RLock()
+ defer cache.lock.RUnlock()
+ copyOfMap := make(map[string]*schedulernode.NodeInfo, len(cache.nodesMap))
+ for k, v := range cache.nodesMap {
+ copyOfMap[k] = v.Clone()
Review comment:
Do we need to clone the NodeInfo here? I think it is enough to just pass it: copyOfMap[k] = v
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org