implement NRI plugin server to inject management CDI devices #1498
Conversation
tariq1890
force-pushed
the
nri-plugin-server
branch
12 times, most recently
from
933aa39 to
446d2f0
Compare
tariq1890
force-pushed
the
nri-plugin-server
branch
11 times, most recently
from
94a00c8 to
cbd8cf9
Compare
tariq1890
force-pushed
the
nri-plugin-server
branch
from
cbd8cf9 to
96b61e9
Compare
|
|
||
| for _, name := range devices { | ||
| a.AddCDIDevice( | ||
| &api.CDIDevice{ |
There was a problem hiding this comment.
As far as I am aware, this introduces restrictions on compatible containerd / cri-o versions.
There was a problem hiding this comment.
Given that we've moved to native CDI, the minimum supported containerd is now v1.7. Will that be affected by this change?
There was a problem hiding this comment.
We would need to check when CDI Devices were added to the NRI APIs. The versions of containerd / cri-o that then started responding to these fields would be the minimum versions.
There was a problem hiding this comment.
Here was the NRI commit: containerd/nri@c9b4798
Tagged as v0.7.0.
We would also need to check which version of containerd includes the related adjustments. (see https://github.com/containerd/containerd/blob/6dc89d8b392dbea8651ab0b141765f196836205c/internal/cri/nri/nri_api_linux.go#L321-L326).
(I wasn't able to find similar code in v1.7 ir v2.0).
There was a problem hiding this comment.
@elezar CDI injection from an NRI plugin does not work in 1.7.x. I have a fix for it which I never filed a PR from. But if it's important enough for you guys, I can file it and then see if we could get it merged.
There was a problem hiding this comment.
@elezar Here is the filed PR against the 1.7 maintenance branch: containerd/containerd#12650
There was a problem hiding this comment.
@elezar And here is one filed against the 2.0 maintenance branch: containerd/containerd#12651
There was a problem hiding this comment.
@elezar @tariq1890 Not sure how realistic is to get containerd/containerd#12651 in. 2.0 is EOL'd. 12651 would require a new tagged release, and if that is an absolute no-go (it's usually not that inflexible), then it's a tough call. Anyway, you might want to chime in there and comment.
There was a problem hiding this comment.
@elezar @tariq1890 Not sure how realistic is to get containerd/containerd#12651 in. 2.0 is EOL'd. 12651 would require a new tagged release, and if that is an absolute no-go (it's usually not that inflexible), then it's a tough call. Anyway, you might want to chime in there and comment.
The v1.7 version of the fix has been now commented as being considered for (a late) inclusion in the upcoming v1.30 release. You might want to chime in there as well.
tariq1890
force-pushed
the
nri-plugin-server
branch
3 times, most recently
from
d593d8b to
13b67bf
Compare
| } | ||
|
|
||
| func getAnnotation(annotations map[string]string, key, ctr string) string { | ||
| nriPluginAnnotationKey := fmt.Sprintf("%s/container.%s", key, ctr) |
There was a problem hiding this comment.
I know that the original example allowed all containers in a pod to automatically have access to devices using a different key. Here, I understand that we're only allowing a SINGLE form, correct?
There was a problem hiding this comment.
Yes, correct we are only allowing a single form. Please let me know if you have any concerns
tariq1890
force-pushed
the
nri-plugin-server
branch
3 times, most recently
from
4e6d0c5 to
a1227ab
Compare
Signed-off-by: Tariq Ibrahim <[email protected]>
tariq1890
force-pushed
the
nri-plugin-server
branch
from
a1227ab to
a4273d0
Compare
No description provided.