Skip to content

Commit 0bc4cce

Browse files
cstyancstyan
committed
significantly simplify
TestExecutorAutostartSkipsWhenNoProvisionersAvailable Signed-off-by: Callum Styan <callumstyan@gmail.com>
1 parent a983dc2 commit 0bc4cce

File tree

3 files changed

+26
-139
lines changed

3 files changed

+26
-139
lines changed

‎coderd/autobuild/lifecycle_executor.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@ import (
3434
"github.com/coder/coder/v2/codersdk"
3535
)
3636

37-
const TestingStaleInterval = time.Second * 5
38-
3937
// Executor automatically starts or stops workspaces.
4038
type Executor struct {
4139
ctx context.Context
@@ -148,19 +146,20 @@ func (e *Executor) hasAvailableProvisioners(ctx context.Context, tx database.Sto
148146
return false, xerrors.Errorf("get provisioner daemons: %w", err)
149147
}
150148

149+
logger := e.log.With(slog.F("tags", templateVersionJob.Tags))
151150
// Check if any provisioners are active (not stale)
152151
for _, pd := range provisionerDaemons {
153152
if pd.LastSeenAt.Valid {
154153
age := t.Sub(pd.LastSeenAt.Time)
155154
if age <= provisionerdserver.StaleInterval {
156-
e.log.Debug(ctx, "hasAvailableProvisioners: found active provisioner",
155+
logger.Debug(ctx, "hasAvailableProvisioners: found active provisioner",
157156
slog.F("daemon_id", pd.ID),
158157
)
159158
return true, nil
160159
}
161160
}
162161
}
163-
e.log.Debug(ctx, "hasAvailableProvisioners: no active provisioners found")
162+
logger.Debug(ctx, "hasAvailableProvisioners: no active provisioners found")
164163
return false, nil
165164
}
166165

‎coderd/autobuild/lifecycle_executor_test.go

Lines changed: 18 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/coder/coder/v2/coderd/database/dbgen"
1111
"github.com/coder/coder/v2/coderd/database/pubsub"
12+
"github.com/coder/coder/v2/coderd/provisionerdserver"
1213
"github.com/coder/coder/v2/coderd/rbac"
1314
"github.com/coder/quartz"
1415

@@ -1669,7 +1670,6 @@ func TestMain(m *testing.M) {
16691670
func TestExecutorAutostartSkipsWhenNoProvisionersAvailable(t *testing.T) {
16701671
t.Parallel()
16711672

1672-
db, ps := dbtestutil.NewDB(t)
16731673
var (
16741674
sched = mustSchedule(t, "CRON_TZ=UTC 0 * * * *")
16751675
tickCh = make(chan time.Time)
@@ -1679,12 +1679,10 @@ func TestExecutorAutostartSkipsWhenNoProvisionersAvailable(t *testing.T) {
16791679
// Create client with provisioner closer
16801680
provisionerDaemonTags := map[string]string{"owner": "testowner", "scope": "organization"}
16811681
t.Logf("Setting provisioner daemon tags: %v", provisionerDaemonTags)
1682-
client, provisionerCloser := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{
1682+
client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
16831683
AutobuildTicker: tickCh,
16841684
IncludeProvisionerDaemon: true,
16851685
AutobuildStats: statsCh,
1686-
Database: db,
1687-
Pubsub: ps,
16881686
ProvisionerDaemonTags: provisionerDaemonTags,
16891687
})
16901688

@@ -1695,147 +1693,34 @@ func TestExecutorAutostartSkipsWhenNoProvisionersAvailable(t *testing.T) {
16951693
})
16961694

16971695
// Stop the workspace while provisioner is available
1698-
workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID,
1699-
codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop)
1700-
1701-
// Wait for provisioner to be registered
1702-
coderdtest.MustWaitForProvisioners(t, db)
1696+
workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop)
17031697

17041698
// Wait for provisioner to be available for this specific workspace
1705-
coderdtest.MustWaitForProvisionersAvailable(t, db, workspace, autobuild.TestingStaleInterval)
1706-
1707-
// Now shut down the provisioner daemon
1708-
ctx := testutil.Context(t, testutil.WaitShort)
1709-
err := provisionerCloser.Close()
1710-
require.NoError(t, err)
1711-
1712-
// Wait for the provisioner to become stale (heartbeat stops)
1713-
// The stale interval is 5 seconds, so we need to wait a bit longer
1714-
time.Sleep(6 * time.Second)
1715-
1716-
// Debug: check what's in the database
1717-
daemons, err := db.GetProvisionerDaemons(ctx)
1718-
require.NoError(t, err)
1719-
t.Logf("After close: found %d daemons", len(daemons))
1720-
for i, daemon := range daemons {
1721-
t.Logf("Daemon %d: ID=%s, Name=%s, LastSeen=%v", i, daemon.ID, daemon.Name, daemon.LastSeenAt)
1722-
}
1699+
id := coderdtest.MustWaitForProvisionersAvailable(t, db, workspace, provisionerdserver.StaleInterval)
17231700

1724-
// Since we commented out the close call, the provisioner should NOT become stale
1725-
// Let's wait a bit but NOT longer than the stale interval
1726-
time.Sleep(2 * time.Second) // Wait less than the 5-second stale interval
1727-
1728-
daemons, err = db.GetProvisionerDaemons(ctx)
1729-
require.NoError(t, err)
1730-
require.NotEmpty(t, daemons, "should have provisioner daemons")
1731-
1732-
now := time.Now()
1733-
for _, daemon := range daemons {
1734-
if daemon.LastSeenAt.Valid {
1735-
age := now.Sub(daemon.LastSeenAt.Time)
1736-
t.Logf("Daemon %s: age=%v, staleInterval=%v, isStale=%v", daemon.Name, age, autobuild.TestingStaleInterval, age > autobuild.TestingStaleInterval)
1737-
// Since we closed the provisioner, it should be stale
1738-
require.True(t, age > autobuild.TestingStaleInterval, "provisioner should be stale since we closed it")
1739-
}
1740-
}
1741-
1742-
// Debug: Check the template version job tags and organization
1743-
templateVersion, err := client.TemplateVersion(context.Background(), workspace.LatestBuild.TemplateVersionID)
1744-
require.NoError(t, err)
1745-
t.Logf("Template version job ID: %s", templateVersion.Job.ID)
1746-
t.Logf("Template version job tags: %v", templateVersion.Job.Tags)
1747-
t.Logf("Workspace organization ID: %s", workspace.OrganizationID)
1748-
t.Logf("Template version organization ID: %s", templateVersion.OrganizationID)
1749-
t.Logf("Workspace LatestBuild.TemplateVersionID: %s", workspace.LatestBuild.TemplateVersionID)
1750-
1751-
// Debug: Get the template version job directly from database to compare
1752-
templateVersionJobFromDB, err := db.GetProvisionerJobByID(context.Background(), templateVersion.Job.ID)
1753-
require.NoError(t, err)
1754-
t.Logf("Template version job from DB - ID: %s, Tags: %v", templateVersionJobFromDB.ID, templateVersionJobFromDB.Tags)
1755-
1756-
// Debug: Query database directly to see what provisioner daemons exist
1757-
allDaemons, err := db.GetProvisionerDaemons(context.Background())
1758-
require.NoError(t, err)
1759-
t.Logf("Total provisioner daemons in database: %d", len(allDaemons))
1760-
for i, daemon := range allDaemons {
1761-
t.Logf("Daemon %d: ID=%s, Name=%s, OrgID=%s, Tags=%v", i, daemon.ID, daemon.Name, daemon.OrganizationID, daemon.Tags)
1762-
}
1701+
// Ensure the provisioner is stale
1702+
staleTime := sched.Next(workspace.LatestBuild.CreatedAt).Add((-1 * provisionerdserver.StaleInterval) + -10*time.Second)
1703+
coderdtest.UpdateProvisionerLastSeenAt(t, db, id, time.Now(), staleTime)
17631704

1764-
// Debug: Test if we can query using the ACTUAL provisioner daemon tags
1765-
if len(allDaemons) > 0 {
1766-
actualDaemonTags := allDaemons[0].Tags
1767-
t.Logf("Testing query with ACTUAL daemon tags: %v", actualDaemonTags)
1768-
queryWithActualTags := database.GetProvisionerDaemonsByOrganizationParams{
1769-
OrganizationID: workspace.OrganizationID,
1770-
WantTags: actualDaemonTags,
1771-
}
1772-
matchingWithActualTags, err := db.GetProvisionerDaemonsByOrganization(context.Background(), queryWithActualTags)
1773-
require.NoError(t, err)
1774-
t.Logf("Query with actual daemon tags returns: %d daemons", len(matchingWithActualTags))
1775-
}
1776-
1777-
// Debug: Test the exact query that hasAvailableProvisioners uses
1778-
queryParams := database.GetProvisionerDaemonsByOrganizationParams{
1779-
OrganizationID: workspace.OrganizationID,
1780-
WantTags: templateVersion.Job.Tags,
1781-
}
1782-
t.Logf("Query params: OrgID=%s, WantTags=%v", queryParams.OrganizationID, queryParams.WantTags)
1783-
t.Logf("Test query detailed params: org_id_type=%T, org_id_value=%s, want_tags_type=%T, want_tags_value=%v",
1784-
queryParams.OrganizationID, queryParams.OrganizationID, queryParams.WantTags, queryParams.WantTags)
1785-
1786-
// Test 1: Transaction isolation - try with different transaction contexts
1787-
t.Logf("=== Testing transaction isolation ===")
1788-
1789-
// Query with context.Background() (what we used above)
1790-
matchingDaemons1, err := db.GetProvisionerDaemonsByOrganization(context.Background(), queryParams)
1791-
require.NoError(t, err)
1792-
t.Logf("With context.Background(): %d daemons", len(matchingDaemons1))
1793-
1794-
// Query with a fresh context
1795-
ctx2 := context.Background()
1796-
matchingDaemons2, err2 := db.GetProvisionerDaemonsByOrganization(ctx2, queryParams)
1797-
require.NoError(t, err2)
1798-
t.Logf("With fresh context: %d daemons", len(matchingDaemons2))
1799-
1800-
// Query within a transaction (like hasAvailableProvisioners might use)
1801-
err = db.InTx(func(tx database.Store) error {
1802-
matchingDaemons3, err := tx.GetProvisionerDaemonsByOrganization(ctx2, queryParams)
1803-
if err != nil {
1804-
return err
1805-
}
1806-
t.Logf("Within transaction: %d daemons", len(matchingDaemons3))
1807-
return nil
1808-
}, nil)
1809-
require.NoError(t, err)
1705+
// Trigger autobuild
1706+
tickCh <- sched.Next(workspace.LatestBuild.CreatedAt)
18101707

1811-
// Test 2: Timing issue - query right before and after hasAvailableProvisioners
1812-
t.Logf("=== Testing timing issue ===")
1708+
stats := <-statsCh
18131709

1814-
// Query right before the autostart trigger
1815-
beforeDaemons, err := db.GetProvisionerDaemonsByOrganization(context.Background(), queryParams)
1816-
require.NoError(t, err)
1817-
t.Logf("Right before autostart: %d daemons", len(beforeDaemons))
1710+
// This assertion should FAIL when provisioner is available (not stale), can confirm by commenting out the
1711+
// UpdateProvisionerLastSeenAt call above.
1712+
assert.Len(t, stats.Transitions, 0, "should not create builds when no provisioners available")
18181713

1819-
// Since we commented out the close call, the provisioner is still active
1820-
// This means autobuild should proceed (not be skipped) and create transitions
1821-
// The test expects 0 transitions (skipped autostart), but with an active provisioner,
1822-
// it should get >0 transitions, causing the test to FAIL as intended
1714+
// Ensure the provisioner is NOT stale, and see if we get a successful state transition.
1715+
notStaleTime := sched.Next(workspace.LatestBuild.CreatedAt).Add((-1 * provisionerdserver.StaleInterval) + 10*time.Second)
1716+
coderdtest.UpdateProvisionerLastSeenAt(t, db, id, time.Now(), notStaleTime)
18231717

18241718
// Trigger autobuild
18251719
go func() {
18261720
tickCh <- sched.Next(workspace.LatestBuild.CreatedAt)
18271721
close(tickCh)
18281722
}()
1723+
stats = <-statsCh
18291724

1830-
stats := <-statsCh
1831-
1832-
// Query right after hasAvailableProvisioners was called
1833-
afterDaemons, err := db.GetProvisionerDaemonsByOrganization(context.Background(), queryParams)
1834-
require.NoError(t, err)
1835-
t.Logf("Right after autostart: %d daemons", len(afterDaemons))
1836-
1837-
// This assertion should FAIL when provisioner is available (demonstrating the fix works)
1838-
// When provisioner close is commented out: provisioner available → autostart proceeds → transitions > 0 → test fails ✓
1839-
// When provisioner close is active: provisioner unavailable → autostart skipped → transitions = 0 → test passes ✓
1840-
assert.Len(t, stats.Transitions, 0, "should not create builds when no provisioners available")
1725+
assert.Len(t, stats.Transitions, 1, "should not create builds when no provisioners available")
18411726
}

