clientv2: base driver plugin #4671

nickethier · 2018-09-12T19:21:53Z

Ignore most of the changes to executor here. That will come in a later PR.

dadgar · 2018-09-12T21:10:25Z

plugins/drivers/base/client.go

+	"golang.org/x/net/context"
+)
+
+type DriverClient interface {


What is the point of this slimmed down interface?

So this interface isn't fully built out yet. I need to finish adding other methods.

If I remember correctly I opted to separate the client and plugin side interfaces so you could pass in a context where appropriate on the client. For example on WaitTask. It feels like I started that code ages ago though and now think that might not be necessary?

dadgar · 2018-09-12T21:11:14Z

plugins/drivers/base/client.go

+
+func (d *driverPluginClient) StartTask(c *TaskConfig) (*TaskHandle, error) {
+	resp, err := d.client.StartTask(context.Background(),
+		&proto.StartTaskRequest{


This looks awkward. Can you make the request outside the call

dadgar · 2018-09-12T21:11:38Z

plugins/drivers/base/client.go

+
+func (d *driverPluginClient) WaitTask(ctx context.Context, id string) chan *TaskResult {
+	ch := make(chan *TaskResult)
+	go func() {


Create a waitTaskImpl instead of an anonymous function

dadgar · 2018-09-12T21:12:00Z

plugins/drivers/base/driver.go

+	"github.com/hashicorp/nomad/plugins/base"
+)
+
+const DriverGoPlugin = "driver"


prefer base.PluginTypeDriver
https://github.com/hashicorp/nomad/blob/master/plugins/base/plugin.go#L19

dadgar · 2018-09-12T21:13:55Z

plugins/drivers/base/proto_utils.go

@@ -0,0 +1,68 @@
+package base


proto_utils -> utils.go? for consistency

dadgar · 2018-09-12T21:15:21Z

plugins/drivers/base/testing.go

+func (h *DriverHarness) MkAllocDir(t *TaskConfig) func() {
+	allocDir, err := ioutil.TempDir("", "nomad_driver_harness-")
+	require.NoError(h.t, err)
+	os.Mkdir(filepath.Join(allocDir, t.Name), os.ModePerm)


require no error on these?

plugins/drivers/base/plugin_test.go

…driver harness

plugins/drivers/base/client.go

plugins/drivers/base/driver.go

schmichael · 2018-09-25T00:02:59Z

plugins/drivers/base/driver.go

+}
+
+func (tc *TaskConfig) EnvList() []string {
+	l := make([]string, len(tc.Env))


Set the cap=len(tc.Env), not the len: https://play.golang.org/p/JtF3Mhv4-i4

plugins/drivers/base/driver.go

schmichael · 2018-09-25T00:09:43Z

plugins/drivers/base/driver.go

+	ID               string
+	Name             string
+	State            TaskState
+	SizeOnDiskMB     int64


From its placement in TaskStatus instead of TaskStats I assume this is EphemeralDisk.Size? I think we should name it EphemeralDiskSize if so to avoid confusion as to whether this is the static allocation vs a runtime measurement.

This is meant to be a runtime measurement of the current disk usage.

Shouldn't it be in Stats with the other runtime measurements then? I don't think we've implemented this anywhere yet, so it can probably be removed entirely.

The original plan was that today InspectTask is actually never called, but we'd like to have a cli (and match UI) that allows you to inspect a task and get all the delicious debug information you need from it. This was one value that I thought should be included here.

plugins/drivers/base/utils.go

schmichael · 2018-09-25T00:26:34Z

plugins/drivers/utils/utils.go

+
+// ValidateCommand validates that the command only has a single value and
+// returns a user friendly error message telling them to use the passed
+// argField.


argField isn't used.

dadgar · 2018-09-25T22:00:55Z

plugins/drivers/base/client.go

@@ -0,0 +1,299 @@
+package base


Why are any of these files in a base sub package?

dadgar · 2018-09-25T22:02:37Z

plugins/drivers/base/client.go

+			break
+		}
+		f := &Fingerprint{
+			Attributes:        pb.Attributes,


Hm shouldn't this expose the error to the caller

dadgar · 2018-09-25T22:04:09Z

plugins/drivers/base/client.go

+		}
+		timestamp, _ := ptypes.Timestamp(ev.Timestamp)
+		event := &TaskEvent{
+			TaskID:      ev.TaskId,


Same err comment

plugins/drivers/base/driver.go

dadgar · 2018-09-25T22:10:54Z

plugins/drivers/utils/utils_windows.go

+	"os/exec"
+)
+
+// TODO Figure out if this is needed in Wondows


dadgar · 2018-09-26T16:54:22Z

plugins/drivers/client.go

@@ -249,6 +251,7 @@ func (d *driverPluginClient) handleTaskEvents(ch chan *TaskEvent, stream proto.D
 		}
 		if err != nil {
 			d.logger.Error("error receiving stream from TaskEvents driver RPC", "error", err)
+			ch <- &TaskEvent{Err: fmt.Errorf("error from RPC stream: %v", err)}


Hm I don't think we should wrap it because that error is likely a gRPC error

dadgar · 2018-09-26T16:54:43Z

plugins/drivers/driver.go

@@ -67,6 +67,9 @@ type Fingerprint struct {
 	Attributes        map[string]string
 	Health            HealthState
 	HealthDescription string
+
+	// Err is only used if an error occured while consuming the RPC stream


Comment is odd since this should be settable by the plugin as well.

Driver plugin framework to facilitate development of driver plugins. Implementing plugins only need to implement the DriverPlugin interface. The framework proxies this interface to the go-plugin GRPC interface generated from the driver.proto spec. A testing harness is provided to allow implementing drivers to test the full lifecycle of the driver plugin. An example use: func TestMyDriver(t *testing.T) { harness := NewDriverHarness(t, &MyDiverPlugin{}) // The harness implements the DriverPlugin interface and can be used as such taskHandle, err := harness.StartTask(...) }

github-actions · 2023-02-26T02:21:41Z

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.

dadgar requested changes Sep 12, 2018

View reviewed changes

nickethier force-pushed the r-clientv2 branch from ba95b44 to 2b03e8b Compare September 14, 2018 02:59

dadgar force-pushed the r-clientv2 branch from d3e5440 to e0114c5 Compare September 16, 2018 00:08

nickethier force-pushed the f-base-driver-plugin branch from 1d7d12b to 7f43217 Compare September 17, 2018 16:07

dadgar force-pushed the r-clientv2 branch 2 times, most recently from 393ee89 to a333efb Compare September 18, 2018 17:50

nickethier force-pushed the f-base-driver-plugin branch from 23f59fe to e9b0ea5 Compare September 18, 2018 19:56

nickethier added 6 commits September 24, 2018 14:39

driver/base: initial driver plugin framework

f040af3

driver/base: adjusted interface design and implement TaskHandle

1f2dabb

driver/base: implement client side of plugin

516695b

driver/base: use seperate interface for driver plugin clients. added …

ea85a78

…driver harness

driver/base: Remove ctx from client interface unless needed

687238e

driver/raw_exec: start raw_exec driver plugin impl

66827c0

nickethier force-pushed the f-base-driver-plugin branch from e9b0ea5 to f0e96a8 Compare September 24, 2018 18:39

nickethier added 15 commits September 24, 2018 14:39

driver/raw_exec: implemented additional plugin methods

0577c3c

driver/base: add task event emitting to proto

e5c9abf

driver/base: fixup comments from code review

a2901ba

driver/base: add size on disk field for task

3de54c4

driver/base: fix proto pr comments

00ef80e

driver/base: Removed ListTasks RPCs and added force flag to destroy

553cc26

driver/base: implement client side driver interface

6c870b6

driver/base: fixup interfaces to match naming conventions

88ae245

driver/base: implemented all server shim funcs

8c068a1

driver/base: implement client side of RPC shim

f6e5d0e

driver/base: added tests

584def3

code review comments

f0e96a8

driver/base: return errors for funcs that return chans

f721b6c

remove unused const

acf06a5

remove raw_exec from PR

4973ab9

schmichael reviewed Sep 25, 2018

View reviewed changes

nickethier added 2 commits September 25, 2018 15:02

driver/base: additional comments and disk size detection capability

cf76e40

lint: spelling

4c66a13

dadgar requested changes Sep 25, 2018

View reviewed changes

plugins/drivers: remove base subpackage

071333f

dadgar approved these changes Sep 26, 2018

View reviewed changes

plugin/drivers: remove SizeOnDiskMB field

8840d6d

nickethier merged commit a73555f into r-clientv2 Sep 26, 2018

nickethier deleted the f-base-driver-plugin branch September 26, 2018 18:05

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clientv2: base driver plugin #4671

clientv2: base driver plugin #4671

nickethier commented Sep 12, 2018

dadgar Sep 12, 2018

nickethier Sep 13, 2018

dadgar Sep 12, 2018

dadgar Sep 12, 2018

dadgar Sep 12, 2018

dadgar Sep 12, 2018

dadgar Sep 12, 2018

schmichael Sep 25, 2018

schmichael Sep 25, 2018

nickethier Sep 25, 2018

schmichael Sep 25, 2018

nickethier Sep 25, 2018

schmichael Sep 25, 2018

dadgar Sep 25, 2018

dadgar Sep 25, 2018

dadgar Sep 25, 2018

dadgar Sep 25, 2018

dadgar Sep 26, 2018

dadgar Sep 26, 2018

github-actions bot commented Feb 26, 2023

clientv2: base driver plugin #4671

clientv2: base driver plugin #4671

Conversation

nickethier commented Sep 12, 2018

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

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

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

github-actions bot commented Feb 26, 2023