Merge Results from delegates

[ritusood]

Currently only Master Plugin result is returned by Multus.
This patch merges the results from all delegates as
per the CNI spec. This fixes Multus and Virtlet compatibility
issue.

Signed-off-by: Ritu Sood [email protected]

[coveralls]

Pull Request Test Coverage Report for Build 610

• 39 of 76 (51.32%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.2%) to 51.489%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
multus/multus.go	39	76	51.32%

Files with Coverage Reduction	New Missed Lines	%
multus/multus.go	1	51.02%

Totals
Change from base Build 607:	-0.2%
Covered Lines:	605
Relevant Lines:	1175

---

💛 - Coveralls

[rkamudhan]

@ritusood Could you please fix the multus_test.go as well. Unit test is failing in this. can you execute ./test.sh after your build and check it ?

[rkamudhan]

@dcbw @dougbtv @s1061123 : This is generic use case to make Multus to work with virtlet, I believe this patch should not break current working with Kubelet. @ritusood will work on the unit test. But I think this use case is valid. What you guys think of it ?

[s1061123]

@rkamudhan @ritusood , I'm not clear that the multus output with the PR.

Could you please show me some example output (with PR, but both are nice)?

[rkamudhan]

@ritusood @s1061123 @dougbtv I agreed to test the PR with Kubelet and also display the output of the CNI result.

[rkamudhan]

Fix the ./test.sh

[ritusood]

After the PR is applied sample Result in case of two interfaces:
{
"cniVersion": "0.4.0",
"interfaces": [
{
"name": "acb1b602fd06350",
"mac": "96:9e:1a:ce:aa:ea"
},
{
"name": "net0",
"mac": "0a:00:00:00:00:15",
"sandbox": "/proc/28686/ns/net"
}
],
"ips": [
{
"version": "4",
"interface": -1,
"address": "10.233.64.61/24",
"gateway": "10.233.64.1"
},
{
"version": "4",
"interface": 1,
"address": "172.16.24.3/24",
"gateway": "172.16.24.1"
}
],
"routes": [
{
"dst": "10.233.64.0/18",
"gw": "10.233.64.1"
}
],
"dns": {}
}

[dcbw]

@ritusood could you also paste in your multus.conf that shows which delegates get called to produce the merged output above?

[rkamudhan]

The PR is moved to backlog and will be revisited later, when PR owners is back.

[dcbw]

It looks like this PR will combine the results from all of the delegates, both from the Multus config file and from the Kubernetes Network Attachments. I don't think that's what we want to do, because all of these are additional "sidecar" networks that should not be reported to Kubernetes. I could be wrong, but it looks like that's what would happen with this PR.

@ritusood can you describe in more detail the issue that virtlet has with Multus that prompted this PR? Thanks!

[kannanvr]

Request you to merge this code changes as early as possible. This code change will solve our issue...
We want to create an multiple network interface for pod

[ritusood]

@dcbw This patch is needed for integrating Multus with Virtlet. Virtlet uses information returned from CNI plugin in form of https://github.com/containernetworking/cni/blob/master/SPEC.md#result and plugin result should contain a list of interfaces with data like ip addresses and routes.
Also this link has discussion with Virtlet team on integration of virtlet with Multus. Mirantis/virtlet#627

[dcbw]

Request you to merge this code changes as early as possible. This code change will solve our issue...
We want to create an multiple network interface for pod

@kannanvr Multus is already capable of creating multiple network interfaces per pod using individual delegates. I still don't understand the specific issue that you have, so it's not possible to merge the code until we figure out whether it's appropriate behavior for Multus.

This patch is needed for integrating Multus with Virtlet.

@ritusood I still don't see an explanation of why the PR combines the results from all delegates; if you could explain the reasoning behind that, and why it's required, then we can discuss my concerns about that behavior.

Let's continue the discussion and see if we can reach and understanding on the behavior changes. Thanks!

[ritusood]