‎coderd/coderdtest/coderdtest.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1661,10 +1661,10 @@ func MustWaitForProvisionersWithClient(t *testing.T, client *codersdk.Client) {
16611661
}
16621662

16631663
// mustWaitForProvisionersAvailable waits for provisioners to be available for a specific workspace
1664-
func MustWaitForProvisionersAvailable(t *testing.T, db database.Store, workspace codersdk.Workspace, staleDuration time.Duration) {
1664+
func MustWaitForProvisionersAvailable(t *testing.T, db database.Store, workspace codersdk.Workspace, staleDuration time.Duration) uuid.UUID {
16651665
t.Helper()
16661666
ctx := ctxWithProvisionerPermissions(testutil.Context(t, testutil.WaitShort))
1667-
1667+
id := uuid.UUID{}
16681668
// Get the workspace from the database
16691669
require.Eventually(t, func() bool {
16701670
ws, err := db.GetWorkspaceByID(ctx, workspace.ID)
@@ -1699,10 +1699,13 @@ func MustWaitForProvisionersAvailable(t *testing.T, db database.Store, workspace
16991699
if pd.LastSeenAt.Valid {
17001700
age := now.Sub(pd.LastSeenAt.Time)
17011701
if age <= staleDuration {
1702+
id = pd.ID
17021703
return true // Found an active provisioner
17031704
}
17041705
}
17051706
}
17061707
return false // No active provisioners found
17071708
}, testutil.WaitShort, testutil.IntervalFast)
1709+
1710+
return id
17081711
}

0 commit comments

Comments
 (0)