Closed gabe565 closed 1 year ago
Thanks for working on this big PR :)) overall i really like it. I'll take a closer look tomorrow during reasonable waking hours.
BTW, configuring the devices using YAML is such a nice a d easy win. It also meshes well in general with a feature i am working on for this project, which is to add a CRD to declare devices that the plugin should discover. This is powerful because it allows multiple projects to declare the devices they need without all needing to coordinate on adding new flags to the plugin: instead they can simply declare a new custom resource for the device that their workloads require and the workloads can request an allocation of that device.
@squat Thank you! Let me know if you have any feedback. So far, I've tested the device flag and config file pretty heavily, but would like to test out the envs a little bit more in the next few hours.
I did have to add a bit of a hack to be able to handle the device flag (which returns []string
) and the Viper config (which returns []interface{}
), but I think it's worth it. If you dislike it, I'd be happy to strip Viper back out and just leave the YAML arg improvement.
It's off topic from this PR, but the CRD will be amazing! Let me know if I can help out in any way. I have some experience using Kubebuilder to build operators with custom CRDs so if you need a second set of eyes on anything, feel free to ping me.
@squat Thank you! I resolved most your requested changes.
I'm curious about the import sorting requests. It looks like you're using goimports, so I used it too and it says the imports are already sorted. Please let me know if I should just do this manually or if you use another tool.
Working on regenerating the README.md now!
Also, to automate these sorts of things, it might be nice to add a pre-commit hook so the commands get run locally before even pushing.
@squat Finished generating the readme. Let me know what you think on my goimports question. Nevermind, I just saw your comment above
How do you feel on #20? Should I re-add the vendor dir and update it or are you planning to merge that PR before this one?
Edit: If you merge that one first, I'll probably rebase this PR onto the merge commit and possibly squash the requested changes into a single commit
Would you like me to update this to use YAML devices instead of JSON? https://github.com/squat/generic-device-plugin/blob/45ef110a38952ec2df60851616a9d2be870a40c7/manifests/generic-device-plugin.yaml#L26-L47
Would you like me to update this to use YAML devices instead of JSON?
I think that would be good! It's definitely easier to read than JSON
Would you like me to update this to use YAML devices instead of JSON?
I think that would be good! It's definitely easier to read than JSON
Agreed. Done!
Thanks again @gabe565!
This PR refactors the way config is handled.
Changes
config.go
.-
with_
. See examples./etc/generic-device-plugin/config.yaml
$PWD/config.yaml
--config
has been added which sets the config file location.device
todevices
.json.Unmarshal
has been replaced withyaml.Unmarshal
when parsing the--device
flag. This adds support for YAML device args and is not a breaking change since YAML is a superset of JSON. See examples.Also note that this PR contains a commit from #20 that removes the vendor dir, which is why it shows so many lines removed. If that PR gets merged, the removed line count will drop pretty dramatically.
Examples
Some Supported Envs
LOG_LEVEL
DOMAIN
LISTEN
PLUGIN_DIRECTORY
YAML arg example
Expand
```yaml args: - --device - | name: serial groups: - paths: - path: /dev/ttyUSB* - paths: - path: /dev/ttyACM* - paths: - path: /dev/tty.usb* - paths: - path: /dev/cu.* - paths: - path: /dev/cuaU* - paths: - path: /dev/rfcomm* - --device - | name: video groups: - paths: - path: /dev/video0 - --device - | name: fuse groups: - count: 10 paths: - path: /dev/fuse - --device - | name: audio groups: - count: 10 paths: - path: /dev/snd - --device - | name: capture groups: - paths: - path: /dev/snd/controlC0 - path: /dev/snd/pcmC0D0c - paths: - path: /dev/snd/controlC1 mountPath: /dev/snd/controlC0 - path: /dev/snd/pcmC1D0c mountPath: /dev/snd/pcmC0D0c - paths: - path: /dev/snd/controlC2 mountPath: /dev/snd/controlC0 - path: /dev/snd/pcmC2D0c mountPath: /dev/snd/pcmC0D0c - paths: - path: /dev/snd/controlC3 mountPath: /dev/snd/controlC0 - path: /dev/snd/pcmC3D0c mountPath: /dev/snd/pcmC0D0c ```config.yaml
ExampleExpand
```yaml devices: - name: serial groups: - paths: - path: /dev/ttyUSB* - paths: - path: /dev/ttyACM* - paths: - path: /dev/tty.usb* - paths: - path: /dev/cu.* - paths: - path: /dev/cuaU* - paths: - path: /dev/rfcomm* - name: video groups: - paths: - path: /dev/video0 - name: fuse groups: - count: 10 paths: - path: /dev/fuse - name: audio groups: - count: 10 paths: - path: /dev/snd - name: capture groups: - paths: - path: /dev/snd/controlC0 - path: /dev/snd/pcmC0D0c - paths: - path: /dev/snd/controlC1 mountPath: /dev/snd/controlC0 - path: /dev/snd/pcmC1D0c mountPath: /dev/snd/pcmC0D0c - paths: - path: /dev/snd/controlC2 mountPath: /dev/snd/controlC0 - path: /dev/snd/pcmC2D0c mountPath: /dev/snd/pcmC0D0c - paths: - path: /dev/snd/controlC3 mountPath: /dev/snd/controlC0 - path: /dev/snd/pcmC3D0c mountPath: /dev/snd/pcmC0D0c ```