Skip to content

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

Closed
wallrj wants to merge 2 commits into
cert-manager:masterfrom
wallrj:3640-finalizer-cleanup-2
Closed

WIP: Only remove the cleanup finalizer if the cleanup succeeds#5126
wallrj wants to merge 2 commits into
cert-manager:masterfrom
wallrj:3640-finalizer-cleanup-2

Conversation

@wallrj

@wallrj wallrj commented May 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

wallrj commented May 13, 2022

Copy link
Copy Markdown
Member Author

/retest

@irbekrm

irbekrm commented May 13, 2022

Copy link
Copy Markdown
Contributor

/test all

@wallrj

wallrj commented May 13, 2022

Copy link
Copy Markdown
Member Author

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
Closed
@wallrj

wallrj commented May 14, 2022

Copy link
Copy Markdown
Member Author

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

@wallrj

wallrj commented May 14, 2022

Copy link
Copy Markdown
Member Author

/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

jetstack-bot commented Mar 19, 2023

Copy link
Copy Markdown
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

jetstack-bot commented Apr 18, 2023

Copy link
Copy Markdown
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

jetstack-bot commented May 24, 2023

Copy link
Copy Markdown
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

jetstack-bot commented May 24, 2023

Copy link
Copy Markdown
Contributor

@jetstack-bot: Closed this PR.

Details

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

jetstack-bot commented Sep 27, 2023

Copy link
Copy Markdown
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

jetstack-bot commented Sep 27, 2023

Copy link
Copy Markdown
Contributor

@jetstack-bot: Closed this PR.

Details

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

jetstack-bot commented Feb 3, 2024

Copy link
Copy Markdown
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.

Details 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

jetstack-bot commented Mar 4, 2024

Copy link
Copy Markdown
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

jetstack-bot commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@jetstack-bot: Closed this PR.

Details

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

inteon commented Mar 4, 2024

Copy link
Copy Markdown
Member

/remove-lifecycle rotten
/reopen

@jetstack-bot jetstack-bot reopened this Mar 4, 2024
@jetstack-bot

jetstack-bot commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@inteon: Reopened this PR.

Details

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

jetstack-bot commented Mar 4, 2024

Copy link
Copy Markdown
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.

Details

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

cert-manager-prow Bot commented Jun 17, 2024

Copy link
Copy Markdown
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.

Details 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

cert-manager-prow Bot commented Jun 17, 2024

Copy link
Copy Markdown
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.

Details

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

cert-manager-prow Bot commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

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
Merged
@SgtCoDFish

SgtCoDFish commented Sep 19, 2024

Copy link
Copy Markdown
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