-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds missing lock for cesTracker operation #18055
Conversation
test-1.21-5.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix. Could you split the rename into a dedicated commit, then the bugfixes into dedicated commit(s)? It's a bit tricky to track what the intended logical changes are here when the two sets of changes are all mixed together.
Couple of concerns around the deferred locking below.
Upon further investigation, I think the refactor needs to be more in depth (taking cesTracker out and adding a layer of locked APIs to access the underlying ces instead of accessing it directly). I will do the refactor in another PR, renamed this one to just fix the racing issue for now. |
The travis failure seems suspicious:
|
It's dead-locked...the more reason this needs refactoring... |
/test Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR: Click to show.Test Name
Failure Output
If it is a flake, comment |
test-race-4.9 |
test-race-4.19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The commit message needs operatrion
-> operation
.
At a glance this LGTM, but I'm very unfamiliar with this part of the code. I'll defer to @cilium/operator for more thorough review.
@aanm @cilium/operator friendly ping :) |
/test Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR: Click to show.Test Name
Failure Output
If it is a flake, comment Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR: Click to show.Test Name
Failure Output
If it is a flake, comment |
I think CI hits same flakiness similar to #18184 |
/mlh new-flake Cilium-PR-K8s-1.22-kernel-4.19 👍 created #18218 |
Fixes: cilium#17914 Signed-off-by: Weilong Cui <cuiwl@google.com>
/test |
test-race-4.9 |
test-race-4.19 |
@Weil0ng JFYI something is causing the agents to crash in those above Ci test runs. |
Hmm, this is what I saw in the agent log:
Seems like some other locks are triggering this? |
Hmm, that's the only abnormal msg I saw in the agent log and the only failure case is FQDN, so I think it is what's causing the test suite to fail, unless I missed something? |
The race detection CI jobs are not required and failing with unrelated errors. All reviews are in. Merging. |
Fixes: #17914
Signed-off-by: Weilong Cui cuiwl@google.com