@dcbw As I understand since CNI Spec 0.3.0 there is a mechanism to provide multiple interfaces and IP addresses in Result structure. https://github.com/containernetworking/cni/blob/master/SPEC.md#result.
Virtlet is using this result structure to get information on multiple interfaces from CNI and then passes that information to be able to create VM's with multiple interfaces.
CNI-Genie works with Virtlet and this PR is based on CNI-Genie's implementation. We have tested this PR and Kubernetes ignores all the interfaces except the first.
Do you see any issues with this approach?

[jellonek]

@dcbw the thing is that at the moment runtime calling multus through cni is receiving cni result from only single plugin (called "main"), while we (Virtlet) are expecting a description what was set up during whole ADD call. This PR is about to provide "merged" information in similar way how CNI Genie is already doing that.

At the moment simply Virtlet relies on full information what was done during ADD and providing an info only from one of plugins is simply misleading and not enough for us.

@ritusood btw. it's not "kubernetes" which is ignoring other than first interface, but a particular shim. In your case it is probably docker shim which afair ignores cni result at all, trying to get info about networking directly from configured namespace, instead of relying on info returned by cni plugin (what is kinda breaking of abstraction provided by CNI API). I don't recall how it's done in containerd or other CRI runtimes (e.g. CRI-O) but they probably are replicating the way chosen by dockershim developers.

[rkamudhan]

@ritusood @dcbw : Here is how, I tested the merge result PR.

Copy the Rebased Ritu's PR Multus binary in the /opt/cni/bin directory, then create a conf file

bash -c 'cat <<EOF > /opt/cni/bin/10-flannel.conf
{
  "name": "defaultnetwork",
  "type": "multus",
  "delegates": [
    {
      "cniVersion": "0.3.1",
      "name": "defaultnetwork",
      "type": "flannel",
      "isDefaultGateway": true
    },
    {
      "cniVersion": "0.3.1",
      "name": "secondarynetwork",
      "type": "ptp",
      "ipam": {
        "type": "host-local",
        "subnet": "10.168.1.0/24",
        "rangeStart": "10.168.1.11",
        "rangeEnd": "10.168.1.20",
        "routes": [
          {
            "dst": "0.0.0.0/0"
          }
        ],
        "gateway": "10.168.1.1"
      }
    }
  ],
  "confDir": "/etc/cni/multus/net.d"
}
EOF'

then as we do in CNI community, we test the CNI as follows:

# CNI_COMMAND=ADD CNI_CONTAINERID=1234567890 CNI_NETNS=/var/run/netns/1234567890 CNI_IFNAME=eth0 CNI_PATH=`pwd` ./multus <10-flannel.conf

{
    "cniVersion": "0.4.0",
    "interfaces": [
        {
            "name": "cni0",
            "mac": "0a:58:0a:f4:00:01"
        },
        {
            "name": "veth5e436074",
            "mac": "5a:1c:92:6e:a8:da"
        },
        {
            "name": "eth0",
            "mac": "0a:58:0a:f4:00:05",
            "sandbox": "/var/run/netns/1234567890"
        },
        {
            "name": "veth78da6f7b",
            "mac": "1a:f1:49:d9:c9:85"
        },
        {
            "name": "net1",
            "sandbox": "/var/run/netns/1234567890"
        }
    ],
    "ips": [
        {
            "version": "4",
            "interface": 2,
            "address": "10.244.0.5/24",
            "gateway": "10.244.0.1"
        },
        {
            "version": "4",
            "interface": 4,
            "address": "10.168.1.14/24",
            "gateway": "10.168.1.1"
        }
    ],
    "routes": [
        {
            "dst": "10.244.0.0/16",
            "gw": "10.244.0.1"
        },
        {
            "dst": "0.0.0.0/0",
            "gw": "10.168.1.1"
        }
    ],
    "dns": {}
}

[rkamudhan]

@ritusood @dcbw

