Support multiple IPPools for secondary network IPAM #3606

jianjuns · 2022-04-08T17:19:49Z

With this change, multiple IPPools can be specified in the CNI IPAM
config, and then antrea-agent will try allocating one IP in each IPPool
for the secondary network interface.

An example CNI config:
  {
      "cniVersion": "0.3.0",
      "type": "macvlan",
      "master": "enp0s9",
      "mode": "bridge",
      "ipam": {
          "type": "antrea-ipam",
          "ippool": ["ippool-v4", "ipppol-v6"]
      }
  }

Signed-off-by: Jianjun Shen shenj@vmware.com

codecov-commenter · 2022-04-08T17:32:59Z

Codecov Report

Merging #3606 (4fe9f24) into main (7d3b595) will decrease coverage by 13.94%.
The diff coverage is 59.52%.

❗ Current head 4fe9f24 differs from pull request most recent head b4edd18. Consider uploading reports for the commit b4edd18 to get more accurate results

@@             Coverage Diff             @@
##             main    #3606       +/-   ##
===========================================
- Coverage   64.27%   50.33%   -13.95%     
===========================================
  Files         278      248       -30     
  Lines       27929    35644     +7715     
===========================================
- Hits        17952    17942       -10     
- Misses       8062    15905     +7843     
+ Partials     1915     1797      -118

