Add API for CDI --devices flag in Docker and Podman for mapping GPUs #3290
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import ( | |
| "regexp" | ||
| "strings" | ||
|
|
||
| "github.com/container-orchestrated-devices/container-device-interface/pkg/parser" | ||
| "sigs.k8s.io/kind/pkg/errors" | ||
| "sigs.k8s.io/kind/pkg/internal/sets" | ||
| ) | ||
|
|
@@ -114,6 +115,10 @@ func (n *Node) Validate() error { | |
| errs = append(errs, errors.New("image is a required field")) | ||
| } | ||
|
|
||
| if err := validateDevices(n.CDIDevices); err != nil { | ||
| errs = append(errs, errors.Wrapf(err, "invalid devices")) | ||
| } | ||
|
|
||
| // validate extra port forwards | ||
| for _, mapping := range n.ExtraPortMappings { | ||
| if err := validatePort(mapping.HostPort); err != nil { | ||
|
|
@@ -192,6 +197,23 @@ func validatePortMappings(portMappings []PortMapping) error { | |
| return nil | ||
| } | ||
|
|
||
| func validateDevices(cdiDevices []string) error { | ||
| for _, device := range cdiDevices { | ||
| device := strings.TrimSpace(device) | ||
|
There was a problem hiding this comment. You should be able to use It should also be fine to allow There was a problem hiding this comment. Please update to what Evan suggests to reduce the dependencies pulled in. There was a problem hiding this comment. @BenTheElder What do you think? There was a problem hiding this comment. @klueska I am using parser.ParseQualifiedName(device) instead of parser.ParseQualifiedName(device). The results are the same except you get the additional error message about why your device string is invalid. I think this is preferable. ParseQualifiedName() is called in IsQualifiedName(). This doesn't change the dependencies at all. There are tests on github.com/container-orchestrated-devices/container-device-interface/pkg/parser pulling in the additional packages. Maybe we should put the tests in a different package? @elezar wdyt? There was a problem hiding this comment. I can update the tests in that package to use a different import for comparison or to be in the There was a problem hiding this comment. I don't personally have a preference - I haven't found any with fewer dependencies. I was thinking use the There was a problem hiding this comment. I have created cncf-tags/container-device-interface#149 with an update to the test package. This should prevent There was a problem hiding this comment. Sorry, a few things came up back in the real world and here with #3277 etc., the dependency discussion wound up forked in https://github.com/kubernetes-sigs/kind/pull/3290/files#r1304873501 I'm going to try to get #3335 after which when this PR is rebased and See linked comment for stance on deps. There was a problem hiding this comment. Currently for example we have a lot of CRI inspired types with in-tree dependency-free code instead, to avoid creating dep hell for our users. That may not be so reasonable here. |
||
| // validate device string is not empty | ||
| if len(device) == 0 { | ||
| return errors.Errorf("invalid device string: '%v'. Empty Strings not allowed", device) | ||
| } | ||
|
|
||
| // validate device string is valid | ||
| _, _, _, err := parser.ParseQualifiedName(device) | ||
| if err != nil { | ||
| return errors.Errorf("invalid device string: '%v'. %v", device, err) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func validatePort(port int32) error { | ||
| // NOTE: -1 is a special value for auto-selecting the port in the container | ||
| // backend where possible as opposed to in kind itself. | ||
|
|
||
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.
regarding dependency management, I'm working on #3335 to switch the modules to 1.17+, which will enable https://go.dev/ref/mod#graph-pruning and make the transitive dependencies clear.
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.
just glancing at the go.sum changes, there's a concerningly large set of deps, but we can't tell with pre 1.17 go.mod files what is actually in the binary.
It remains important that
kindthe go libraries and CLI are trivially embeddable with minimal dependencies which also must meet kubernetes / CNCF's (allowed licenses) dependency standards. We have a number of users using kind directly in test tools etc and we don't want to bring in more dependencies.https://github.com/cncf-tags/container-device-interface/blob/main/go.mod has a lot of deps for what I would expect to just be a spec/parser.