-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Use intel-gpu-plugin with intel-gpu-fakedev generated devices #1118
base: main
Are you sure you want to change the base?
Conversation
Updated the kustomize change to pass CI test. If kustomize cannot be made to work (order init containers as needed) that change (along with first 2 commits) is redundant. |
How the new YAMLs work: Fake device generator (with suitable fake configuration file) is run as the first init container, and (existing) NFD hook init container produces NFD feature file instead installing hook binary to host. Sysfs and devfs content created by the fake device generator is mounted both to NFD hook init container and GPU plugin container. Devfs needs to be a real host directory so that container runtime can mount the assigned (fake) device file to a (fake) workload, and workload can then report to test runner which device was assigned to it. Because current GPU plugin + container runtime require host and GPU plugin container paths to match, and container runtime does not allow creating files to regular sysfs/devfs paths, different paths need to be used for the fake ones. |
If kustomize init container order cannot be forced so that fake device generator runs first, init container writing NFD features file could be changed into a regular container running forever (that's what needs to be done eventually any way, when NFD drops hook support). So, the remaining question is which alternative device-plugin project wants... Fake device example (for scalability testing) as an overlay for GPU plugin kustomize directory, or as its own gpu_fakedev/ kustomize directory? |
fa7cfa7
to
eeb989d
Compare
Two months have passed. Any comments on this? |
Additional month passed. Any comments? Note: |
is it still WIP? |
It's waiting for you (I mean reviewers) to comment on which of the two implemented alternatives you'd rather have; it being a GPU plugin overlay, or its own "gpu_fakedev" kustomize directory? I can then drop the WIP prefix and other alternative. |
Oh OK, I've not looked at it at all since it says WIP :-) |
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.
+1 for the overlay base approach
@@ -0,0 +1,15 @@ | |||
apiVersion: kustomize.config.k8s.io/v1beta1 | |||
kind: Kustomization |
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.
kind: Kustomization | |
kind: Kustomization | |
nameSuffix: -fake |
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.
@mythi I'm not really sure about adding -fake suffix to all objects: service, serviceAccount, clusterRole, clusterRoleBinding, configMap, daemonSet.
I think it would be rare to run both fake and real GPU plugin at the same time in the same cluster, but even if you would:
- Only daemonSet would need a different name
- Generator configMap is unique to fake device plugin deployment, so it does not need a new name
- Rest are fine to be shared between real and fake GPU plugins
What if it would use a different namespace instead?
E.g. if one would want operator to support also fake plugin, wouldn't different namespace be easier for that, than every object having a different name?
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 added the suggestion since the "standalone" daemonset was named with that -fake
. I'm OK not to use this suggestion if you think it make more sense as it is.
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.
Is there a way to change just the deployment name with kustomize?
Even if one would use separate namespace for the whole thing, it may still make sense to have -fake
suffix for the deployment name, just to make sure nobody confuses it with the real thing e.g. in kubectl get pods -A
output.
In my own fake GPU plugin tests, I've used "validation" namespace for it, but something like "fake-validation" would make it clearer that it's not the real thing...
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.
on that namespace topic, one idea could be to add namespace: somefake
into kustomization.yaml
Adding new kustomization (base) directory for this is (a bit simpler) alternative than kustomizing existing gpu_plugin/ base, which requires changing current initcontainer to a run-time one, and adding init container for fake device generator. See: intel#1118 Signed-off-by: Eero Tamminen <[email protected]>
eeb989d
to
7e7c54d
Compare
@mythi Ok, I split the changes for a new Merged the remaining commits to 2 and rebased it to latest Except for the dropped large commit (+ a typo fix), content is same as earlier. EDIT: Pros for standalone daemonset:
Pros for kustomize overlay:
|
Any comments on the |
Adding new kustomization (base) directory for this is (a bit simpler) alternative than kustomizing existing gpu_plugin/ base, which requires changing current initcontainer to a run-time one, and adding init container for fake device generator. See: intel#1118 Signed-off-by: Eero Tamminen <[email protected]>
It's OK like this. |
Adding new kustomization (base) directory for this is (a bit simpler) alternative than kustomizing existing gpu_plugin/ base, which requires changing current initcontainer to a run-time one, and adding init container for fake device generator. See: intel#1118 Signed-off-by: Eero Tamminen <[email protected]>
Change GPU plugin NFD init container to run-time container: * To work around kustomize inability to enforce correct init container order * This is more likely how things will work once NFD drops support for hooks: kubernetes-sigs/node-feature-discovery#856 Signed-off-by: Eero Tamminen <[email protected]>
7e7c54d
to
da4fb39
Compare
Rebased this to main to drop merged #1327. I will do more testing on this & remove WIP after "intel/intel-gpu-fakedev" image referred in kustomize file is available. |
do we need to wait for the image? |
IMHO there's not much point in merging kustomize files that rely on non-existing image. Users are unlikely to do their own builds and kustomize the kustomize in this PR to point to their own registry... PS. If this waits long enough, NFD will drop support for hooks, and GPU plugin needs to adapt to that. That would simplify this PR. Whereas if NFD moves further, from feature files to CRDs, more changes may be needed. |
We have several deployments in the repo without having the images available so this would not be an exception. I don't mind keeping this open but I think it's OK to merge as well. |
NFD & GPU plugin have moved from hooks to feature files, so this needs to be updated. I'm not sure when I'll have time for that though. |
This PR depends on #1114 + #1116 PRs being merged first.
It offers two alternatives for integrating support for installing GPU device plugin to a cluster with fake device support. Either by adding separate
gpu_fakedev
YAML directory which configures GPU plugin differently from start, or by providing kustomization overlay for existing GPU plugin YAML files.(Once there's consensus which approach is better, I'll remove the other one.)
I'd prefer adding just an overlay for existing GPU plugin YAMLs, but I have not been able to force kustomize to order init containers as needed. Any advice?
PS. The whole picture and initial review comments are in the RFC PR #1104, from which this is split off.