From 04b9d312afbfc175575d3ee817a810d7ed5648c6 Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 20 Aug 2026 17:15:29 +0000 Subject: [PATCH 01/12] Add a K8s readiness check for Cisco pods. Cisco pods may have a running process, but fail to come to "Router up" state. Prior to this change, KNE just hangs forever waiting for Router up even for permanent failures. This resolves this in two ways (belt and suspenders.) First, for Cisco pods we add the check for "Router up" as a readiness check. That way, a failure ends up reflected in K8s' reported pod status. The pod will not transition to Ready state until that message is logged. And if it doesn't show up within the threshold, the pod will report as failed. Second, KNE explicitly checks for failure in the logs and treats it as such, rather than just waiting indefinitely. KNE may notice first, since the failure message will likely arrive before the readiness check exceeds its threshold. --- topo/node/cisco/cisco.go | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index 76b876bc..3f7cdf4d 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -114,7 +114,10 @@ var ( } ) -var podIsUpRegex = regexp.MustCompile(`Router up`) +var ( + podIsUpRegex = regexp.MustCompile(`Router up`) + podIsFailedRegex = regexp.MustCompile(`Router failed to come up|FATAL sim:|LoginTimeoutError`) +) func New(nodeImpl *node.Impl) (node.Node, error) { if nodeImpl == nil { @@ -180,6 +183,19 @@ func (n *Node) Create(ctx context.Context) error { tty = true stdin = true } + var readinessProbe *corev1.Probe + if pb.Model != ModelXRD { + readinessProbe = &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + Exec: &corev1.ExecAction{ + Command: []string{"sh", "-c", "grep -q 'Router up' /nobackup/vxr.out/logs/localhost/sim-check.log 2>/dev/null || grep -q 'Router up' /var/log/vxr.log 2>/dev/null"}, + }, + }, + InitialDelaySeconds: 30, + PeriodSeconds: 10, + FailureThreshold: 30, + } + } pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: n.Name(), @@ -200,6 +216,7 @@ func (n *Node) Create(ctx context.Context) error { Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", SecurityContext: secContext, + ReadinessProbe: readinessProbe, VolumeMounts: []corev1.VolumeMount{{ Name: fmt.Sprintf("%s-run-mount", pb.Name), ReadOnly: false, @@ -630,6 +647,11 @@ func (n *Node) Status(ctx context.Context) (node.Status, error) { pb := n.Proto if pb.GetModel() != ModelXRD { req := n.KubeClient.CoreV1().Pods(p[0].Namespace).GetLogs(p[0].Name, &corev1.PodLogOptions{}) + if isNode8000eFailed(ctx, req) { + log.Warningf("Cisco %s node %s failed to boot", n.Proto.Model, n.Name()) + return node.StatusFailed, fmt.Errorf("cisco %s node %s failed to boot", n.Proto.Model, n.Name()) + } + req = n.KubeClient.CoreV1().Pods(p[0].Namespace).GetLogs(p[0].Name, &corev1.PodLogOptions{}) if !isNode8000eUp(ctx, req) { log.V(2).Infof("Cisco %s node %s status is %v", n.Proto.Model, n.Name(), node.StatusPending) return node.StatusPending, nil @@ -662,6 +684,20 @@ func isNode8000eUp(ctx context.Context, req *rest.Request) bool { return podIsUpRegex.Match(buf.Bytes()) } +func isNode8000eFailed(ctx context.Context, req *rest.Request) bool { + podLogs, err := req.Stream(ctx) + if err != nil { + return false + } + defer podLogs.Close() + buf := new(bytes.Buffer) + len, err := io.Copy(buf, podLogs) + if err != nil || len == 0 { + return false + } + return podIsFailedRegex.Match(buf.Bytes()) +} + // No op function to override default network on open function. func noOp(d *scraplinetwork.Driver) error { return nil From 69ce9e4494289795f852b9e9e0d48c28784710d6 Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 20 Aug 2026 17:53:37 +0000 Subject: [PATCH 02/12] Check log close error --- topo/node/cisco/cisco.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index 3f7cdf4d..d19f6cd8 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -675,7 +675,11 @@ func isNode8000eUp(ctx context.Context, req *rest.Request) bool { if err != nil { return false } - defer podLogs.Close() + defer func() { + if err := podLogs.Close(); err != nil { + log.V(2).Infof("Failed to close pod logs stream: %v", err) + } + }() buf := new(bytes.Buffer) len, err := io.Copy(buf, podLogs) if err != nil || len == 0 { @@ -689,7 +693,11 @@ func isNode8000eFailed(ctx context.Context, req *rest.Request) bool { if err != nil { return false } - defer podLogs.Close() + defer func() { + if err := podLogs.Close(); err != nil { + log.V(2).Infof("Failed to close pod logs stream: %v", err) + } + }() buf := new(bytes.Buffer) len, err := io.Copy(buf, podLogs) if err != nil || len == 0 { From 691bfb7063aaf4b674ab3ebadd75f517367969d4 Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 20 Aug 2026 22:52:26 +0000 Subject: [PATCH 03/12] Check the correct file for "Router up" as readiness --- topo/node/cisco/cisco.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index d19f6cd8..c89932a7 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -188,7 +188,10 @@ func (n *Node) Create(ctx context.Context) error { readinessProbe = &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ Exec: &corev1.ExecAction{ - Command: []string{"sh", "-c", "grep -q 'Router up' /nobackup/vxr.out/logs/localhost/sim-check.log 2>/dev/null || grep -q 'Router up' /var/log/vxr.log 2>/dev/null"}, + Command: []string{ + "sh", "-c", + "grep -q 'Router up' /nobackup/root/pyvxr/vxr.out/logs/console.R0.log 2>/dev/null", + }, }, }, InitialDelaySeconds: 30, From e7cba320a632a5ccc148e74b7bcf01da994f1ef9 Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Fri, 21 Aug 2026 19:43:17 +0000 Subject: [PATCH 04/12] Move to a more K8s native readiness check - gRPC port is open The log check isn't working. This is a more typical approach for K8s. --- topo/node/cisco/cisco.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index c89932a7..19c57995 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -35,6 +35,7 @@ import ( "google.golang.org/protobuf/proto" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/rest" log "k8s.io/klog/v2" "k8s.io/utils/pointer" @@ -115,7 +116,7 @@ var ( ) var ( - podIsUpRegex = regexp.MustCompile(`Router up`) + podIsUpRegex = regexp.MustCompile(`Router up|Vxr up`) podIsFailedRegex = regexp.MustCompile(`Router failed to come up|FATAL sim:|LoginTimeoutError`) ) @@ -187,16 +188,13 @@ func (n *Node) Create(ctx context.Context) error { if pb.Model != ModelXRD { readinessProbe = &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ - Exec: &corev1.ExecAction{ - Command: []string{ - "sh", "-c", - "grep -q 'Router up' /nobackup/root/pyvxr/vxr.out/logs/console.R0.log 2>/dev/null", - }, + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(57400), }, }, - InitialDelaySeconds: 30, + InitialDelaySeconds: 10, PeriodSeconds: 10, - FailureThreshold: 30, + FailureThreshold: 60, } } pod := &corev1.Pod{ From 480014f62aa7cde5ce0ef5af5f81234a705d613a Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Wed, 2 Sep 2026 23:54:14 +0000 Subject: [PATCH 05/12] Do readiness for Cisco XRD also --- topo/node/cisco/cisco.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index 19c57995..d4fabb4b 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -184,18 +184,15 @@ func (n *Node) Create(ctx context.Context) error { tty = true stdin = true } - var readinessProbe *corev1.Probe - if pb.Model != ModelXRD { - readinessProbe = &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(57400), - }, + readinessProbe := &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(57400), }, - InitialDelaySeconds: 10, - PeriodSeconds: 10, - FailureThreshold: 60, - } + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, } pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ From 2e4702e608cd773b08e0360e1813b402209cc95b Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Wed, 2 Sep 2026 23:50:41 +0000 Subject: [PATCH 06/12] Add a readiness check for Juniper also --- topo/node/juniper/juniper.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/topo/node/juniper/juniper.go b/topo/node/juniper/juniper.go index 95ed2b51..104f1eda 100644 --- a/topo/node/juniper/juniper.go +++ b/topo/node/juniper/juniper.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/util/intstr" log "k8s.io/klog/v2" "k8s.io/utils/pointer" ) @@ -546,6 +547,16 @@ func (n *Node) Create(ctx context.Context) error { Env: node.ToEnvVar(pb.Config.Env), Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), RunAsUser: pointer.Int64(0), From b27b4c5f9a612244d207de3221bd7f59799fc89e Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 3 Sep 2026 00:00:27 +0000 Subject: [PATCH 07/12] Add a readiness check for Arista pods also --- topo/node/arista/arista.go | 7 ++++++- topo/node/arista/arista_test.go | 30 +++++++++++++++++++++++------- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/topo/node/arista/arista.go b/topo/node/arista/arista.go index fea19833..b5e46c2d 100644 --- a/topo/node/arista/arista.go +++ b/topo/node/arista/arista.go @@ -177,7 +177,12 @@ func (n *Node) Status(ctx context.Context) (node.Status, error) { case corev1.PodPending: return node.StatusPending, nil case corev1.PodRunning: - return node.StatusRunning, nil + for _, cond := range p[0].Status.Conditions { + if cond.Type == corev1.PodReady && cond.Status == corev1.ConditionTrue { + return node.StatusRunning, nil + } + } + return node.StatusPending, nil case corev1.PodFailed: return node.StatusFailed, nil default: diff --git a/topo/node/arista/arista_test.go b/topo/node/arista/arista_test.go index 3a99ade8..1294ea2e 100644 --- a/topo/node/arista/arista_test.go +++ b/topo/node/arista/arista_test.go @@ -558,11 +558,12 @@ func TestResetCfg(t *testing.T) { func TestStatus(t *testing.T) { tests := []struct { - desc string - cantWatch bool - noPodYet bool - phase corev1.PodPhase - status node.Status + desc string + cantWatch bool + noPodYet bool + phase corev1.PodPhase + conditions []corev1.PodCondition + status node.Status }{ { desc: "can't watch pod status", @@ -581,10 +582,24 @@ func TestStatus(t *testing.T) { status: node.StatusPending, }, { - desc: "pod running", + desc: "pod running not ready", phase: corev1.PodRunning, + status: node.StatusPending, + }, + { + desc: "pod running and ready", + phase: corev1.PodRunning, + conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, status: node.StatusRunning, }, + { + desc: "pod failed", + phase: corev1.PodFailed, + status: node.StatusFailed, + }, } ctx := context.Background() @@ -602,7 +617,8 @@ func TestStatus(t *testing.T) { Namespace: ns, }, Status: corev1.PodStatus{ - Phase: tt.phase, + Phase: tt.phase, + Conditions: tt.conditions, }, }) } From 4dec2f6897010a41f26e251cc7212152d1bf309f Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 3 Sep 2026 15:06:51 +0000 Subject: [PATCH 08/12] Fix a readiness race also for Nokia, on certificate application --- topo/node/nokia/nokia.go | 15 ++++++++++++--- topo/node/nokia/nokia_test.go | 4 ++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/topo/node/nokia/nokia.go b/topo/node/nokia/nokia.go index 72cef344..a9df2713 100644 --- a/topo/node/nokia/nokia.go +++ b/topo/node/nokia/nokia.go @@ -141,7 +141,7 @@ func (n *Node) GenerateSelfSigned(ctx context.Context) error { return nil } log.Infof("%s - generating self signed certs", n.Name()) - log.Infof("%s - waiting for pod to be running", n.Name()) + log.Infof("%s - waiting for pod to be ready", n.Name()) w, err := n.KubeClient.CoreV1().Pods(n.Namespace).Watch(ctx, metav1.ListOptions{ FieldSelector: fields.SelectorFromSet( fields.Set{metav1.ObjectNameField: n.Name()}, @@ -156,10 +156,19 @@ func (n *Node) GenerateSelfSigned(ctx context.Context) error { continue } if p.Status.Phase == corev1.PodRunning { - break + var ready bool + for _, cond := range p.Status.Conditions { + if cond.Type == corev1.PodReady && cond.Status == corev1.ConditionTrue { + ready = true + break + } + } + if ready { + break + } } } - log.Infof("%s - pod running.", n.Name()) + log.Infof("%s - pod ready.", n.Name()) if err := n.SpawnCLIConn(); err != nil { return err diff --git a/topo/node/nokia/nokia_test.go b/topo/node/nokia/nokia_test.go index dfeea873..c25a1e8a 100644 --- a/topo/node/nokia/nokia_test.go +++ b/topo/node/nokia/nokia_test.go @@ -210,6 +210,10 @@ func TestGenerateSelfSigned(t *testing.T) { Object: &corev1.Pod{ Status: corev1.PodStatus{ Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, }, }, }}, From 37b2da89d9cac33c21f70b9f9ab104be0de89b9f Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 3 Sep 2026 15:12:36 +0000 Subject: [PATCH 09/12] Add a readiness check for Sonic --- topo/node/sonic/sonic.go | 11 +++++++++++ topo/node/sonic/sonic_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/topo/node/sonic/sonic.go b/topo/node/sonic/sonic.go index f62f4e11..866b3546 100644 --- a/topo/node/sonic/sonic.go +++ b/topo/node/sonic/sonic.go @@ -20,6 +20,7 @@ import ( "github.com/openconfig/kne/topo/node" "google.golang.org/protobuf/proto" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/pointer" corev1 "k8s.io/api/core/v1" @@ -122,6 +123,16 @@ func (n *Node) CreatePod(ctx context.Context) error { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, }} pod := &corev1.Pod{ diff --git a/topo/node/sonic/sonic_test.go b/topo/node/sonic/sonic_test.go index db6dd1e9..8a2a0dee 100644 --- a/topo/node/sonic/sonic_test.go +++ b/topo/node/sonic/sonic_test.go @@ -26,6 +26,7 @@ import ( "google.golang.org/protobuf/proto" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" kfake "k8s.io/client-go/kubernetes/fake" "k8s.io/utils/pointer" ) @@ -191,6 +192,16 @@ func TestCreatePod(t *testing.T) { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, }, }, { desc: "sonic container with custom init image and interfaces", @@ -235,6 +246,16 @@ func TestCreatePod(t *testing.T) { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, }, }, { desc: "sonic container with config data", @@ -273,6 +294,16 @@ func TestCreatePod(t *testing.T) { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, VolumeMounts: []corev1.VolumeMount{{ Name: "startup-config-volume", ReadOnly: true, From 6279b5a946b76a9c209e989d5b1364119777d741 Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 3 Sep 2026 15:16:11 +0000 Subject: [PATCH 10/12] Add readiness check for ciena --- topo/node/ciena/ciena.go | 11 +++++++++++ topo/node/ciena/ciena_test.go | 15 +++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/topo/node/ciena/ciena.go b/topo/node/ciena/ciena.go index 174b86b1..fb8a03d3 100644 --- a/topo/node/ciena/ciena.go +++ b/topo/node/ciena/ciena.go @@ -27,6 +27,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" log "k8s.io/klog/v2" "k8s.io/utils/ptr" ) @@ -238,6 +239,16 @@ func (n *Node) CreatePod(ctx context.Context) error { SecurityContext: &corev1.SecurityContext{ Privileged: ptr.To(true), }, + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, VolumeMounts: extraMounts, }}, Volumes: extraVolumes, diff --git a/topo/node/ciena/ciena_test.go b/topo/node/ciena/ciena_test.go index 6789e6cf..b7d4904b 100644 --- a/topo/node/ciena/ciena_test.go +++ b/topo/node/ciena/ciena_test.go @@ -25,7 +25,9 @@ import ( "github.com/openconfig/kne/topo/node" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/anypb" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" kfake "k8s.io/client-go/kubernetes/fake" ) @@ -337,6 +339,19 @@ func TestNode_CreatePod_EquipmentFile(t *testing.T) { if ctr.Image != "vrnetlab/ciena_waverouter:config" { t.Errorf("container image: got %q, want %q", ctr.Image, "vrnetlab/ciena_waverouter:config") } + wantProbe := &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + } + if diff := cmp.Diff(wantProbe, ctr.ReadinessProbe); diff != "" { + t.Errorf("container readiness probe mismatch (-want +got):\n%s", diff) + } foundMount := false for _, m := range ctr.VolumeMounts { if m.Name == "equipment-file" && m.MountPath == "/equipment/setup.json" && m.SubPath == "setup.json" && m.ReadOnly { From 2c77f24943c3f178190bf83eaa44a234108b29cd Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Tue, 8 Sep 2026 22:18:49 +0000 Subject: [PATCH 11/12] Pick up service definition for readiness dynamically --- topo/node/ciena/ciena.go | 14 +-- topo/node/ciena/ciena_test.go | 6 ++ topo/node/cisco/cisco.go | 12 +-- topo/node/juniper/juniper.go | 12 +-- topo/node/node.go | 46 +++++++++ topo/node/node_test.go | 172 +++++++++++++++++++++++++++++++++- topo/node/sonic/sonic.go | 12 +-- topo/node/sonic/sonic_test.go | 72 +++++++++----- 8 files changed, 274 insertions(+), 72 deletions(-) diff --git a/topo/node/ciena/ciena.go b/topo/node/ciena/ciena.go index fb8a03d3..4757814e 100644 --- a/topo/node/ciena/ciena.go +++ b/topo/node/ciena/ciena.go @@ -27,7 +27,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/intstr" log "k8s.io/klog/v2" "k8s.io/utils/ptr" ) @@ -239,17 +238,8 @@ func (n *Node) CreatePod(ctx context.Context) error { SecurityContext: &corev1.SecurityContext{ Privileged: ptr.To(true), }, - ReadinessProbe: &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(22), - }, - }, - InitialDelaySeconds: 10, - PeriodSeconds: 10, - FailureThreshold: 60, - }, - VolumeMounts: extraMounts, + ReadinessProbe: node.ServiceReadinessProbe(pb), + VolumeMounts: extraMounts, }}, Volumes: extraVolumes, TerminationGracePeriodSeconds: ptr.To[int64](0), diff --git a/topo/node/ciena/ciena_test.go b/topo/node/ciena/ciena_test.go index b7d4904b..30eb6fa7 100644 --- a/topo/node/ciena/ciena_test.go +++ b/topo/node/ciena/ciena_test.go @@ -307,6 +307,12 @@ func TestNode_CreatePod_EquipmentFile(t *testing.T) { Image: "vrnetlab/ciena_waverouter:config", VendorData: vendorData, }, + Services: map[uint32]*tpb.Service{ + 22: { + Names: []string{"ssh"}, + Inside: 22, + }, + }, } n := &Node{ diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index d4fabb4b..47fbba8f 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -35,7 +35,6 @@ import ( "google.golang.org/protobuf/proto" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/rest" log "k8s.io/klog/v2" "k8s.io/utils/pointer" @@ -184,16 +183,7 @@ func (n *Node) Create(ctx context.Context) error { tty = true stdin = true } - readinessProbe := &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(57400), - }, - }, - InitialDelaySeconds: 10, - PeriodSeconds: 10, - FailureThreshold: 60, - } + readinessProbe := node.ServiceReadinessProbe(pb) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: n.Name(), diff --git a/topo/node/juniper/juniper.go b/topo/node/juniper/juniper.go index 104f1eda..4e1bf03a 100644 --- a/topo/node/juniper/juniper.go +++ b/topo/node/juniper/juniper.go @@ -23,7 +23,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/util/intstr" log "k8s.io/klog/v2" "k8s.io/utils/pointer" ) @@ -547,16 +546,7 @@ func (n *Node) Create(ctx context.Context) error { Env: node.ToEnvVar(pb.Config.Env), Resources: node.ToResourceRequirements(pb.Constraints), ImagePullPolicy: "IfNotPresent", - ReadinessProbe: &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(22), - }, - }, - InitialDelaySeconds: 10, - PeriodSeconds: 10, - FailureThreshold: 60, - }, + ReadinessProbe: node.ServiceReadinessProbe(pb), SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), RunAsUser: pointer.Int64(0), diff --git a/topo/node/node.go b/topo/node/node.go index 091956aa..b7420f57 100644 --- a/topo/node/node.go +++ b/topo/node/node.go @@ -229,6 +229,51 @@ func ToResourceRequirements(kv map[string]string) corev1.ResourceRequirements { return r } +// ServiceReadinessProbe returns a TCPSocket readiness probe for the node if a probeable service +// (e.g. ssh, gnmi) is defined in its service map. +func ServiceReadinessProbe(pb *tpb.Node) *corev1.Probe { + if pb == nil || len(pb.Services) == 0 { + return nil + } + // Prefer SSH first (typically available at boot across all NOS), then gNMI + preferred := []string{"ssh", "gnmi"} + for _, pref := range preferred { + for k, svc := range pb.Services { + if svc == nil { + continue + } + matched := strings.EqualFold(svc.Name, pref) + if !matched { + for _, name := range svc.Names { + if strings.EqualFold(name, pref) { + matched = true + break + } + } + } + if matched { + port := int(svc.Inside) + if port == 0 { + port = int(k) + } + if port > 0 { + return &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(port), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + } + } + } + } + } + return nil +} + // Create will create the node in the k8s cluster with all services and config // maps. func (n *Impl) Create(ctx context.Context) error { @@ -440,6 +485,7 @@ func (n *Impl) CreatePod(ctx context.Context) error { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, + ReadinessProbe: ServiceReadinessProbe(pb), }}, TerminationGracePeriodSeconds: pointer.Int64(0), NodeSelector: map[string]string{}, diff --git a/topo/node/node_test.go b/topo/node/node_test.go index 3119861c..9d4882c9 100644 --- a/topo/node/node_test.go +++ b/topo/node/node_test.go @@ -365,7 +365,7 @@ func TestValidateConstraints(t *testing.T) { constraintValues map[string]int }{ { - desc: "Invalid case - contraint value is greater than upper bound", + desc: "Invalid case - constraint value is greater than upper bound", node: &topopb.Node{ Name: "node1", HostConstraints: []*topopb.HostConstraint{ @@ -457,3 +457,173 @@ func TestValidateConstraints(t *testing.T) { }) } } + +func TestServiceReadinessProbe(t *testing.T) { + tests := []struct { + desc string + node *topopb.Node + want *corev1.Probe + }{ + { + desc: "nil node", + node: nil, + want: nil, + }, + { + desc: "no services", + node: &topopb.Node{}, + want: nil, + }, + { + desc: "ssh service", + node: &topopb.Node{ + Services: map[uint32]*topopb.Service{ + 22: { + Name: "ssh", + Inside: 22, + }, + }, + }, + want: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, + }, + { + desc: "ssh service custom inside port", + node: &topopb.Node{ + Services: map[uint32]*topopb.Service{ + 22: { + Name: "ssh", + Inside: 2222, + }, + }, + }, + want: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(2222), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, + }, + { + desc: "ssh service in names slice", + node: &topopb.Node{ + Services: map[uint32]*topopb.Service{ + 22: { + Names: []string{"ssh", "cli"}, + Inside: 22, + }, + }, + }, + want: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, + }, + { + desc: "gnmi service only", + node: &topopb.Node{ + Services: map[uint32]*topopb.Service{ + 9339: { + Name: "gnmi", + Inside: 57400, + }, + }, + }, + want: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(57400), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, + }, + { + desc: "both ssh and gnmi prefers ssh", + node: &topopb.Node{ + Services: map[uint32]*topopb.Service{ + 22: { + Name: "ssh", + Inside: 22, + }, + 9339: { + Names: []string{"gnmi", "gnoi"}, + Inside: 57400, + }, + }, + }, + want: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, + }, + { + desc: "unsupported service only", + node: &topopb.Node{ + Services: map[uint32]*topopb.Service{ + 179: { + Name: "bgp", + Inside: 179, + }, + }, + }, + want: nil, + }, + { + desc: "service with zero inside falls back to map key", + node: &topopb.Node{ + Services: map[uint32]*topopb.Service{ + 22: { + Name: "ssh", + Inside: 0, + }, + }, + }, + want: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + got := ServiceReadinessProbe(tt.node) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("ServiceReadinessProbe() mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/topo/node/sonic/sonic.go b/topo/node/sonic/sonic.go index 866b3546..df2be70f 100644 --- a/topo/node/sonic/sonic.go +++ b/topo/node/sonic/sonic.go @@ -20,7 +20,6 @@ import ( "github.com/openconfig/kne/topo/node" "google.golang.org/protobuf/proto" - "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/pointer" corev1 "k8s.io/api/core/v1" @@ -123,16 +122,7 @@ func (n *Node) CreatePod(ctx context.Context) error { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, - ReadinessProbe: &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(22), - }, - }, - InitialDelaySeconds: 10, - PeriodSeconds: 10, - FailureThreshold: 60, - }, + ReadinessProbe: node.ServiceReadinessProbe(pb), }} pod := &corev1.Pod{ diff --git a/topo/node/sonic/sonic_test.go b/topo/node/sonic/sonic_test.go index 8a2a0dee..9ea43856 100644 --- a/topo/node/sonic/sonic_test.go +++ b/topo/node/sonic/sonic_test.go @@ -192,16 +192,6 @@ func TestCreatePod(t *testing.T) { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, - ReadinessProbe: &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(22), - }, - }, - InitialDelaySeconds: 10, - PeriodSeconds: 10, - FailureThreshold: 60, - }, }, }, { desc: "sonic container with custom init image and interfaces", @@ -246,16 +236,6 @@ func TestCreatePod(t *testing.T) { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, - ReadinessProbe: &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(22), - }, - }, - InitialDelaySeconds: 10, - PeriodSeconds: 10, - FailureThreshold: 60, - }, }, }, { desc: "sonic container with config data", @@ -294,6 +274,52 @@ func TestCreatePod(t *testing.T) { SecurityContext: &corev1.SecurityContext{ Privileged: pointer.Bool(true), }, + VolumeMounts: []corev1.VolumeMount{{ + Name: "startup-config-volume", + ReadOnly: true, + MountPath: "/etc/sonic/config_db.json", + SubPath: "config_db.json", + }}, + }, + }, { + desc: "sonic container with ssh service has readiness probe", + nImpl: &node.Impl{ + Proto: &tpb.Node{ + Name: "sonic-node", + Config: &tpb.Config{ + Image: "sonicImage", + Command: []string{"sonicCommand"}, + Args: []string{"sonicArgs"}, + }, + Services: map[uint32]*tpb.Service{ + 22: { + Name: "ssh", + Inside: 22, + }, + }, + }, + }, + wantInitCtr: corev1.Container{ + Name: "init-sonic-node", + Image: node.DefaultInitContainerImage, + Args: []string{"1", "0", "1"}, + ImagePullPolicy: "IfNotPresent", + SecurityContext: &corev1.SecurityContext{ + Privileged: pointer.Bool(true), + }, + }, + wantSonicCtr: corev1.Container{ + Name: "sonic-node", + Image: "sonicImage", + Command: []string{"sonicCommand"}, + Args: []string{"sonicArgs"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{}, + }, + ImagePullPolicy: "IfNotPresent", + SecurityContext: &corev1.SecurityContext{ + Privileged: pointer.Bool(true), + }, ReadinessProbe: &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ TCPSocket: &corev1.TCPSocketAction{ @@ -304,12 +330,6 @@ func TestCreatePod(t *testing.T) { PeriodSeconds: 10, FailureThreshold: 60, }, - VolumeMounts: []corev1.VolumeMount{{ - Name: "startup-config-volume", - ReadOnly: true, - MountPath: "/etc/sonic/config_db.json", - SubPath: "config_db.json", - }}, }, }} for _, tt := range tests { From e2790e9912e22841e8fdebf0f73694b910190649 Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Wed, 9 Sep 2026 20:18:40 +0000 Subject: [PATCH 12/12] Design review comments 1. **Deterministic Probe Selection in node.go:234-275**: * Sorted pb.Services map keys before iterating in node.go:234-275 to prevent non-deterministic probe selection when multiple services match preferred names. 2. **Pod Readiness Condition Check in cisco.go:619-658**: * Updated cisco.go:619-658 to inspect p[0].Status.Conditions for corev1.PodReady == corev1.ConditionTrue before returning node. StatusRunning, ensuring XRD (and other Cisco models) do not bypass readiness probes. 3. **Single-Stream Log Fetching & Shadowing Fix in cisco.go:636-690**: * Added cisco.go:663-677 which streams pod logs once into a buffer, replacing len with n in io.Copy(buf, podLogs). * Updated isNode8000eFailed and isNode8000eUp to operate directly on the fetched byte slice. 4. **Unit Tests in cisco_test.go**: * Added unit tests TestIsNode8000eFailed and TestIsNode8000eUp verifying pattern matches for Router failed to come up, FATAL sim:, and LoginTimeoutError. * Updated TestNodeStatus to verify node.StatusFailed on failure patterns and node.StatusPending when pod readiness condition is false or missing. * Updated TestCreate to assert ReadinessProbe is properly populated on the created pod container. 5. **Unit Tests in juniper_test.go:1042-1057**: * Updated TestCreate to verify ReadinessProbe on the created cPTX pod (matching Sonic and Ciena test patterns). --- topo/node/cisco/cisco.go | 57 ++++--- topo/node/cisco/cisco_test.go | 237 ++++++++++++++++++++++++++---- topo/node/juniper/juniper_test.go | 14 ++ topo/node/node.go | 10 +- 4 files changed, 256 insertions(+), 62 deletions(-) diff --git a/topo/node/cisco/cisco.go b/topo/node/cisco/cisco.go index 47fbba8f..25767e63 100644 --- a/topo/node/cisco/cisco.go +++ b/topo/node/cisco/cisco.go @@ -615,7 +615,8 @@ func defaults(pb *tpb.Node) (*tpb.Node, error) { } // Status returns the current node state. -// For 8000e nodes it checks the logs and return running if log contains "Router up" +// For 8000e nodes it checks the logs and return running if log contains "Router up" and pod is ready. +// For XRD nodes it returns running if pod is ready. func (n *Node) Status(ctx context.Context) (node.Status, error) { p, err := n.Pods(ctx) if err != nil { @@ -635,33 +636,37 @@ func (n *Node) Status(ctx context.Context) (node.Status, error) { pb := n.Proto if pb.GetModel() != ModelXRD { req := n.KubeClient.CoreV1().Pods(p[0].Namespace).GetLogs(p[0].Name, &corev1.PodLogOptions{}) - if isNode8000eFailed(ctx, req) { + logBytes, err := getPodLogs(ctx, req) + if err != nil { + log.V(2).Infof("Cisco %s node %s status is %v", n.Proto.Model, n.Name(), node.StatusPending) + return node.StatusPending, nil + } + if isNode8000eFailed(logBytes) { log.Warningf("Cisco %s node %s failed to boot", n.Proto.Model, n.Name()) return node.StatusFailed, fmt.Errorf("cisco %s node %s failed to boot", n.Proto.Model, n.Name()) } - req = n.KubeClient.CoreV1().Pods(p[0].Namespace).GetLogs(p[0].Name, &corev1.PodLogOptions{}) - if !isNode8000eUp(ctx, req) { + if !isNode8000eUp(logBytes) { log.V(2).Infof("Cisco %s node %s status is %v", n.Proto.Model, n.Name(), node.StatusPending) return node.StatusPending, nil } } for _, cond := range p[0].Status.Conditions { - if cond.Type == corev1.PodReady && cond.Status != corev1.ConditionTrue { - log.V(2).Infof("Cisco %s node %s status is %v", n.Proto.Model, n.Name(), node.StatusPending) - return node.StatusPending, nil + if cond.Type == corev1.PodReady && cond.Status == corev1.ConditionTrue { + log.Infof("Cisco %s node %s status is %v ", n.Proto.Model, n.Name(), node.StatusRunning) + return node.StatusRunning, nil } } - log.Infof("Cisco %s node %s status is %v ", n.Proto.Model, n.Name(), node.StatusRunning) - return node.StatusRunning, nil + log.V(2).Infof("Cisco %s node %s status is %v", n.Proto.Model, n.Name(), node.StatusPending) + return node.StatusPending, nil default: return node.StatusUnknown, nil } } -func isNode8000eUp(ctx context.Context, req *rest.Request) bool { +func getPodLogs(ctx context.Context, req *rest.Request) ([]byte, error) { podLogs, err := req.Stream(ctx) if err != nil { - return false + return nil, err } defer func() { if err := podLogs.Close(); err != nil { @@ -669,29 +674,19 @@ func isNode8000eUp(ctx context.Context, req *rest.Request) bool { } }() buf := new(bytes.Buffer) - len, err := io.Copy(buf, podLogs) - if err != nil || len == 0 { - return false + n, err := io.Copy(buf, podLogs) + if err != nil || n == 0 { + return nil, fmt.Errorf("failed to read pod logs or empty log") } - return podIsUpRegex.Match(buf.Bytes()) + return buf.Bytes(), nil } -func isNode8000eFailed(ctx context.Context, req *rest.Request) bool { - podLogs, err := req.Stream(ctx) - if err != nil { - return false - } - defer func() { - if err := podLogs.Close(); err != nil { - log.V(2).Infof("Failed to close pod logs stream: %v", err) - } - }() - buf := new(bytes.Buffer) - len, err := io.Copy(buf, podLogs) - if err != nil || len == 0 { - return false - } - return podIsFailedRegex.Match(buf.Bytes()) +func isNode8000eUp(logBytes []byte) bool { + return podIsUpRegex.Match(logBytes) +} + +func isNode8000eFailed(logBytes []byte) bool { + return podIsFailedRegex.Match(logBytes) } // No op function to override default network on open function. diff --git a/topo/node/cisco/cisco_test.go b/topo/node/cisco/cisco_test.go index 609011ab..3e395dbd 100644 --- a/topo/node/cisco/cisco_test.go +++ b/topo/node/cisco/cisco_test.go @@ -37,6 +37,7 @@ import ( "google.golang.org/protobuf/testing/protocmp" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes/fake" ) @@ -923,52 +924,214 @@ var ( } ) +func TestIsNode8000eFailed(t *testing.T) { + tests := []struct { + desc string + log string + want bool + }{ + { + desc: "Router failed to come up", + log: "Error: Router failed to come up after timeout", + want: true, + }, + { + desc: "FATAL sim:", + log: "2023-01-01 FATAL sim: initialization aborted", + want: true, + }, + { + desc: "LoginTimeoutError", + log: "Encountered LoginTimeoutError during boot", + want: true, + }, + { + desc: "Normal log with Router up", + log: "Router up and running", + want: false, + }, + { + desc: "Empty log", + log: "", + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + if got := isNode8000eFailed([]byte(tt.log)); got != tt.want { + t.Errorf("isNode8000eFailed(%q) = %v, want %v", tt.log, got, tt.want) + } + }) + } +} + +func TestIsNode8000eUp(t *testing.T) { + tests := []struct { + desc string + log string + want bool + }{ + { + desc: "Router up", + log: "System initialized: Router up", + want: true, + }, + { + desc: "Vxr up", + log: "Line card online: Vxr up", + want: true, + }, + { + desc: "Booting log", + log: "Booting kernel...", + want: false, + }, + { + desc: "Empty log", + log: "", + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + if got := isNode8000eUp([]byte(tt.log)); got != tt.want { + t.Errorf("isNode8000eUp(%q) = %v, want %v", tt.log, got, tt.want) + } + }) + } +} + func TestNodeStatus(t *testing.T) { tests := []struct { - desc string - status node.Status - ni *node.Impl - podLogErr bool + desc string + status node.Status + model string + phase corev1.PodPhase + conditions []corev1.PodCondition + podLogErr bool + podLogFail bool + wantErr bool }{ { - desc: "Status test for 8000e Node", + desc: "Status running for 8000e Node when Ready and Up", status: node.StatusRunning, - ni: node8000e, + model: "8201-32FH", + phase: corev1.PodRunning, + conditions: []corev1.PodCondition{ + {Type: corev1.PodReady, Status: corev1.ConditionTrue}, + }, }, { - desc: "Negative Status test for 8000e Node", + desc: "Status pending for 8000e Node when Up but Not Ready", + status: node.StatusPending, + model: "8201-32FH", + phase: corev1.PodRunning, + conditions: []corev1.PodCondition{ + {Type: corev1.PodReady, Status: corev1.ConditionFalse}, + }, + }, + { + desc: "Status pending for 8000e Node when log does not match up", status: node.StatusPending, - ni: node8000e, + model: "8201-32FH", + phase: corev1.PodRunning, podLogErr: true, + conditions: []corev1.PodCondition{ + {Type: corev1.PodReady, Status: corev1.ConditionTrue}, + }, + }, + { + desc: "Status failed for 8000e Node when log contains failure pattern", + status: node.StatusFailed, + model: "8201-32FH", + phase: corev1.PodRunning, + podLogFail: true, + wantErr: true, }, { - desc: "Status test for XRD Node", + desc: "Status running for XRD Node when Ready", status: node.StatusRunning, - ni: nodeXRD, + model: ModelXRD, + phase: corev1.PodRunning, + conditions: []corev1.PodCondition{ + {Type: corev1.PodReady, Status: corev1.ConditionTrue}, + }, }, { - desc: "Status test for XRD Node, pod logs do not matter", - status: node.StatusRunning, - ni: nodeXRD, - podLogErr: true, + desc: "Status pending for XRD Node when Not Ready", + status: node.StatusPending, + model: ModelXRD, + phase: corev1.PodRunning, + conditions: []corev1.PodCondition{ + {Type: corev1.PodReady, Status: corev1.ConditionFalse}, + }, + }, + { + desc: "Status pending for XRD Node without Ready condition", + status: node.StatusPending, + model: ModelXRD, + phase: corev1.PodRunning, + }, + { + desc: "Status pending for PodPending phase", + status: node.StatusPending, + model: ModelXRD, + phase: corev1.PodPending, + }, + { + desc: "Status failed for PodFailed phase", + status: node.StatusFailed, + model: ModelXRD, + phase: corev1.PodFailed, }, } for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { ctx := context.Background() - if !tt.podLogErr { + podName := "test-pod" + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName, + Namespace: "test", + }, + Status: corev1.PodStatus{ + Phase: tt.phase, + Conditions: tt.conditions, + }, + } + ki := fake.NewSimpleClientset(pod) + ni := &node.Impl{ + KubeClient: ki, + Namespace: "test", + Proto: &tpb.Node{ + Name: podName, + Vendor: tpb.Vendor_CISCO, + Config: &tpb.Config{}, + Model: tt.model, + }, + } + if tt.podLogFail { + origPodIsFailedRegex := podIsFailedRegex + defer func() { + podIsFailedRegex = origPodIsFailedRegex + }() + podIsFailedRegex = regexp.MustCompile("fake log") + } else if !tt.podLogErr { origPodIsUpRegex := podIsUpRegex defer func() { podIsUpRegex = origPodIsUpRegex }() podIsUpRegex = regexp.MustCompile("fake log") // this is the expected log from a fake pod } - nImpl, _ := New(tt.ni) + nImpl, err := New(ni) + if err != nil { + t.Fatalf("New() failed: %v", err) + } n, _ := nImpl.(*Node) status, err := n.Status(ctx) - if err != nil { - t.Errorf("Error is not expected for Node Status") + if (err != nil) != tt.wantErr { + t.Errorf("node.Status() error = %v, wantErr %v", err, tt.wantErr) } if status != tt.status { t.Errorf("node.Status() = %v, want %v", status, tt.status) @@ -1203,21 +1366,22 @@ func TestCreate(t *testing.T) { if tt.configData != nil { cfg.ConfigData = &tpb.Config_Data{Data: tt.configData} } - n := &Node{ - Impl: &node.Impl{ - Namespace: "test", - KubeClient: ki, - Proto: &tpb.Node{ - Name: "node1", - Model: tt.model, - Config: cfg, - Interfaces: map[string]*tpb.Interface{ - "eth1": {Name: "GigabitEthernet0/0/0/0"}, - }, + nImpl, err := New(&node.Impl{ + Namespace: "test", + KubeClient: ki, + Proto: &tpb.Node{ + Name: "node1", + Model: tt.model, + Config: cfg, + Interfaces: map[string]*tpb.Interface{ + "eth1": {Name: "GigabitEthernet0/0/0/0"}, }, }, + }) + if err != nil { + t.Fatalf("New() unexpected error = %v", err) } - if err := n.Create(context.Background()); (err != nil) != tt.wantErr { + if err := nImpl.Create(context.Background()); (err != nil) != tt.wantErr { t.Fatalf("Create() unexpected error = %v, wantErr = %v", err, tt.wantErr) } pod, err := ki.CoreV1().Pods("test").Get(context.Background(), "node1", metav1.GetOptions{}) @@ -1240,6 +1404,19 @@ func TestCreate(t *testing.T) { if len(pod.Spec.Containers[0].VolumeMounts) != tt.wantMainMountsLen { t.Errorf("main container volume mounts len = %d, want %d", len(pod.Spec.Containers[0].VolumeMounts), tt.wantMainMountsLen) } + wantProbe := &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + } + if diff := cmp.Diff(wantProbe, pod.Spec.Containers[0].ReadinessProbe); diff != "" { + t.Errorf("container readiness probe mismatch (-want +got):\n%s", diff) + } for _, sub := range tt.wantInitScriptSub { if !strings.Contains(initC.Args[0], sub) { t.Errorf("init container script missing expected substring %q", sub) diff --git a/topo/node/juniper/juniper_test.go b/topo/node/juniper/juniper_test.go index 88278fe0..67ca4cf7 100644 --- a/topo/node/juniper/juniper_test.go +++ b/topo/node/juniper/juniper_test.go @@ -28,6 +28,7 @@ import ( "google.golang.org/protobuf/testing/protocmp" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes/fake" ktest "k8s.io/client-go/testing" @@ -1041,6 +1042,19 @@ func TestCreate(t *testing.T) { if len(pod.Spec.Containers[0].VolumeMounts) != tt.wantMainMountsLen { t.Errorf("main container volume mounts len = %d, want %d", len(pod.Spec.Containers[0].VolumeMounts), tt.wantMainMountsLen) } + wantProbe := &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + TCPSocket: &corev1.TCPSocketAction{ + Port: intstr.FromInt(22), + }, + }, + InitialDelaySeconds: 10, + PeriodSeconds: 10, + FailureThreshold: 60, + } + if diff := cmp.Diff(wantProbe, pod.Spec.Containers[0].ReadinessProbe); diff != "" { + t.Errorf("container readiness probe mismatch (-want +got):\n%s", diff) + } for _, sub := range tt.wantInitScriptSub { if !strings.Contains(initC.Args[0], sub) { t.Errorf("init container script missing expected substring %q", sub) diff --git a/topo/node/node.go b/topo/node/node.go index b7420f57..01ea3766 100644 --- a/topo/node/node.go +++ b/topo/node/node.go @@ -7,6 +7,7 @@ import ( "math" "os" "path/filepath" + "sort" "strconv" "strings" "sync" @@ -235,10 +236,17 @@ func ServiceReadinessProbe(pb *tpb.Node) *corev1.Probe { if pb == nil || len(pb.Services) == 0 { return nil } + keys := make([]uint32, 0, len(pb.Services)) + for k := range pb.Services { + keys = append(keys, k) + } + sort.Slice(keys, func(i, j int) bool { return keys[i] < keys[j] }) + // Prefer SSH first (typically available at boot across all NOS), then gNMI preferred := []string{"ssh", "gnmi"} for _, pref := range preferred { - for k, svc := range pb.Services { + for _, k := range keys { + svc := pb.Services[k] if svc == nil { continue }