nomad exec part 1: plumbing and docker driver #5632

notnoop · 2019-04-30T18:19:11Z

Context

This PR is the first of PRs that aim to introduce nomad exec: a command allowing operators to execute commands into running tasks that run anywhere in cluster in any driver that supports exec. Think docker exec but that can run against task running remotely and with exec/java tasks as well.

The PRs are relatively very large, and best reviewed by examining individual commits along with the commit messages that dig deeper into implementation choices.

This PR: Plumbing and initial driver support

This PR introduces an API endpoint for executing arbitrary commands on running tasks, that forwards the API call to the nomad client running the task.

The endpoint uses websockets for processing user inputs and streaming back process output. The agent handler would forward the stream entities all the way to client. Some care is given to reuse the same serialization bits all the way to drivers, and only unpack the stream at the very last moment (driver or executor); this allows us to reduce transfer/conversion overhead and in some cases allow reuse of buffers.

When ACLs are enabled, an operator must have "alloc-exec" capability enabled to exec into any command. If the task is a raw_exec or using a driver that has no isolation, "alloc-node-exec" capability.

In this PR, we add support for docker as it's the most trivial implementation. Follow up PRs would add support for other drivers.

High level walk through and key ideas:

(please review the commit messages for details)

The PR starts with few helper changes that are relevant for subsequent significant changes - wanted to get them out of the way before getting into the meaty commits:

6d711d0:

introduces the core structs and interfaces involved. It introduces two (one high-level and one low-level) interfaces for drivers intending to support nomad exec. Drivers support is opt-in, and existing drivers can compile without any modification.
Also introduces the websocket streaming entities (e.g. ExecTaskStreamingRequest and ExecTaskStreamingResponse) that will get reused in the API as well in e78f7ce#diff-bafd99d510459580cfc433bada3fde5cR535 .

4ade58e and 781b255:

contain the client ultimate endpoint handler and the boilerplate necessary for forwarding connection from api agent handler to the client running allocation.
they perform basic checks (e.g. checking ACL, running state) and then delegate to a driver handler that inspects if driver supports nomad exec and invokes the appropriate call

58a4c71

adds the agent websocket handler that unwraps the API call and translate the websocket interface to the RPC method.

e78f7ce

Adds an Allocations.Exec API convenience method for invoking the API.

d144553

adds a test harness to be used for all drivers - mostly modeled after docker exec behavior in terms of tty/children-process behaviors.

a8e460a:

adds docker support for nomad exec as a working driver example to vet this implementation

This helper returns the token as well as the ACL policy, to be used in a later commit for logging the token info associated with nomad exec invocation.

Test helper that allows registration of jobs when ACL is activated.

Extract command parsing and execution mocking into a separate struct. Also, allow mocking of different fs_isolation for testing.

This adds `alloc-exec` capability to allow operator to execute command into a running task. Furthermore, it adds `alloc-node-exec` capability, required when the alloc task is raw_exec or a driver with no FSIsolation.

In this commit, we add two driver interfaces for supporting `nomad exec` invocation: * A high level `ExecTaskStreamingDriver`, that operates on io reader/writers. Drivers should prefer using this interface * A low level `ExecTaskStreamingRawDriver` that operates on the raw stream of input structs; useful when a driver delegates handling to driver backend (e.g. across RPC/grpc). The interfaces are optional for a driver, as `nomad exec` support is opt-in. Existing drivers continue to compile without exec support, until their maintainer add such support. Furthermore, we create protobuf structures to represent exec stream entities: `ExecTaskStreamingRequest` and `ExecTaskStreamingResponse`. We aim to reuse the protobuf generated code as much as possible, without translation to avoid conversion overhead. `ExecTaskStream` abstract fetching and sending stream entities. It's influenced by the grpc bi-directional stream interface, to avoid needing any adapter. I considered using channels, but the asynchronisity and concurrency makes buffer reuse too complicated, which would put more pressure on GC and slows exec operation.

schmichael

Review is still a WIP. Just dumping a first batch of comments.

api/allocations.go

schmichael · 2019-05-01T18:03:41Z

api/allocations.go

