Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BGP] Fix FRRConfiguration cleanup #340

Merged
merged 1 commit into from
Feb 4, 2025
Merged

[BGP] Fix FRRConfiguration cleanup #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 31 additions & 25 deletions controllers/network/bgpconfiguration_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
Expand Down
140 changes: 127 additions & 13 deletions tests/functional/bgpconfiguration_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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())
}
})
})
})
Loading