fix(bbr): add read lock to GetBaseModel to prevent concurrent map crash#2570
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @gyliu513. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
/kind bug |
nirrozenbaum
left a comment
nirrozenbaum
left a comment
There was a problem hiding this comment.
@gyliu513 nice catch!
Thanks! 🙏🏼
/ok-to-test
/lgtm
/approve
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gyliu513, nirrozenbaum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
- `GetBaseModel()` reads `loraAdapterToBaseModel` and `baseModels` maps without holding a lock, while `ConfigMapUpdateOrAddIfNotExist()` and `ConfigMapDelete()` write to these maps under `ds.lock.Lock()`. In Go, concurrent map read and write causes `fatal error: concurrent map read and map write`, which crashes the process and cannot be recovered. - Add `ds.lock.RLock()`/`RUnlock()` to `GetBaseModel()` to ensure safe concurrent access. - Add a concurrent read/write test to prevent regression. - go test -race ./pkg/bbr/datastore/... test passed.
k8s-ci-robot
removed
the
lgtm
|
|
||
| go func() { | ||
| for i := 0; i < 10000; i++ { | ||
| _ = ds.ConfigMapUpdateOrAddIfNotExist(cm) |
There was a problem hiding this comment.
linter is unhappy when ignoring the error.
maybe add err := ..
and assert.NoError(..)
There was a problem hiding this comment.
Yes, I should run make ci-lint, it should be working now, thanks @nirrozenbaum
There was a problem hiding this comment.
@nirrozenbaum , my previous code is just ds.ConfigMapUpdateOrAddIfNotExist(cm) and this makes CI not happy, updating to _ = ds.ConfigMapUpdateOrAddIfNotExist(cm) is good enough to fix the lint error, hope this is ok.
There was a problem hiding this comment.
sure. as long as the linter is happy, I'm happy 😄
|
/lgtm |
k8s-ci-robot
added
the
lgtm
…sh (kubernetes-sigs#2570) - `GetBaseModel()` reads `loraAdapterToBaseModel` and `baseModels` maps without holding a lock, while `ConfigMapUpdateOrAddIfNotExist()` and `ConfigMapDelete()` write to these maps under `ds.lock.Lock()`. In Go, concurrent map read and write causes `fatal error: concurrent map read and map write`, which crashes the process and cannot be recovered. - Add `ds.lock.RLock()`/`RUnlock()` to `GetBaseModel()` to ensure safe concurrent access. - Add a concurrent read/write test to prevent regression. - go test -race ./pkg/bbr/datastore/... test passed.
…sh (kubernetes-sigs/gateway-api-inference-extension#2570) - `GetBaseModel()` reads `loraAdapterToBaseModel` and `baseModels` maps without holding a lock, while `ConfigMapUpdateOrAddIfNotExist()` and `ConfigMapDelete()` write to these maps under `ds.lock.Lock()`. In Go, concurrent map read and write causes `fatal error: concurrent map read and map write`, which crashes the process and cannot be recovered. - Add `ds.lock.RLock()`/`RUnlock()` to `GetBaseModel()` to ensure safe concurrent access. - Add a concurrent read/write test to prevent regression. - go test -race ./pkg/bbr/datastore/... test passed.
3 participants
GetBaseModel()readsloraAdapterToBaseModelandbaseModelsmaps without holding a lock, whileConfigMapUpdateOrAddIfNotExist()andConfigMapDelete()write to these maps underds.lock.Lock(). In Go, concurrent map read and write causesfatal error: concurrent map read and map write, which crashes the process and cannot be recovered.ds.lock.RLock()/RUnlock()toGetBaseModel()to ensure safe concurrent access.What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: