fix(provisioner): handle multiple agents, apps, scripts and envs #13741
Conversation
provisioner/terraform/resources.go
Outdated
| appNodeSuffix := fmt.Sprintf(`] %s.%s (expand)"`, resource.Type, resource.Name) | ||
| agentNodeSuffix := fmt.Sprintf(`] coder_agent.%s (expand)"`, agent.Name) |
There was a problem hiding this comment.
It's unfortunate that we need to match this :(
There was a problem hiding this comment.
Yes, I agree. It is the biggest drawback of this PR, and we need to preserve the format.
provisioner/terraform/resources.go
Outdated
| for _, dst := range graph.Edges.SrcToDsts { | ||
| for _, edges := range dst { | ||
| for _, edge := range edges { | ||
| if strings.HasSuffix(edge.Src, appNodeSuffix) && | ||
| strings.HasSuffix(edge.Dst, agentNodeSuffix) { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
My main concern is that we are traversing this graph multiple times in a tight loop.
Could we instead traverse the graph once, build a shallow map of agent -> resource, and use that to determine the agent-resource dependency?
There was a problem hiding this comment.
The main reason I'm hesitant to do this, it would be another cache/state used only to prevent this problem. Considering we have 1 agent on average, 2 in total, it may not introduce a heavy performance impact.
Anyway, if you think this is a must-do, I will improve the loop 👍 I admit, I focused mostly on fixing the problem.
There was a problem hiding this comment.
That's fair. Let's make it work correctly first, and then see about benchmarking / speeding it up.
Fixes: #12949
This PR adjusts provisioner logic to properly handle apps, scripts, and envs assigned to different agent. Without this PR every app/script/env is assigned to every agent. Unfortunately, there isn't an easy way to achieve it, and we need to snoop the dependency information from the graph.
Note: I added a couple of .tf tests, which blew up the PR size. The code change itself is relatively small.