Bug 1766141: Ensures LB service finalizer is removed #320
Conversation
|
@danehans: This pull request references Bugzilla bug 1766141, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
bugzilla/valid-bug
|
/assign @Miciah @ironcladlou |
openshift-ci-robot
added
the
approved
| // finalizer does not exist for the load balancer service of ic. For additional background on | ||
| // this finalizer, see: | ||
| // https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190423-service-lb-finalizer.md | ||
| func (r *reconciler) ensureLoadBalancerCleanupFinalizer(ic *operatorv1.IngressController) error { |
There was a problem hiding this comment.
If this logic were moved inline to finalizeLoadBalancerService() you could avoid a new function and call to r.currentLoadBalancerService() (and I think it makes sense anyway because we're literally delaying finalization of the LB service until some other condition is true)
There was a problem hiding this comment.
@ironcladlou I tried that initially, but (a) the "service.kubernetes.io/load-balancer-cleanup" finalizer needs to be checked after the deployment (ownerReferences) is deleted and (b) ensureIngressDeleted() never proceeds to remove other ingresscontroller dependent resources until the cloud infra is cleaned-up. With this approach, the final ingresscontroller reconciliation only needs to remove the "ingresscontroller.operator.openshift.io/finalizer-ingresscontroller" finalizer.
There was a problem hiding this comment.
Hm, is this back to @Miciah's point about failing too soon? To avoid dealing with graphs directly (a path we played with once), the easy thing I've learned to do (from @deads2k) is to make as much progress as possible with parents, aggregate errors, and retry.
Right now we do
finalizeLoadBalancerService() // if fail return early
deleteWildcardDNSRecord() // if fail return early
ensureRouterDeleted() // if fail return early — but finalizeLoadBalancerService will never succeed until this is called!
// finalize ingresscontrollerWould this make things better?
finalizeLoadBalancerService() // append errors
deleteWildcardDNSRecord() // append errors
ensureRouterDeleted() // append errors
if errors > 0 return // and retry
// finalize ingresscontrollerThere was a problem hiding this comment.
@ironcladlou ptal at the latest commit that follows your guidance.
openshift-ci-robot
added
size/S
openshift-ci-robot
added
size/M
| return err | ||
| } | ||
| } | ||
| log.Info("deleted deployment for ingress", "namespace", ci.Namespace, "name", ci.Name) |
There was a problem hiding this comment.
If it's worth logging at v=0, would an event be better? Not sure I have a consistent set of principles to apply for logging nothing vs. logging v=0, v=1, vs. events in some of these cases. cc @Miciah
There was a problem hiding this comment.
Both log.Info and emitting an event for "I deleted your router" seem reasonable to me. The operator's log level is hard-coded to "Debug", which is I guess like v=6 or higher (because glog and zap have to be contrary for whatever reason).
|
Just a non-blocking question about logging, lgtm though. Will let @Miciah tag after his review |
|
Build error:
/test e2e-aws |
|
openshift/origin#24085 should have fixed the test flake. /test e2e-aws |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, knobunc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@danehans: All pull requests linked via external trackers have merged. Bugzilla bug 1766141 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.2 |
|
@danehans: new pull request created: #325 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
7 participants
Previously, a service of type
loadBalancerwould still exist after aningresscontrollerresource is deleted. Allowing the service to exist after aningresscontrolleris deleted can cause it to be reused when the sameingresscontrolleris recreated.