[CDI Hooks] add ability to specify OCI hook type#1907
Conversation
tariq1890
force-pushed
the
set-oci-hook-type
branch
from
c7ac514 to
d753b7e
Compare
tariq1890
force-pushed
the
set-oci-hook-type
branch
2 times, most recently
from
643879b to
dfad56e
Compare
Coverage Report for CI Build 28810161382Coverage increased (+0.009%) to 43.765%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
tariq1890
force-pushed
the
set-oci-hook-type
branch
from
dfad56e to
d6cca5e
Compare
| // This hook is responsible for setting up CUDA compatibility in the container and depends on the host driver version. | ||
| func NewCUDACompatHookDiscoverer(logger logger.Interface, hookCreator HookCreator, o *EnableCUDACompatHookOptions) Discover { | ||
| return hookCreator.Create(EnableCudaCompatHook, o.args()...) | ||
| return hookCreator.Create(OCIHookTypeCreateContainer, EnableCudaCompatHook, o.args()...) |
There was a problem hiding this comment.
Since we're changing the API anyway, what about rather changing the type of the first argument to accept a Hook type that would give us the lifecycle AND the name?
type Hook interface {
Type() HookType
Name() HookName
}
For the default hooks we could have HookName implement this interface:
func (h HookName) Type() HookType {
return OCIHookTypeCreateContainer
}
func (h HookName) Name() HookName {
return h
}
There was a problem hiding this comment.
I've updated the PR without the API changes now
| } | ||
|
|
||
| hook := d.hookCreator.Create("create-symlinks", d.deviceLinks...) | ||
| hook := d.hookCreator.Create(discover.OCIHookTypeCreateContainer, "create-symlinks", d.deviceLinks...) |
There was a problem hiding this comment.
My concern is that the "create-symlink" hook (and others) MUST -- as it's currently designed -- run as a createContainer hook. Splitting these arguments seems to indicate that there is more flexibility than we actually have.
There was a problem hiding this comment.
My motivation for this change is to make possible the addition of another CDI hook which will run as createRuntime.
The alternative to this that I can think of is adding a switch-case block within the method to select the OCI hook type based on the CDI hook name. Right now, only createcontainer will get selected of course.
The drawback here is that this approach would not work well for CDI hooks which could be run at different stages. But I am not sure if this is a legitimate scenario that we even want to address.
There was a problem hiding this comment.
I share Evan's concern as well. Having the caller set the hook type when currently there is only one "right" choice doesn't feel right to me.
The alternative to this that I can think of is adding a switch-case block within the method to select the OCI hook type based on the CDI hook name. Right now, only createcontainer will get selected of course.
I would prefer this. We could always make this more flexible in the future if a use case arises for it.
There was a problem hiding this comment.
@cdesiniotis @elezar I've made the changes without modifying the interface
tariq1890
force-pushed
the
set-oci-hook-type
branch
3 times, most recently
from
3a80cdb to
2e49c1c
Compare
Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
tariq1890
force-pushed
the
set-oci-hook-type
branch
from
2e49c1c to
ed1161d
Compare
|
|
||
| func (c cdiHookCreator) getOCIHookType(name HookName) OCIHookType { | ||
| switch name { | ||
| case CreateSymlinksHook, ChmodHook, DisableDeviceNodeModificationHook, EnableCudaCompatHook, UpdateLDCacheHook: |
There was a problem hiding this comment.
I've created the method intentionally this way for readability's sake
|
Thanks for the review @elezar @cdesiniotis ! |
4 participants
This PR makes it possible to specify a desired OCI hook type should we want the CDI spec generator to create OCI hooks that run in stages other than
createContainer.