Return the allocated IP to a pod with the same namespace/name

pliurh commented 3 months ago

For statefulset pods, it will reuse the pod names after a node reboot. Whereabouts shall always return the same IP address that has already been allocated, when the pods start.

This patch also changes the IP deallocation logic. Now, the allocation record is identified by podRef instead of containerID.

pliurh commented 3 months ago

fix https://github.com/k8snetworkplumbingwg/whereabouts/issues/176

pliurh commented 3 months ago

@andreaskaris PTAL

andreaskaris commented 2 months ago

Looks o.k. to me but I'm horrible at reviewing without actually testing the code. If I have time I'll test this tomorrow, but IMO this looks simpler (and thus better) than the earlier approach

dougbtv commented 2 months ago

I tried to surf around the history to figure out if there was a reason we didn't use podRef, but, I can't find any smoking gun.

I think this approach of switching to base on podRef is an improvement, thanks.

Thanks Peng for the PR, and Andreas for the review, as well.

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 8612252580

Details

7 of 12 (58.33%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.04%) to 72.317%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/allocate/allocate.go	6	11	54.55%
<!--	Total:	7	12	58.33%	-->

Totals
Change from base Build 8282575075:	-0.04%
Covered Lines:	1139
Relevant Lines:	1575

💛 - Coveralls

wwcd commented 2 months ago

When a pod references multiple virtual function interfaces and all are allocated addresses, they will all be assigned the same address.

pliurh commented 2 months ago

When a pod references multiple virtual function interfaces and all are allocated addresses, they will be assigned the same address.

Yeah, it is a limitation when you have a pod with multiple interfaces using the same IP range. A workaround is to divide the IP range into sections and assign different ranges to each interface of the pod.

If we want to fix that, we need to update the API to store interface names. So that we can identify the IP allocation per interface. @dougbtv @andreaskaris @maiqueb WDYT

andreaskaris commented 2 months ago

I'm trying to reproduce https://issues.redhat.com/browse/OCPBUGS-29648 or https://github.com/k8snetworkplumbingwg/whereabouts/issues/176 with upstream/master whereabouts on kind, to get a baseline for my testing. I can reproduce OCPBUGS-29648 on OCP 4.14, but I cannot reproduce the same issue on kind; but I wouldn't exclude that I'm doing something wrong ... @pliurh could you repro the issues on upstream prior to applying the PR?

pliurh commented 2 months ago

@andreaskaris I couldn't reproduce with a kind cluster either. I reproduced it with OCP.

andreaskaris commented 2 months ago

I just tested the reboot --force scenario from https://issues.redhat.com/browse/OCPBUGS-29648 manually and everything works for me with the fix.

WRT attaching the same network 2x:

 25   template:                                                                                                             
 26     metadata:                                                                                                           
 27       annotations:                                                                                                      
 28         k8s.v1.cni.cncf.io/networks: test-ipv4,test-ipv4

I get:

[akaris@workstation whereabouts-cni ((dfaf5249...))]$ oc get pods tools-ipv4-0 --output=custom-columns="NAME:.metadata.name,NETWORK:.metadata.annotations.k8s\.v1\.cni\.cncf\.io\/network-status"
NAME           NETWORK
tools-ipv4-0   [{
    "name": "ovn-kubernetes",
    "interface": "eth0",
    "ips": [
        "10.128.0.101",
        "fd01:0:0:1::65"
    ],
    "mac": "0a:58:0a:80:00:65",
    "default": true,
    "dns": {}
},{
    "name": "test/test-ipv4",
    "interface": "net1",
    "ips": [
        "192.168.10.140"
    ],
    "mac": "ae:3d:0f:f3:28:4e",
    "dns": {}
},{
    "name": "test/test-ipv4",
    "interface": "net2",
    "ips": [
        "192.168.10.140"
    ],
    "mac": "22:3b:b7:71:d5:c9",
    "dns": {}
}]
[akaris@workstation whereabouts-cni ((dfaf5249...))]$ oc exec tools-ipv4-0 -- ip a | grep 192
    inet 192.168.10.140/24 brd 192.168.10.255 scope global net1
    inet 192.168.10.140/24 brd 192.168.10.255 scope global net2

That's indeed a regression to how it worked before, though:

[akaris@workstation whereabouts-cni ((dfaf5249...))]$ oc get pods tools-ipv4-0 --output=custom-columns="NAME:.metadata.name,NETWORK:.metadata.annotations.k8s\.v1\.cni\.cncf\.io\/network-status"
NAME           NETWORK
tools-ipv4-0   [{
    "name": "ovn-kubernetes",
    "interface": "eth0",
    "ips": [
        "10.128.0.108",
        "fd01:0:0:1::6c"
    ],
    "mac": "0a:58:0a:80:00:6c",
    "default": true,
    "dns": {}
},{
    "name": "test/test-ipv4",
    "interface": "net1",
    "ips": [
        "192.168.10.140"
    ],
    "mac": "56:31:3a:61:53:8f",
    "dns": {}
},{
    "name": "test/test-ipv4",
    "interface": "net2",
    "ips": [
        "192.168.10.141"
    ],
    "mac": "ce:84:dd:e5:91:d4",
    "dns": {}
}]
[akaris@workstation whereabouts-cni ((dfaf5249...))]$ oc exec tools-ipv4-0 -- ip a | grep 192
    inet 192.168.10.140/24 brd 192.168.10.255 scope global net1
    inet 192.168.10.141/24 brd 192.168.10.255 scope global net2

k8snetworkplumbingwg / whereabouts