Add parallel execution to assist. by jakule · Pull Request #26563 · gravitational/teleport

jakule · 2023-05-19T02:19:23Z

Execute Assist commands in parallel.

Closes https://github.com/gravitational/teleport.e/issues/1468

jakule · 2023-05-23T17:19:30Z

xacrimon · 2023-05-24T15:44:16Z

-				h.log.WithError(err).Debug("Unable to generate new ssh session.")
-				return trace.Wrap(err)
-			}
+	runCmd := func(host *hostInfo) error {


can we move this into a seperate function at all? executeCommand is already pretty unwieldy to look at.

I've tried, but this function uses 11 variables from the outer scope. I've tried to move them to a new struct, but IMO, at that point, it's not worth it. If you have a strong opinion about that I can do it.

👍 fine then

xacrimon · 2023-05-24T15:46:10Z

+func runCommands(hosts []hostInfo, runCmd func(host *hostInfo) error, log logrus.FieldLogger) {
+	// Create a synchronization channel to limit the number of concurrent commands.
+	// The maximum number of concurrent commands is 30 - it is arbitrary.
+	syncChan := make(chan struct{}, 30)


can/should we put this in a configuration resource somewhere? I can imagine someone wanting to play with/adjust this.

I was also thinking about it, but our "configuration management" looks a bit messy now.
@justinas Any concerns with adding this value to teleport.yaml to the assist section?

No concerns, we'll likely leave it as default on Cloud.

@xacrimon I created a new issue to address this comment, as I don't want to introduce more changes after people have already reviewed it. https://github.com/gravitational/teleport.e/issues/1516

xacrimon · 2023-05-24T15:51:34Z

+func runCommands(hosts []hostInfo, runCmd func(host *hostInfo) error, log logrus.FieldLogger) {
+	// Create a synchronization channel to limit the number of concurrent commands.
+	// The maximum number of concurrent commands is 30 - it is arbitrary.
+	syncChan := make(chan struct{}, 30)


should this use sync/semaphore?

I was also thinking about it, but Go has divided opinions about semaphores in general. I remember a proposal to remove semaphores in Go 2, which had many examples of why you should not use than. Now even stdlib recommends "kind of" against them https://pkg.go.dev/sync#Cond

that's uh, certainly a way to design a language. anyhow, in that case, fine by me.

xacrimon

lgtm but if we could get @justinas to chime in on the config problem, that'd be nice

timothyb89

This looks reasonable to me, I don't have strong opinions w.r.t. configuration.

* Add parallel execution to assist. * Extract execution logic to a new function. * Add test * Switch uber to std * Address code review comments

* Assist - API implementation (#25810) * Assist - API implementation * Add Assistant CRUD test * Move assistant API to a separate file. * Rename GRPC methods Fix comments * Address PR comments * Update GRPC API - addressing code review comments * Move assist API to a new GRPC service * Add Assistant RBAC rules to this backport. * Add missing license * Update comment. * Move username and ID out of AssistantMessage (#25964) * Move username and ID out of AssistantMessage #25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc * Assist - Configuration and usage (#25953) * Assist - Configuration and usage * Add network config test * Add config test * Run GCI * Address review comments * Assist - OpenAI library port (#25948) * Assist - OpenAI library port * Add tests * Address code review comments * Added comment * Partial backport of #26058 * Move AI messages to a new file * Prevent blocking on error * Add comments. Fix typo * Assist - Execution web endpoint (#25955) * Assist - Execution web endpoint * Add test Clean up code a bit * Add missing username * Address review comments * Make more implementation shared between Terminal and Command Web Handlers * Address review comments * Address review comments * Fixes after rebase Add comments * Add comments Fix linter * Add TELEPORT to Teleport related environment variable. * Assist - Web endpoints (#26046) * Assist - Web endpoints * GCI * proto rpc * enforce endpoint checks * disable in ui if disallowed by auth * add comment * unwrap stack * Refactor code --------- Co-authored-by: Joel Wejdenstål <jwejdenstal@icloud.com> * [Assist] Fix random user selection (#26183) Currently, after selecting a user for command execution in Teleport Assist the user can randomly change. This PR fixes this behavior. * Add rate limiting to Assist (#26011) * Add rate limiting to Assist * Only rate limit Assist in Cloud * Add a comment to assistantLimiter * Fixes after rebase * Add 'rate-limited' test case to assistant_test * Handle CHAT_MESSAGE_UI in Assist web UI * Add godoc * CHAT_MESSAGE_UI -> CHAT_MESSAGE_ERROR * Run assistant test cases in parallel * Rename `ChatGPT` to `GPT-4` (#26272) * Rename `ChatGPT` to `GPT4` ChatGPT is a user friendly name, but is technically inaccurate. * Apply suggestions from code review Co-authored-by: Reed Loden <reed@goteleport.com> * Lint fix * Additional ChatGPT --> OpenAI GPT-4 fixes --------- Co-authored-by: Reed Loden <reed@goteleport.com> * Fix Assist rate-limiting in Cloud (#26342) When Proxy is separate from Auth, Proxy 'modules' will not contain meaningful data. Instead, one must use ClusterFeatures fetched from the Auth server * Assist UI improvements (#26365) * Update assist warning wording, add link to ToS (#26396) * Always render the portal for the assist title to go into (#26733) * Stop using TokenPath for API key in Assist (#26671) * [Assist] MFA support (#26719) * Initial support for MFA in Assist * UI webauth handler * WebUI - WIP * Run prettier * Perform MFA ceremony only once. * Cleanup JS * Remove hacky WS logic * Add cancel MFA logic * [Assist] Prevent creating messages without conversation (#26797) * Add parallel execution to assist. (#26563) * Add parallel execution to assist. * Extract execution logic to a new function. * Add test * Switch uber to std * Address code review comments * Fix display of Assist command executions with empty output (#27010) * Fix display of Assist executions with empty output * Lint * Nit * Improve display of commands that failed with code * Address lint * [Assist] Allow removing assist conversations (#26788) * [Assist] Allow removing assist conversations * Display landing page after the conversion is removed * Improve styling and add a confirmation dialog * Change the icon opacity to copy the main navigation * Remove unused minus icon * Add missing trace.wrap --------- Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Backport fixes * Regenerate go.sum to make the linter happy. * Sync files after rebase * After rebase fixes --------- Co-authored-by: Joel Wejdenstål <jwejdenstal@icloud.com> Co-authored-by: Justinas Stankevičius <justinas@users.noreply.github.com> Co-authored-by: Mike Jensen <jentfoo@users.noreply.github.com> Co-authored-by: Reed Loden <reed@goteleport.com> Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>

jakule force-pushed the jakule/assist-parallel-exec branch from 87fbc46 to 4d4287c Compare May 22, 2023 17:28

jakule marked this pull request as ready for review May 22, 2023 19:26

github-actions Bot requested review from timothyb89 and xacrimon May 22, 2023 19:26

github-actions Bot added the size/sm label May 22, 2023

xacrimon reviewed May 24, 2023

View reviewed changes

jakule force-pushed the jakule/assist-parallel-exec branch from cc9b4c1 to c2ad46d Compare May 24, 2023 16:31

jakule requested a review from xacrimon May 24, 2023 16:32

xacrimon approved these changes May 25, 2023

View reviewed changes

timothyb89 approved these changes May 25, 2023

View reviewed changes

jakule added 5 commits May 26, 2023 17:32

Add parallel execution to assist.

cd954d8

Extract execution logic to a new function.

9aed31b

Add test

fec0872

Switch uber to std

1ab7a36

Address code review comments

ce1babc

jakule force-pushed the jakule/assist-parallel-exec branch from c2ad46d to ce1babc Compare May 26, 2023 21:40

jakule enabled auto-merge May 26, 2023 21:57

jakule added this pull request to the merge queue May 26, 2023

Merged via the queue into master with commit 692d1d4 May 26, 2023

jakule deleted the jakule/assist-parallel-exec branch May 26, 2023 22:38

jakule added a commit that referenced this pull request Jun 2, 2023

Add parallel execution to assist. (#26563)

71b1f49

* Add parallel execution to assist. * Extract execution logic to a new function. * Add test * Switch uber to std * Address code review comments

Conversation

jakule commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakule commented May 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xacrimon May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xacrimon left a comment

Choose a reason for hiding this comment

Uh oh!

timothyb89 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jakule commented May 19, 2023 •

edited

Loading

xacrimon May 24, 2023 •

edited

Loading