Skip to content

Commit b6b0842

Browse files
tssuryatssurya
authored
Merge pull request #5300 from arkadeepsen/remove-ex-gw
External gateway: Remove routes for external gateway pods in terminating or not ready state
2 parents 4dce9cb + d565fd8 commit b6b0842

File tree

12 files changed

+1454
-35
lines changed

12 files changed

+1454
-35
lines changed

‎go-controller/pkg/ovn/controller/apbroute/external_controller.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"k8s.io/client-go/tools/cache"
2323
"k8s.io/client-go/util/workqueue"
2424
"k8s.io/klog/v2"
25+
v1pod "k8s.io/kubernetes/pkg/api/v1/pod"
2526

2627
adminpolicybasedrouteapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1"
2728
adminpolicybasedrouteinformer "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1/apis/informers/externalversions/adminpolicybasedroute/v1"
@@ -565,10 +566,14 @@ func (m *externalPolicyManager) onPodUpdate(oldObj, newObj interface{}) {
565566
utilruntime.HandleError(errors.New("invalid Pod provided to onPodUpdate()"))
566567
return
567568
}
568-
// if labels AND assigned Pod IPs AND the multus network status annotations are the same, skip processing changes to the pod.
569+
// if labels AND assigned Pod IPs AND the multus network status annotations AND
570+
// pod PodReady condition AND deletion timestamp (PodTerminating) are
571+
// the same, skip processing changes to the pod.
569572
if reflect.DeepEqual(o.Labels, n.Labels) &&
570573
reflect.DeepEqual(o.Status.PodIPs, n.Status.PodIPs) &&
571-
reflect.DeepEqual(o.Annotations[nettypes.NetworkStatusAnnot], n.Annotations[nettypes.NetworkStatusAnnot]) {
574+
reflect.DeepEqual(o.Annotations[nettypes.NetworkStatusAnnot], n.Annotations[nettypes.NetworkStatusAnnot]) &&
575+
reflect.DeepEqual(v1pod.GetPodReadyCondition(o.Status), v1pod.GetPodReadyCondition(n.Status)) &&
576+
reflect.DeepEqual(o.DeletionTimestamp, n.DeletionTimestamp) {
572577
return
573578
}
574579
m.podQueue.Add(n)

‎go-controller/pkg/ovn/controller/apbroute/external_controller_namespace_test.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -201,14 +201,32 @@ var _ = Describe("OVN External Gateway namespace", func() {
201201
"k8s.ovn.org/routing-network": "",
202202
nettypes.NetworkStatusAnnot: fmt.Sprintf(network_status, annotatedPodIP)},
203203
},
204-
Status: corev1.PodStatus{PodIPs: []corev1.PodIP{{IP: annotatedPodIP}}, Phase: corev1.PodRunning},
204+
Status: corev1.PodStatus{
205+
PodIPs: []corev1.PodIP{{IP: annotatedPodIP}},
206+
Phase: corev1.PodRunning,
207+
Conditions: []corev1.PodCondition{
208+
{
209+
Type: corev1.PodReady,
210+
Status: corev1.ConditionTrue,
211+
},
212+
},
213+
},
205214
}
206215

207216
podGW = &corev1.Pod{
208217
ObjectMeta: metav1.ObjectMeta{Name: "pod", Namespace: namespaceGW.Name,
209218
Labels: map[string]string{"name": "pod"},
210219
Annotations: map[string]string{nettypes.NetworkStatusAnnot: fmt.Sprintf(network_status, dynamicHopHostNetPodIP)}},
211-
Status: corev1.PodStatus{PodIPs: []corev1.PodIP{{IP: dynamicHopHostNetPodIP}}, Phase: corev1.PodRunning},
220+
Status: corev1.PodStatus{
221+
PodIPs: []corev1.PodIP{{IP: dynamicHopHostNetPodIP}},
222+
Phase: corev1.PodRunning,
223+
Conditions: []corev1.PodCondition{
224+
{
225+
Type: corev1.PodReady,
226+
Status: corev1.ConditionTrue,
227+
},
228+
},
229+
},
212230
}
213231
namespaceTargetWithPod, namespaceTarget2WithPod, namespaceTarget2WithoutPod, namespaceGWWithPod *namespaceWithPods
214232
)

‎go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ import (
1111
"k8s.io/apimachinery/pkg/util/sets"
1212
"k8s.io/client-go/util/workqueue"
1313
"k8s.io/klog/v2"
14+
v1pod "k8s.io/kubernetes/pkg/api/v1/pod"
1415
utilnet "k8s.io/utils/net"
16+
17+
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
1518
)
1619

