Make podRef to be unique #433

pliurh · 2024-03-04T08:17:27Z

We used podRef with format <namespace>/<pod_name> to identify the IP allocation. However, in scenarios like replicaSet, a different pod can reuse the pod name. This patch changes the format of podRef to <namespace>/<pod_name>:<pod_uid> to ensure each ip allocation will have a unique identifier. The legacy podRef format will be updated automatically during an upgrade.

Sometimes, we want to run the reconciler outside of the cluster for development convenience. This patch allows the reconciler process to use the KUBECONFIG environment variable. Signed-off-by: Peng Liu <[email protected]>

pliurh · 2024-03-04T08:20:47Z

fix #176

maiqueb · 2024-03-04T08:40:10Z

Why have you chosen that format - /<pod_name>:<pod_uid> ?

If you just need the pod ref to be unique, shouldn't we simply use the pod's UID ?

Is the name part meant to help debugging or something like that ?

pliurh · 2024-03-04T08:45:18Z

The new naming format shall be namespace/pod_name:pod_uid. And yes, it's meant for debugging.

dougbtv

This looks good to me. I'd love to have another review on it before a merge, but overall this approach makes sense. Thank you Peng!

dougbtv · 2024-03-04T18:16:40Z

cmd/controlloop/controlloop.go

+		cfg, err = clientcmd.BuildConfigFromFlags("", kubeConfigFile)
+		if err != nil {
+			return nil, fmt.Errorf("failed to generate kubeconfig from file %s: %v", kubeConfigFile, err)
+		}


thanks for the improved logging on error

dougbtv · 2024-03-04T18:19:58Z

pkg/reconciler/iploop.go

