-
Notifications
You must be signed in to change notification settings - Fork 956
fix: don't create autostart workspace builds with no available provisioners #19067
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
base: main
Are you sure you want to change the base?
Changes from all commits
314bb66
310511e
1256877
26f5001
78a8f5e
a983dc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,11 +28,14 @@ import ( | |
"github.com/coder/coder/v2/coderd/database/provisionerjobs" | ||
"github.com/coder/coder/v2/coderd/database/pubsub" | ||
"github.com/coder/coder/v2/coderd/notifications" | ||
"github.com/coder/coder/v2/coderd/provisionerdserver" | ||
"github.com/coder/coder/v2/coderd/schedule" | ||
"github.com/coder/coder/v2/coderd/wsbuilder" | ||
"github.com/coder/coder/v2/codersdk" | ||
) | ||
|
||
const TestingStaleInterval = time.Second * 5 | ||
|
||
// Executor automatically starts or stops workspaces. | ||
type Executor struct { | ||
ctx context.Context | ||
|
@@ -131,6 +134,36 @@ func (e *Executor) Run() { | |
}() | ||
} | ||
|
||
func (e *Executor) hasAvailableProvisioners(ctx context.Context, tx database.Store, t time.Time, ws database.Workspace, templateVersionJob database.ProvisionerJob) (bool, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||
queryParams := database.GetProvisionerDaemonsByOrganizationParams{ | ||
OrganizationID: ws.OrganizationID, | ||
WantTags: templateVersionJob.Tags, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you should put something up here like |
||
|
||
// nolint: gocritic // The user (in this case, the user/context for autostart builds) may not have the full | ||
// permissions to read provisioner daemons, but we need to check if there's any for the job prior to the | ||
// execution of the job via autostart to fix: https://github.com/coder/coder/issues/17941 | ||
provisionerDaemons, err := tx.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), queryParams) | ||
if err != nil { | ||
return false, xerrors.Errorf("get provisioner daemons: %w", err) | ||
} | ||
|
||
// Check if any provisioners are active (not stale) | ||
for _, pd := range provisionerDaemons { | ||
if pd.LastSeenAt.Valid { | ||
age := t.Sub(pd.LastSeenAt.Time) | ||
if age <= provisionerdserver.StaleInterval { | ||
e.log.Debug(ctx, "hasAvailableProvisioners: found active provisioner", | ||
slog.F("daemon_id", pd.ID), | ||
) | ||
return true, nil | ||
} | ||
} | ||
} | ||
e.log.Debug(ctx, "hasAvailableProvisioners: no active provisioners found") | ||
return false, nil | ||
} | ||
|
||
func (e *Executor) runOnce(t time.Time) Stats { | ||
stats := Stats{ | ||
Transitions: make(map[uuid.UUID]database.WorkspaceTransition), | ||
|
@@ -280,6 +313,22 @@ func (e *Executor) runOnce(t time.Time) Stats { | |
return nil | ||
} | ||
|
||
// Get the template version job to access tags | ||
templateVersionJob, err := tx.GetProvisionerJobByID(e.ctx, activeTemplateVersion.JobID) | ||
if err != nil { | ||
return xerrors.Errorf("get template version job: %w", err) | ||
} | ||
|
||
// Before creating the workspace build, check for available provisioners | ||
hasProvisioners, err := e.hasAvailableProvisioners(e.ctx, tx, t, ws, templateVersionJob) | ||
if err != nil { | ||
return xerrors.Errorf("check provisioner availability: %w", err) | ||
} | ||
if !hasProvisioners { | ||
log.Warn(e.ctx, "skipping autostart - no available provisioners") | ||
return nil // Skip this workspace | ||
} | ||
|
||
if nextTransition != "" { | ||
builder := wsbuilder.New(ws, nextTransition, *e.buildUsageChecker.Load()). | ||
SetLastWorkspaceBuildInTx(&latestBuild). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be only used in tests and not in this file. We should try to avoid test-only constants like this anyways, in favor of letting it be configured as a new argument to
NewExecutor