Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix data race in discovery filter #51048

Merged
merged 1 commit into from
May 17, 2024
Merged

Fix data race in discovery filter #51048

merged 1 commit into from
May 17, 2024

Conversation

howardjohn
Copy link
Member

@howardjohn howardjohn requested a review from a team as a code owner May 14, 2024 23:47
@howardjohn howardjohn added release-notes-none Indicates a PR that does not require release notes. cherrypick/release-1.21 Set this label on a PR to auto-merge it to the release-1.21 branch cherrypick/release-1.22 Set this label on a PR to auto-merge it to the release-1.22 branch labels May 14, 2024
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 14, 2024
@zirain
Copy link
Member

zirain commented May 15, 2024

/retest-required

hzxuzhonghu
hzxuzhonghu requested changes May 15, 2024
View reviewed changes
pkg/kube/namespace/filter.go
// No need to notify handlers for deletes
f.namespaceDeleted(ns.ObjectMeta)
f.lock.Lock()
defer f.lock.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think this is the critical fix.

But what is the target for updating pkg/kube/kclient/client.go

pkg/kube/namespace/filter.go
// namespaceDeleted : if deleted namespace was a member, remove it
func (d *discoveryNamespacesFilter) namespaceDeleted(ns metav1.ObjectMeta) (membershipChanged bool) {
// namespaceDeletedLocked : if deleted namespace was a member, remove it
func (d *discoveryNamespacesFilter) namespaceDeletedLocked(ns metav1.ObjectMeta) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we donot need this function or move the lock here

pkg/kube/kclient/client.go
}
}
// Removes are currently NOT handled. We only have the namespace name here. We would need to have the object
// filter passthrough the entire namespace object, so we can pass the last known state to OnDelete.
// Fortunately, missing a namespace delete event usually doesn't matter since everything in the namespace gets torn down.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean there is no namespace delete triggered? I think this is not right, this package can be used by not only istiod, but also other thirdparty users

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the state even before this PR. Istio packages are not intended for usage outside istio so I don't really care much about the 3p use case?

We can make it send deletes consistently but it's extremely hard to do so. The problem is the filter and the namespace handler ordering is random (from informer) so we cannot guarantee the order. I don't think the complexity is worth it imo.

Either way IMO we could merge this even if we want to do this eventually, as it is strictly fixing the bug -- the missing deletes was already present

hzxuzhonghu
hzxuzhonghu approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but better to improve. I have know many projects import istio libs

@istio-testing istio-testing merged commit 9bf6c0d into istio:master May 17, 2024
28 checks passed
@istio-testing
Copy link
Collaborator

In response to a cherrypick label: #51048 failed to apply on top of branch "release-1.21":

Applying: Fix data race in discovery filter
Using index info to reconstruct a base tree...
M	pkg/kube/kclient/client.go
M	pkg/kube/kclient/client_test.go
M	pkg/kube/namespace/filter.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/kube/namespace/filter.go
CONFLICT (content): Merge conflict in pkg/kube/namespace/filter.go
Auto-merging pkg/kube/kclient/client_test.go
CONFLICT (content): Merge conflict in pkg/kube/kclient/client_test.go
Auto-merging pkg/kube/kclient/client.go
CONFLICT (content): Merge conflict in pkg/kube/kclient/client.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix data race in discovery filter
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new issue created for failed cherrypick: #51115

@istio-testing istio-testing mentioned this pull request May 17, 2024
Closed
@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new pull request created: #51116

@istio-testing istio-testing mentioned this pull request May 17, 2024
Merged
howardjohn added a commit to howardjohn/istio that referenced this pull request May 17, 2024
@howardjohn
Fix data race in discovery filter (istio#51048)
0c8c5bd 
(cherry picked from commit 9bf6c0d)
istio-testing pushed a commit that referenced this pull request May 18, 2024
@howardjohn
Fix data race in discovery filter (#51048) (#51124)
5fe3eb2 
(cherry picked from commit 9bf6c0d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

Assignees
No one assigned
Labels
cherrypick/release-1.21 Set this label on a PR to auto-merge it to the release-1.21 branch cherrypick/release-1.22 Set this label on a PR to auto-merge it to the release-1.22 branch release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@howardjohn @zirain @istio-testing @hzxuzhonghu