IPAM controller implementation #5

ykulazhenkov · 2023-05-03T17:19:35Z

Code is not covered by test and is not tested well, but main flows should work.

cmd/ipam-controller/app/options/options.go

moshe010 · 2023-05-03T19:58:52Z

pkg/ipam-controller/allocator/allocator.go

+	}
+	nodeRange := allocatedRange{
+		StartIP: startIP,
+		EndIP:   ip.NextIPWithOffset(startIP, int64(pa.cfg.PerNodeBlockSize)),


will NextIPWithOffset prevent EndIP to be broadcast IP?

also you can create the allocatedRange object after Line 115

I added a check to avoid allocation of broadcast ip

moshe010 · 2023-05-03T20:04:52Z

pkg/ipam-controller/allocator/allocator.go

+		}
+	}
+	if len(startIP) == 0 {
+		startIP = ip.NextIP(ip.Network(pa.cfg.Subnet).IP)


maybe add a comment that this to skip the network ip address

this code path changed a bit to cover additional use-cases

nit: firs -> first

moshe010 · 2023-05-03T20:20:59Z

pkg/ipam-controller/allocator/allocator.go

+		return nodeAllocationInfo{}, fmt.Errorf("endIP is incorrect ip")
+	}
+
+	if !subnet.Contains(nodeAllocStart) || !subnet.Contains(nodeAllocEnd) {


you don't validate that the gateway is in the subnet.

moshe010 · 2023-05-03T20:22:32Z

pkg/ipam-controller/allocator/allocator.go

+		return nodeAllocationInfo{}, fmt.Errorf("invalid allocation allocators: start or end IP is out of the subnet")
+	}
+
+	if ip.Cmp(nodeAllocEnd, nodeAllocStart) <= 0 {


I don't think you need this validation. It seem it will fail with the ip.Distance as negative value

true, but I think it is better to keep the check and return more accurate message

lets keep this as is.

moshe010 · 2023-05-03T20:35:18Z

pkg/ipam-controller/controllers/configmap/configmap.go

+	if err := r.Client.List(ctx, nodeList); err != nil {
+		return ctrl.Result{}, err
+	}
+	for _, n := range nodeList.Items {


this will trigger the node controller Reconcile to sync again all nodes right?

Code is designed to be able to handle each node object individually, and it is able to handle multiple node objects concurrently.
The Node controller currently runs with a single worker to avoid possible concurrency-related issues. We can increase the count of workers later when we will have good enough test coverage.

pkg/pool/pool.go

moshe010 · 2023-05-03T22:07:45Z

The anonnotaion is not as defined in the doc
yours {"my-pool":{"name":"my-pool","subnet":"192.168.0.0/16","startIP":"192.168.0.26","endIP":"192.168.0.50","gateway":"192.168.0.1"}}
and in the doc it without the name key. Why do we need the name key anyway...

moshe010 · 2023-05-03T22:20:19Z

The offset code is adding addition ip to the block
for example for this config

     "pools": {
         "my-pool": {
             "subnet": "192.168.0.0/29",
             "perNodeBlockSize": 2,
             "gateway": "192.168.0.1"
         }

each node get range of 3 ip
node 1: start="192.168.0.1" end="192.168.0.3"
node 2: start="192.168.0.4" end="192.168.0.6"

I0503 22:16:59.739984 628030 allocator.go:117] "allocator/pool=my-pool: range allocated" controller="node" controllerGroup="" controllerKind="Node" Node="worker-node01" namespace="" name="worker-node01" reconcileID=684b8ca2-d904-47fa-905c-8f735c12872a node="worker-node01" start="192.168.0.1" end="192.168.0.3"
I0503 22:16:59.766752 628030 node.go:104] "node object updated" controller="node" controllerGroup="" controllerKind="Node" Node="worker-node01" namespace="" name="worker-node01" reconcileID=684b8ca2-d904-47fa-905c-8f735c12872a
I0503 22:16:59.767120 628030 allocator.go:117] "allocator/pool=my-pool: range allocated" controller="node" controllerGroup="" controllerKind="Node" Node="worker-node02" namespace="" name="worker-node02" reconcileID=0d051c99-8076-48f4-8cc0-be72ce99af8c node="worker-node02" start="192.168.0.4" end="192.168.0.6"

