Further to#15429, this reduces the amount of false-positives returned by the 'is eligible for autostart' part of the query. We achieve this by calculating the 'next start at' time of the workspace, storing it in the database, and using it in ourGetWorkspacesEligibleForTransition query.

The prior implementation of the 'is eligible for autostart' query would returnall workspaces that at some point in the futuremight be eligible for autostart. This now ensures we only return workspaces thatshould be eligible for autostart.

We also now passcurrentTick instead oft to theGetWorkspacesEligibleForTransition query as otherwise we'll have one round of workspaces that are skipped byisEligibleForTransition due tocurrentTick being a truncated version oft.

github-actionsbot assignedDanielleMaywood

Nov 19, 2024

DanielleMaywood force-pushed thedm-experiment-autostart branch from7e5652b toa982168Compare

November 20, 2024 08:54

DanielleMaywood added7 commits

November 20, 2024 15:31

fix: begin impl of reducing autostart false-positive rate

78d9bef

chore: update dbmem.go

dea0c20

fix: dbmem.go impl

d0f1851

test: refactor test slightly

077439c

fix: update golden files

3b41985

fix: dbmem.go convertToWorkspaceRowsNoLock

1a66a14

fix: pass currentTick to GetWorkspacesEligibleForTransition

9725ec7

DanielleMaywood force-pushed thedm-experiment-autostart branch from0172c5b to9725ec7Compare

November 20, 2024 15:31

DanielleMaywood marked this pull request as ready for review

November 20, 2024 15:47

DanielleMaywood requested review frommafredri,dannykopping andjohnstcn

November 20, 2024 15:48

johnstcn reviewed

Nov 20, 2024

View reviewed changes

coderd/database/queries/workspaces.sql OutdatedShow resolvedHide resolved

coderd/database/queries/workspaces.sqlShow resolvedHide resolved

coderd/provisionerdserver/provisionerdserver.go OutdatedShow resolvedHide resolved

coderd/workspaces.go OutdatedShow resolvedHide resolved

revert: query change

5e9b806

DanielleMaywood changed the title~~fix: make GetWorkspacesEligibleForTransition return no false positives~~fix: make GetWorkspacesEligibleForTransition return even less false positives

Nov 21, 2024

fix: ensure times are converted back to UTC

b2a037b

dannykopping removed their request for review

November 21, 2024 10:54

Copy link

Contributor

dannykopping commentedNov 21, 2024

Removed myself as reviewer; Cian & Mathias have much more context here.

johnstcn reviewed

Nov 21, 2024

View reviewed changes

coderd/database/migrations/000277_workspace_next_start_at.up.sql

Copy link

Member

johnstcnNov 21, 2024

Choose a reason for hiding this comment

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

Obligatory reminder to double-check migration number before merge 👍

Merge branch 'main' into dm-experiment-autostart

997c902

mafredri reviewed

Nov 21, 2024

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

I think this approach looks fine on the surface level, but I'm a bit worried that I'm not seeing any invalidation for "next start at". I.e. changes that mean we should disregard or recompute it.

For instance, it could be set in the future, but a change is done to start it sooner. Will it propagate?

Just a few thoughts that pop into my mind:

What happens if a template scheduling options are changed?
What happens if workspace settings are updated (e.g. autostart schedule)?
What happens if a workspace is updated to a new template version
What happens if a user is disabled after the nextStartAt has been set (this might be OK, just mentioning for completeness)

coderd/database/queries/workspaces.sql OutdatedShow resolvedHide resolved

coderd/database/dbmem/dbmem.goShow resolvedHide resolved

aaronlehmann reviewed

Nov 21, 2024

View reviewed changes

coderd/database/migrations/000277_workspace_next_start_at.up.sql OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

DanielleMaywood commentedNov 21, 2024•
edited
Loading

What happens if a template scheduling options are changed?
What happens if a workspace is updated to a new template version

It appears I completely forgot to consider the template being updated, my bad, will resolve this! Thanks for spotting 👍

What happens if workspace settings are updated (e.g. autostart schedule)?

I've handled this (as far as I can tell), the/workspaces/{workspace}/autostart [put] endpoint updates thenext_start_at column.

What happens if a user is disabled after the nextStartAt has been set (this might be OK, just mentioning for completeness)

The query hasusers.status = 'active'::user_status in it. So it will not return that is eligible for autostart.

DanielleMaywood added4 commits

November 22, 2024 10:18

test: next start at is only a valid value

dd05558

fix: appease linter

8bddfdf

chore: add an index for next_start_at

66a01a3

fix: run 'make gen'

b7434c0

DanielleMaywood mentioned this pull request

Nov 25, 2024

Reduce DB load of autobuild#15082

Closed

DanielleMaywood added18 commits

November 25, 2024 16:36

chore: begin impl of tests

100f54c

Merge branch 'main' into dm-experiment-autostart

