pkg/nri: pull in latest NRI, update NRI configuration. #8140
Conversation
klihub
force-pushed
the
devel/update-nri-config
branch
from
832e431 to
f522269
Compare
mikebrow
left a comment
mikebrow
left a comment
There was a problem hiding this comment.
see comments ..
we might need some extra description to cover how one would need to change the defaults to avoid conflicts with other installs of containerd/crio that are using the NRI plugin defaults..
| SocketPath string `toml:"socket_path" json:"socketPath"` | ||
| // PluginPath is the path to search for NRI plugins to launch on startup. | ||
| PluginPath string `toml:"plugin_path" json:"pluginPath"` | ||
| // PluginConfigPath is the path to search for plugin-specific configuration. |
There was a problem hiding this comment.
need to copy the field descriptions for these NRI options to the containerd config toml description.. above in NRI.md
There was a problem hiding this comment.
Updated the sample configuration fragment adding similar comments that explain the purpose of each field. Added a note about the common default NRI socket path preventing multiple NRI-enabled runtimes or runtime instances running on a single node with the same default socket config.
| SocketPath: nri.DefaultSocketPath, | ||
| PluginPath: nri.DefaultPluginPath, | ||
| Disable: true, | ||
| SocketPath: nri.DefaultSocketPath, |
There was a problem hiding this comment.
is odd to be retrieving containerd config.toml defaults values from constants in a vendor'd package.. should these config defaults be specified here in the plugin?
There was a problem hiding this comment.
We considered the (well-known default) socket path to be part of the (runtime-agnostic) NRI API, so we defined it on the NRI side of the fence and made the configurable NRI socket path default to that in both NRI-enabled runtimes.
I think the alternative would have been (and still is) to instead co-locate the NRI socket with the runtime one in the runtime-specific directory and then take on the NRI side the cri-toolsish approach of probing sockets in known runtime-specific locations (unless specifically told otherwise) until one is found that accepts connections. However, if we jump to the NRI side of the fence, then this does not feel right. ATM NRI is runtime agnostic and is unaware of any runtime-specific details. It is the runtimes that already know about NRI.
No too strong feelings/opinion leaning in either direction. Just trying to provide some background how we ended up opting for the current choice...
klihub
force-pushed
the
devel/update-nri-config
branch
from
f522269 to
51a2ae2
Compare
mikebrow
left a comment
mikebrow
left a comment
There was a problem hiding this comment.
change looks good here.. would like to hold for containerd/nri#28 service fix.. and a release bump for containerd/nri for this breaking config change..
|
The PR has been merged. Waiting containerd/nri v0.3.x. |
|
@klihub https://github.com/containerd/nri/releases/tag/v0.3.0 done. Please update it. Thanks! |
Update NRI plugin configuration to match that of NRI. Remove option for the eliminated NRI configuration file. Add option to disable connections from externally launched plugins. Add options to override default plugin registration and request timeouts. Signed-off-by: Krisztian Litkey <[email protected]>
Update instructions for enabling NRI. Remove the now unnecessary step of creating an NRI configuration file. Add a note about the shared default NRI socket path preventing two NRI-enabled runtimes to be run with the default configuration on a single node. Remove a leftover/half sentence that slipped through in an earlier commit. Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/update-nri-config
branch
from
51a2ae2 to
2e9aaf0
Compare
Update fork-external/main with upstream main at commit [081d818](containerd@081d818) Marged upstream container/main into fork-external/main Related work items: containerd#7864, containerd#7954, containerd#8041, containerd#8044, containerd#8051, containerd#8062, containerd#8096, containerd#8103, containerd#8109, containerd#8110, containerd#8113, containerd#8114, containerd#8119, containerd#8120, containerd#8128, containerd#8130, containerd#8134, containerd#8140, containerd#8142, containerd#8143, containerd#8152, containerd#8154, containerd#8162, containerd#8164, containerd#8165, containerd#8172, containerd#8173, containerd#8177, containerd#8178, containerd#8181, containerd#8183, containerd#8187, containerd#8188, containerd#8189, containerd#8190, containerd#8191, containerd#8192, containerd#8193
5 participants
Update NRI dependency to pull in the latest NRI fixes. Update NRI configuration to match that of NRI itself. In particular,
Update NRI-related documentation to reflect these changes.