moshe010 · 2023-05-03T22:22:59Z

when the configmap get deleted I see this message [1] in a loop. restart the controller solves it.

[1] -
E0503 22:21:50.096217 628030 configmap.go:58] "failed to get ConfigMap object from the cache" err="ConfigMap "nvidia-k8s-ipam-config" not found" controller="configmap" controllerGroup="" controllerKind="ConfigMap" ConfigMap="default/nvidia-k8s-ipam-config" namespace="default" name="nvidia-k8s-ipam-config" reconcileID=221b2639-634e-45bc-aa01-474d33e84108

moshe010 · 2023-05-03T22:32:02Z

with this config (blockSize is 4 but I mean 5 see +1 shift issue above)
{
"pools": {
"my-pool": {
"subnet": "192.168.0.0/28",
"perNodeBlockSize": 4,
"gateway": "192.168.0.1"
}
}
}

see below [1] "worker-node02" got 192.168.0.15 which is broadcast address (start="192.168.0.11" end="192.168.0.15")

[1]
571eb2f46d8 node="master-node" start="192.168.0.1" end="192.168.0.5"
I0503 22:27:54.316226 630910 node.go:104] "node object updated" controller="node" controllerGroup="" controllerKind="Node" Node="master-node" namespace="" name="master-node" reconcileID=bf39f805-1ae8-48e4-87ac-7571eb2f46d8
I0503 22:27:54.316576 630910 allocator.go:117] "allocator/pool=my-pool: range allocated" controller="node" controllerGroup="" controllerKind="Node" Node="worker-node01" namespace="" name="worker-node01" reconcileID=5ee200ff-adb6-41c5-8d0a-a9ad7c7f2824 node="worker-node01" start="192.168.0.6" end="192.168.0.10"
I0503 22:27:54.347169 630910 node.go:104] "node object updated" controller="node" controllerGroup="" controllerKind="Node" Node="worker-node01" namespace="" name="worker-node01" reconcileID=5ee200ff-adb6-41c5-8d0a-a9ad7c7f2824
I0503 22:27:54.347544 630910 allocator.go:117] "allocator/pool=my-pool: range allocated" controller="node" controllerGroup="" controllerKind="Node" Node="worker-node02" namespace="" name="worker-node02" reconcileID=f7c6dd88-31f1-4f54-ab6b-1f7b5a8708b9 node="worker-node02" start="192.168.0.11" end="192.168.0.15"

moshe010 · 2023-05-04T07:25:13Z

when the configmap get deleted I see this message [1] in a loop. restart the controller solves it.

[1] - E0503 22:21:50.096217 628030 configmap.go:58] "failed to get ConfigMap object from the cache" err="ConfigMap "nvidia-k8s-ipam-config" not found" controller="configmap" controllerGroup="" controllerKind="ConfigMap" ConfigMap="default/nvidia-k8s-ipam-config" namespace="default" name="nvidia-k8s-ipam-config" reconcileID=221b2639-634e-45bc-aa01-474d33e84108

So when creating again the configmap the error stoped which is good. We can just open issue to see how to handle configmap delete as this is not critical

moshe010 · 2023-05-04T07:30:37Z

we should clean annotation from nodes when configmap is invalid.
scenario:

create valid config
annotation on all nodes
delete config
create invalid config
annotation should be removed (current code keep valid configmap annotation)

adrianchiris · 2023-05-04T07:48:26Z

we should clean annotation from nodes when configmap is invalid.

CNI relies on valid configuration for all CNI calls CMD Add and CMD Del

we can keep the "last valid" annotation on node
or we need to write a caching mechanism in CNI to save it from the add call.

we may need the latter eventually, but not sure if we need it now.

ykulazhenkov · 2023-05-04T08:21:02Z

PR re-based, testing is in progress

pkg/ip/cidr.go