Flag	Coverage Δ
e2e-tests	`50.33% <59.52%> (?)`
kind-e2e-tests	`?`
unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/ipam/poolallocator/allocator.go	`30.36% <52.17%> (-21.77%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	`46.82% <65.62%> (-29.47%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go	`32.87% <83.33%> (-49.70%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`4.58% <0.00%> (-86.85%)`	⬇️
pkg/agent/util/iptables/lock.go	`0.00% <0.00%> (-81.82%)`	⬇️
pkg/controller/externalippool/validate.go	`0.00% <0.00%> (-76.20%)`	⬇️
...lowaggregator/clickhouseclient/clickhouseclient.go	`0.00% <0.00%> (-75.84%)`	⬇️
pkg/cni/client.go	`0.00% <0.00%> (-75.52%)`	⬇️
pkg/controller/networkpolicy/crd_utils.go	`14.48% <0.00%> (-75.42%)`	⬇️
pkg/controller/networkpolicy/clustergroup.go	`3.50% <0.00%> (-74.80%)`	⬇️
... and 271 more

jianjuns · 2022-04-11T22:26:32Z

/test-all
/test-flexible-ipam-e2e

jianjuns · 2022-04-12T01:28:11Z

/test-flexible-ipam-e2e

pkg/agent/cniserver/ipam/antrea_ipam_controller.go

pkg/ipam/poolallocator/allocator.go

annakhm · 2022-04-12T23:02:47Z

pkg/ipam/poolallocator/allocator.go

+
+	index := -1
+	for i, allocator := range allocators {
+		if allocator.Has(ip) {


This returns true if IP is in given range, regardless if it is allocated - I think this can cause confusion while reading this code. Perhaps we should rename it to avoid confusion.

What is the right name in your mind? But anyway it is not relevant to this PR, and I prefer not to change it in this PR.

Yes, its just a thought not relevant for the PR. Perhaps belongs would be a more fitting name.

jianjuns · 2022-04-12T23:34:02Z

/test-all

gran-vmv

LGTM

tnqn · 2022-04-12T17:38:55Z

pkg/ipam/poolallocator/allocator.go

-func (a *IPPoolAllocator) HasContainer(containerID, ifName string) (bool, error) {
-
+// HasContainer checks whether an IP was associated with specified container. It returns error if the IPPool CR fails to be retrieved.
+func (a *IPPoolAllocator) HasContainer(containerID, ifName string) (net.IP, error) {


should we call the method something like "GetIPByContainer" given the change?

I prefer to keep the name, as we have other HasXXX(), though I know this func now returns an IP.

But HasPod method indeed returns whether it "Has" the Pod

Ok. Changed to GetContainerIP().

tnqn · 2022-04-13T04:08:33Z

pkg/ipam/poolallocator/allocator.go

@@ -540,11 +567,9 @@ func (a *IPPoolAllocator) ReleaseContainer(containerID, ifName string) error {
 	return err
 }

-// HasResource checks whether an IP was associated with specified pod. It returns error if the resource is crd fails to be retrieved.
+// HasResource checks whether an IP was associated with specified pod. It returns error if the IPPool CR fails to be retrieved.


Not introduced by this PR, but could be corrected with the change. s/HasResource/HasPod/

Sure. Let me change it.

With the change, multiple IPPools can be specified in the CNI IPAM config, and then antrea-agent will try allocating one IP in each IPPool for the secondary network interface. An example CNI config: { "cniVersion": "0.3.0", "type": "macvlan", "master": "enp0s9", "mode": "bridge", "ipam": { "type": "antrea-ipam", "ippool": ["ippool-v4", "ipppol-v6"] } } Signed-off-by: Jianjun Shen <shenj@vmware.com>

jianjuns · 2022-04-13T04:51:58Z

/test-all

tnqn

LGTM

jianjuns force-pushed the secnet-ipam-mips branch from 55d5c11 to c3cd66f Compare April 8, 2022 17:25

jianjuns force-pushed the secnet-ipam-mips branch from c3cd66f to 2c35aeb Compare April 8, 2022 18:23

jianjuns added the area/ipam Issues or PRs related to IP address management (IPAM). label Apr 8, 2022

jianjuns requested a review from gran-vmv April 8, 2022 18:23

jianjuns mentioned this pull request Apr 8, 2022

Antrea IPAM for secondary networks managed by Multus #3529

Merged

jianjuns force-pushed the secnet-ipam-mips branch 2 times, most recently from b980589 to e9fab29 Compare April 8, 2022 23:20

jianjuns requested a review from annakhm April 8, 2022 23:20

jianjuns force-pushed the secnet-ipam-mips branch 3 times, most recently from 7b325e4 to 9d8d6f3 Compare April 11, 2022 20:57

jianjuns requested review from gran-vmv and removed request for gran-vmv April 11, 2022 20:58

gran-vmv reviewed Apr 12, 2022

View reviewed changes

pkg/agent/cniserver/ipam/antrea_ipam_controller.go Outdated Show resolved Hide resolved

pkg/ipam/poolallocator/allocator.go Outdated Show resolved Hide resolved

jianjuns force-pushed the secnet-ipam-mips branch 3 times, most recently from ce7c150 to 70e6157 Compare April 12, 2022 02:57

annakhm reviewed Apr 12, 2022

View reviewed changes

annakhm previously approved these changes Apr 12, 2022

View reviewed changes

gran-vmv previously approved these changes Apr 13, 2022

View reviewed changes

tnqn reviewed Apr 13, 2022

View reviewed changes

jianjuns dismissed stale reviews from gran-vmv and annakhm via b4edd18 April 13, 2022 04:51

jianjuns force-pushed the secnet-ipam-mips branch from 70e6157 to b4edd18 Compare April 13, 2022 04:51

tnqn approved these changes Apr 13, 2022

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Apr 13, 2022

tnqn added this to the Antrea v1.7 release milestone Apr 13, 2022

jianjuns merged commit c463028 into antrea-io:main Apr 13, 2022

jianjuns deleted the secnet-ipam-mips branch April 13, 2022 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple IPPools for secondary network IPAM #3606

Support multiple IPPools for secondary network IPAM #3606

jianjuns commented Apr 8, 2022 •

edited

Loading

codecov-commenter commented Apr 8, 2022 •

edited

Loading

jianjuns commented Apr 11, 2022

jianjuns commented Apr 12, 2022

annakhm Apr 12, 2022 •

edited

Loading

jianjuns Apr 12, 2022

annakhm Apr 12, 2022

jianjuns commented Apr 12, 2022

gran-vmv left a comment

tnqn Apr 12, 2022

jianjuns Apr 13, 2022

tnqn Apr 13, 2022

jianjuns Apr 13, 2022 •

edited

Loading

tnqn Apr 13, 2022

jianjuns Apr 13, 2022

jianjuns commented Apr 13, 2022

tnqn left a comment

Support multiple IPPools for secondary network IPAM #3606

Support multiple IPPools for secondary network IPAM #3606

Conversation

jianjuns commented Apr 8, 2022 • edited Loading

codecov-commenter commented Apr 8, 2022 • edited Loading

Codecov Report

jianjuns commented Apr 11, 2022

jianjuns commented Apr 12, 2022

annakhm Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

jianjuns Apr 12, 2022

Choose a reason for hiding this comment

annakhm Apr 12, 2022

Choose a reason for hiding this comment

jianjuns commented Apr 12, 2022

gran-vmv left a comment

Choose a reason for hiding this comment

tnqn Apr 12, 2022

Choose a reason for hiding this comment

jianjuns Apr 13, 2022

Choose a reason for hiding this comment

tnqn Apr 13, 2022

Choose a reason for hiding this comment

jianjuns Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

tnqn Apr 13, 2022

Choose a reason for hiding this comment

jianjuns Apr 13, 2022

Choose a reason for hiding this comment

jianjuns commented Apr 13, 2022

tnqn left a comment

Choose a reason for hiding this comment

jianjuns commented Apr 8, 2022 •

edited

Loading

codecov-commenter commented Apr 8, 2022 •

edited

Loading

annakhm Apr 12, 2022 •

edited

Loading

jianjuns Apr 13, 2022 •

edited

Loading