Skip to content

Conversation

@Iceber
Copy link
Member

@Iceber Iceber commented Apr 1, 2025

issue: #153

  • Delete ./examples and ./skel. If users really need them, they can use the NRI package from the old tag.
  • Move ./README-0.1.0.md, ./types, and ./client-go to ./plugins/v010-adapter.
  • Removed the Background section from the README.md. It primarily described the differences compared to NRI v0.1.0.

@klihub klihub requested review from klihub and mikebrow and removed request for klihub April 4, 2025 07:54
[containerd](https://github.com/containerd/containerd) and
[CRI-O](https://github.com/cri-o/cri-o).

## Background
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still keep a revisited short 'Background' or 'Introduction' section which briefly tells in a few sentences what NRI is ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we have the Goal section. Maybe that's good enough then.

Copy link
Member

@klihub klihub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good just a few more changes I think

@@ -1,110 +0,0 @@
# nri - Node Resource Interface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leave a short description and a hyper link reference to the new 0.1.0/readme

e.g.
original v0.1.0 CLI invoke style version of NRI has been moved
v0.1.0 docs

[containerd](https://github.com/containerd/containerd) and
[CRI-O](https://github.com/cri-o/cri-o).

## Background
Copy link
Member

@mikebrow mikebrow Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave the background section in, remove the first paragraph .. move the second v0.1.0 paragraph to the new 0.1.0 readme.. and revise the 3rd paragraph with a more forward presence.. vs the change to now perspective.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adjusting the third paragraph of the reservation to read as follows:

## Background
Plugins in NRI are daemon-like entities. A single instance of a plugin is responsible for handling the full stream of NRI events and requests. A unix-domain socket is used as the transport for communication. 

NRI is defined as a formal, protobuf-based 'NRI plugin protocol' which is compiled into ttRPC bindings. This should result in improved communication efficiency with lower per-message overhead, and enable straightforward implementation of stateful NRI plugins.

Since the Background section no longer contains content related to 0.1.0, the section name seems a bit odd. It reads more like a description of the Plugin.
Do you think it would be better to either make it a subsection within the Goal section or just move it directly into the Goal section?
eg.

## Goal
....

### Plugins
Plugins in NRI are daemon-like entities. A single instance of a plugin is responsible for handling the full stream of NRI events and requests. A unix-domain socket is used as the transport for communication. 
...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both options work for me :-)

@samuelkarp
Copy link
Member

@Iceber Do you plan to address @mikebrow's comments?

@Iceber Iceber force-pushed the move_nri_0.1.0 branch 3 times, most recently from ada414f to d1f8d2b Compare May 29, 2025 10:21
@Iceber
Copy link
Member Author

Iceber commented May 29, 2025

@mikebrow @samuelkarp I am sorry for not handling it in a timely manner.

I have now updated it, please take a look.

@samuelkarp samuelkarp requested a review from mikebrow May 29, 2025 23:49
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikebrow mikebrow merged commit 6120e63 into containerd:main May 30, 2025
9 checks passed
@samuelkarp
Copy link
Member

FYI I opened #176 to revert this; turns out there's still some usage of this inside containerd that we'll probably want to figure out a deprecation/removal path for before actually removing the old NRI client code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants