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

pkg/k8s: fix race condition #13471

Merged
merged 2 commits into from
Oct 9, 2020
Merged

pkg/k8s: fix race condition #13471

merged 2 commits into from
Oct 9, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Oct 8, 2020

Fix #13434

@aanm aanm added release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.8 labels Oct 8, 2020
@aanm aanm requested a review from a team October 8, 2020 18:07
@maintainer-s-little-helper
Copy link

Commit d69407ce5cfbc5e3e1e7254f391dab77140957db does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 8, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.5 Oct 8, 2020
@aanm
Revert "pkg/k8s: fix data race in CNP rule validation"
18f965b 
This reverts commit c1814ba.

Signed-off-by: André Martins <andre@cilium.io>
@aanm
k8s/watcher: check if spec and specs are equal
82a1b6f 
If both Spec and Specs are considered the same we don't need to re-add
the CNP into the policy repository. The commit 134fdb5 should have
added the Specs.DeepEqual and not remove the Spec.DeepEqual.

Fixes: 134fdb5 ("k8s/watchers: fix missing missing CNP/CCNP updates")
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/fix-race-condition-13434 branch from 8d17b61 to 82a1b6f Compare October 8, 2020 18:08
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 8, 2020
@aanm
Copy link
Member Author

aanm commented Oct 8, 2020

test-me-please

@aanm
Copy link
Member Author

aanm commented Oct 8, 2020

retest-gke

@aanm
Copy link
Member Author

aanm commented Oct 8, 2020

retest-4.9

christarazi
christarazi approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM. How does this fix a race condition? From what I can tell, it reverts a potential fix, but I can't tell how there was a race condition from the fix in the second commit.

aanm
aanm commented Oct 9, 2020
View reviewed changes
pkg/k8s/watchers/cilium_network_policy.go
}
k.updateCiliumNetworkPolicyV2AnnotationsOnly(ciliumNPClient, ciliumV2Store, newRuleCpy)
return nil
Copy link
Member Author

Choose a reason for hiding this comment

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

@christarazi This return, that was previously done, prevented the newRuleCpy from being concurrently accessed by the controller inside updateCiliumNetworkPolicyV2AnnotationsOnly and the addCiliumNetworkPolicyV2 below. The addCiliumNetworkPolicyV2 will then execute a Parse which will overwrite the fields concurrently.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 9, 2020
@aanm aanm merged commit 30160bc into cilium:master Oct 9, 2020
@qmonnet qmonnet mentioned this pull request Oct 9, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.5 Oct 9, 2020
@joestringer joestringer mentioned this pull request Oct 12, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.5 Oct 13, 2020
@aanm aanm mentioned this pull request Oct 19, 2020
Closed
@christarazi christarazi mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.5
Backport done to v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RACE Detector]: pkg/policy/api.(*PortProtocol).Sanitize()
4 participants
@aanm @christarazi @tklauser @qmonnet