Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Allow CNI caller to supply blank netns; upgrade cni library to version 0.5.0 #2850

Merged
merged 4 commits into from
Mar 16, 2017

Conversation

bboreham
Copy link
Contributor

Prompted by the impending release of Kubernetes 1.6, which seems to rely on the behaviour that a blank netns is allowed on DEL.

Lots of lines changed, because the CNI return format changed. The library should take care of returning backwards-compatible results to older runtimes.

Also don't error on DEL with no namespace and no addresses
CNI_VERSION env var was never used, so removed from the test script.
The second test is crashing on v1, so bumped to v3.
@jbeda
Copy link

jbeda commented Mar 15, 2017

I think one problem is that there is no cniVersion member in the weave configuration:

cat /etc/cni/net.d/10-weave.conf
{
    "name": "weave",
    "type": "weave-net"
}

I suspect (no confirmation) that the runtime then assumes the latest version. It might be that setting "cniVersion": "0.2.0" there fixes things.

@bboreham
Copy link
Contributor Author

That's an interesting idea, but the CNI spec says that an absence of version shall be interpreted as version 0.1, and the currently-released Weave Net plugin is at CNI spec level 0.1.

As discussed on sig-network Slack, kubelet needs to be prepared for the DEL to error, and move on past that operation at some point.

@marccarre marccarre self-requested a review March 16, 2017 09:50
@@ -70,28 +77,31 @@ func (c *CNIPlugin) CmdAdd(args *skel.CmdArgs) error {
if err != nil {
return fmt.Errorf("unable to allocate IP address: %s", err)
}
// Only expecting one address
ip := result.IPs[0]

This comment was marked as abuse.

This comment was marked as abuse.

@luxas
Copy link
Contributor

luxas commented Mar 16, 2017

I can confirm that this fixes the weave-kube problem with v1.6.
However, we should bump the version of weave-plugin so the new plugin binary will be used; when I tested this I had to manually remove the current weave CNI plugin binary in order to get the new one deployed

@bboreham
Copy link
Contributor Author

Thanks @luxas; we will make a new release soon which will have a higher version number.

@marccarre marccarre merged commit 31b7e06 into 1.9 Mar 16, 2017
@marccarre marccarre deleted the upgrade-cni branch March 16, 2017 18:23
@luxas
Copy link
Contributor

luxas commented Mar 16, 2017

Was this merged into master as well?
Is there any reason to not fix this on master as well?
I'd like to be able to use the CI-built (:latest) version to actually test it out and make sure it's working from official builds

@bboreham
Copy link
Contributor Author

@luxas merged to master now; sorry for oversight

However the CI build flaked first time

@bboreham bboreham changed the title Upgrade cni library to version 0.5.0 Allow CNI caller to supply blank netns; upgrade cni library to version 0.5.0 Mar 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants