From 6c13135c2e834627f9ab51c9665ce82f99a2567a Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Fri, 24 Jan 2025 12:02:17 +0100 Subject: [PATCH] [BGP] Fix FRRConfiguration cleanup Currently the process of processing the FRRConfigurations is: * get a list of all pods in the ctlplane namespace * filter that list to the pods which have a secondary IF configured * run through that list of pods with an IF attached and validate if there is a change of the node it is running on. While running through that nested loop to check if there is a an update needed we are also checking there is an frr config for an already deleted pod. This is ok unless there is at least one pod with a secondary IF, if not the cleanup won't happen, which is the case for the FRRConfiguration for the last deleted pod. This issue was only seen randomly because the functional test was not waiting for the pod and its FRRConfiguration to exist before it started the delete test. This change fixes the functional test and moves the cleanup out of the mentioned loop so it also gets checked properly when the last pod gets deleted. Signed-off-by: Martin Schuppert --- .../network/bgpconfiguration_controller.go | 56 +++---- .../bgpconfiguration_controller_test.go | 140 ++++++++++++++++-- 2 files changed, 158 insertions(+), 38 deletions(-) diff --git a/controllers/network/bgpconfiguration_controller.go b/controllers/network/bgpconfiguration_controller.go index 5324953d..6d4f965c 100644 --- a/controllers/network/bgpconfiguration_controller.go +++ b/controllers/network/bgpconfiguration_controller.go @@ -353,7 +353,6 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan // Get a list of pods which are in the same namespace as the ctlplane // to verify if a FRRConfiguration needs to be created for. podList := &corev1.PodList{} - listOpts := []client.ListOption{ client.InNamespace(instance.Namespace), } @@ -367,18 +366,19 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan return ctrl.Result{}, err } + // get all frrconfigs + frrConfigList := &frrk8sv1.FRRConfigurationList{} + listOpts = []client.ListOption{ + client.InNamespace(instance.Spec.FRRConfigurationNamespace), // defaults to metallb-system + } + if err := r.Client.List(ctx, frrConfigList, listOpts...); err != nil { + return ctrl.Result{}, fmt.Errorf("Unable to retrieve FRRConfigurationList %w", err) + } + // get all frr configs for the nodes pods are scheduled on groupLabel := labels.GetGroupLabel("bgpconfiguration") frrNodeConfigs := map[string]frrk8sv1.FRRConfiguration{} for _, nodeName := range bgp.GetNodesRunningPods(podNetworkDetailList) { - frrConfigList := &frrk8sv1.FRRConfigurationList{} - listOpts := []client.ListOption{ - client.InNamespace(instance.Spec.FRRConfigurationNamespace), // defaults to metallb-system - } - if err := r.Client.List(ctx, frrConfigList, listOpts...); err != nil { - return ctrl.Result{}, fmt.Errorf("Unable to retrieve FRRConfigurationList %w", err) - } - var nodeSelector metav1.LabelSelector // validate if a nodeSelector is configured for the nodeName @@ -405,23 +405,7 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan for _, cfg := range frrConfigList.Items { frrLabels := cfg.GetLabels() // skip checking our own managed FRRConfigurations, - // but validate if the FRRConfiguration is still needed, - // or the pod was deleted/completed/failed/unknown if _, ok := frrLabels[labels.GetOwnerNameLabelSelector(labels.GetGroupLabel("bgpconfiguration"))]; ok { - if podName, ok := frrLabels[groupLabel+"/pod-name"]; ok { - f := func(p bgp.PodDetail) bool { - return p.Name == podName && p.Namespace == instance.Namespace - } - idx := slices.IndexFunc(podNetworkDetailList, f) - if idx < 0 { - // There is no pod in the namespace corrsponding to the FRRConfiguration, delete it - if err := r.Client.Delete(ctx, &cfg); err != nil && k8s_errors.IsNotFound(err) { - return ctrl.Result{}, fmt.Errorf("Unable to delete FRRConfiguration %w", err) - } - Log.Info(fmt.Sprintf("pod with name: %s either in state deleted, completed, failed or unknown , deleted FRRConfiguration %s", podName, cfg.Name)) - } - } - // skip our own managed FRRConfigurations continue } if equality.Semantic.DeepEqual(cfg.Spec.NodeSelector, nodeSelector) { @@ -455,6 +439,27 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan } } + // delete our managed FRRConfigurations where there is no longer a pod in podNetworkDetailList + // because the pod was deleted/completed/failed/unknown + for _, cfg := range frrConfigList.Items { + frrLabels := cfg.GetLabels() + if _, ok := frrLabels[labels.GetOwnerNameLabelSelector(labels.GetGroupLabel("bgpconfiguration"))]; ok { + if podName, ok := frrLabels[groupLabel+"/pod-name"]; ok { + f := func(p bgp.PodDetail) bool { + return p.Name == podName && p.Namespace == instance.Namespace + } + idx := slices.IndexFunc(podNetworkDetailList, f) + if idx < 0 { + // There is no pod in the namespace corrsponding to the FRRConfiguration, delete it + if err := r.Client.Delete(ctx, &cfg); err != nil && k8s_errors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("Unable to delete FRRConfiguration %w", err) + } + Log.Info(fmt.Sprintf("pod with name: %s either in state deleted, completed, failed or unknown , deleted FRRConfiguration %s", podName, cfg.Name)) + } + } + } + } + Log.Info("Reconciled Service successfully") return ctrl.Result{}, nil } @@ -474,6 +479,7 @@ func getPodNetworkDetails( for _, pod := range pods.Items { // skip pods which are in deletion to make sure its FRRConfiguration gets deleted if !pod.DeletionTimestamp.IsZero() { + Log.Info(fmt.Sprintf("Skipping pod as its in deletion with DeletionTimestamp set: %s", pod.Name)) continue } // skip pods which are not in Running phase (deleted/completed/failed/unknown) diff --git a/tests/functional/bgpconfiguration_controller_test.go b/tests/functional/bgpconfiguration_controller_test.go index 7274c2cf..d86ca1df 100644 --- a/tests/functional/bgpconfiguration_controller_test.go +++ b/tests/functional/bgpconfiguration_controller_test.go @@ -21,6 +21,7 @@ import ( . "github.com/onsi/ginkgo/v2" //revive:disable:dot-imports . "github.com/onsi/gomega" //revive:disable:dot-imports "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" frrk8sv1 "github.com/metallb/frr-k8s/api/v1beta1" corev1 "k8s.io/api/core/v1" @@ -179,9 +180,89 @@ var _ = Describe("BGPConfiguration controller", func() { }) }) + When("a pod gets re-created on another node", func() { + var podFrrNameList []types.NamespacedName + var podNameList []types.NamespacedName + + BeforeEach(func() { + metallbNS := th.CreateNamespace(frrCfgNamespace + "-" + namespace) + // create a FRR configuration for 2 nodes + meallbFRRCfgWorker0 := CreateFRRConfiguration( + types.NamespacedName{Namespace: metallbNS.Name, Name: "worker-0"}, + GetMetalLBFRRConfigurationSpec("worker-0")) + Expect(meallbFRRCfgWorker0).To(Not(BeNil())) + meallbFRRCfgWorker1 := CreateFRRConfiguration( + types.NamespacedName{Namespace: metallbNS.Name, Name: "worker-1"}, + GetMetalLBFRRConfigurationSpec("worker-1")) + Expect(meallbFRRCfgWorker1).To(Not(BeNil())) + + // create a nad config with gateway + nad := th.CreateNAD(types.NamespacedName{Namespace: namespace, Name: "internalapi"}, GetNADSpec()) + + bgpcfg := CreateBGPConfiguration(namespace, GetBGPConfigurationSpec(metallbNS.Name)) + bgpcfgName.Name = bgpcfg.GetName() + bgpcfgName.Namespace = bgpcfg.GetNamespace() + + podNameList = []types.NamespacedName{ + {Namespace: namespace, Name: "foo"}, + {Namespace: namespace, Name: "bar"}, + } + + for _, podName := range podNameList { + // create pod with NAD annotation + th.CreatePod(podName, GetPodAnnotation(namespace), GetPodSpec("worker-0")) + th.SimulatePodPhaseRunning(podName) + + podFrrName := types.NamespacedName{ + Name: podName.Namespace + "-" + podName.Name, + Namespace: metallbNS.Name, + } + podFrrNameList = append(podFrrNameList, podFrrName) + + Eventually(func(g Gomega) { + frr := GetFRRConfiguration(podFrrName) + g.Expect(frr).To(Not(BeNil())) + }, timeout, interval).Should(Succeed()) + } + + DeferCleanup(th.DeleteInstance, bgpcfg) + DeferCleanup(th.DeleteInstance, nad) + DeferCleanup(th.DeleteInstance, meallbFRRCfgWorker0) + DeferCleanup(th.DeleteInstance, meallbFRRCfgWorker1) + }) + + It("should re-create/update the FRRConfiguration with the new nodeselector", func() { + // delete pod foo + pod := th.GetPod(podNameList[0]) + Expect(pod).To(Not(BeNil())) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Delete(ctx, pod)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + // validate that the corresponding frr cfg is gone + frr := &frrk8sv1.FRRConfiguration{} + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, podFrrNameList[0], frr)).Should(Not(Succeed())) + }, timeout, interval).Should(Succeed()) + + // re-create pod on different node + th.CreatePod(podNameList[0], GetPodAnnotation(namespace), GetPodSpec("worker-1")) + th.SimulatePodPhaseRunning(podNameList[0]) + + Eventually(func(g Gomega) { + frr := GetFRRConfiguration(podFrrNameList[0]) + g.Expect(frr).To(Not(BeNil())) + g.Expect(frr.Spec.NodeSelector.MatchLabels).Should(BeEquivalentTo( + map[string]string{ + corev1.LabelHostname: "worker-1", + })) + }, timeout, interval).Should(Succeed()) + }) + }) + When("a pod with NAD gets deleted", func() { - var podFrrName types.NamespacedName - var podName types.NamespacedName + var podFrrNameList []types.NamespacedName + var podNameList []types.NamespacedName BeforeEach(func() { metallbNS := th.CreateNamespace(frrCfgNamespace + "-" + namespace) @@ -197,32 +278,65 @@ var _ = Describe("BGPConfiguration controller", func() { bgpcfgName.Name = bgpcfg.GetName() bgpcfgName.Namespace = bgpcfg.GetNamespace() - podName = types.NamespacedName{Namespace: namespace, Name: uuid.New().String()} - // create pod with NAD annotation - th.CreatePod(podName, GetPodAnnotation(namespace), GetPodSpec("worker-0")) - th.SimulatePodPhaseRunning(podName) + podNameList = []types.NamespacedName{ + {Namespace: namespace, Name: "foo"}, + {Namespace: namespace, Name: "bar"}, + } - podFrrName.Name = podName.Namespace + "-" + podName.Name - podFrrName.Namespace = metallbNS.Name + for _, podName := range podNameList { + // create pod with NAD annotation + th.CreatePod(podName, GetPodAnnotation(namespace), GetPodSpec("worker-0")) + th.SimulatePodPhaseRunning(podName) + + podFrrName := types.NamespacedName{ + Name: podName.Namespace + "-" + podName.Name, + Namespace: metallbNS.Name, + } + podFrrNameList = append(podFrrNameList, podFrrName) + + Eventually(func(g Gomega) { + frr := GetFRRConfiguration(podFrrName) + g.Expect(frr).To(Not(BeNil())) + }, timeout, interval).Should(Succeed()) + } DeferCleanup(th.DeleteInstance, bgpcfg) DeferCleanup(th.DeleteInstance, nad) DeferCleanup(th.DeleteInstance, meallbFRRCfg) }) - It("should delete the FRRConfiguration for the pod", func() { - // delete the pod - pod := th.GetPod(podName) + It("should delete the FRRConfiguration when on pod gets deleted", func() { + // delete pod foo + pod := th.GetPod(podNameList[0]) Expect(pod).To(Not(BeNil())) Eventually(func(g Gomega) { g.Expect(k8sClient.Delete(ctx, pod)).Should(Succeed()) }, timeout, interval).Should(Succeed()) - // validate that the frr cfg is gone + // validate that the corresponding frr cfg is gone frr := &frrk8sv1.FRRConfiguration{} Eventually(func(g Gomega) { - g.Expect(k8sClient.Get(ctx, podFrrName, frr)).Should(Not(Succeed())) + g.Expect(k8sClient.Get(ctx, podFrrNameList[0], frr)).Should(Not(Succeed())) + }, timeout, interval).Should(Succeed()) + }) + + It("should delete all FRRConfiguration when all pod gets deleted", func() { + // delete all pod + Eventually(func(g Gomega) { + g.Expect(k8sClient.DeleteAllOf( + ctx, + &corev1.Pod{}, + client.InNamespace(podNameList[0].Namespace), + )).Should(Succeed()) }, timeout, interval).Should(Succeed()) + + for idx := range podNameList { + // validate that the frr cfgs are gone + frr := &frrk8sv1.FRRConfiguration{} + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, podFrrNameList[idx], frr)).Should(Not(Succeed())) + }, timeout, interval).Should(Succeed()) + } }) }) })