pkg/pool/pool.go

pkg/ipam-controller/config/config.go

adrianchiris · 2023-05-04T09:20:02Z

pkg/ipam-controller/controllers/node/node.go

+		return ctrl.Result{}, err
+	}
+
+	if err := r.Client.Update(ctx, node); err != nil {


should we use patch here ? to reduce chance of error ?

Error is not critical. In case of error we will restart the loop for specific node only. But in general maybe it is a good idea to use Patch, I will return back to it when work on test coverage.

cool thx, in the past we got folks complaining on errors in controller, if we can avoid them its better.
(will save time in the long term ...)

ykulazhenkov · 2023-05-04T09:22:59Z

we should clean annotation from nodes when configmap is invalid. scenario:

1. create valid config

2. annotation on all nodes

3. delete config

4. create invalid config

5. annotation should be removed (current code keep valid configmap annotation)

I updated the controller behavior. Now the controller explicitly handles ConfigMap removal - simply waits for ConfigMap recreation.

Current behavior is following: in case if there is no ConfigMap or if it contains invalid configuration then controller will simply keep all existing annotations for nodes and wait for ConfigMap update/creation. I think this is the safest behavior but we can change it later.

pkg/ipam-controller/controllers/configmap/configmap.go

adrianchiris · 2023-05-04T11:07:41Z

pkg/ipam-controller/allocator/allocator.go

+		StartIP: startIP,
+		EndIP:   ip.NextIPWithOffset(startIP, int64(pa.cfg.PerNodeBlockSize)-1),
+	}
+	if !pa.cfg.Subnet.Contains(nodeRange.EndIP) || ip.IsBroadcast(nodeRange.EndIP, pa.cfg.Subnet) {


can you also add check that start IP is in subnet ?

far fetched scenario where startIP has wrap around to 0 and EndIP is somehow within the subnet.

yes, it looks like the code may fail if the next IP become an invalid address. WIll address this in a separate PR

adrianchiris · 2023-05-04T11:28:38Z

pkg/ipam-controller/allocator/allocator.go

+			return err
+		}
+		// check if r.StartIP or r.EndIP match start or end IP of the "a"
+		if r.StartIP.Equal(a.StartIP) || r.StartIP.Equal(a.EndIP) ||


nit: allocatedRange can have a method "Overlaps(other *allocatedRange) bool"

adrianchiris · 2023-05-04T11:33:32Z

pkg/ipam-controller/allocator/allocator.go

