ip-reconciler: Use ContainerID instead of PodRef #180

xagent003 · 2022-01-20T03:35:24Z

Fixes issue: #176

xagent003 · 2022-01-20T21:00:49Z

@dougbtv @maiqueb Can you please review?

One of the failing testcases did not make sense, but I fixed it regardless by using container ID. It is negative testcase that checks whether the ip-reconciler fails to delete a reservation because the podRef name does not match.

But this could never be the actual case, because the orphanedIP Reservation object is formed from the IPPool itself. I instead changed the testcase so now the podRef names are the same, but the container ID is different, because my fix changes the MatchingFunc to use containerID instead of podName.

roopakparikh

Looks good

maiqueb

I've added this PR to my to-do list, I'll get back to you next week.

What about the unit test currently failing on this PR ?
I think you need to add the containerID to this remainingAllocation allocation map

whereabouts/cmd/reconciler/ip_test.go

Line 159 in 55be906

remainingAllocation := map[string]v1alpha1.IPAllocation{

EDIT: couldn't help noticing there's also something wrong with the vendoring ... You'll need to sort that out as well.

maiqueb · 2022-01-21T14:53:56Z

cmd/reconciler/ip_test.go

@@ -289,7 +291,8 @@ func generateIPPoolSpec(ipRange string, namespace string, poolName string, podNa
 	allocations := map[string]v1alpha1.IPAllocation{}
 	for i, podName := range podNames {
 		allocations[fmt.Sprintf("%d", i+1)] = v1alpha1.IPAllocation{
-			PodRef: fmt.Sprintf("%s/%s", namespace, podName),
+			PodRef:      fmt.Sprintf("%s/%s", namespace, podName),
+            ContainerID: strconv.Itoa(rand.Intn(1000)),


please run gofmt before pushing; there's something wrong with the formatting on this file.

maiqueb

I think the code looks good; I honestly do not understand right now why I hadn't matched using the containerID instead.

Let's wait for the follow up work :)

Good catch.

maiqueb · 2022-01-24T12:23:49Z

pkg/reconciler/iploop.go

-func findOutPodRefsToDeallocateIPsFrom(orphanedIP OrphanedIPReservations) []string {
-	var podRefsToDeallocate []string
+func findOutContainerIDsToDeallocateIPsFrom(orphanedIP OrphanedIPReservations) []string {
+	var cidsToDeallocate []string


please rename this variable. All the time I've seen cid my brain translated it to CIDR and I couldn't understand what was happening. I suggest using containerID , which makes it crystal clear.

dougbtv · 2022-01-25T13:01:54Z

cmd/reconciler/ip_test.go

@@ -289,7 +291,8 @@ func generateIPPoolSpec(ipRange string, namespace string, poolName string, podNa
 	allocations := map[string]v1alpha1.IPAllocation{}
 	for i, podName := range podNames {
 		allocations[fmt.Sprintf("%d", i+1)] = v1alpha1.IPAllocation{
-			PodRef: fmt.Sprintf("%s/%s", namespace, podName),
+			PodRef:      fmt.Sprintf("%s/%s", namespace, podName),
+            ContainerID: strconv.Itoa(rand.Intn(1000)),


maiqueb · 2022-01-26T13:01:23Z

@TothFerenc would you review this PR as well, since you're introduced the pod ref attribute to the model.

I'm also thinking of replacing the whole reconciler pod / cron by a controller that would listen to pod deletions, and assure the allocations for the deleted pods are indeed gone.

If / once this route holds, we can drop the reconciler, and, for instance, merge your bash-based alternative for it.

xagent003 · 2022-03-29T22:52:51Z

@dougbtv @maiqueb Took care of comments and gofmt, rebased to top of tree upstream

maiqueb

One thing I worry about this is upgrade: let's say you have a running cluster with an existing IPPool. We then upgrade whereabouts to store the allocations (and garbage collect them) based on the container ID. Wouldn't we garbage collect the entire pool ?

My point is... don't we need some sort of an upgrade out of band tool to re-generate the pool, porting it to the "new" model ?

maiqueb · 2022-04-01T10:57:42Z

BTW: you should rebase this with latest master. It will fix the unit test issue you're currently facing, and give us results.

xagent003 · 2022-04-02T19:24:24Z

One thing I worry about this is upgrade: let's say you have a running cluster with an existing IPPool. We then upgrade whereabouts to store the allocations (and garbage collect them) based on the container ID. Wouldn't we garbage collect the entire pool ?

My point is... don't we need some sort of an upgrade out of band tool to re-generate the pool, porting it to the "new" model ?

@maiqueb The storage model doesnt change nor the logic to determine what is a stale reservation in first place (that still uses podRef, since getting the pause Container ID from a Pod resource is impossible).

This is just the matching function that looks for reservations to delete AFTER on the already computed orphaned reservations list. The bug was that it tried to match an already computed orphaned reservation to a IP reservation in the IPPool and used the podRef in the matching function

We've already upgraded and used this fix, FYI in our custom whereabouts release.

dougbtv · 2022-06-23T13:58:03Z

@xagent003 do you know if the reconciliation control loop covers this one appropriately? It might.

If it doesn't, mind rebasing it and we can take another look? Thanks!

xagent003 force-pushed the private/arjun/useCIDnotPodRef branch 2 times, most recently from 5f03ce5 to 4b04987 Compare January 20, 2022 20:56

xagent003 force-pushed the private/arjun/useCIDnotPodRef branch 14 times, most recently from a48b24c to 8d22508 Compare January 21, 2022 04:19

roopakparikh approved these changes Jan 21, 2022

View reviewed changes

maiqueb requested changes Jan 21, 2022

View reviewed changes

maiqueb requested changes Jan 25, 2022

View reviewed changes

dougbtv requested changes Jan 25, 2022

View reviewed changes

xagent003 mentioned this pull request Feb 2, 2022

ip-reconciler issue with StatefullSet cleanup #182

Closed

xagent003 force-pushed the private/arjun/useCIDnotPodRef branch from 8d22508 to f7d884c Compare March 29, 2022 22:52

ip-reconciler: Use ContainerID instead of PodRef

f5a64e6

xagent003 force-pushed the private/arjun/useCIDnotPodRef branch from f7d884c to f5a64e6 Compare March 30, 2022 03:14

xagent003 mentioned this pull request Mar 30, 2022

Rechecking pending Pods #195

Open

maiqueb reviewed Apr 1, 2022

View reviewed changes

dougbtv mentioned this pull request Dec 20, 2023

Rebases #180: ip-reconciler: Use ContainerID instead of PodRef #399

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ip-reconciler: Use ContainerID instead of PodRef #180

ip-reconciler: Use ContainerID instead of PodRef #180

xagent003 commented Jan 20, 2022

xagent003 commented Jan 20, 2022

roopakparikh left a comment

maiqueb left a comment •

edited

Loading

maiqueb Jan 21, 2022

dougbtv Jan 25, 2022

maiqueb left a comment

maiqueb Jan 24, 2022

dougbtv Jan 25, 2022

maiqueb commented Jan 26, 2022

xagent003 commented Mar 29, 2022 •

edited

Loading

maiqueb left a comment

maiqueb commented Apr 1, 2022

xagent003 commented Apr 2, 2022

dougbtv commented Jun 23, 2022

ip-reconciler: Use ContainerID instead of PodRef #180

Are you sure you want to change the base?

ip-reconciler: Use ContainerID instead of PodRef #180

Conversation

xagent003 commented Jan 20, 2022

xagent003 commented Jan 20, 2022

roopakparikh left a comment

Choose a reason for hiding this comment

maiqueb left a comment • edited Loading

Choose a reason for hiding this comment

maiqueb Jan 21, 2022

Choose a reason for hiding this comment

dougbtv Jan 25, 2022

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb Jan 24, 2022

Choose a reason for hiding this comment

dougbtv Jan 25, 2022

Choose a reason for hiding this comment

maiqueb commented Jan 26, 2022

xagent003 commented Mar 29, 2022 • edited Loading

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb commented Apr 1, 2022

xagent003 commented Apr 2, 2022

dougbtv commented Jun 23, 2022

maiqueb left a comment •

edited

Loading

xagent003 commented Mar 29, 2022 •

edited

Loading