-
Notifications
You must be signed in to change notification settings - Fork 430
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't enqueue VPC update when DeletionTimestamp is zero #3302
Conversation
We change the check to not enqueue a VPC update when the DeletionTimestamp is zero. Previously, the VPC update would basically always be enqueued, as the DeletionTimestamp would be zero until it was scheduled for deletion. This lead to a huge amount of enqueued VPC updates and therefore a lot of Kubernetes API requests and load on the API. The logical not was removed in kubeovn#3107 which therefore introduced this bug. Signed-off-by: Tobias Kantusch <git@kantusch.dev>
@@ -44,7 +44,7 @@ func (c *Controller) enqueueUpdateVpc(oldObj, newObj interface{}) { | |||
oldVpc := oldObj.(*kubeovnv1.Vpc) | |||
newVpc := newObj.(*kubeovnv1.Vpc) | |||
|
|||
if newVpc.DeletionTimestamp.IsZero() || | |||
if !newVpc.DeletionTimestamp.IsZero() || |
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.
How about just removing the !newVpc.DeletionTimestamp.IsZero(), I think the vpc handleUpdate
has nothing to do with the delete logic. all the clear logic should in the handleDelVpc
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.
Looking around, the code contains the same pattern in multiple places, like
kube-ovn/pkg/controller/qos_policy.go
Lines 55 to 59 in 60ef1ad
if !newQos.DeletionTimestamp.IsZero() { | |
klog.V(3).Infof("enqueue update to clean qos %s", key) | |
c.updateQoSPolicyQueue.Add(key) | |
return | |
} |
kube-ovn/pkg/controller/subnet.go
Lines 70 to 73 in 60ef1ad
if !newSubnet.DeletionTimestamp.IsZero() && usingIPs == 0 || (usingIPs == 1 && u2oInterconnIP != "") { | |
c.addOrUpdateSubnetQueue.Add(key) | |
return | |
} |
So there might be a reason for why this check exists? Although I currently tend to agree and don't really see a reason for why you'd want to enqueue an update, when the DeletionTimestamp
has been set, i.e. the VPC is scheduled for deletion at some point later in time.
Also, I didn't find any logic in handleAddOrUpdateVpc
which would rely on the DeletionTimestamp
being set, so the other checks should cover any cases where some important properties have been changed, such that anything in the OVN should be adjusted.
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.
This pattern is aimed to prevent some race condition but I cannot recall the specific scenario. Maybe the controller receive an update event then a deletion event, but it would process the deletion first then the update and lead to undesired garbage so we add a check before doing update. I think we can just keep the code like this as it can resolve this issue.
We change the check to not enqueue a VPC update when the DeletionTimestamp is zero. Previously, the VPC update would basically always be enqueued, as the DeletionTimestamp would be zero until it was scheduled for deletion. This lead to a huge amount of enqueued VPC updates and therefore a lot of Kubernetes API requests and load on the API. The logical not was removed in #3107 which therefore introduced this bug. Signed-off-by: Tobias Kantusch <git@kantusch.dev>
We change the check to not enqueue a VPC update when the DeletionTimestamp is zero. Previously, the VPC update would basically always be enqueued, as the DeletionTimestamp would be zero until it was scheduled for deletion. This lead to a huge amount of enqueued VPC updates and therefore a lot of Kubernetes API requests and load on the API.
The logical not was removed in #3107 which therefore introduced this bug.
What type of this PR
Bug Fix
Which issue(s) this PR fixes:
Fixes #3301
WHAT
馃 Generated by Copilot at 01fc78b
Fix Vpc deletion bug by enqueuing update events when Vpc has deletion timestamp. Modify
pkg/controller/vpc.go
to implement this logic.馃 Generated by Copilot at 01fc78b
HOW
馃 Generated by Copilot at 01fc78b