Antrea IPAM for secondary networks managed by Multus #3529
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3529 +/- ##
===========================================
- Coverage 64.26% 45.97% -18.30%
===========================================
Files 278 362 +84
Lines 27821 51015 +23194
===========================================
+ Hits 17878 23452 +5574
- Misses 8045 25352 +17307
- Partials 1898 2211 +313
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/test-flexible-ipam-e2e |
f6e5b4f to
e283c2d
Compare
|
/test-e2e |
996a1ac to
ed94010
Compare
| "keyA": ["some more", "plugin specific", "configuration"], | ||
| "ipam": { | ||
| "type": "antrea", | ||
| "ippools": [ "test-ippool-ipv4-1" ], |
There was a problem hiding this comment.
Just a question, where does this parameter come from?
There was a problem hiding this comment.
I created the IPPool named "test-ippool-ipv4-1" in this test. Check line 30 - 46.
There was a problem hiding this comment.
To clarify this question, I want to ask how this parameter passed in this cniNetworkConfig.
From the PR description, we have a NetworkAttachmentDefinition, and I know some special config will be passed in CNI call, but I think we still need some other configuration to make the whole pipeline work.
Another question: do we want to cover dns/routes support in this PR or in Antrea?
There was a problem hiding this comment.
In the real usage with Multus, Multus will pass the whole CNI config json in NetworkAttachmentDefinition to the main CNI plugin (e.g. MACVLAN), which then passes the json to the IPAM plugin (just like how Antrea invokes node-ipam plugin). The behavior of the latter part (from main CNI to IPAM CNI) is defined in the CNI spec.
I have not heard about any request to support dns/routes for the primary CNI. Added them for secondary network also for feature parity with Whereabouts.
| if err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| err = fmt.Errorf("failed to get IPPool %s: %v", p, err) | ||
| break |
There was a problem hiding this comment.
Why not return error directly? If not, err would be reset in line 223, and the real error would be lost.
There was a problem hiding this comment.
Ah, I wanted not to repeat "mineTrue, allocator, ips, reservedOwner, err", as I feel that is harder to track all 4 return values. But if you think return directly is better, I can change to that too.
| if !s.enableSecondaryNetworkIPAM { | ||
| return s.unsupportedFieldResponse("type", cniConfig.Type) | ||
| } | ||
| if ipamType != ipam.AntreaIPAMType { |
There was a problem hiding this comment.
So only AntreaIPAM is the supported IPAM type for Multus secondary network, it also requires CNI type is not "antrea"?
There was a problem hiding this comment.
Yes. If another IPAM type is required, antrea CNI should not be called; Multus should execute that IPAM CNI.
Today Antrea cannot be secondary CNI with Multus. If later we really allow that, we can revisit the CNI type here can be "antrea" or not.
| return nil, resp | ||
| } | ||
| if !s.isChaining && !cniConfig.secondaryNetworkIPAM { | ||
| s.updateLocalIPAMSubnet(cniConfig) |
There was a problem hiding this comment.
Just confirm: if secondaryNetworkIPAM == true, cniConfig.NetworkConfig.IPAM.Ranges is possibly empty. Then that means we forbid to allocate IP from AntreaIPAM to host-local in secondary network case?
There was a problem hiding this comment.
Yes, as I said this should be for Antrea native IPAM with IPPool only. If users want to use host-local, they should just set IPAM type to "host-local", then antrea CNI will be not called.
| return s.ipamFailureResponse(err), nil | ||
| } | ||
| klog.InfoS("Allocated IP addresses", "container", cniConfig.ContainerId, "result", ipamResult) | ||
| return resultToResponse(ipamResult), nil |
There was a problem hiding this comment.
So Antrea only allocates IP in this scenario? Since no OVS port or memory interfaceConfig created, other features in Antrea would not know this second IP. Besides, who will configure IP address inside the container? If we assume another CNI plugin processes it, does it mean kubelet should send request to both two CNI plugins (antrea and the other plugin) at the same time?
There was a problem hiding this comment.
Yes, Antrea is only doing IPAM - as it is configured as IPAM plugin, not the CNI plugin in CNI NetworkConfiguration.
Typically, antrea CNI is executed by the CNI plugin specified in "Type" (just like Antrea agent executes host-local plugin).
| } | ||
|
|
||
| func (s *CNIServer) ipamCheck(cniConfig *CNIConfig) (*cnipb.CniCmdResponse, error) { | ||
| if err := ipam.SecondaryNetworkCheck(cniConfig.CniCmdArgs, cniConfig.K8sArgs, cniConfig.NetworkConfig); err != nil { |
There was a problem hiding this comment.
Since ipam.SecondaryNetworkCheck always returns error, line 415 won't execute.
There was a problem hiding this comment.
Yes, but I hope to keep the ipamCheck() complete and independent of the SecondaryNetworkCheck() implementation. I added some comments to explain this.
| var ips []net.IP | ||
| var reservedOwner *crdv1a2.IPAddressOwner | ||
| pod, err := c.podLister.Pods(namespace).Get(name) | ||
| if err != nil { | ||
| // For CNI DEL case, getting Pod may fail. Try to get information from allocated IPs of all IPPools. |
There was a problem hiding this comment.
This logic is removed in the new code, is it possible to lead to IP leak in CNI DEL event because IPPool fails to get as Pod is removed?
There was a problem hiding this comment.
Checked Jianjun's change, I think it will use a new function to check ipPool when CNIDel, thus this is OK.
func (d *AntreaIPAM) del(podOwner *crdv1a2.PodOwner) (foundAllocation bool, err error)
There was a problem hiding this comment.
Yes, I changed the code to always look up IPPools by Pod. I think it might be possible a pool status update is not received by agent yet when CNI DEL comes, and so we might not be able to release the allocated IP (@gran-vmv : do you agree?). But this can happen even with the existing code.
I made change more for secondary network and primary network to share the same DEL code. Also, I feel in this way the code for ADD is also simpler.
There was a problem hiding this comment.
@jianjuns I think your method should work, but not sure if the cost for searching PodOwner in IPPools for all PodDel is acceptable.
There was a problem hiding this comment.
But the cache is indexed by Pod name (+ Namespace)? Beside code sharing, I also feel this approach is safer - it can cover Pod annotation or CNI config change (for secondary network).
Sure. I added the milestone for 1.7. I am also working on the documentation for secondary network IPAM, in which I will make it clear AntreaIPAM feature gate is needed by both secondary network IPAM and bridging mode. However, I plan to create the documentation PR, after an enhancement of support IPv6/dual stack, after merging this PR. |
|
/test-e2e |
Extend Antrea IPAM and Antrea CNI plugin to support IPAM for Pod
secondary networks. A CNI call that specifies a non-Antrea CNI type and
Antrea IPAM type is identified as an IPAM request for a secondary
network. The IPAM configuration of the CNI call should specify the
Antrea IPPool(s) to allocate IPs for the secondary network.
Additionally, Routes and DNS parameters are supported in the IPAM
configuration.
This implementation is for secondary network managed by Multus, not
Antrea native secondary network support. Only a single IPv4 IPPool is
supported as of now.
An example Multus NetworkAttachmentDefinition with Antrea IPAM:
kind: NetworkAttachmentDefinition
metadata:
name: macvlan-network1
spec:
config: '{
"cniVersion": "0.3.0",
"type": "macvlan",
"master": "enp0s9",
"mode": "bridge",
"ipam": {
"type": "antrea-ipam",
"ippool": "macvlan-subnet1"
"routes": [
{ "dst": "0.0.0.0/0" },
{ "dst": "192.168.0.0/16", "gw": "10.10.5.1" },
{ "dst": "3ffe:ffff:0:01ff::1/64" }
],
"dns": {
"nameservers" : ["8.8.8.8"],
"domain": "example.com",
"search": [ "example.com" ]
}
}
}'
Signed-off-by: Jianjun Shen <shenj@vmware.com>
|
/test-all |
| err = allocator.ReleaseContainerIfPresent(owner.Pod.ContainerID) | ||
| return true, err | ||
| // If no allocation found, pass CNI DEL to the next driver. | ||
| return foundAllocation, nil |
There was a problem hiding this comment.
Here if allocation was not found (due to duplicate del, for example) we might be passing irrelevant request to next driver. I see no functional harm in it but it might be confusing.
There was a problem hiding this comment.
Yes. But we have no way to know we indeed should pass to the next driver or not right? @gran-vmv: as you implemented the behavior, you might comment have you verified any issue if the next driver (host-local) does not handle the allocation? I assume it will just silently return no error.
But maybe we can add more comments here to explain the situation.
There was a problem hiding this comment.
The previous implementation was more robust since it mainly relied on annotation rather than pool status. But I tend to think that code simplicity that resulted from your refactoring is worth this cost.
There was a problem hiding this comment.
But on the another aspect - it is not safe to rely on the Pod annotation either as it can be changed. And even the previous implementation can still get the same when the Pod resource is already deleted.
Extend Antrea IPAM and Antrea CNI plugin to support IPAM for Pod
secondary networks. A CNI call that specifies a non-Antrea CNI type and
Antrea IPAM type is identified as an IPAM request for a secondary
network. The IPAM configuration of the CNI call should specify the
Antrea IPPool(s) to allocate IPs for the secondary network.
Additionally, Routes and DNS parameters are supported in the IPAM
configuration.
This implementation is for secondary network managed by Multus, not
Antrea native secondary network support. Only a single IPv4 IPPool is
supported as of now.
An example Multus NetworkAttachmentDefinition with Antrea IPAM:
Signed-off-by: Jianjun Shen shenj@vmware.com