-
Notifications
You must be signed in to change notification settings - Fork 208
FPGA: support CDI #1745
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
FPGA: support CDI #1745
Conversation
03c7322
to
e81ad2a
Compare
37bfcf3
to
20e8676
Compare
20e8676
to
d1e5597
Compare
d1e5597
to
1fa557e
Compare
@bart0sh thanks! quick question: I believe the word |
Thanks for pointing out! I didn't know that. Will try to replace. |
@mythi renamed in the code. Will update docs if/when all tests pass. |
`prestart` hook is marked as deprecated in the OCI runtime spec: https://github.com/opencontainers/runtime-spec/blob/main/config.md#posix-platform-hooks Renamed `prestart` to the `createRuntime` as suggested in the spec. Replaced `CDI hook` with `OCI hook` to be more clear. CDI is just a way to update OCI config and theoretically there is no such thing as CDI hook.
cea73ac
to
e58369e
Compare
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.
@bart0sh thanks so much! Only one thing I'm not fully sure: the API change for NewDeviceInfo()
. IIRC with topology info, we added NewDeviceInfoWithTopologyHints()
when we needed to allow user provided hints. Would NewDeviceInfoWithCDI()
make any sense?
@tkatila Can you review this PR please? |
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.
Looks good.
As this change removes the old prestart hook approach, should the README note that if one still wants to use that way, he or she must use <=0.30.0 version of the FPGA components?
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.
LGTM
New approach designed to be a replacement for the old one. It shouldn't require any configuration or other changes comparing to the previous one. I'd consider it as an internal change and wouldn't add anything to the README as it might confuse users. However, if you think it's needed, I'd be happy to do that. Just let me know. |
In that case, I don't think it's required. |
@uniemimu and/or @hj-johannes-lee can you review this? Seems to require one more approval. |
@bart0sh has it also been unconditionally enabled in kubelet since it was added? |
@mythi yes, it's graduated to Beta in Kubernetes in 1.29 and since then it's enabled by default. |
In section "Configuring CRI runtimes" we add some comments how the feature is available. We don't say much about kubelet itself. In theory there's a gap but I don't think it's important at all since by the time we release this, the oldest k8s we "support" is 1.29 which has it enabled. |
Nothing from my side. Good to go. |
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.
Generally looks ok, one small improvement that can be added: in documentation part, write down explicitly requirements for CDI mode: k8s,containerd/cri-o.
CRI-O and Containerd are mentioned here. Would it make sense to add version info for CRI-O, Containerd and Kubernetes there? They only make sense for the hook, so it looks like a good place for me. WDYT? |
there or in overall FPGA plugin README, stating that programming mode is available for systems with CDI enabled, which means k8s+containerd/cri-o of not less than.... |
This PR uses CDI support for Device Plugins to implement FPGA programming hooks.
CRI-O hooks are no longer needed and removed.
As CDI is currently supported by CRI-O and Containerd, this PR enables FPGA orchestration programmed operation mode for both most used CRI runtimes: CRI-O and Containerd. Previously it was only supported by CRI-O.
Ref: #1457