This repository has been archived by the owner on Feb 27, 2020. It is now read-only.
Implement video feature in docker engine #383
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package dockerengine | ||
|
|
||
| import ( | ||
| "path/filepath" | ||
| "regexp" | ||
|
|
||
| "github.com/pkg/errors" | ||
| funk "github.com/thoas/go-funk" | ||
| ) | ||
|
|
||
| type device struct { | ||
| path string | ||
| claimed bool | ||
| } | ||
|
|
||
| type videoDeviceManager struct { | ||
| devices []device | ||
| } | ||
|
|
||
| func newVideoDeviceManager() (*videoDeviceManager, error) { | ||
|
There was a problem hiding this comment. 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 While techincally all this is in the same package, I only use methods starting with capital outside this file... the only exception being: |
||
| matches, err := filepath.Glob("/dev/video*") | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "Failed to call filepath.Glob function") | ||
| } | ||
|
|
||
| r := regexp.MustCompile("/dev/video[0-9]+") | ||
|
There was a problem hiding this comment. Don't we want to make the devices in here? Also I think a 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... There was a problem hiding this comment. Don't know if I followed the idea, but let's keep things simple for now. |
||
| matches = funk.FilterString(matches, r.MatchString) | ||
|
|
||
| devices := make([]device, len(matches)) | ||
| for i := range devices { | ||
| devices[i].path = matches[i] | ||
| } | ||
|
|
||
| return &videoDeviceManager{ | ||
| devices: devices, | ||
| }, nil | ||
| } | ||
|
|
||
| func (d *videoDeviceManager) claim() *device { | ||
| for i := range d.devices { | ||
|
|
||
| if !d.devices[i].claimed { | ||
| return &d.devices[i] | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. At no point do you set, |
||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (d *videoDeviceManager) release(dev *device) { | ||
| dev.claimed = false | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| // +build dockervideo | ||
|
|
||
| package dockerengine | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/taskcluster/taskcluster-worker/engines/enginetest" | ||
| ) | ||
|
|
||
| // Image and tag used in test cases below | ||
| const ( | ||
| videoDockerImageName = "alpine:3.6" | ||
| ) | ||
|
|
||
| var videoProvider = &enginetest.EngineProvider{ | ||
| Engine: "docker", | ||
| Config: `{ | ||
| "privileged": "allow", | ||
| "enableDevices": true | ||
| }`, | ||
| } | ||
|
|
||
| func TestVideo(t *testing.T) { | ||
| c := enginetest.LoggingTestCase{ | ||
| EngineProvider: videoProvider, | ||
| Target: "/dev/video0", | ||
| TargetPayload: `{ | ||
| "command": ["sh", "-c", "ls /dev/video0"], | ||
| "devices": ["video"], | ||
| "image": "` + videoDockerImageName + `" | ||
| }`, | ||
| } | ||
|
|
||
| c.Test() | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We could just make the option of
videobe dependent on whether devices are enabled or not... anyways, nit... I'm still not sure what's ideal in either case.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.
I think I would suggest that we do:
That way, if in some future we decide we want additional options, we can add them to the object...