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 1 commit into
Conversation
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 |
|
@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. |
…223) Pins the parameter, preset, and tag values that flow from `resource` blocks, ahead of #221 (which evaluates only the parameter/preset/tag closure via coder/trivy#74). Everything passes on `main`, where nothing is pruned, so a failure after rebasing #221 means pruning dropped a resource a target depends on. - `testdata/resourceclosure`: the fixture from #221 plus conditional, `try()`, expression index, `option` and `dynamic "option"` blocks, `count` on a target, a data-source intermediary, tag -> resource -> parameter, preset `prebuilds`, and a local module. - `testdata/resourceclosurejson`: a `.tf.json` template with two resources in one expression. This currently fails against the #221 pin (second resource pruned, default unknown); see the [comment on coder/trivy#74](coder/trivy#74 (comment)). - `testdata/diskaccess`: revives #85. `file()` on an in-tree path is read; a path escaping the template root is unknown and flags the option as invalid. Not closure-related, but a cheap guard that resource pruning does not disturb function evaluation against the template fs. - `testdata/chain`: revives #164. Pins two-hop count chaining: a parameter count-gated on a count-gated parameter's `[0].value` is silently dropped today because its count is evaluated before the input flows in. The vector asserts current behavior and the comment says how to flip it once fixed. Prepared with Coder Agents assistance.
Follow-up to #223. Pins more of the evaluation paths a parameter, preset, or tag can take to reach a `resource`, ahead of #221. All pass on `main`, and on #221 with the trivy pin at coder/trivy#74's current head (`fc995ac`). - `testdata/resourceclosure2`: `validation` block text and regex from a resource (asserted with a failing input), `[for ...]` and `{for ...}` over `count` and `for_each` resources, a splat directly in a tag, a `count = 0` resource behind `try()`, a resource whose `for_each` is driven by a parameter value and is read by a tag, a computed attribute (stays unknown), a two-level `module.outer` -> `module.inner` chain, and three-hop locals. - `testdata/resourceclosuresubmod`: the only parameter is in a submodule and reads a root resource through a module input; the root has no target block at all. Prepared with Coder Agents assistance.
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. Uses the OptionWithResourceClosure addition merged in coder/trivy#74; the trivy replace points at that merged commit. Correctness is guarded by the resource closure test corpus already in main (coder#223, coder#224).
b32c4a1 to
3ca76fb
Compare
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.