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

lrp: Skip clusterIP service restore in service delete callback #16548

Merged
merged 2 commits into from
Jun 22, 2021
Merged

lrp: Skip clusterIP service restore in service delete callback #16548

merged 2 commits into from
Jun 22, 2021

Conversation

aditighag
Copy link
Member

See commit message.

@aditighag
lrp: Skip restoring service on delete operation
69506f4 
Previously, we were restoring the original clusterIP
service even when the service was deleted.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag
lrp: Refactor logic executed on policy delete
0ef28f9 
The `deletePolicyService` function was previously
common to both delete policy and delete service callbacks.
Refactor the logic to pass the policy config directly, thereby
skip config look up.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag requested a review from a team June 16, 2021 04:49
@aditighag aditighag added area/lrp Impacts Local Redirect Policy. kind/bug This is a bug in the Cilium logic. labels Jun 16, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 16, 2021
@aditighag aditighag requested a review from joamaki June 16, 2021 04:49
@aditighag aditighag added the release-note/misc This PR makes changes that have no direct user impact. label Jun 16, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 16, 2021
@aditighag
Copy link
Member Author

test-only --focus="K8sServicesTest.* LRP" --kernel_version="net-next"

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.9 Jun 16, 2021
@aditighag
Copy link
Member Author

Focused LRP tests, including the restore test, have passed.

@aditighag aditighag requested a review from Weil0ng June 16, 2021 18:26
@Weil0ng
Copy link
Contributor

Weil0ng commented Jun 21, 2021

I think this change makes the logic much more clearer, but I'm curious is the old behavior causing a bug though? I mean we restore from the svc cache, so if the svc is deleted, there should not be any entry for us to restore to even if we try?

@aditighag
Copy link
Member Author

I think this change makes the logic much more clearer, but I'm curious is the old behavior causing a bug though? I mean we restore from the svc cache, so if the svc is deleted, there should not be any entry for us to restore to even if we try?

I found this unnecessary logic being executed while reading through the code. This could potentially lead to problems if there is a race between service cache deletion and LRP manager service look up. I think the current service event callbacks handlers are synchronous, but that's likely to change in future. Regardless, it's logically incorrect?

@aditighag
Copy link
Member Author

Focused LRP tests, including the restore test, have passed.

Review is in. Marking as ready for merge.

@aditighag aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 21, 2021
@christarazi christarazi merged commit 92d851d into cilium:master Jun 22, 2021
@aditighag aditighag mentioned this pull request Jun 22, 2021
Merged
@aditighag aditighag mentioned this pull request Jun 22, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.9 Jun 22, 2021
@Weil0ng
Copy link
Contributor

Weil0ng commented Jun 22, 2021

I think this change makes the logic much more clearer, but I'm curious is the old behavior causing a bug though? I mean
I found this unnecessary logic being executed while reading through the code. This could potentially lead to problems if there is a race between service cache deletion and LRP manager service look up. I think the current service event callbacks handlers are synchronous, but that's likely to change in future. Regardless, it's logically incorrect?

Yea, totally, I'm with you there, was just curious what problem this could cause :)

@aanm aanm mentioned this pull request Jul 2, 2021
Merged
@pchaigno pchaigno added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Jul 2, 2021
@pchaigno
Copy link
Member

pchaigno commented Jul 2, 2021 •
edited
Loading

@aditighag I switched this PR from release-note/misc to release-note/bug.

@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.9 Jul 2, 2021
@pchaigno pchaigno mentioned this pull request Jul 2, 2021
Closed
@joestringer joestringer mentioned this pull request Jul 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Weil0ng Weil0ng Weil0ng approved these changes

@joamaki joamaki Awaiting requested review from joamaki

Assignees

@joamaki joamaki

Labels
area/lrp Impacts Local Redirect Policy. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.9.9
Backport done to v1.9
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aditighag @Weil0ng @pchaigno @errordeveloper @joamaki @christarazi