Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: implement expiration policy logic for prebuilds#17996

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

ssncferreira
Copy link
Contributor

@ssncferreirassncferreira commentedMay 22, 2025
edited
Loading

Summary

This PR introduces support for expiration policies in prebuilds. The TTL (time-to-live) is retrieved from the Terraform configuration (terraform-provider-coder PR):

prebuilds = {  instances = 2  expiration_policy {  ttl = 86400  }  }

Note: Since there is no need for precise TTL enforcement down to the second, in this implementation expired prebuilds are handled in a single reconciliation cycle: they are deleted, and new instances are created only if needed to match the desired count.

Changes

  • The outcome of a reconciliation cycle is now expressed as a slice of reconciliation actions, instead of a single aggregated action.
  • Adjusted reconciliation logic to delete expired prebuilds and guarantee that the number of desired instances is correct.
  • Updated relevant data structures and methods to support expiration policies parameters.
  • Added documentation toPrebuilt workspaces page
  • Updateterraform-provider-coder to version 2.5.0:https://github.com/coder/terraform-provider-coder/releases/tag/v2.5.0

Depends on:coder/terraform-provider-coder#404
Fixes:#17916

// filterExpiredWorkspaces splits running workspaces into expired and non-expired
// based on the preset's InvalidateAfterSecs TTL. If TTL is missing or zero,
// all workspaces are considered non-expired.
func filterExpiredWorkspaces(preset database.GetTemplatePresetsWithPrebuildsRow, runningWorkspaces []database.GetRunningPrebuiltWorkspacesRow) (nonExpired []database.GetRunningPrebuiltWorkspacesRow, expired []database.GetRunningPrebuiltWorkspacesRow) {
Copy link
ContributorAuthor

@ssncferreirassncferreiraMay 22, 2025
edited
Loading

Choose a reason for hiding this comment

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

There’s an alternative to perform this filtering directly in the database, but for now, opted for an in-memory approach to keep the implementation simpler and the PR smaller. The main trade-off here is performance at scale, while in-memory filtering is sufficient for the current scale, we may want to revisit a DB-based solution if we encounter issues at higher scale. Let me know if you think a database solution would be a better fit now.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand this correctly, the un-filtered data still comes directly from the database but is filtered in-memory.

From what I can tell, the changes required to move this logic inside the database would involve passing in the preset TTL and doing the TTL calculation inside the database. Is that correct?

If so, I agree that it can be kept in-memory for the moment, but we should add a follow-up issue to track this.

Copy link
ContributorAuthor

@ssncferreirassncferreiraMay 26, 2025
edited
Loading

Choose a reason for hiding this comment

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

If I understand this correctly, the un-filtered data still comes directly from the database but is filtered in-memory.

Yes, that's correct, the filtering currently happens in-memory after fetching all the running prebuilds (expired and non-expired) from the database.

From what I can tell, the changes required to move this logic inside the database would involve passing in the preset TTL and doing the TTL calculation inside the database. Is that correct?

Right, moving it into the database would require doing the TTL check within the query. We don’t need to pass the preset TTL manually since it's already stored in the database, so the query can join against thetemplate_version_presets table and compute the expiration inline.
We’d also need to update the currentGetRunningPrebuiltWorkspacesRow function to return only the non-expired running prebuilds.

If so, I agree that it can be kept in-memory for the moment, but we should add a follow-up issue to track this.

I’ll create a follow-up issue to track moving it into the database for better efficiency/scalability 👍

// - Running: prebuilds running and non-expired
// - Expired: prebuilds running and expired due to the preset's TTL
// - InProgress: prebuilds currently in progress
// - Backoff: holds failure info to decide if prebuild creation should be backed off
type PresetSnapshot struct {
Preset database.GetTemplatePresetsWithPrebuildsRow
Running []database.GetRunningPrebuiltWorkspacesRow
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I’m not a fan of theRunning field name here, it’s a bit misleading since now it only includes non-expired running prebuilds. Maybe something likeRunningValid would make that clearer. Open to other naming ideas too if anyone has suggestions.
I can make this change in a follow-up PR to avoid adding more entropy here.

SasSwart reacted with thumbs up emoji
@ssncferreirassncferreiraforce-pushed thessncferreira/feat-prebuilds-cache-invalidation branch from5a27ee4 toef33f48CompareMay 22, 2025 18:54
@ssncferreirassncferreiraforce-pushed thessncferreira/feat-prebuilds-cache-invalidation branch fromef33f48 to5538be5CompareMay 22, 2025 18:59
@ssncferreirassncferreiraforce-pushed thessncferreira/feat-prebuilds-cache-invalidation branch from5538be5 toa38ee7cCompareMay 22, 2025 19:20
@ssncferreirassncferreira changed the titlefeat: implement cache invalidation logic for prebuildsfeat: implement expiration policy logic for prebuildsMay 26, 2025
@ssncferreirassncferreira marked this pull request as ready for reviewMay 26, 2025 11:58
@SasSwart
Copy link
Contributor

LGTM, but given my proximity to the project, I'll defer formal approval to an independent reviewer.

ssncferreira reacted with thumbs up emoji

// filterExpiredWorkspaces splits running workspaces into expired and non-expired
// based on the preset's InvalidateAfterSecs TTL. If TTL is missing or zero,
// all workspaces are considered non-expired.
func filterExpiredWorkspaces(preset database.GetTemplatePresetsWithPrebuildsRow, runningWorkspaces []database.GetRunningPrebuiltWorkspacesRow) (nonExpired []database.GetRunningPrebuiltWorkspacesRow, expired []database.GetRunningPrebuiltWorkspacesRow) {
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this correctly, the un-filtered data still comes directly from the database but is filtered in-memory.

From what I can tell, the changes required to move this logic inside the database would involve passing in the preset TTL and doing the TTL calculation inside the database. Is that correct?

If so, I agree that it can be kept in-memory for the moment, but we should add a follow-up issue to track this.

@evgeniy-scherbina
Copy link
Contributor

DespiteTestExpiredPrebuilds contains good test coverage, I noticed that we don't have new tests inreconcile_test.go which testsReconcileAll API? So we don't test howReconcilePreset handles reconciliation based on multiple actions?

@ssncferreira
Copy link
ContributorAuthor

DespiteTestExpiredPrebuilds contains good test coverage, I noticed that we don't have new tests inreconcile_test.go which testsReconcileAll API? So we don't test howReconcilePreset handles reconciliation based on multiple actions?

You’re right thatTestExpiredPrebuilds covers the lower-level logic, but we currently don’t have tests directly targetingReconcileAll orReconcilePreset to verify the full reconciliation flow with multiple actions.

I agree it would be valuable to add tests at theReconcileAll level to ensure the end-to-end behavior. As discussed internally, I will address those in a follow-up PR.

@ssncferreirassncferreira merged commit6f6e73a intomainMay 26, 2025
38 of 39 checks passed
@ssncferreirassncferreira deleted the ssncferreira/feat-prebuilds-cache-invalidation branchMay 26, 2025 19:31
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 26, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@evgeniy-scherbinaevgeniy-scherbinaevgeniy-scherbina left review comments

@johnstcnjohnstcnjohnstcn approved these changes

@spikecurtisspikecurtisAwaiting requested review from spikecurtisspikecurtis is a code owner

@SasSwartSasSwartAwaiting requested review from SasSwart

Assignees

@ssncferreirassncferreira

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prebuild TTL Configuration Support
4 participants
@ssncferreira@SasSwart@evgeniy-scherbina@johnstcn

[8]ページ先頭

©2009-2025 Movatter.jp