Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Finalizers for Ingress #613

Merged
merged 2 commits into from
Jan 28, 2019
Merged

Conversation

agau4779
Copy link
Contributor

@agau4779 agau4779 commented Jan 16, 2019

The finalizer ingress.finalizer.cloud.google.com will 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.

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2019
@agau4779
Copy link
Contributor Author

/assign @rramkumar1 @bowei

pkg/utils/finalizer.go Outdated Show resolved Hide resolved
pkg/utils/finalizer.go Outdated Show resolved Hide resolved
pkg/controller/controller_test.go Outdated Show resolved Hide resolved
pkg/controller/controller.go Outdated Show resolved Hide resolved
pkg/controller/controller.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2019
@rramkumar1
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 17, 2019
@bowei
Copy link
Member

bowei commented Jan 17, 2019

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...

@agau4779
Copy link
Contributor Author

@bowei added errorf logs for when the finalizer fails to be added/removed. Successful adds/removes are glog.V(3) - logged.

pkg/controller/types.go Outdated Show resolved Hide resolved
pkg/controller/controller.go Outdated Show resolved Hide resolved
@rramkumar1
Copy link
Contributor

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.

@bowei
Copy link
Member

bowei commented Jan 18, 2019

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.

@@ -131,13 +131,16 @@ type Features struct {
NEGExposed bool
// ManagedCertificates enables using ManagedCertificate CRD
ManagedCertificates bool
// Finalizer enables adding a finalizer on Ingress
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will need two flags. One to enable the removal logic and one to enable to adding of the finalizers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a flag for Ingress removal.

@bowei
Copy link
Member

bowei commented Jan 22, 2019

How is this tested? (esp. with multiple Ingress objects to make sure we don't accidentally GC other Ingresses)...

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 25, 2019
@agau4779
Copy link
Contributor Author

@bowei added tests for creating and deleting multiple ingresses, with and without finalizers enabled

@rramkumar1
Copy link
Contributor

Also as per the new naming policy, we should rename the finalizer to "networking.gke.io/ingress-finalizer"

@rramkumar1
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2019
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2019
@k8s-ci-robot k8s-ci-robot merged commit cecfead into kubernetes:master Jan 28, 2019
@agau4779 agau4779 deleted the finalizers branch January 28, 2019 18:58
rramkumar1 added a commit that referenced this pull request Feb 6, 2019
k8s-ci-robot added a commit that referenced this pull request Feb 6, 2019
rramkumar1 added a commit that referenced this pull request Feb 12, 2019
rramkumar1 added a commit that referenced this pull request Feb 12, 2019
rramkumar1 added a commit that referenced this pull request Feb 12, 2019
rramkumar1 added a commit that referenced this pull request Feb 12, 2019
…ck-1

Revert "Cherry-pick #613 onto 1.4, part 1"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants