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

Add scheduled delete #3

Merged
merged 38 commits into from
Mar 19, 2024
Merged

Add scheduled delete #3

merged 38 commits into from
Mar 19, 2024

Conversation

JohnGarbutt
Copy link
Collaborator

No description provided.

@JohnGarbutt JohnGarbutt changed the title Add schedule delete Add scheduled delete Mar 12, 2024
@JohnGarbutt JohnGarbutt marked this pull request as ready for review March 12, 2024 19:03
@JohnGarbutt JohnGarbutt requested a review from mkjpryor March 12, 2024 19:09
Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

Not sure how I feel about this approach.

A lot of me favours a polling approach using kopf timers where we check to see if the current time is after the notAfter time and delete if it is.

The implementation would be a lot simpler, but obviously the delete might not happen until the polling interval after notAfter. However, the execution time of async tasks is also not completely deterministic either, since it is relying on cooperative concurrency.

Because we are using asyncio, I don't have any concerns over scaling with the timers approach. In fact I have more concerns over tracking a large number of tasks.

azimuth_schedule_operator/models/v1alpha1/schedule.py Outdated Show resolved Hide resolved
azimuth_schedule_operator/models/v1alpha1/schedule.py Outdated Show resolved Hide resolved
@JohnGarbutt
Copy link
Collaborator Author

Yeah, I got confused with the timers really, couldn't decide on a polling interval, probably simpler than tracking tasks though.

Thoughts? I guess the memo can hold the time, so we just check that in the timer?

@JohnGarbutt JohnGarbutt requested a review from mkjpryor March 14, 2024 14:36
Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

This looks much nicer with the timers, I think.

charts/operator/templates/prometheusrule.yaml Outdated Show resolved Hide resolved
azimuth_schedule_operator/models/v1alpha1/schedule.py Outdated Show resolved Hide resolved
azimuth_schedule_operator/operator.py Outdated Show resolved Hide resolved
@JohnGarbutt JohnGarbutt requested a review from mkjpryor March 18, 2024 11:07
Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

Just one minor change

azimuth_schedule_operator/operator.py Outdated Show resolved Hide resolved
@JohnGarbutt JohnGarbutt requested a review from mkjpryor March 18, 2024 16:54
@JohnGarbutt
Copy link
Collaborator Author

OK, ready for review again. That last comment opened a whole can of (quite useful!) worms.

Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

LGTM

@JohnGarbutt JohnGarbutt merged commit ea79d63 into main Mar 19, 2024
5 checks passed
@JohnGarbutt JohnGarbutt deleted the add-schedule-delete branch March 19, 2024 15:00
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.

2 participants