d201025

test: fix tests

eac63b7

fix: use american english

1b31cbb

fix: make NextAllowedAutostart look up to 7 days into the future

eaf32ee

fix: TestExecutorAutostartBlocked test

1599b2a

fix: make enterprise template schedule store use quartz.clock

142e335

fix: infinite loop

3f17e46

refactor: give template schedule store a quartz.clock

e993643

fix: ensure tests give template schedule store a clock

d349c56

fix: nullify next_start_at on schedule update

a48fb99

fix: tests

cc93075

chore: remove clock from test

45350d1

Merge branch 'main' into dm-experiment-autostart

c1648ae

chore: relocate test

03d0b7f

chore: nits

23b2ec9

Merge branch 'main' into dm-experiment-autostart

18f3c8a

chore: bump migration number

8b1b6d9

DanielleMaywood commented

Nov 28, 2024

View reviewed changes

coderd/autobuild/lifecycle_executor.goShow resolvedHide resolved

DanielleMaywood requested review fromjohnstcn andmafredri

November 28, 2024 10:01

mafredri reviewed

Nov 28, 2024

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

Nice work! Seeing it, I'm not sure how I feel about the db trigger approach, but I like the enforcement and think it's worth giving it a shot. I think we still have some possibilities for data inconsistency without a trigger on the templates table, though.

coderd/database/dbmem/dbmem.go

		// We do not know if workspace with a zero next start is eligible
		// for autostart, so we accept this false-positive. This can occur
		// when a coder version is upgraded and next_start_at has yet to
		// be set.

Copy link

Member

mafredriNov 28, 2024

Choose a reason for hiding this comment

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

We have a new known case, reset by db trigger. Should we implement the trigger in memory db as well? (I know it's probably going away soon, but still, might cause flakes?)

Copy link

ContributorAuthor

DanielleMaywoodNov 28, 2024

Choose a reason for hiding this comment

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

I don'tthink it'd cause flakes, but instead cause dbmem tests to fail when postgres tests do not. Happy to add it though

Copy link

ContributorAuthor

DanielleMaywoodNov 28, 2024

Choose a reason for hiding this comment

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

On second thought, I've checkeddbmem.go for how we emulate triggers and I'm not sure there is a good way to do it.

Copy link

Member

EmyrkDec 2, 2024

Choose a reason for hiding this comment

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

There is no good way to do it, just ad hoc code in various functions 😢

Copy link

ContributorAuthor

DanielleMaywoodDec 2, 2024

Choose a reason for hiding this comment

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

Ah that's unfortunate.

I think attempting to emulate this trigger isn't worth it then. If someone writes a test that is dependent on the trigger (which they ideally shouldn't) then they'll have to disable it when running underdbmem.go.

coderd/database/migrations/000278_workspace_next_start_at.up.sql Outdated

		CREATETRIGGERtrigger_update_workspaces_schedule
		AFTERUPDATEON workspaces
		FOR EACH ROW
		EXECUTE PROCEDURE nullify_workspace_next_start_at();

Copy link

Member

mafredriNov 28, 2024

Choose a reason for hiding this comment

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

I'm thinking we also need a trigger for changes to a template time-til-dormant? Or do I misunderstand that interaction?

My thought process: if the time-til-dormant is reduced, we might've previously calculated a next start for the workspace which might no longer be valid because dormancy was sped up.

It's probably a fringe edge-case, but might be hard to debug customer reports if they run into it.

I'm thinking this applies to template allowed days changing as well. Tomorrow might've been blocked but is now unblocked yet we computed the autostart for the day after.

I'm wondering if there are other such interactions between template and workspace we should cover as well?

Copy link

ContributorAuthor

DanielleMaywoodNov 28, 2024

Choose a reason for hiding this comment

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

I think I agree with you here, I'll try and think if there are any other similar interactions 👍

Copy link

ContributorAuthor

DanielleMaywoodNov 28, 2024

Choose a reason for hiding this comment

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

I'm thinking we also need a trigger for changes to a template time-til-dormant? Or do I misunderstand that interaction?

My thought process: if the time-til-dormant is reduced, we might've previously calculated a next start for the workspace which might no longer be valid because dormancy was sped up.

After digging into this a little bit more, I don't think this is an issue.

I had forgotten to includedormant_at IS NOT NULL inautostart part of theGetWorkspacesEligibleForTransition SQL query, so I've added that now (it already existed in the coderd lifecycle executor side).

Instead of handling changes totime_til_dormancy, I've instead modified the existing trigger to check if the workspace is dormant. When a workspace is marked as dormant, the workspace'snext_start_at is made NULL.

I'm thinking this applies to template allowed days changing as well. Tomorrow might've been blocked but is now unblocked yet we computed the autostart for the day after.

This edge case has been handled in theEnterpriseTemplateScheduleStore