Skip to content

fix: retain inventory for pruning fail/skip#622

Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
sdowell:fix-partial-destroy
Jul 24, 2023
Merged

fix: retain inventory for pruning fail/skip#622
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
sdowell:fix-partial-destroy

Conversation

@sdowell
Copy link
Copy Markdown
Contributor

@sdowell sdowell commented Jul 12, 2023 •
edited by karlkfi
Loading

Prior to this change, the inventory always was deleted at the end of a Destroy event. This would occur even in the case of a pruning skip/failure, resulting in the objects being removed from the inventory without being deleted. This change makes it so that the inventory is only deleted if all objects have been deleted and reconciled.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 12, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 12, 2023

Welcome @sdowell!

It looks like this is your first PR to kubernetes-sigs/cli-utils 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cli-utils has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 12, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 12, 2023

Hi @sdowell. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 12, 2023
@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 12, 2023

/assign @karlkfi

karlkfi
karlkfi reviewed Jul 12, 2023
View reviewed changes
karlkfi
karlkfi reviewed Jul 12, 2023
View reviewed changes
@sdowell sdowell force-pushed the fix-partial-destroy branch from 858bd76 to 1cb7531 Compare July 12, 2023 23:54
karlkfi
karlkfi reviewed Jul 14, 2023
View reviewed changes
karlkfi
karlkfi requested changes Jul 14, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@karlkfi karlkfi left a comment

Choose a reason for hiding this comment

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

good start!

needs some e2e tests validating that destroy can fail, be retried, and succeed.

@sdowell sdowell force-pushed the fix-partial-destroy branch from 1cb7531 to 4947cec Compare July 14, 2023 23:54
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 14, 2023
@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 14, 2023

@karlkfi Added an e2e test and updated the logic to not consider delete/reconcile skips. I also added timeoutReconciles as a condition to consider a failure

karlkfi
karlkfi reviewed Jul 17, 2023
View reviewed changes
karlkfi
karlkfi reviewed Jul 17, 2023
View reviewed changes
karlkfi
karlkfi reviewed Jul 17, 2023
View reviewed changes
@sdowell sdowell force-pushed the fix-partial-destroy branch from 4947cec to ff34124 Compare July 17, 2023 18:53
karlkfi
karlkfi reviewed Jul 17, 2023
View reviewed changes
@karlkfi karlkfi 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 Jul 17, 2023
@karlkfi
Copy link
Copy Markdown
Contributor

karlkfi commented Jul 17, 2023

/ok-to-test

karlkfi
karlkfi reviewed Jul 17, 2023
View reviewed changes
@sdowell sdowell force-pushed the fix-partial-destroy branch from ff34124 to aa6ad0c Compare July 17, 2023 20:08
karlkfi
karlkfi reviewed Jul 17, 2023
View reviewed changes
@sdowell sdowell force-pushed the fix-partial-destroy branch from aa6ad0c to d0cd736 Compare July 17, 2023 20:12
@sdowell sdowell force-pushed the fix-partial-destroy branch from d0cd736 to f6c9f33 Compare July 17, 2023 20:13
karlkfi
karlkfi approved these changes Jul 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@karlkfi karlkfi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karlkfi, sdowell

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2023
@sdowell sdowell mentioned this pull request Jul 17, 2023
Merged
@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 17, 2023

/retest

@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 17, 2023

Job execution failed: Pod got deleted unexpectedly
/retest

@karlkfi
Copy link
Copy Markdown
Contributor

karlkfi commented Jul 17, 2023

/retest

2 similar comments
@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 18, 2023

/retest

@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 18, 2023

/retest

@sdowell
fix: retain inventory for pruning failures
1c4eae9 
Prior to this change, the inventory always was deleted at the end of a
Destroy event. This would occur even in the case of a pruning
failure, resulting in the objects being removed from the inventory
without being deleted. This change makes it so that the inventory is
only deleted if all objects have been pruned.
@sdowell sdowell force-pushed the fix-partial-destroy branch from f6c9f33 to 1c4eae9 Compare July 18, 2023 22:49
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2023
@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 19, 2023

/retest

3 similar comments
@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 19, 2023

/retest

@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 19, 2023

/retest

@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 19, 2023

/retest

@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 19, 2023

/test cli-utils-presubmit-master-stress

@karlkfi
Copy link
Copy Markdown
Contributor

karlkfi commented Jul 20, 2023

/retest

@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 23, 2023

/test cli-utils-presubmit-master-stress

2 similar comments
@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 24, 2023

/test cli-utils-presubmit-master-stress

@sdowell
Copy link
Copy Markdown
Contributor Author

sdowell commented Jul 24, 2023

/test cli-utils-presubmit-master-stress

@karlkfi
Copy link
Copy Markdown
Contributor

karlkfi commented Jul 24, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2023
@k8s-ci-robot k8s-ci-robot merged commit 833d70f into kubernetes-sigs:master Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karlkfi karlkfi karlkfi approved these changes

@haiyanmeng haiyanmeng Awaiting requested review from haiyanmeng

@seans3 seans3 Awaiting requested review from seans3

Assignees

@karlkfi karlkfi

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sdowell @k8s-ci-robot @karlkfi