Found some interesting investigation from the testing current package:

# cat 10-flannel.conf
{
  "cniVersion": "0.2.0",
  "name": "defaultnetwork",
  "type": "multus",
  "delegates": [
    {
      "cniVersion": "0.2.0",
      "name": "defaultnetwork",
      "type": "flannel",
      "isDefaultGateway": true
    },
    {
      "cniVersion": "0.2.0",
      "name": "secondarynetwork",
      "type": "ptp",
      "ipam": {
        "type": "host-local",
        "subnet": "10.168.1.0/24",
        "rangeStart": "10.168.1.11",
        "rangeEnd": "10.168.1.20",
        "routes": [
          {
            "dst": "0.0.0.0/0"
          }
        ],
        "gateway": "10.168.1.1"
      }
    }
  ],
  "confDir": "/etc/cni/multus/net.d"
}
[root@silpixa00397128 bin]# CNI_COMMAND=ADD CNI_CONTAINERID=0987654321 CNI_NETNS=/var/run/netns/0987654321 CNI_IFNAME=eth0 CNI_PATH=`pwd` ./multus-clean <10-flannel.conf
{
    "cniVersion": "0.2.0",
    "ip4": {
        "ip": "10.244.0.9/24",
        "gateway": "10.244.0.1",
        "routes": [
            {
                "dst": "10.244.0.0/16",
                "gw": "10.244.0.1"
            }
        ]
    },
    "dns": {}
}

No issues, with CNI version it is only 0.2.0

With the PRs, the result is as follows:

# CNI_COMMAND=ADD CNI_CONTAINERID=0987654321 CNI_NETNS=/var/run/netns/0987654321 CNI_IFNAME=eth0 CNI_PATH=`pwd` ./multus <10-flannel.conf
{
    "cniVersion": "0.4.0",
    "ips": [
        {
            "version": "4",
            "address": "10.244.0.11/24",
            "gateway": "10.244.0.1"
        },
        {
            "version": "4",
            "address": "10.168.1.20/24",
            "gateway": "10.168.1.1"
        }
    ],
    "routes": [
        {
            "dst": "10.244.0.0/16",
            "gw": "10.244.0.1"
        },
        {
            "dst": "0.0.0.0/0",
            "gw": "10.168.1.1"
        }
    ],
    "dns": {}
}

As the current package support 0.4.0. This having isses in the test, as we expect result to 020 type. #L230 #L322 #L401

Major problem is in-build code for Version()

[rkamudhan]

@ritusood @jellonek Is the Virtlet support with CNI comes only from v0.3.0 ? @dcbw Is the testing in this case to valid to test cni version 0.2.0 as in the testing #L401 .

@rkamudhan will create a PR for the unit testing

[jellonek]

So what is that existing functionality that doesn't expect this merging? It looks like it's expecting an output of only single cni plugin which in fact will be incorrect looking from perspective of CNI SPEC (output in Result should contain info about whole configured networking in container namespace).

I still don't understand why you are calling that as a technical debt, while this clearly is removing a... technical debt against the SPEC.

So I'm asking once again - is this colliding with kubevirt or anything else (openshift?) what is already using multus?
@dcbw can you share these mentioned implications?

[dcbw]

@dcbw the thing is that at the moment runtime calling multus through cni is receiving cni result from only single plugin (called "main"), while we (Virtlet) are expecting a description what was set up during whole ADD call. This PR is about to provide "merged" information in similar way how CNI Genie is already doing that.

At the moment simply Virtlet relies on full information what was done during ADD and providing an info only from one of plugins is simply misleading and not enough for us.

