Skip to content
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

improvement: skip unnecessary status nodes #6663

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions core/src/graph/nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ export class RequestTaskNode<TaskType extends Task = Task> extends TaskNode<Task
readonly executionType: NodeType = "request"

override get concurrencyLimit() {
return gardenEnv.GARDEN_HARD_CONCURRENCY_LIMIT
return 1
}

override get concurrencyGroupKey() {
Expand Down Expand Up @@ -246,7 +246,7 @@ export class RequestTaskNode<TaskType extends Task = Task> extends TaskNode<Task
}

getDependencies(): TaskNode[] {
if (this.statusOnly) {
if (this.statusOnly && this.task.needsStatus) {
return [this.getNode("status", this.task)]
} else {
return [this.getNode("process", this.task)]
Expand Down Expand Up @@ -288,15 +288,15 @@ export class ProcessTaskNode<T extends Task = Task> extends TaskNode<T> {

getDependencies() {
const statusTask = this.getNode("status", this.task)
const statusResult = this.getDependencyResult(statusTask) as GraphResult<any>
const statusResult = this.getDependencyResult(statusTask)

if (statusResult === undefined) {
if (statusResult === undefined && this.task.needsStatus) {
// Status is still missing
return [statusTask]
}

// Either forcing, or status is not ready
const processDeps = this.task.getProcessDependencies({ status: statusResult.result })
const processDeps = this.task.getProcessDependencies({ status: statusResult?.result ?? null })
return processDeps.map((task) => this.getNode("process", task))
}

Expand All @@ -305,15 +305,15 @@ export class ProcessTaskNode<T extends Task = Task> extends TaskNode<T> {

const statusTask = this.getNode("status", this.task)
// TODO: make this more type-safe
const statusResult = this.getDependencyResult(statusTask) as GraphResultFromTask<T>
const statusResult = this.getDependencyResult(statusTask)

if (statusResult === undefined) {
if (statusResult === undefined && this.task.needsStatus) {
throw new InternalError({
message: `Attempted to execute ${this.describe()} before resolving status.`,
})
}

const status = statusResult.result
const status = statusResult?.result ?? null

if (!this.task.force && status?.state === "ready") {
return status
Expand Down
19 changes: 15 additions & 4 deletions core/src/graph/solver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,11 @@ export class GraphSolver extends TypedEventEmitter<SolverEvents> {
return
}

this.log.debug(`processing ${nodesToProcess.length} nodes`)
if (nodesToProcess.length === 1) {
this.log.debug(`processing ${nodesToProcess[0]!.getKey()}`)
}

this.emit("process", {
keys: nodesToProcess.map((n) => n.getKey()),
inProgress: inProgressNodes.map((n) => n.getKey()),
Expand Down Expand Up @@ -411,8 +416,14 @@ export class GraphSolver extends TypedEventEmitter<SolverEvents> {

// See what is missing to fulfill the request, or resolve
const task = request.task
const statusNode = this.getNode({ type: "status", task, statusOnly: request.statusOnly })
const status = this.getPendingResult(statusNode) as GraphResult<ValidResultType>
const statusNode = task.needsStatus
? this.getNode({ type: "status", task, statusOnly: request.statusOnly })
: undefined
const status = (
statusNode
? this.getPendingResult(statusNode)
: { error: null, aborted: false, startedAt: null, result: null, node: request }
) as GraphResult<ValidResultType>

if (status?.aborted || status?.error) {
// Status is either aborted or failed
Expand All @@ -422,11 +433,11 @@ export class GraphSolver extends TypedEventEmitter<SolverEvents> {
// Status is resolved, and that's all we need
this.log.silly(() => `Request ${request.getKey()} is statusOnly and the status is available. Completing.`)
this.completeTask({ ...status, node: request })
} else if (status === undefined) {
} else if (status === undefined && statusNode) {
// We're not forcing, and we don't have the status yet, so we ensure that's pending
this.log.silly(() => `Request ${request.getKey()} is missing its status.`)
this.ensurePendingNode(statusNode, request)
} else if (status.result?.state === "ready" && !task.force) {
} else if (status?.result?.state === "ready" && !task.force) {
this.log.silly(() => `Request ${request.getKey()} has ready status and force=false, no need to process.`)
this.completeTask({ ...status, node: request })
} else {
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ export abstract class BaseTask<O extends ValidResultType = ValidResultType> exte

abstract getDescription(): string

abstract needsStatus: boolean

abstract getStatus(params: TaskProcessParams): null | Promise<O | null>

abstract process(params: TaskProcessParams): Promise<O>
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export class BuildTask extends ExecuteActionTask<BuildAction, BuildStatus> {
return this.action.longDescription()
}

override readonly needsStatus = true

@OtelTraced({
name(_params) {
return `${this.action.key()}.getBuildStatus`
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/delete-deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ export class DeleteDeployTask extends BaseActionTask<DeployAction, DeployStatus>
return `delete ${this.action.longDescription()})`
}

override readonly needsStatus = false

async getStatus() {
return null
}
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export class DeployTask extends ExecuteActionTask<DeployAction, DeployStatus> {
return this.action.longDescription()
}

override readonly needsStatus = true

@OtelTraced({
name(_params) {
return `${this.action.key()}.getDeployStatus`
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ export class PublishTask extends BaseActionTask<BuildAction, PublishActionResult
return `publish ${this.action.longDescription()}`
}

override readonly needsStatus = false

async getStatus() {
// TODO-0.13.1
return null
Expand Down
7 changes: 4 additions & 3 deletions core/src/tasks/resolve-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,8 @@ export interface ResolveActionResults<T extends Action> extends ValidResultType
export class ResolveActionTask<T extends Action> extends BaseActionTask<T, ResolveActionResults<T>> {
readonly type = "resolve-action"

// TODO: resolving template strings is CPU bound, does single-threaded concurrent execution make it faster or slower?
override readonly executeConcurrencyLimit = 10
override readonly statusConcurrencyLimit = 10
override readonly executeConcurrencyLimit = 1
override readonly statusConcurrencyLimit = 1

getDescription() {
return `resolve ${this.action.longDescription()}`
Expand All @@ -52,6 +51,8 @@ export class ResolveActionTask<T extends Action> extends BaseActionTask<T, Resol
return this.action.key()
}

override readonly needsStatus = false

getStatus({}: ActionTaskStatusParams<T>) {
return null
}
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/resolve-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ export class ResolveProviderTask extends BaseTask<Provider> {
)
}

override readonly needsStatus = false

async getStatus() {
return null
}
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ export class RunTask extends ExecuteActionTask<RunAction, GetRunResult> {
return this.action.longDescription()
}

override readonly needsStatus = true

@OtelTraced({
name(_params) {
return `${this.action.key()}.getRunStatus`
Expand Down
2 changes: 2 additions & 0 deletions core/src/tasks/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export class TestTask extends ExecuteActionTask<TestAction, GetTestResult> {
return this.action.longDescription()
}

override readonly needsStatus = true

@OtelTraced({
name: "getTestStatus",
getAttributes(_params) {
Expand Down
2 changes: 2 additions & 0 deletions core/test/unit/src/graph/solver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ export class TestTask extends BaseTask<TestTaskResult> {
return "v-" + this.uid.slice(0, 6)
}

override readonly needsStatus = true

async getStatus(params: TaskProcessParams) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let callbackResult: any = undefined
Expand Down
2 changes: 2 additions & 0 deletions core/test/unit/src/tasks/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ describe("BaseActionTask", () => {
return "foo"
}

override readonly needsStatus = true

async getStatus() {
return { state: "ready", outputs: {} } as ValidResultType
}
Expand Down
2 changes: 2 additions & 0 deletions plugins/pulumi/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,8 @@ class PulumiPluginCommandTask extends PluginActionTask<PulumiDeploy, PulumiComma
return tasks
}

override readonly needsStatus = false

async getStatus() {
return null
}
Expand Down