- Notifications
You must be signed in to change notification settings - Fork914
fix(provisioner): handle multiple agents, apps, scripts and envs#13741
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's unfortunate that we need to match this :(
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ofagent -> resource, and use that to determine the agent-resource dependency?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's fair. Let's make it work correctly first, and then see about benchmarking / speeding it up.
07d4171 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.