The largest issue is that Kubernetes assumes that the ADD call results in one network attachment, while this PR will return a result that is the combination of multiple network attachments, only one of which is the cluster-wide default network that Kubernetes normally attached. The NPWG specification explicitly attempts not to change Kubernetes default behavior, and while at the moment this is not technically changing behavior under Kube (because kubelet doesn't use the Result yet), it's certainly changing the semantics in a way that is not future-proof.

Kubernetes will soon get multiple IP address support but it is mainly for v4/v6 dual-stack and it will assume that IPs from the Result structure (once something like kubernetes/kubernetes#71653 lands) are from the same network attachment. When that happens, the combined Result from this PR will produce incorrect Kube results as the IPs in the combined Result are from multiple network attachments which are not guaranteed to be on the same data plane, which Kube requires.

If you really want a fully combined result, then I'd suggest either the config option Doug mentions, or proposing a change to Multus that stuffs the combined result into a vendor-namespaced key in the Result structure that Kubernetes will ignore. That way you keep compatibility with various specifications and don't confuse a near-future kubelet. Or something like that.

[rkamudhan]

I'd suggest either the config option Doug mentions, or proposing a change to Multus that stuffs the combined result into a vendor-namespaced key in the Result structure that Kubernetes will ignore.

@ritusood Moving forward, the suggestion is to introduce config option in multus, that could combine the results or it could be flag for the multus build that combine the result. I will sync up internally, to explain the options with you..

[jellonek]

@dcbw the thing is that multus is from the start a cni plugin multiplexer, so it's directly designed to attach on ADD container to multiple networks.

btw. Kubernetes have not clue about CNI and it's not calling the ADD - this is responsibility of runtime behind CRI.

Kubernetes does not care actually about result at all.

The whole misunderstanding lays in the lack of distinction between kubelet and dockershim (one of CRI implementations). It's based on the fact that dockershim is not yet extracted from kubelet sources, but in fact it is started as a separate, independent process. So from "kubernetes" perspective there is no such thing like CNI - kubelet is calling a runtime which does the job using or ignoring CNI result. kubelet only expects an ip address in the result of PodSandboxStatus CRI call. It's up to runtime (which do not need to be dockershim, it can be containerd, it can be cri-o, it can be virtlet) how it will configure the networking and if it will use CNI plugin - how to interpret (or ignore at all, what does dockershim) the cni plugin result.

Also please remember that Kubernetes (even if it's the biggest user of CNI) is not the direct target of CNI, so crippling a plugin (this one as example) to match a behavior expected by only single implementation of Kubernetes CRI (dockershim in this example) instead of making it fulfilling the CNI Specification - seems to be odd for me, especially when it comes from CNI maintainer...

Other thing is this assumption that multiple attachments should be guaranteed to be on the same data plane - why anyone would want such limitation? It's contradiction to how actual kubernetes users are using multiple networks, where only one is used to connect to kubernetes services - the one which is using the ip address known by kubelet (I'll repeat - kubelet, not dockershim).
If this near-future feature of is relying on an undocumented behavior of CNI, not included in specification it's in this case:
a) issue on CNI spec side, or
b) issue on how is prepared this feature.

Once again - please look on this from Specification perspective, which is NOT the expected by dockershim behavior perspective.

[dcbw]

@dcbw the thing is that multus is from the start a cni plugin multiplexer, so it's directly designed to attach on ADD container to multiple networks.

btw. Kubernetes have not clue about CNI and it's not calling the ADD - this is responsibility of runtime behind CRI.

Kubernetes does not care actually about result at all.

The whole misunderstanding lays in the lack of distinction between kubelet and dockershim (one of CRI implementations). It's based on the fact that dockershim is not yet extracted from kubelet sources, but in fact it is started as a separate, independent process. So from "kubernetes" perspective there is no such thing like CNI - kubelet is calling a runtime which does the job using or ignoring CNI result. kubelet only expects an ip address in the result of PodSandboxStatus CRI call. It's up to runtime (which do not need to be dockershim, it can be containerd, it can be cri-o, it can be virtlet) how it will configure the networking and if it will use CNI plugin - how to interpret (or ignore at all, what does dockershim) the cni plugin result.