1720
func (m *externalPolicyManager) syncPod(pod *corev1.Pod, routeQueue workqueue.TypedRateLimitingInterface[string]) error {
@@ -28,6 +31,13 @@ func (m *externalPolicyManager) syncPod(pod *corev1.Pod, routeQueue workqueue.Ty
2831
}
2932

3033
func getExGwPodIPs(gatewayPod *corev1.Pod, networkName string) (sets.Set[string], error) {
34+
// If an external gateway pod is in terminating or not ready state then don't return the
35+
// IPs for the external gateway pod
36+
if util.PodTerminating(gatewayPod) || !v1pod.IsPodReadyConditionTrue(gatewayPod.Status) {
37+
klog.Warningf("External gateway pod cannot serve traffic; it's in terminating or not ready state: %s/%s", gatewayPod.Namespace, gatewayPod.Name)
38+
return nil, nil
39+
}
40+
3141
if networkName != "" {
3242
return getMultusIPsFromNetworkName(gatewayPod, networkName)
3343
}

‎go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,163 @@ var _ = Describe("OVN External Gateway pod", func() {
448448
})
449449

450450
})
451+
452+
var _ = Context("When pod goes into terminating or not ready state", func() {
453+
454+
DescribeTable("reconciles a pod gateway in terminating or not ready state that matches two policies", func(
455+
terminating bool,
456+
) {
457+
initController([]runtime.Object{namespaceGW, namespaceTarget, namespaceTarget2, targetPod1, targetPod2, pod1},
458+
[]runtime.Object{dynamicPolicy, dynamicPolicyDiffTargetNS})
459+
460+
expectedPolicy1, expectedRefs1 := expectedPolicyStateAndRefs(
461+
[]*namespaceWithPods{namespaceTarget2WithPod},
462+
nil,
463+
[]*namespaceWithPods{namespaceGWWithPod}, false)
464+
465+
expectedPolicy2, expectedRefs2 := expectedPolicyStateAndRefs(
466+
[]*namespaceWithPods{namespaceTargetWithPod},
467+
nil,
468+
[]*namespaceWithPods{namespaceGWWithPod}, false)
469+
470+
eventuallyExpectNumberOfPolicies(2)
471+
eventuallyExpectConfig(dynamicPolicy.Name, expectedPolicy1, expectedRefs1)
472+
eventuallyExpectConfig(dynamicPolicyDiffTargetNS.Name, expectedPolicy2, expectedRefs2)
473+
474+
if terminating {
475+
By("Setting deletion timestamp for the ex gw pod")
476+
setPodDeletionTimestamp(pod1, &metav1.Time{Time: time.Now().Add(1000 * time.Second)}, fakeClient)
477+
} else {
478+
By("Updating the ex gw pod status to mark it as not ready")
479+
setPodConditionReady(pod1, corev1.ConditionFalse, fakeClient)
480+
}
481+
482+
expectedPolicy1, expectedRefs1 = expectedPolicyStateAndRefs(
483+
[]*namespaceWithPods{namespaceTarget2WithPod},
484+
nil,
485+
[]*namespaceWithPods{namespaceGWWithoutPod}, false)
486+
487+
expectedPolicy2, expectedRefs2 = expectedPolicyStateAndRefs(
488+
[]*namespaceWithPods{namespaceTargetWithPod},
489+
nil,
490+
[]*namespaceWithPods{namespaceGWWithoutPod}, false)
491+
492+
eventuallyExpectNumberOfPolicies(2)
493+
eventuallyExpectConfig(dynamicPolicy.Name, expectedPolicy1, expectedRefs1)
494+
eventuallyExpectConfig(dynamicPolicyDiffTargetNS.Name, expectedPolicy2, expectedRefs2)
495+
},
496+
Entry("Gateway pod in terminating state", true),
497+
Entry("Gateway pod in not ready state", false),
498+
)
499+
500+
DescribeTable("reconciles a pod gateway in terminating or not ready state that does not match any policy", func(
501+
terminating bool,
502+
) {
503+
noMatchPolicy := newPolicy(
504+
"noMatchPolicy",
505+
&metav1.LabelSelector{MatchLabels: targetNamespace1Match},
506+
nil,
507+
&metav1.LabelSelector{MatchLabels: gatewayNamespaceMatch},
508+
&metav1.LabelSelector{MatchLabels: map[string]string{"key": "nomatch"}},
509+
false,
510+
)
511+
initController([]runtime.Object{namespaceGW, namespaceTarget, pod1}, []runtime.Object{noMatchPolicy})
512+
513+
expectedPolicy, expectedRefs := expectedPolicyStateAndRefs(
514+
[]*namespaceWithPods{namespaceTargetWithoutPod},
515+
nil,
516+
[]*namespaceWithPods{namespaceGWWithoutPod}, false)
517+
518+
eventuallyExpectNumberOfPolicies(1)
519+
eventuallyExpectConfig(noMatchPolicy.Name, expectedPolicy, expectedRefs)
520+
521+
if terminating {
522+
By("Setting deletion timestamp for the ex gw pod")
523+
setPodDeletionTimestamp(pod1, &metav1.Time{Time: time.Now().Add(1000 * time.Second)}, fakeClient)
524+
} else {
525+
By("Updating the ex gw pod status to mark it as not ready")
526+
setPodConditionReady(pod1, corev1.ConditionFalse, fakeClient)
527+
}
528+
// make sure pod event is handled
529+
time.Sleep(100 * time.Millisecond)
530+
531+
eventuallyExpectNumberOfPolicies(1)
532+
eventuallyExpectConfig(noMatchPolicy.Name, expectedPolicy, expectedRefs)
533+
},
534+
Entry("Gateway pod in terminating state", true),
535+
Entry("Gateway pod in not ready state", false),
536+
)
537+
538+
DescribeTable("reconciles a pod gateway in terminating or not ready state that is one of two pods that matches two policies", func(
539+
terminating bool,
540+
) {
541+
initController([]runtime.Object{namespaceGW, namespaceTarget, namespaceTarget2, targetPod1, targetPod2, pod1, pod2},
542+
[]runtime.Object{dynamicPolicy, dynamicPolicyDiffTargetNS})
543+
namespaceGWWith2Pods := newNamespaceWithPods(namespaceGW.Name, pod1, pod2)
544+
expectedPolicy1, expectedRefs1 := expectedPolicyStateAndRefs(
545+
[]*namespaceWithPods{namespaceTarget2WithPod},
546+
nil,
547+
[]*namespaceWithPods{namespaceGWWith2Pods}, false)
548+
549+
expectedPolicy2, expectedRefs2 := expectedPolicyStateAndRefs(
550+
[]*namespaceWithPods{namespaceTargetWithPod},
551+
nil,
552+
[]*namespaceWithPods{namespaceGWWith2Pods}, false)
553+
554+
eventuallyExpectNumberOfPolicies(2)
555+
eventuallyExpectConfig(dynamicPolicy.Name, expectedPolicy1, expectedRefs1)
556+
eventuallyExpectConfig(dynamicPolicyDiffTargetNS.Name, expectedPolicy2, expectedRefs2)
557+
558+
if terminating {
559+
By("Setting deletion timestamp for the ex gw pod")
560+
setPodDeletionTimestamp(pod1, &metav1.Time{Time: time.Now().Add(1000 * time.Second)}, fakeClient)
561+
} else {
562+
By("Updating the ex gw pod status to mark it as not ready")
563+
setPodConditionReady(pod1, corev1.ConditionFalse, fakeClient)
564+
}
565+
566+
namespaceGWWith1Pod := newNamespaceWithPods(namespaceGW.Name, pod2)
567+
568+
expectedPolicy1, expectedRefs1 = expectedPolicyStateAndRefs(
569+
[]*namespaceWithPods{namespaceTarget2WithPod},
570+
nil,
571+
[]*namespaceWithPods{namespaceGWWith1Pod}, false)
572+
573+
expectedPolicy2, expectedRefs2 = expectedPolicyStateAndRefs(
574+
[]*namespaceWithPods{namespaceTargetWithPod},
575+
nil,
576+
[]*namespaceWithPods{namespaceGWWith1Pod}, false)
577+
578+
eventuallyExpectNumberOfPolicies(2)
579+
eventuallyExpectConfig(dynamicPolicy.Name, expectedPolicy1, expectedRefs1)
580+
eventuallyExpectConfig(dynamicPolicyDiffTargetNS.Name, expectedPolicy2, expectedRefs2)
581+
582+
if terminating {
583+
By("Removing deletion timestamp for the ex gw pod")
584+
setPodDeletionTimestamp(pod1, nil, fakeClient)
585+
} else {
586+
By("Updating the ex gw pod status to mark it as ready")
587+
setPodConditionReady(pod1, corev1.ConditionTrue, fakeClient)
588+
}
589+
590+
expectedPolicy1, expectedRefs1 = expectedPolicyStateAndRefs(
591+
[]*namespaceWithPods{namespaceTarget2WithPod},
592+
nil,
593+
[]*namespaceWithPods{namespaceGWWith2Pods}, false)
594+
595+
expectedPolicy2, expectedRefs2 = expectedPolicyStateAndRefs(
596+
[]*namespaceWithPods{namespaceTargetWithPod},
597+
nil,
598+
[]*namespaceWithPods{namespaceGWWith2Pods}, false)
599+
600+
eventuallyExpectNumberOfPolicies(2)
601+
eventuallyExpectConfig(dynamicPolicy.Name, expectedPolicy1, expectedRefs1)
602+
eventuallyExpectConfig(dynamicPolicyDiffTargetNS.Name, expectedPolicy2, expectedRefs2)
603+
},
604+
Entry("Gateway pod in terminating state", true),
605+
Entry("Gateway pod in not ready state", false),
606+
)
607+
})
451608
})
452609

