Change logic of deleting RBS service #2126
Conversation
- Delete only if service has finalizer or rbs forwarding rule, meaning it was provisioned by RBS controller at some point - Don't delete RBS annotation on garbage collection
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
approved
| @@ -621,14 +622,14 @@ func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service) | |||
|
|
|||
| // IMPORTANT: Remove LB annotations from the Service LAST, cause changing service fields are emitted as service update to other controllers | |||
| if lc.enableDualStack { | |||
| if err := deleteL4RBSDualStackAnnotations(lc.ctx, svc); err != nil { | |||
There was a problem hiding this comment.
This is the same error we found yesterday, the last thing to be removed should be the finalizer, also why do we need to remove the annotations if it is going to be deleted?
There was a problem hiding this comment.
should not we just delete the finalizer and bail out?
There was a problem hiding this comment.
no...
it may well be just migrated to be handled by ILB
so NetLBFinalizerV2 is not needed anymore
but maybe we need to delete the annotations before the finalizer
There was a problem hiding this comment.
now I see what is the problem
there may be 404 on updating the annotations on deleted service (finalizer removal triggers deletion)
We should move this annotation removal before the finalizer removal (as it happens in L4 ILB)
There was a problem hiding this comment.
Annotation deletion covers the case of garbage collection on switching service types (to clusterIP or ILB), when service not really completely deleted
There was a problem hiding this comment.
But we should not remove RBS annotation (as it was set up by user) so this PR removes deleting RBS annotation
But we should still remove annotations that we use for storing created resources
There was a problem hiding this comment.
switching service types (
ok, but then we should have 2 flows , one for deletion and other for mutation.
The deletion just removes the finalizer
The mutation just removes the annotations added by the controller but keeps the finalizer
IIUIC this still conflates both
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cezarygerard, panslava The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @cezarygerard @aojea