@jellonek I'm aware of the history surrounding dockershim and Kubernetes. I'm also quite aware that other runtimes exist and I contribute to CRIO-O and dockershim upstream. I'm also quite aware that the NPWG specification that Multus is implementing is specifically targeted at Kubernetes, which despite the CRI situation is still not aware of multiple IP addresses or data planes. Kubernetes itself is still the driver, regardless of what the CRI itself does. And it does not expect separate data planes.

Also please remember that Kubernetes (even if it's the biggest user of CNI) is not the direct target of CNI, so crippling a plugin (this one as example) to match a behavior expected by only single implementation of Kubernetes CRI (dockershim in this example) instead of making it fulfilling the CNI Specification - seems to be odd for me, especially when it comes from CNI maintainer...

Nothing is being crippled. Two options were suggested that provides the functionality you desire.

Other thing is this assumption that multiple attachments should be guaranteed to be on the same data plane - why anyone would want such limitation? It's contradiction to how actual kubernetes users are using multiple networks, where only one is used to connect to kubernetes services - the one which is using the ip address known by kubelet (I'll repeat - kubelet, not dockershim).

Nobody is expecting the networks attached by Multus to be on the same data plane. But if you combine all the networks into the same Result then Kubernetes will think the IPs are on the same data plane if that Result is passed back to Kubernetes, which is a common thing to do. While dockershim doesn't look at the Result right now, that is going to happen very soon and combinations will break that.

If this near-future feature of is relying on an undocumented behavior of CNI, not included in specification it's in this case:
a) issue on CNI spec side, or
b) issue on how is prepared this feature.

This issue is not about CNI. This issue is about the Multus implementation of the NPWG specification which is inherently Kubernetes specific. And again, two solutions were proposed (a config option, or an additional combined result in the Result JSON dict that Virtlet would understand.

I honestly don't know why we're still discussing this. What is the problem with one of those two options?

[jellonek]

We are discussing that because even if PR is already accepted by @rkamudhan and ready to be merged it should be updated to confirm to one of these two options or at least should have a followup in this direction. Btw. that could be also interesting for @kshafiee (probably also to @satyaranjanp and @sushanthakumar) to keep same approach also in CNI-Genie.
Maybe that should also have a followup on CNI spec side because it now has a place for multiple interfaces and multiple addresses for them, but looks like they should be used in a specific way not described in SPEC.

But i see there also the same again and again confusing of what's on what level. You still are repeating that kubernetes has something to do with cni result. In fact - it's not on kubernetes (kubelet, or kubernetes objects like pods) side, but on CRI side. Even if in future kubernetes will gain the knowledge about multiple addresses - this information does not need to be generated from CNI result at all. It's up to CRI what it will return "to kubernetes". CRI can just return addresses accessible to data plane, but once again - it's not on CNI side to decide about that, it's all about how CRI will interpret the result and what it will return "to kubernetes".

Repeating that there is an issue only because Virtlet expects something, while "kubernetes" expects something other is only becoming more and more annoying.

[rorysavage77]

[Requesting Merge]

Can we please get this merged in? There's a pile of people waiting on this. Thanks...

[rkamudhan]

This PR will be merge on the top v3.2 and will be released as a separate version.

[leyao-daily]

I use multus-v3.3-tp with intel/sriov-cni to add vf into a vm build by virtlet,
but it reports:
Warning FailedCreatePodSandBox 3s (x4 over 25s) kubelet, ubuntu Failed create pod sandbox: rpc error: code = Unknown desc = "/run/virtlet.sock": rpc error: code = 2 desc = Error adding pod cirros-vm (95b3c055-e817-46ae-9803-242fca82a79b) to CNI network: server returned error: error getting fd: missing link #0 in the container network namespace (Virtlet pod restarted?)

Is there lack of something else?

[s1061123]

Looking the error message, it is not related to multus but seems to be virtlet issue.
Could you please ask virtlet community about the issue?