453610
func deletePod(pod *corev1.Pod, fakeClient *fake.Clientset) {
@@ -478,6 +635,36 @@ func updatePodStatus(pod *corev1.Pod, podStatus corev1.PodStatus) {
478635
Expect(err).NotTo(HaveOccurred())
479636
}
480637

638+
func setPodDeletionTimestamp(pod *corev1.Pod, deletionTimestamp *metav1.Time, fakeClient *fake.Clientset) {
639+
p, err := fakeClient.CoreV1().Pods(pod.Namespace).Get(context.TODO(), pod.Name, metav1.GetOptions{})
640+
Expect(err).NotTo(HaveOccurred())
641+
incrementResourceVersion(p)
642+
p.DeletionTimestamp = deletionTimestamp
643+
_, err = fakeClient.CoreV1().Pods(pod.Namespace).Update(context.Background(), p, metav1.UpdateOptions{})
644+
Expect(err).NotTo(HaveOccurred())
645+
}
646+
647+
func setPodConditionReady(pod *corev1.Pod, condStatus corev1.ConditionStatus, fakeClient *fake.Clientset) {
648+
p, err := fakeClient.CoreV1().Pods(pod.Namespace).Get(context.TODO(), pod.Name, metav1.GetOptions{})
649+
Expect(err).NotTo(HaveOccurred())
650+
incrementResourceVersion(p)
651+
if p.Status.Conditions != nil {
652+
for i := range p.Status.Conditions {
653+
if p.Status.Conditions[i].Type == corev1.PodReady {
654+
p.Status.Conditions[i].Status = condStatus
655+
}
656+
}
657+
} else {
658+
notReadyCondition := corev1.PodCondition{
659+
Type: corev1.PodReady,
660+
Status: corev1.ConditionFalse,
661+
}
662+
p.Status.Conditions = []corev1.PodCondition{notReadyCondition}
663+
}
664+
_, err = fakeClient.CoreV1().Pods(pod.Namespace).Update(context.Background(), p, metav1.UpdateOptions{})
665+
Expect(err).NotTo(HaveOccurred())
666+
}
667+
481668
func incrementResourceVersion(obj metav1.Object) {
482669
var rs int64
483670
if obj.GetResourceVersion() != "" {

‎go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,16 @@ func newPodWithPhaseAndIP(podName, namespace string, phase corev1.PodPhase, podI
4040
p := &corev1.Pod{
4141
ObjectMeta: metav1.ObjectMeta{Name: podName, Namespace: namespace,
4242
Labels: labels},
43-
Spec: corev1.PodSpec{NodeName: "node"},
44-
Status: corev1.PodStatus{Phase: phase},
43+
Spec: corev1.PodSpec{NodeName: "node"},
44+
Status: corev1.PodStatus{
45+
Phase: phase,
46+
Conditions: []corev1.PodCondition{
47+
{
48+
Type: corev1.PodReady,
49+
Status: corev1.ConditionTrue,
50+
},
51+
},
52+
},
4553
}
4654
if len(podIP) > 0 {
4755
p.Annotations = map[string]string{nettypes.NetworkStatusAnnot: fmt.Sprintf(network_status, podIP)}

‎go-controller/pkg/ovn/egressgw.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
ktypes "k8s.io/apimachinery/pkg/types"
1616
"k8s.io/apimachinery/pkg/util/sets"
1717
"k8s.io/klog/v2"
18+
v1pod "k8s.io/kubernetes/pkg/api/v1/pod"
1819
utilnet "k8s.io/utils/net"
1920

2021
libovsdbclient "github.com/ovn-kubernetes/libovsdb/client"
@@ -49,6 +50,13 @@ func (oc *DefaultNetworkController) addPodExternalGW(pod *corev1.Pod) error {
4950

5051
klog.Infof("External gateway pod: %s, detected for namespace(s) %s", pod.Name, podRoutingNamespaceAnno)
5152

53+
// If an external gateway pod is in terminating or not ready state then don't add the
54+
// routes for the external gateway pod
55+
if util.PodTerminating(pod) || !v1pod.IsPodReadyConditionTrue(pod.Status) {
56+
klog.Warningf("External gateway pod cannot serve traffic; it's in terminating or not ready state: %s/%s", pod.Namespace, pod.Name)
57+
return nil
58+
}
59+
5260
foundGws, err := getExGwPodIPs(pod)
5361
if err != nil {
5462
klog.Errorf("Error getting exgw IPs for pod: %s, error: %v", pod.Name, err)

0 commit comments

Comments
 (0)