Add Finalizers for Ingress #613
Conversation
|
Hi @agau4779. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/assign @rramkumar1 @bowei |
k8s-ci-robot
added
the
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
Can we add copious glog logging in each of the places where the finalizer is being added/removed? We can put those actions are V(3), errors at Errorf level... |
|
@bowei added errorf logs for when the finalizer fails to be added/removed. Successful adds/removes are glog.V(3) - logged. |
|
General comment: We need to be very careful on the upgrade story here. Theoretically a user could upgrade to a version where we support finalizers, the controller adds a finalizer and then the user downgrades. Once they downgrade, the controller software no longer has the ability to remove finalizers, so if the user tries to delete it, the deletion will never succeed. We may need to stagger the rollout of this. |
|
Hmm -- that is a good point regarding rollout. It seems like the most we can do now is to enable the code to remove the finalizer. We can flag gate the rest of the code so it is tested but not active in production. |
pkg/flags/flags.go
Outdated
| @@ -131,13 +131,16 @@ type Features struct { | |||
| NEGExposed bool | |||
| // ManagedCertificates enables using ManagedCertificate CRD | |||
| ManagedCertificates bool | |||
| // Finalizer enables adding a finalizer on Ingress | |||
There was a problem hiding this comment.
I think we will need two flags. One to enable the removal logic and one to enable to adding of the finalizers.
There was a problem hiding this comment.
Added a flag for Ingress removal.
|
How is this tested? (esp. with multiple Ingress objects to make sure we don't accidentally GC other Ingresses)... |
k8s-ci-robot
added
size/L
|
@bowei added tests for creating and deleting multiple ingresses, with and without finalizers enabled |
|
Also as per the new naming policy, we should rename the finalizer to "networking.gke.io/ingress-finalizer" |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agau4779, rramkumar1 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 |
k8s-ci-robot
added
the
approved
Cherry-pick #613 onto 1.4, part 1
Cherry-pick #613 onto 1.4 branch
Revert "Cherry-pick #613 onto 1.4 branch"
…ck-1 Revert "Cherry-pick #613 onto 1.4, part 1"
The finalizer
ingress.finalizer.cloud.google.comwill be applied to the Ingress upon update if it does not have a finalizer yet. It will be removed only after the resources related to the Ingress have been deleted first. Absence of a finalizer means the Ingress itself can be safely deleted.If a resource used for this ingress fails to be deleted because it’s ‘in use’ by another resource, then the resource will be skipped. If it cannot be deleted for another reason, the controller will attempt deletion at a later attempt.