Bug 1914127: Delete the ingress.openshift.io/operator finalizer #549
Conversation
|
@Miciah: This pull request references Bugzilla bug 1914127, which is invalid:
Comment 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/severity-high
openshift-ci-robot
added
the
approved
|
/bugzilla refresh |
|
@Miciah: This pull request references Bugzilla bug 1914127, 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. 3 validation(s) were run on this bug
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
Add logic to remove the operator's finalizer on LoadBalancer services, which is no longer needed to clean up DNS records since the DNSRecord CRD was added. This commit fixes bug 1914127. https://bugzilla.redhat.com/show_bug.cgi?id=1914127 * pkg/operator/controller/ingress/load_balancer_service.go (ensureLoadBalancerService): Use the new deleteLoadBalancerService method to delete the service as needed. Use the new deleteLoadBalancerServiceFinalizer method to delete any finalizer on any existing service in the delete and update cases. (desiredLoadBalancerService): Delete logic to add a finalizer to the service. (deleteLoadBalancerServiceFinalizer): New method. Delete any finalizer from the service. (finalizeLoadBalancerService): Refactor to use the new deleteLoadBalancerServiceFinalizer method. (deleteLoadBalancerService): New method.
Miciah
force-pushed
the
BZ1914127-delete-the-ingress-dot-openshift-dot-io-slash-operator-finalizer
branch
from
6dab9e7 to
081081a
Compare
sgreene570
left a comment
sgreene570
left a comment
There was a problem hiding this comment.
Overall LGTM, I just have one small question.
|
|
||
| // deleteLoadBalancerService deletes a load balancer service. | ||
| func (r *reconciler) deleteLoadBalancerService(service *corev1.Service, options *crclient.DeleteOptions) error { | ||
| if err := r.client.Delete(context.TODO(), service, options); err != nil { |
There was a problem hiding this comment.
Is there a foreseeable reason to include an *crclient.DeleteOptions parameter? I'm not opposed to adding one here, but I recognize that the other r.client.Delete calls in this repo exclude this paramter.
There was a problem hiding this comment.
Good question, and I had to go back to see why I did this. Yes, we will want to specify PropagationPolicy: "Foreground" when deleting and recreating the service if/when we re-introduce mutable scope.
There was a problem hiding this comment.
aha! That makes sense. Looking forward to taking advantage of that 😉
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@Miciah: All pull requests linked via external trackers have merged: Bugzilla bug 1914127 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. |
|
@Miciah: Bugzilla bug 1914127 is in an unrecognized state (VERIFIED) and will not be 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. |
5 participants
Add logic to remove the operator's finalizer on LoadBalancer services, which is no longer needed to clean up DNS records since the DNSRecord CRD was added.
pkg/operator/controller/ingress/load_balancer_service.go(ensureLoadBalancerService): Use the newdeleteLoadBalancerServicemethod to delete the service as needed. Use the newdeleteLoadBalancerServiceFinalizermethod to delete any finalizer on any existing service in the delete and update cases.(
desiredLoadBalancerService): Delete logic to add a finalizer to the service.(
deleteLoadBalancerServiceFinalizer): New method. Delete any finalizer from the service.(
finalizeLoadBalancerService): Refactor to use the newdeleteLoadBalancerServiceFinalizermethod.(
deleteLoadBalancerService): New method.For reference, this PR incorporates changes from #472, which was reverted in #514.