+			return -1, ctx.Err()
+		case frame, ok := <-output:
+			if !ok {
+				return -1, nil


Is closing the output chan an expected way to signal an exit? If not we should probably return an error so it doesn't appear the -1 exit signal came from the remote side?

Nope - the only legitimate expected way to exit is by receiving a frame with exit results.

updated to use -2 here

Shouldn't it return an error then?

good point - i'll make it return an error

schmichael · 2019-05-01T18:06:27Z

api/allocations.go

+			case frame.Exited && frame.Result != nil:
+				return frame.Result.ExitCode, nil
+			default:
+				// unexpected event, TODO: log it?!


Log or error. Logging seems more reasonable in this case as it allows us to tuck more metadata into the frames in the future without completely breaking old clients.

I'm unclear how to log in the cli side of things. Do we have a pattern of cli logging? Also, logging can be very disruptive, consider the case where you are running in vim/cli-with-rich-interface, having log messages emited may cause display to go weird. That was my concern with all the other places where I would have logged if we were in the agent side.

Ah, you're right! I forgot we can't really log in the api package. Let's update the comment.

Suggested change

// unexpected event, TODO: log it?!

// Unexpected event! Can't log as the user may be in an interactive session.

// Silently drop unexpected frames to provide graceful degradation for future protocol changes.

schmichael · 2019-05-02T22:32:47Z

client/alloc_endpoint_test.go

+	}
+}
+
+// TestAlloc_ExecStreaming_ACL_WithIsolation_Image assets that token only needs


Suggested change

// TestAlloc_ExecStreaming_ACL_WithIsolation_Image assets that token only needs

// TestAlloc_ExecStreaming_ACL_WithIsolation_Image asserts that token only needs

schmichael · 2019-05-02T22:33:06Z

client/alloc_endpoint_test.go

+	}
+}
+
+// TestAlloc_ExecStreaming_ACL_WithIsolation_Chroot assets that token only needs


Suggested change

// TestAlloc_ExecStreaming_ACL_WithIsolation_Chroot assets that token only needs

// TestAlloc_ExecStreaming_ACL_WithIsolation_Chroot asserts that token only needs

schmichael · 2019-05-02T22:33:39Z

client/alloc_endpoint_test.go

+	}
+}
+
+// TestAlloc_ExecStreaming_ACL_WithIsolation_None assets that token needs


Suggested change

// TestAlloc_ExecStreaming_ACL_WithIsolation_None assets that token needs

// TestAlloc_ExecStreaming_ACL_WithIsolation_None asserts that token needs

schmichael · 2019-05-02T22:34:28Z

client/alloc_endpoint_test.go

+		}
+
+		var frame drivers.ExecTaskStreamingResponseMsg
+		json.Unmarshal(msg.Payload, &frame)


Check error

client/allocrunner/taskrunner/driver_handle.go

schmichael

First pass complete! Overall structure looks great, just worried about some of the trickier multiple-goroutines-handling-errors cases.

schmichael · 2019-05-03T19:16:55Z

client/allocrunner/taskrunner/driver_handle.go

+		return err
+	}
+
+	stream.Send(drivers.NewExecStreamingResponseExit(result.ExitCode))


Handle error or comment why we don't need to

client/pluginmanager/drivermanager/manager.go

command/agent/alloc_endpoint.go

plugins/drivers/execstreaming.go

plugins/drivers/proto/driver.proto

plugins/drivers/server.go

schmichael · 2019-05-09T17:05:39Z

plugins/drivers/execstreaming.go

+					Width:  int(msg.TtySize.Width),
+				}:
+				case <-ctx.Done():
+					// process terminated before resize is processed


Are you missing a return here?

notnoop · 2019-05-09T17:19:56Z

plugins/drivers/execstreaming.go

+			msg, err := stream.Recv()
+			if err == io.EOF {
+				return
+			}


Need to handle non EOF errors here

schmichael

Looks great! A couple minor nitpicks and questions remain, but none are blockers.

Also add a helper that converts the adapts the high level interface to the low-level interface of nomad exec interfaces.

Add a client streaming RPC endpoint for processing nomad exec tasks, by invoking the relevant task handler for execution.

This adds a websocket endpoint for handling `nomad exec`. The endpoint is a websocket interface, as we require a bi-directional streaming (to handle both input and output), which is not very appropriate for plain HTTP 1.0. Using websocket makes implementing the web ui a bit simpler. I considered using golang http hijack capability to treat http request as a plain connection, but the web interface would be too complicated potentially. Furthermore, the API endpoint operates against the raw core nomad exec streaming datastructures, defined in protobuf, with json serializer. Our APIs use json interfaces in general, and protobuf generates json friendly golang structs. Reusing the structs here simplify interface and reduce conversion overhead.

Add consolidated testing package to serve as conformance tests for all drivers.

Adds nomad exec support in our API, by hitting the websocket endpoint. We introduce API structs that correspond to the drivers streaming exec structs. For creating the websocket connection, we reuse the transport setting from api http client.