+	}
+
+	// migrate the podRef format if needed
+	if err := looper.migrationPodRef(ctx, ipPools); err != nil {


Ok, I think I get it how it triggers the migration, so here in the control loop, we check if the migration has happened and apply it here.

dougbtv · 2024-03-04T18:22:58Z

pkg/reconciler/iploop.go

-		k8sClient:           *k8sClient,
-		liveWhereaboutsPods: indexPods(pods, whereaboutsPodRefs),
-		requestTimeout:      timeout,
+	pods, err := k8sClient.ListPods(ctx)


Can we limit this to just pods with */networks annotations maybe? Unsure if this is premature optimization?

Something like:

// Define label selector to filter pods with specific annotations labelSelector := metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { Key: "k8s.v1.cni.cncf.io/networks", Operator: metav1.LabelSelectorOpExists, }, }, } // List pods matching the label selector pods, err := k8sClient.CoreV1().Pods("").List(ctx, metav1.ListOptions{ LabelSelector: metav1.FormatLabelSelector(&labelSelector), })

Sorry I had this wrong, this is abstracted. It's fine as is. Thanks!

dougbtv · 2024-03-04T18:24:31Z

pkg/reconciler/iploop_test.go

+	fakewbclient "github.com/k8snetworkplumbingwg/whereabouts/pkg/client/clientset/versioned/fake"
+)
+
+var _ = FDescribe("MigrationPodRef", func() {


Thanks for the migration unit test.

You need to remove the focus from these tests, otherwise, you'll highjack the CI to only run these tests.

Good catch. I forgot it.

maiqueb · 2024-03-04T19:01:58Z

@dougbtv please allow me some time to review this PR.

@pliurh you need to sort out the e2e and the unit tests.

I'll keep peeking to see once they're good.

pliurh · 2024-03-06T12:27:19Z

/hold

pliurh · 2024-03-07T04:22:31Z

/unhold
@maiqueb @dougbtv I revised the PR. Please take another look.

We used podRef with format <namespace>/<pod_name> to identify the IP allocation. However, in scenarios like replicaSet, a different pod can reuse the pod name. This patch changes the format of podRef to <namespace>/<pod_name>:<pod_uid> to ensure each ip allocation will have a unique identifier. The legacy podRef format will be updated automatically during an upgrade. Signed-off-by: Peng Liu <[email protected]>

The containerID of a pod will change after node reboot. It caused the IP allocation/deallocation operation cannot find the IP that has been reserved by the pod. Signed-off-by: Peng Liu <[email protected]>

andreaskaris · 2024-03-20T16:06:01Z

cmd/controlloop/controlloop.go

 	if err != nil {
-		return nil, fmt.Errorf("failed to implicitly generate the kubeconfig: %w", err)
+		logging.Debugf("failed to generate the kubeconfig from service account: %v", err)
+		kubeConfigFile := os.Getenv("KUBECONFIG")


both newPodController and ReconcileIPs are exclusively called from controlloop.go:main().
I know that the operation doesn't cost anything, but it rubs me the wrong way to do kubeConfigFile := os.Getenv("KUBECONFIG") in each of them, the correct place imo is in controlloop.go:main() and then pass kubeConfigFile as an argument to both newPodController and ReconcileIPs . Perhaps that allows even for more optimizations

andreaskaris · 2024-03-20T16:07:19Z

cmd/controlloop/controlloop.go

-	cfg, err := rest.InClusterConfig()
+	var cfg *rest.Config
+	var err error
+	cfg, err = rest.InClusterConfig()


If this is run inside the cluster, but the KUBECONFIG env variable is set:

ip.go will use NewReconcileLooperWithKubeconfig due to the if condition

newPodController will use rest.InClusterConfig because in cluster rest.InClusterConfig will succeed

So your code can end up using the in cluster config for one, and the kubeconfig for the other

andreaskaris · 2024-03-20T16:13:07Z

cmd/controlloop/controlloop.go

@@ -39,6 +39,7 @@ const (
 	cronSchedulerCreationError
 	fileWatcherError
 	couldNotCreateConfigWatcherError
+	initialReconsileFailed


nit: initialReconsileFailed -> initialReconcileFailed

andreaskaris · 2024-03-20T16:21:12Z

cmd/controlloop/controlloop.go

+	// trigger one immediate reconcile before cron job start
+	go reconciler.ReconcileIPs(errorChan)
+	err = <-errorChan
+	if err == nil {


nit:

if err = <-errorChan; err != nil { logging.Verbosef("initial reconciler failure: %s", err) os.Exit(initialReconcileFailed) } logging.Verbosef("initial reconciler success")

(see https://go.dev/play/p/ltsR09kXKW9)

andreaskaris · 2024-03-20T17:58:16Z

pkg/reconciler/iploop.go

+		return logging.Errorf("failed to list overlappingrangeipreservations %v", err)
+	}
+	for _, ip := range reservations {
+		if regexp.MustCompile(pattern).MatchString(ip.Spec.PodRef) {


nit - the regexp doesn't have to be compiled on every loop iteration:

const ( podRefPattern = `^([^\s/]+)/([^\s/]+):([^\s/]+)$` ) (...) func (rl ReconcileLooper) migrationPodRef(ctx context.Context, ipPools []storage.IPPool) error { re := regexp.MustCompile(podRefPattern) (...) for _, ip := range reservations { if re.MatchString(ip.Spec.PodRef) {

andreaskaris · 2024-03-20T17:59:00Z

pkg/reconciler/iploop.go

+		ipReservations = nil
+		needUpdate := false
+		for _, r := range pool.Allocations() {
+			if !regexp.MustCompile(pattern).MatchString(r.PodRef) {


andreaskaris · 2024-03-20T18:04:07Z

pkg/reconciler/iploop_test.go

+			Expect(err).NotTo(HaveOccurred())
+
+			found := false
+			pattern := `^([^\s/]+)/([^\s/]+):([^\s/]+)$`


nit: reuse podRefPattern from iploop.go once that const is created, instead of redefining the pattern (less error prone):

const ( podRefPattern = `^([^\s/]+)/([^\s/]+):([^\s/]+)$` )

andreaskaris · 2024-03-20T18:04:49Z

pkg/reconciler/iploop_test.go

+			Expect(err).NotTo(HaveOccurred())
+			for _, pool := range ipPools {
+				for _, r := range pool.Allocations() {
+					if !regexp.MustCompile(pattern).MatchString(r.PodRef) {


nit: dito as earlier, compile outside of the loop, instead of compiling every single time

andreaskaris · 2024-03-20T18:05:42Z

pkg/reconciler/iploop_test.go

+			ips, err := reconcileLooper.k8sClient.ListOverlappingIPs(ctx)
+			Expect(err).NotTo(HaveOccurred())
+			for _, ip := range ips {
+				if !regexp.MustCompile(pattern).MatchString(ip.Spec.PodRef) {


andreaskaris · 2024-03-20T18:10:08Z

pkg/storage/kubernetes/client.go

+		return nil, logging.Errorf("Error marshaling overlappingrangeipreservations.whereabouts.cni.cncf.io spec: %v", err)
+	}
+
+	return i.client.WhereaboutsV1alpha1().OverlappingRangeIPReservations(clusterWideIP.GetNamespace()).Patch(


question: does this have to be Patch, don't we have an Update() for that? (I might be completely wrong)

regardless, isn't https://pkg.go.dev/k8s.io/client-go/util/retry#RetryOnConflict needed?

The IP reconciler runs on multiple nodes, there will be racing conditions if using Update() in this case. However, using Patch() could cause too many API requests. We may need a leader election here.

I agree that we need to add a retry mechanism. This is a general problem in the IP reconciler, it doesn’t retry when reconciliation fails.

andreaskaris · 2024-03-22T16:06:29Z

I've got a question ... will this actually work with Statefulsets and guarantee that the same IP address is always assigned to the same Statefulset pod? Because the uid between Statefulset pods changes:

 resourceVersion: ""
[akaris@workstation ~]$ oc get pods -o yaml | grep uid
      uid: b5a56769-43e0-4d8c-85bd-8a5f105986fb
    uid: 4ecd9b66-976e-4be3-84fa-cfe7939abfa9
[akaris@workstation ~]$ oc get pods -o yaml | grep uid
      uid: b5a56769-43e0-4d8c-85bd-8a5f105986fb
    uid: 4ecd9b66-976e-4be3-84fa-cfe7939abfa9
[akaris@workstation ~]$ oc delete pod tools-ipv4-0
pod "tools-ipv4-0" deleted
[akaris@workstation ~]$ 
[akaris@workstation ~]$ oc get pods
NAME           READY   STATUS              RESTARTS   AGE
tools-ipv4-0   0/1     ContainerCreating   0          3s
[akaris@workstation ~]$ oc get pods -o yaml | grep uid
      uid: b5a56769-43e0-4d8c-85bd-8a5f105986fb
    uid: f4dc518a-da66-4bad-832b-32394ea08299
[akaris@workstation ~]$ 
[akaris@workstation ~]$ 
[akaris@workstation ~]$ oc get pods -o yaml | grep uid
      uid: b5a56769-43e0-4d8c-85bd-8a5f105986fb
    uid: f4dc518a-da66-4bad-832b-32394ea08299
[akaris@workstation ~]$ 
[akaris@workstation ~]$ 
[akaris@workstation ~]$ oc get pods -o yaml | grep uid
      uid: b5a56769-43e0-4d8c-85bd-8a5f105986fb
    uid: f4dc518a-da66-4bad-832b-32394ea08299

I'm trying to understand when and why it would ever be a problem that an allocation is being reused (in the case of Statefulsets, isn't it even desired that an allocation is reused?) when the combination of namespace/name is actually a unique identifier for a pod https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/

.. well, just looking at this right now in an SNO cluster that I have, the IP address of my single statefulset pod is actually changed after reboot ..

To clarify my point:

to ensure each ip allocation will have a unique identifier.

The commit's comment is misleading, because the identifier <namespace>/<name> is guaranteed to be unique, always, by kubernetes:
https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/. That's used for e.g. statefulsets, where the pod uid is not the same, but the pod after recreation is "the same" for a set of specific attributes.
So yes, identity is not the same ... but as an admin, I personally would expect key attributes to be the same, and I'd say I'd expect whereabouts to assign the same IP address! So, if pod uid-a goes down, and pod uid-b comes up with the same <namespace>/<name>, ideally, whereabouts would assign the exact same IP address; at the very least, it shouldn't matter if it assigned the same IP.

In kubernetes, there will never ever be 2 pods with the same <namespace>/<name> at the same time. So I would argue (not looking at the current implementation) that the allocator should look at the current allocations, and if an allocation for <namespace>/<pod> already exists, it should return the IP address and maintain the allocation, instead of trying to de-allocate and then re-allocate a new IP; wouldn't that likewise fix https://issues.redhat.com/browse/OCPBUGS-29648 and be easier / cleaner?

pliurh · 2024-03-25T07:54:34Z

@andreaskaris I think you made a great point. Instead of addressing it from the IP reconciler's perspective, resolving it at the allocation stage would be more effective.

pliurh · 2024-03-27T07:31:40Z

replaced by #446

Support running ip reconciler with KUBECONFIG

c9e55dd

Sometimes, we want to run the reconciler outside of the cluster for development convenience. This patch allows the reconciler process to use the KUBECONFIG environment variable. Signed-off-by: Peng Liu <[email protected]>

pliurh requested review from maiqueb and dougbtv as code owners March 4, 2024 08:17

pliurh force-pushed the podref branch from 43fc151 to 9812f44 Compare March 4, 2024 08:19

pliurh force-pushed the podref branch from 9812f44 to 68a693c Compare March 4, 2024 08:43

pliurh force-pushed the podref branch from 68a693c to 4e479a6 Compare March 4, 2024 09:25

dougbtv approved these changes Mar 4, 2024

View reviewed changes

pliurh force-pushed the podref branch 3 times, most recently from 9fa1805 to 037d68b Compare March 6, 2024 04:33

pliurh force-pushed the podref branch 2 times, most recently from 3a84167 to 84bcac4 Compare March 7, 2024 03:49

pliurh force-pushed the podref branch 6 times, most recently from 5e0d6ab to b823fee Compare March 11, 2024 07:50

pliurh force-pushed the podref branch from b823fee to 809528c Compare March 12, 2024 05:14

User podRef as indentifier in IP allocation/deallocation

8402e6b

The containerID of a pod will change after node reboot. It caused the IP allocation/deallocation operation cannot find the IP that has been reserved by the pod. Signed-off-by: Peng Liu <[email protected]>

pliurh force-pushed the podref branch from 809528c to 8402e6b Compare March 18, 2024 02:25

andreaskaris reviewed Mar 20, 2024

View reviewed changes

pliurh closed this Mar 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make podRef to be unique #433

Make podRef to be unique #433

pliurh commented Mar 4, 2024 •

edited

Loading

pliurh commented Mar 4, 2024

maiqueb commented Mar 4, 2024

pliurh commented Mar 4, 2024

dougbtv left a comment

dougbtv Mar 4, 2024

dougbtv Mar 4, 2024

dougbtv Mar 4, 2024

dougbtv Mar 4, 2024

dougbtv Mar 4, 2024

maiqueb Mar 4, 2024

pliurh Mar 5, 2024

maiqueb commented Mar 4, 2024

pliurh commented Mar 6, 2024

pliurh commented Mar 7, 2024

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024 •

edited

Loading

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024 •

edited

Loading

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024

andreaskaris Mar 20, 2024 •

edited

Loading

pliurh Mar 21, 2024

andreaskaris commented Mar 22, 2024 •

edited

Loading

pliurh commented Mar 25, 2024

pliurh commented Mar 27, 2024

Make podRef to be unique #433

Make podRef to be unique #433

Conversation

pliurh commented Mar 4, 2024 • edited Loading

pliurh commented Mar 4, 2024

maiqueb commented Mar 4, 2024

pliurh commented Mar 4, 2024

dougbtv left a 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

maiqueb commented Mar 4, 2024

pliurh commented Mar 6, 2024

pliurh commented Mar 7, 2024

Choose a reason for hiding this comment

andreaskaris Mar 20, 2024 • 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

andreaskaris Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskaris Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskaris commented Mar 22, 2024 • edited Loading

pliurh commented Mar 25, 2024

pliurh commented Mar 27, 2024

pliurh commented Mar 4, 2024 •

edited

Loading

andreaskaris Mar 20, 2024 •

edited

Loading

andreaskaris Mar 20, 2024 •

edited

Loading

andreaskaris Mar 20, 2024 •

edited

Loading

andreaskaris commented Mar 22, 2024 •

edited

Loading