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

adding delayed removal of xobjectbucket #239

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

wejdross
Copy link
Member

@wejdross wejdross commented Oct 3, 2024

Summary

  • this PR adds logic to delay deletion of xobjectbucket so we're sure billing run at least once

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.
  • Link this PR to related issues.

@wejdross wejdross added enhancement New feature or request patch labels Oct 3, 2024
@wejdross wejdross self-assigned this Oct 3, 2024
@wejdross wejdross requested review from a team, Kidswiss, TheBigLee and zugao and removed request for a team October 3, 2024 08:54
@Kidswiss
Copy link
Contributor

Kidswiss commented Oct 3, 2024

Did you test, that Crossplane correctly removes the XObjectBucket after 61 minutes?

age := now.Sub(creationTimestamp.Time)

if age < 61*time.Minute {
return nil, fmt.Errorf("XObjectBucket is too young to be deleted")
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Maybe print how many minutes remaining until it should be deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

Copy link
Collaborator

Choose a reason for hiding this comment

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

the phrasing "too young" really does not help. Either print the minutes as suggested above or just something like "XObjectBucket has been created less than 1 hour".

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added enhanced logging

@wejdross
Copy link
Member Author

wejdross commented Oct 3, 2024

Did you test, that Crossplane correctly removes the XObjectBucket after 61 minutes?

I tested if xobjectbucket was gone after 61 seconds :D

@Kidswiss
Copy link
Contributor

Kidswiss commented Oct 3, 2024

Did you test, that Crossplane correctly removes the XObjectBucket after 61 minutes?

I tested if xobjectbucket was gone after 61 seconds :D

Could you still try it with a larger amount of time? 1 minute is within a single Crossplane reconcile loop.

Also, something interesting to test as well: does it still delete it, if Crossplane gets restarted before the timer runs out?

Copy link
Collaborator

@zugao zugao left a comment

Choose a reason for hiding this comment

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

LGTM

@wejdross wejdross force-pushed the xobjectbucket_delayed_removal branch from b5d6a1f to 3156fd9 Compare October 3, 2024 10:29
@wejdross
Copy link
Member Author

wejdross commented Oct 3, 2024

Could you still try it with a larger amount of time? 1 minute is within a single Crossplane reconcile loop.

Also, something interesting to test as well: does it still delete it, if Crossplane gets restarted before the timer runs out?

  1. it removes it correctly on small time intervals
  2. it removes it correctly on longer intervals like 3 minutes
  3. it removes it correctly during longer time intervals with many concurrent restarts of crossplane and providers (just to make some mess)

@wejdross wejdross requested a review from Kidswiss October 3, 2024 11:09
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

Great!

@wejdross wejdross merged commit 1b45013 into master Oct 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants