-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
implement GC keyword for CNI spec 1.1 #1022
Conversation
7454f04
to
2dc1cbe
Compare
571cedb
to
11bbb59
Compare
8435d87
to
91c9496
Compare
@@ -256,15 +268,15 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error, | |||
if err != nil { | |||
return types.NewError(types.ErrDecodingFailure, err.Error(), "") | |||
} else if gtet { | |||
if err := t.checkVersionAndCall(cmdArgs, versionInfo, cmdCheck); err != nil { | |||
if err := t.checkVersionAndCall(cmdArgs, versionInfo, funcs.Check); err != nil { |
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.
This requires func.Check
must not be nil (otherwise nil pointer reference happen). Should we add validate all func.{Add,Del,Check,GC} in initiliazation (or support 'nil' to mention unsupported)
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.
This patch added the nil reference check here.
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.
Chatted with @dcbw for a bit, and we both agreed we want to see just one more thing: can you somehow expand the debug plugin so we can determine the containerID and ifname that is being passed to the DEL?
After that we can merge this. Thanks!
I'm wondering that plugin's GC might be causes several race conditions: For example:
In addition, we also have another example:
To simplify plugin code, both should be blocked (i.e. prevent to invoke), I suppose and it should be implemented common code, in libcni. What do you think about that? |
The GC spec explicitly called out those race conditions. The proposed mechanism is letting the runtimes to serialize the operations. |
Signed-off-by: Casey Callendrello <c1@caseyc.net>
Signed-off-by: Casey Callendrello <c1@caseyc.net>
Signed-off-by: Casey Callendrello <c1@caseyc.net>
Signed-off-by: Henry Wang <henwang@amazon.com>
This PR implements the
GC
keyword inlibcni
for CNI spec 1.1Fixes: #974