fix: don't create autostart workspace builds with no available provisioners #19067
Conversation
| } | ||
|
|
||
| // Check if any provisioners are active (not stale) | ||
| now := dbtime.Now() |
There was a problem hiding this comment.
This should be using the passed in time from runOnce rather than getting it's own time
| if testing.Testing() { | ||
| staleInterval = TestingStaleInterval | ||
| } |
There was a problem hiding this comment.
If you use the passed in time from runOnce I don't think changing the interval would be necessary, you could write the test in a way that the default interval works fine
| @@ -52,6 +56,9 @@ type Executor struct { | |||
| experiments codersdk.Experiments | |||
|
|
|||
| metrics executorMetrics | |||
|
|
|||
| // Skip provisioner availability check (should only be true for *some* tests) | |||
There was a problem hiding this comment.
It says it should only be true for some tests, but it seems it's true for almost every test using coderdtest. How many tests are actually broken by this change? If it's only a few, I'd rather we just fix those tests (e.g. by adding provisioners) rather than add a skip flag.
provisioner is not available Signed-off-by: Callum Styan <callumstyan@gmail.com>
transition so that we don't accumulate autostart jobs that will eventually have to be cleaned up by the reaper routine Signed-off-by: Callum Styan <callumstyan@gmail.com>
DB/provisioner to be available, so add a way to skip the check Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
…ndling This commit addresses the requirements to: 1. Comment out provisionerCloser.Close() in TestExecutorAutostartSkipsWhenNoProvisionersAvailable 2. Remove the need for SkipProvisionerCheck by refactoring tests Changes: - Remove SkipProvisionerCheck field and logic from Executor - Remove SkipProvisionerCheck option from coderdtest.Options - Add proper provisioner daemon tags to match template requirements - Add helper functions for waiting for provisioner availability - Update tests to use real provisioner availability checks - Fix timing issues in provisioner staleness tests - Comment out provisioner close call to test proper behavior The test now correctly validates that provisioners remain available when not explicitly closed, and uses the real hasAvailableProvisioners logic instead of bypassing it. Signed-off-by: Callum Styan <callumstyan@gmail.com>
9971794 to
78a8f5e
Compare
autobuild path have a valid provisioner available and pass an appropriate time.Time to the channel for triggering the build based on whether the provisioner is supposed to be valid or stale Signed-off-by: Callum Styan <callumstyan@gmail.com>
|
Okay @deansheather thanks for your patience, I think this is finally ready for another review. Took quite a bit of digging with the help of Blink to find all the bits and pieces that had to be updated to get the tests working without the The TL; DR of the changes is that we in tests which deal with autobuilds we need:
|
This should fix #17941
With just the test addition the test will fail since the workspace build is triggered, we attempt to move the workspace into the
startstate:require.Equal(t, codersdk.WorkspaceTransitionStop, workspace.LatestBuild.Transition)With the 2nd commit we skip the calls to
wsbuilder.Newandbuilder.Buildand so the transition is never scheduled.However, many tests don't fully setup provisioner or wait for the provisioner + DB to be fully available, so the third commit adds a somewhat smelly way to skip the provisioner check in tests that don't need to test that functionality.