Implement video feature in docker engine

[walac]

No description provided.

[coveralls]

Coverage decreased (-0.3%) to 45.396% when pulling e655143 on walac:loopback-video into 4d93412 on taskcluster:master.

[jonasfj]

Is this for deduplication? If so, just add Unique: true in the JSON schema for array...

[jonasfj]

We could just make the option of video be dependent on whether devices are enabled or not... anyways, nit... I'm still not sure what's ideal in either case.

[jonasfj]

I think I would suggest that we do:

devices: [
  {type: 'video'},
],

That way, if in some future we decide we want additional options, we can add them to the object...

[jonasfj]

for loop + a switch is likely better...

[walac]

Maybe when we add more options.

[jonasfj]

Don't we want to make the devices in here? Also I think a devices sub-package would be neat.

That way we can hide some of the locking and internals away, and the engine just deals with a device which is something that has a path... and is created when the array from payload is passed to the device manager...

[walac]

Don't know if I followed the idea, but let's keep things simple for now.

[jonasfj]

I like the test case :)

And this doesn't have to be a glorified as I suggest in my comments... I just think it's not that hard to isolate the various aspects, so that things will be extensible.

But at a minimum we should fix missing locking and ensure we do life-cycle management correctly.

[jonasfj]

locking?

[jonasfj]

returning a reference to an object stored in a list... it's probably better to make a list pointers to devices..

Just so we don't have to think too much about happens when if we mutate the device... or something like that.

[jonasfj]

At no point do you set, dev.claimed = true

[jonasfj]

It's probably best to decide what is ecapsulated where... You have a method videoDeviceManager.release

[jonasfj]

This is probably too soon to release, the container still exists...

[jonasfj]

I would suggest something like:

package dockerengine

import (
	"errors"
	"sync"

	"github.com/taskcluster/taskcluster-worker/runtime/atomics"
)

type deviceHandle interface {
	Path() string
	Release()
}

type videoDeviceManager struct {
	m       sync.Mutex
	devices []string
}

// newVideoDeviceManager creates a new videoDeviceManager
func newVideoDeviceManager() (*videoDeviceManager, error) {
	// TODO: Find or create devices
	devices := []string{"/dev/video0"}
	return &videoDeviceManager{devices: devices}, nil
}

// GetVideoDevice returns an idle video device
func (vdm *videoDeviceManager) GetVideoDevice() deviceHandle {
	vdm.m.Lock()
	defer vdm.m.Unlock()

	// ensure we have enough devices
	if len(vdm.devices) < 1 {
		panic(errors.New("insufficent number of video devices available"))
	}

	// Create a device handle, ensuring we can't release twice
	vdh := &videoDeviceHandle{owner: vdm}
	vdh.path, vdm.devices = vdm.devices[0], vdm.devices[1:]

	return vdh
}

// Dispose releases all resources assocated with the videoDeviceManager
func (vdm *videoDeviceManager) Dispose() error {
	vdm.m.Lock()
	defer vdm.m.Unlock()

	return nil // TODO: remove devices if any was created
}

type videoDeviceHandle struct {
	owner    *videoDeviceManager
	path     string
	released atomics.Once
}

func (vdh *videoDeviceHandle) Path() string {
	if vdh.released.IsDone() {
		panic(errors.New("videoDeviceHandle.Path() was called after release"))
	}
	return vdh.path
}

func (vdh *videoDeviceHandle) Release() string {
	ok := vdh.released.Do(func() {
		vdh.owner.m.Lock()
		defer vdh.owner.m.Unlock()

		vdh.owner.devices = append(vdh.owner.devices, vdh.path)
	})
	if !ok {
		debug("WARNING: harmless double release() of videoDeviceHandle")
	}
}

In the sandbox and resultset code you just release() the deviceHandle the same way we release imageHandle and pass it around the same way.

While techincally all this is in the same package, I only use methods starting with capital outside this file... the only exception being: newVideoDeviceManager(). This makes it easier to track life-cycles, and it is a motivation for moving device management into separate package, though that's a lot less important..

[jonasfj]

I think I would suggest that we do:

devices: [
  {type: 'video'},
],

That way, if in some future we decide we want additional options, we can add them to the object...