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

(feat): automatically delete temporary resources #248

Closed
wants to merge 20 commits into from

Conversation

schristoff
Copy link
Member

This is an attempt to solve #54 - I added OwnerReferences to some smaller resources to make sure they get cleaned up.

Unsure if we should add the TTLSecondsAfterFinished to that resource as it has this note:

/ If a Job is marked as failure, the pod has to be deleted when RestartPolicy is set to OnFailure to prevent the pod keeps restarting.
// To preserve the failed pods, the RestartPolicy needs to be set as Never. The AgentAction job will create a  new pod on retry and leave the failed ones alone.
// For more details, see the github issue: https://github.com/kubernetes/kubernetes/issues/74848#issuecomment-971487582

Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #248 (05f0df6) into main (3a42448) will increase coverage by 0.42%.
Report is 15 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 05f0df6 differs from pull request most recent head 674d136. Consider uploading reports for the commit 674d136 to get more accurate results

@@            Coverage Diff             @@
##             main     #248      +/-   ##
==========================================
+ Coverage   71.75%   72.18%   +0.42%     
==========================================
  Files          13       13              
  Lines        2160     2193      +33     
==========================================
+ Hits         1550     1583      +33     
  Misses        477      477              
  Partials      133      133              
Flag Coverage Δ
unittests 72.18% <100.00%> (+0.42%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
api/v1/agentconfig_types.go 69.19% <100.00%> (+0.29%) ⬆️
controllers/agentaction_controller.go 82.90% <100.00%> (+0.84%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@troy0820 troy0820 left a comment

Choose a reason for hiding this comment

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

So far so good.

Just a word of caution, these types did not have owner references on them before and because the owner reference has Controller: true on them it will requeue the parent object any time this object changes. If that behavior doesn't mess with the actual parent object we are good but I believe it will run them through the loop and cause a cascading deletion problem being that the parent object is requeued.

This is used for garbage collection of the controlled object and for reconciling the owner object on changes to controlled (with a Watch + EnqueueRequestForOwner).

api/v1/agentconfig_types.go Show resolved Hide resolved
api/v1/agentconfig_types.go Outdated Show resolved Hide resolved
controllers/agentaction_controller.go Show resolved Hide resolved
controllers/agentaction_controller.go Outdated Show resolved Hide resolved
controllers/agentaction_controller.go Outdated Show resolved Hide resolved
Co-authored-by: Troy Connor  <troy0820@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
@schristoff schristoff marked this pull request as ready for review August 17, 2023 03:23
Copy link
Member

@troy0820 troy0820 left a comment

Choose a reason for hiding this comment

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

Just a couple of changes, and the testing effort may need to be done in an env test or some integration that can show that the resources are cleaned up and the default value is set.

api/v1/agentconfig_types_test.go Show resolved Hide resolved
api/v1/zz_generated.deepcopy.go Show resolved Hide resolved
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
…what this will break

Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
…one of those

Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
… sure this is the problem

Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
@schristoff
Copy link
Member Author

Closing due to breaking this apart

@schristoff schristoff closed this Sep 4, 2023
@schristoff schristoff deleted the schristoff_ownerRefandTTL branch September 4, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants