Skip to content

WIP: Only remove the cleanup finalizer if the cleanup succeeds #5126

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

WIP: Only remove the cleanup finalizer if the cleanup succeeds #5126

wants to merge 2 commits into from

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented May 12, 2022 •
edited
Loading

This addresses two problems:

  • The finalizer was being removed unconditionally, regardless of whether solver.Cleanup succeeds
  • The finalizer was assumed to be the first in the list of finalizers, potentially resulting in the removal of foreign finalizers and resulting in the cleanup finalizer never being removed.
  • solver.Cleanup was invoked inconsistently from two different places, in handleFinalizer and in an acme.IsFinalState block, but in the former, the Status.Processing and Status.Presented fields were not being reset and in the latter case, the finalizer wasn't being removed if the cleanup succeeded.

Part of:

NONE

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/acme Indicates a PR directly modifies the ACME Issuer code area/testing Issues relating to testing approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 12, 2022
@wallrj wallrj mentioned this pull request May 12, 2022
Merged
@wallrj
Copy link
Member Author

wallrj commented May 13, 2022

/retest

@irbekrm
Copy link
Contributor

irbekrm commented May 13, 2022

/test all

@wallrj
Copy link
Member Author

wallrj commented May 13, 2022

Thanks @irbekrm

I assume that I've somehow managed to break tons of E2E tests with this additional change, but I haven't quite figured out what I've changed.
I'm working on it.

@wallrj wallrj force-pushed the 3640-finalizer-cleanup-2 branch from 01e382f to 9b8cf15 Compare May 14, 2022 16:38
@wallrj wallrj mentioned this pull request May 14, 2022
Open
@wallrj
Copy link
Member Author

wallrj commented May 14, 2022

/test pull-cert-manager-e2e-v1-23

@wallrj
Copy link
Member Author

wallrj commented May 14, 2022

/test pull-cert-manager-make-e2e-v1-23

@wallrj wallrj force-pushed the 3640-finalizer-cleanup-2 branch from 9b8cf15 to 46d912a Compare May 16, 2022 12:19
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 16, 2022
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2022
@jetstack-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 19, 2023
@jetstack-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle rotten
/remove-lifecycle stale

@jetstack-bot jetstack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 18, 2023
@jetstack-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

@jetstack-bot
Copy link
Contributor

@jetstack-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

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.

@inteon inteon reopened this Aug 28, 2023
@jetstack-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

@jetstack-bot
Copy link
Contributor

@jetstack-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

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.

@wallrj wallrj reopened this Feb 3, 2024
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from wallrj. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@jetstack-bot jetstack-bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2024
@wallrj wallrj force-pushed the 3640-finalizer-cleanup-2 branch from 6a57296 to 9a93b4c Compare February 3, 2024 15:44
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2024
@wallrj wallrj force-pushed the 3640-finalizer-cleanup-2 branch from 9a93b4c to 5c8615d Compare February 3, 2024 16:01
@jetstack-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
/close

@jetstack-bot
Copy link
Contributor

@jetstack-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
/close

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.

@inteon
Copy link
Member

inteon commented Mar 4, 2024

/remove-lifecycle rotten
/reopen

@jetstack-bot jetstack-bot reopened this Mar 4, 2024
@jetstack-bot
Copy link
Contributor

@inteon: Reopened this PR.

In response to this:

/remove-lifecycle rotten
/reopen

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.

@jetstack-bot jetstack-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 4, 2024
@jetstack-bot
Copy link
Contributor

@wallrj: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cert-manager-make-e2e-v1-23 46d912a link true /test pull-cert-manager-make-e2e-v1-23
pull-cert-manager-e2e-v1-23 46d912a link true /test pull-cert-manager-e2e-v1-23
pull-cert-manager-make-test 6a57296 link true /test pull-cert-manager-make-test
pull-cert-manager-e2e-v1-24 6a57296 link true /test pull-cert-manager-e2e-v1-24
pull-cert-manager-master-e2e-v1-24 6a57296 link true /test pull-cert-manager-master-e2e-v1-24
pull-cert-manager-master-e2e-v1-25 6a57296 link true /test pull-cert-manager-master-e2e-v1-25
pull-cert-manager-master-e2e-v1-24-upgrade 6a57296 link true /test pull-cert-manager-master-e2e-v1-24-upgrade
pull-cert-manager-master-e2e-v1-25-upgrade 6a57296 link true /test pull-cert-manager-master-e2e-v1-25-upgrade
pull-cert-manager-master-e2e-v1-26-upgrade 6a57296 link true /test pull-cert-manager-master-e2e-v1-26-upgrade
pull-cert-manager-master-e2e-v1-27-upgrade 6a57296 link true /test pull-cert-manager-master-e2e-v1-27-upgrade
pull-cert-manager-master-e2e-v1-27 6a57296 link true /test pull-cert-manager-master-e2e-v1-27
pull-cert-manager-master-e2e-v1-28 5c8615d link true /test pull-cert-manager-master-e2e-v1-28
pull-cert-manager-master-make-test 5c8615d link true /test pull-cert-manager-master-make-test
pull-cert-manager-master-e2e-v1-28-upgrade 5c8615d link true /test pull-cert-manager-master-e2e-v1-28-upgrade

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@inteon inteon self-assigned this Jun 17, 2024
wallrj added 2 commits June 17, 2024 09:16
@wallrj @inteon
Only remove the cleanup finalizer if the cleanup succeeds
aff921f 
And avoid changing any existing finalizers

Signed-off-by: Richard Wall <richard.wall@jetstack.io>
@wallrj @inteon
WIP
9bf1de7 
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
@inteon inteon force-pushed the 3640-finalizer-cleanup-2 branch from 5c8615d to 9bf1de7 Compare June 17, 2024 07:17
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from inteon. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@inteon inteon added this to the 1.16 milestone Jun 17, 2024
@cert-manager-prow
Copy link
Contributor

@wallrj: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cert-manager-master-make-verify 9bf1de7 link true /test pull-cert-manager-master-make-verify
pull-cert-manager-master-e2e-v1-30 9bf1de7 link true /test pull-cert-manager-master-e2e-v1-30
pull-cert-manager-master-e2e-v1-30-upgrade 9bf1de7 link true /test pull-cert-manager-master-e2e-v1-30-upgrade
pull-cert-manager-master-make-test 9bf1de7 link true /test pull-cert-manager-master-make-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

@cert-manager-prow cert-manager-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2024
@cert-manager-prow
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

@inteon inteon mentioned this pull request Sep 18, 2024
Open
@SgtCoDFish
Copy link
Member

Closing in favour of @inteon 's #7286 !

@SgtCoDFish SgtCoDFish closed this Sep 19, 2024
@SgtCoDFish SgtCoDFish mentioned this pull request Sep 19, 2024
Merged
@wallrj wallrj deleted the 3640-finalizer-cleanup-2 branch September 20, 2024 10:58
@wallrj wallrj mentioned this pull request May 9, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@inteon inteon

Labels
area/acme Indicates a PR directly modifies the ACME Issuer code area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.16
Development

Successfully merging this pull request may close these issues.
5 participants
@wallrj @irbekrm @jetstack-bot @inteon @SgtCoDFish