feat: evaluate only the parameter/preset/tag closure when rendering - #221
feat: evaluate only the parameter/preset/tag closure when rendering#221PushTheLimit wants to merge 6 commits into
Conversation
Preview evaluates the entire Terraform module graph on every call, even though rendering a workspace form only needs coder_parameter, coder_workspace_preset and coder_workspace_tags (and what they reference). The resources a workspace would create cannot feed those blocks, so evaluating them is wasted work that dominates request latency on large templates. Pass OptionWithResourceClosure with the three target block types so the parser drops root-module resources that nothing in that closure references. On a real template this cuts EvaluateAll from ~2s to ~0.16s (~12x) with byte-identical parameters, presets and tags. Depends on the OptionWithResourceClosure addition in the trivy fork (coder/trivy#74). The trivy replace is temporarily pinned to that PR's commit; it will be moved to the merged coder/trivy commit before this merges.
f3a2bc9 to
adb5edc
Compare
Emyrk
left a comment
Emyrk
left a comment
There was a problem hiding this comment.
Have to block on the go mod update.
I'm curious how much of a speedup this is
| // Trivy has some issues that we're floating patches for, and will hopefully | ||
| // be upstreamed eventually. | ||
| replace github.com/aquasecurity/trivy => github.com/coder/trivy v0.0.0-20260309164037-c413f5a2f511 | ||
| replace github.com/aquasecurity/trivy => github.com/PushTheLimit/trivy v0.0.0-20260825213047-3916002b18a9 |
There was a problem hiding this comment.
We need to continue to use our fork. Is this just a matter of pulling their latest?
There was a problem hiding this comment.
@Emyrk I state this in the PR description:
Requires OptionWithResourceClosure from the trivy fork: coder/trivy#74. The trivy replace in go.mod is temporarily pinned to that PR's commit so this builds; it will be moved to the merged coder/trivy commit before this is taken out of draft.
There was a problem hiding this comment.
I do the same thing in the draft of the coder/coder PR: coder/coder#28602 that will also have to do the same thing if/once these upstream deps merge.
There was a problem hiding this comment.
👍
The PR here: https://github.com/coder/trivy/pull/74/changes
Does that only pull the change that adds OptionWithResourceClosure? Should we instead pull the latest trivy release and rebase the 2 coder commits ontop?
There was a problem hiding this comment.
Yeah, https://github.com/coder/trivy/pull/74/changes is just one commit of new code I wrote, not a pull or rebase of anything from upstream trivy. It adds the OptionWithResourceClosure option plus the closure-pruning logic in evaluator.go / option.go / parser.go, with a test. Nothing comes from the upstream fossabot/trivy. It needs to be reviewed/approved/merged before we can pull that change here.
There was a problem hiding this comment.
@PushTheLimit got it. So this is new change, not in Trivy.
It's worth sending upstream too, always makes it easier for us in the long run.
They have been pretty open to changes in the past.
But we can work this into our fork in the interim
For our case with a template with a large Terraform module graph it went from ~4.5s per change to ~0.4s .. including the initial page load to the create workspace form. Big improvement. |
Oh that is huge and very worth it |
Picks up the fix that retains resources referenced through an index expression (count/for_each) when pruning the target closure, plus the accompanying test coverage. Required by the closure corpus added next.
Adds a fixture whose parameters and workspace tags derive from resource blocks through direct, local-indirected, and count-indexed references, with an unreferenced orphan resource that pruning drops. Asserts the parameter defaults and tag values are exactly what an unpruned evaluation produces, so an over-aggressive prune is caught end to end (the count-indexed case fails against the pre-fix evaluator). Excluded from the terraform-apply e2e suite (skipe2e); the closure logic lives in static preview evaluation, which Test_Extract covers.
…ive edges Extends the resourceclosure fixture so each closure target type keeps a resource independently: a preset and a workspace tag now each keep a resource nothing else references, alongside the parameter paths. Adds a for_each index reference and a transitive resource->resource chain. The for_each and count-index cases fail against the pre-fix evaluator and pass with the indexed-reference fix; the full existing corpus still passes with the closure enabled.
|
@Emyrk added the coverage you were after 👍 New The go.mod replace here is on the trivy branch with the fix for now; I'll swap it to the merged coder/trivy commit before this comes out of draft. |
|
@PushTheLimit awesome 👍 I'm going to kick an agent to see if it can think of any other test vectors to throw in. |
Fills gaps in the corpus with thorough parameter/tag assertions for fixtures that had no Test_Extract case: - countsub: count-guarded parameter and a tag driven by a submodule output - http: a tag from an http data source that is unknown without plan data - notstringtag: non-string tag keys and values coerced to strings - wordle: seven parameters that default to the empty string Assertion-only; no changes to preview behavior.
Asserts each parameter's value, default and form type, including the four whose form_type is resolved from a sibling *_format dropdown's value (single->radio, number->input, boolean->radio, list->multi-select). The count-guarded satisfaction parameter is correctly absent because like_it defaults false.
2 participants
What
Pass
OptionWithResourceClosure(["coder_parameter", "coder_workspace_preset", "coder_workspace_tags"])to the Terraform parser inPreview, so root-module resource blocks that nothing in the parameter/preset/tag closure references are skipped during evaluation.Why
Previewevaluates the entire module graph on every call, but rendering a workspace form only needs the parameter, preset and tag blocks and what they reference. The resources a workspace would create cannot feed those blocks, so evaluating them is wasted work that dominates request latency.Concretely this is what makes the Coder dynamic-parameters websocket slow: on a real Altana
nexus-stacktemplate,EvaluateAllis ~2s per request (prod ~4.5s), and it re-runs on every parameter change.Result (real nexus-stack template, local)
EvaluateAll~2.0s to ~0.16s (~12x)Dependency
Requires
OptionWithResourceClosurefrom the trivy fork: coder/trivy#74. The trivyreplaceingo.modis temporarily pinned to that PR's commit so this builds; it will be moved to the mergedcoder/trivycommit before this is taken out of draft.Testing
go build ./...and the non-E2E unit suite pass.coder/coderat v2.35.4:coderd/dynamicparameterstests pass against this change, with identical parameter output.