+		}
+		// check if r.StartIP or r.EndIP match start or end IP of the "a"
+		if r.StartIP.Equal(a.StartIP) || r.StartIP.Equal(a.EndIP) ||
+			r.EndIP.Equal(a.StartIP) || r.EndIP.Equal(a.EndIP) {


since block size is the same, then an overlap means the blocks are necessarily equal.
is there a case that this is not correct ?

if so, its enough to just compare StartIP which can technically act as an absolute "block index" in the subnet

Yes, you are right. I changed this logic. Now we check that StartIP has correct offset and simply validate that there is no duplicates registered.

pkg/ipam-controller/allocator/allocator.go

adrianchiris · 2023-05-04T14:22:22Z

pkg/ipam-controller/allocator/allocator.go

+	}
+	allocations := pa.getAllocationsAsSlice()
+	for _, a := range allocations {
+		if allocData.allocatedRange.StartIP.Equal(a.StartIP) {


can you add a comment here on why this is enough ?

to the naive reader it many not be apparent.

adrianchiris · 2023-05-04T14:27:39Z

pkg/ipam-controller/allocator/allocator.go

+		float64(pa.cfg.PerNodeBlockSize)) != 0 {
+		return fmt.Errorf("invalid start IP offset")
+	}
+	if ip.Distance(allocData.StartIP, allocData.EndIP) != int64(pa.cfg.PerNodeBlockSize)-1 {


seems like adding a Wrapper of ip.IPsInRange(start, end) which will just add one to distance result
will save adding -1 in different places. (which later on folks may forget to add )

adrianchiris · 2023-05-04T14:32:08Z

pkg/ipam-controller/allocator/allocator.go

+	if !allocData.Gateway.Equal(pa.cfg.Gateway) {
+		return fmt.Errorf("gateway mismatch")
+	}
+	if math.Mod(float64(ip.Distance(ip.Network(pa.cfg.Subnet).IP, allocData.StartIP))-1,


-1 is because we skip the network address ? (e.g for 192.168.0.0/16 , 192.168.0.0 is not allocated)

simple example:

network: 0.0.0.0
block size 10

StartIPs wil be: 1, 11, 21, 31, 41 ...
Distance per StartIP from Network IP is same
subsracting 1 will yield 0 in mod 10 (block size)

so it seems to work :).

adrianchiris

Added a couple of comments.
mainly nits so up to u if you want to address them :)

feel free to self merge.

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

This package is shared by components Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

ykulazhenkov requested review from moshe010 and adrianchiris May 3, 2023 17:19

ykulazhenkov force-pushed the controller branch from f2ddef4 to d98741b Compare May 3, 2023 17:20

moshe010 reviewed May 3, 2023

View reviewed changes

ykulazhenkov force-pushed the controller branch from d98741b to 3aba620 Compare May 4, 2023 08:20

ykulazhenkov force-pushed the controller branch from 3aba620 to e9c0675 Compare May 4, 2023 08:44

adrianchiris reviewed May 4, 2023

View reviewed changes

pkg/ip/cidr.go Show resolved Hide resolved

ykulazhenkov force-pushed the controller branch 2 times, most recently from 5d56133 to 63c9ff4 Compare May 4, 2023 09:05

adrianchiris reviewed May 4, 2023

View reviewed changes

pkg/pool/pool.go Outdated Show resolved Hide resolved

pkg/pool/pool.go Outdated Show resolved Hide resolved

pkg/pool/pool.go Outdated Show resolved Hide resolved

pkg/pool/pool.go Outdated Show resolved Hide resolved

pkg/ipam-controller/config/config.go Show resolved Hide resolved

adrianchiris reviewed May 4, 2023

View reviewed changes

ykulazhenkov force-pushed the controller branch from 63c9ff4 to 7f93c2f Compare May 4, 2023 09:33

adrianchiris reviewed May 4, 2023

View reviewed changes

pkg/ipam-controller/controllers/configmap/configmap.go Outdated Show resolved Hide resolved

ykulazhenkov force-pushed the controller branch from 7f93c2f to 9ab96ae Compare May 4, 2023 10:06

adrianchiris reviewed May 4, 2023

View reviewed changes

pkg/ipam-controller/allocator/allocator.go Show resolved Hide resolved

ykulazhenkov force-pushed the controller branch 2 times, most recently from b236eae to b7c00ff Compare May 4, 2023 12:45

ykulazhenkov changed the title ~~[WIP] IPAM controller implementation~~ IPAM controller implementation May 4, 2023

adrianchiris reviewed May 4, 2023

View reviewed changes

adrianchiris approved these changes May 4, 2023

View reviewed changes

ykulazhenkov added 2 commits May 4, 2023 19:15

initial implementation

8e3bdf1

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

Move pool package to the pkg root

adb8c16

This package is shared by components Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

ykulazhenkov force-pushed the controller branch from b7c00ff to adb8c16 Compare May 4, 2023 16:15

ykulazhenkov merged commit 4c5bda4 into Mellanox:main May 4, 2023

ykulazhenkov deleted the controller branch May 8, 2023 08:52

IPAM controller implementation #5

IPAM controller implementation #5

Conversation

ykulazhenkov commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshe010 commented May 3, 2023

moshe010 commented May 3, 2023

moshe010 commented May 3, 2023

moshe010 commented May 3, 2023

moshe010 commented May 4, 2023

moshe010 commented May 4, 2023

adrianchiris commented May 4, 2023 • edited Loading

ykulazhenkov commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykulazhenkov commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris commented May 4, 2023 •

edited

Loading

adrianchiris May 4, 2023 •

edited

Loading