Add additional functionality to the mock dgxa100 server #114
Conversation
pkg/nvml/mock/dgxa100/dgxa100.go
Outdated
| }, | ||
| }, | ||
| } | ||
|
|
||
| func New() nvml.Interface { |
There was a problem hiding this comment.
Question: Does using the generated mocks add value here?
It means that we don't have to implement an exhaustive interface and can rely on the stubbing by moq (or allow it to panic if an unimplemneted function is called).
There was a problem hiding this comment.
I'm not sure I follow. The types in this package extend the generated mock types for exactly this purpose. I only overwrite the methods I care about, and the rest are left as stubs that panic.
There was a problem hiding this comment.
It's actually how figured out which ones I needed to implement -- those that were missing were panicking via the moq stubs.
There was a problem hiding this comment.
One thing to note is that this breaks information that we get from the mock such as the call history -- number of calls and the parameters.
The "Correct" way to handle this is to implement the funcs as required. See for example: https://github.com/NVIDIA/go-gpuallocator/blob/82d8924b3d40c7d31cc18d56e65b3bffe59d91fd/gpuallocator/device_test.go#L28-L52
There was a problem hiding this comment.
Good point. Updated throughout.
Signed-off-by: Kevin Klues <[email protected]>
For now this only holds GI placement information with placeholders for the CI placement informatin. It should need to be extended to hold CI placement information in the future. Signed-off-by: Kevin Klues <[email protected]>
Signed-off-by: Kevin Klues <[email protected]>
Signed-off-by: Kevin Klues <[email protected]>
klueska
force-pushed
the
add-functionality-to-mock-dgxa100
branch
from
bc76c78 to
37bdc54
Compare
| DriverVersion: "550.54.15", | ||
| NvmlVersion: "12.550.54.15", | ||
| CudaDriverVersion: 12040, |
There was a problem hiding this comment.
| DriverVersion: "550.54.15", | |
| NvmlVersion: "12.550.54.15", | |
| CudaDriverVersion: 12040, | |
| SystemGetDriverVersionFunc: func() (string, nvml.Return) { | |
| return "550.54.15", nvml.SUCCESS | |
| }, | |
| SystemGetNvmlVersionFunc: func() (string, nvml.Return) { | |
| return "12.550.54.15", nvml.SUCCESS | |
| }, | |
| SystemGetCudaDriverVersionFunc: func() (int, nvml.Return) { | |
| return 12040, nvml.SUCCESS | |
| }, |
Signed-off-by: Kevin Klues <[email protected]>
There was a problem hiding this comment.
Looks good. Let's get these in and iterate if we need anything else.
One thought ... if we return mock instances from the constructors instead of nvml.Interface and nvml.Device a caller can add / override mocks as required without having to modify the "upstream" code.
Probably out of scope for this PR though.
|
No, I think that makes sense. It will still implement the interface (so we can assign it to the interface in the calling code), but it can also be extended if desired from the calling code (which the current implementation doesn't allow). Let me add that in this PR. |
This way the callers can extend these types to futher override their functions if desired, while still being able to assign them to the appropriate nvml interfaces. Signed-off-by: Kevin Klues <[email protected]>
|
OK, updated with that change. I also vendored it into mig-parted and tested it to make sure it didn't break anything by doing this. |
Signed-off-by: Kevin Klues <[email protected]>
|
Added one more commit to introduce locks to protect from concurrent reads/writes of maps |
No description provided.