github-actions · 2023-02-10T02:17:38Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Mahmood Ali added 5 commits April 30, 2019 10:23

aux: helper method that returns token as well as ACL policy

43cd7a7

This helper returns the token as well as the ACL policy, to be used in a later commit for logging the token info associated with nomad exec invocation.

test helper for registering jobs with acl

75349d6

Test helper that allows registration of jobs when ACL is activated.

drivers/mock: extract command related operations

e79ce1f

Extract command parsing and execution mocking into a separate struct. Also, allow mocking of different fs_isolation for testing.

Add ACL capabilities for nomad exec

4f7bd68

This adds `alloc-exec` capability to allow operator to execute command into a running task. Furthermore, it adds `alloc-node-exec` capability, required when the alloc task is raw_exec or a driver with no FSIsolation.

This was referenced Apr 30, 2019

nomad exec part 2: CLI #5633

Merged

nomad exec part 3: executor based drivers #5634

Merged

notnoop requested a review from schmichael April 30, 2019 18:38

schmichael reviewed May 2, 2019

View reviewed changes

schmichael requested changes May 3, 2019

View reviewed changes

notnoop requested a review from schmichael May 8, 2019 20:49

schmichael reviewed May 9, 2019

View reviewed changes

notnoop commented May 9, 2019

View reviewed changes

schmichael approved these changes May 9, 2019

View reviewed changes

Mahmood Ali added 10 commits May 9, 2019 16:49

implemment streaming exec handling in driver grpc handlers

94ed649

Also add a helper that converts the adapts the high level interface to the low-level interface of nomad exec interfaces.

drivers/mock: implement nomad exec interface

cb4ad3f

implement client endpoint of nomad exec

979a6a1

Add a client streaming RPC endpoint for processing nomad exec tasks, by invoking the relevant task handler for execution.

server: server forwarding logic for nomad exec endpoint

a77a3ba

vendor github.com/gorilla/websocket

f908d6b

Add basic drivers conformance tests

3a77502

Add consolidated testing package to serve as conformance tests for all drivers.

drivers/docker: implement streaming exec

67160a6

add api support for nomad exec

6168cc5

Adds nomad exec support in our API, by hitting the websocket endpoint. We introduce API structs that correspond to the drivers streaming exec structs. For creating the websocket connection, we reuse the transport setting from api http client.

add e2e tests for nomad exec

980e4f5

notnoop force-pushed the f-nomad-exec-parts-01-base branch from bffe3d2 to 980e4f5 Compare May 9, 2019 20:49

notnoop merged commit 5abbee5 into master May 9, 2019

notnoop mentioned this pull request May 12, 2019

implement nomad exec for rkt #5690

Merged

notnoop deleted the f-nomad-exec-parts-01-base branch May 18, 2019 00:47

angrycub mentioned this pull request Sep 9, 2019

Docs: Added alloc_exec and alloc_node_exec capabilities #6298

Merged

picatz mentioned this pull request Apr 9, 2021

nomad port-forward command #6925

Open

github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nomad exec part 1: plumbing and docker driver #5632

nomad exec part 1: plumbing and docker driver #5632

notnoop commented Apr 30, 2019

schmichael left a comment

schmichael May 1, 2019

notnoop May 3, 2019

notnoop May 8, 2019

schmichael May 9, 2019

notnoop May 9, 2019

schmichael May 1, 2019

notnoop May 3, 2019 •

edited

Loading

schmichael May 9, 2019

schmichael May 2, 2019

schmichael May 2, 2019

schmichael May 2, 2019

schmichael May 2, 2019

schmichael left a comment

schmichael May 3, 2019

schmichael May 9, 2019

notnoop May 9, 2019

schmichael left a comment

github-actions bot commented Feb 10, 2023

	// unexpected event, TODO: log it?!
	// Unexpected event! Can't log as the user may be in an interactive session.
	// Silently drop unexpected frames to provide graceful degradation for future protocol changes.

	// TestAlloc_ExecStreaming_ACL_WithIsolation_Image assets that token only needs
	// TestAlloc_ExecStreaming_ACL_WithIsolation_Image asserts that token only needs

nomad exec part 1: plumbing and docker driver #5632

nomad exec part 1: plumbing and docker driver #5632

Conversation

notnoop commented Apr 30, 2019

Context

This PR: Plumbing and initial driver support

High level walk through and key ideas:

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnoop May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 10, 2023

notnoop May 3, 2019 •

edited

Loading