Merge pull request #235 from openconfig/localpref-med

Fix <PolicyTest>/installPolicyAfterRoutes

bgp/gobgp.go

@@ -234,7 +234,7 @@ func (t *bgpDeclTask) startGoBGPFuncDecl(_ context.Context, yclient *ygnmi.Clien
 					Safi: api.Family_SAFI_UNICAST,
 				},
 			}, func(d *api.Destination) {
-				log.V(0).Infof("%s: GoBGP global table path: %v", t.targetName, d)
+				log.V(1).Infof("%s: GoBGP global table path: %v", t.targetName, d)
 			}); err != nil {
 				log.Errorf("GoBGP ListPath call failed (global table): %v", err)
 			} else {

bgp/tests/local_tests/policy_test.go

@@ -123,6 +123,7 @@ func testPolicyAux(t *testing.T, testspec PolicyTestCase, installPolicyAfterRout
 		Await(t, dut2, v4uni.LocRib().Route(prefix, oc.UnionString(dut1spec.RouterID), 0).Prefix().State(), prefix)
 		Await(t, dut2, v4uni.Neighbor(dut3spec.RouterID).AdjRibOutPre().Route(prefix, 0).Prefix().State(), prefix)
 		if routeTest.GetExpectedResult() == policyval.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT || installPolicyAfterRoutes {
+			t.Logf("Waiting for %s to be propagated", prefix)
 			Await(t, dut2, v4uni.Neighbor(dut3spec.RouterID).AdjRibOutPost().Route(prefix, 0).Prefix().State(), prefix)
 			Await(t, dut3, v4uni.Neighbor(dut2spec.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), prefix)
 		} else {
@@ -131,13 +132,35 @@ func testPolicyAux(t *testing.T, testspec PolicyTestCase, installPolicyAfterRout
 				return ok
 			})
 			if _, ok := w.Await(t); ok {
-				t.Errorf("prefix %q was not rejected.", prefix)
+				t.Errorf("prefix %q (%s) was not rejected.", prefix, routeTest.GetDescription())
 			}
 		}
 	}
 
 	if installPolicyAfterRoutes {
+		awaitNewSession := make(chan error)
+		go func() {
+			if _, err := AwaitWithErr(dut2, bgp.BGPPath.Neighbor(dut3spec.RouterID).SessionState().State(), oc.Bgp_Neighbor_SessionState_IDLE); err != nil {
+				awaitNewSession <- err
+			}
+			if _, err := AwaitWithErr(dut3, bgp.BGPPath.Neighbor(dut2spec.RouterID).SessionState().State(), oc.Bgp_Neighbor_SessionState_IDLE); err != nil {
+				awaitNewSession <- err
+			}
+			if _, err := AwaitWithErr(dut2, bgp.BGPPath.Neighbor(dut3spec.RouterID).SessionState().State(), oc.Bgp_Neighbor_SessionState_ESTABLISHED); err != nil {
+				awaitNewSession <- err
+			}
+			if _, err := AwaitWithErr(dut3, bgp.BGPPath.Neighbor(dut2spec.RouterID).SessionState().State(), oc.Bgp_Neighbor_SessionState_ESTABLISHED); err != nil {
+				awaitNewSession <- err
+			}
+			awaitNewSession <- nil
+		}()
 		testspec.installPolicies(t, dut2)
+		// Changing policy resets the BGP session, which causes routes
+		// to disappear from the AdjRIBs, so we need to wait for
+		// re-establishment first.
+		if err := <-awaitNewSession; err != nil {
+			t.Fatal(err)
+		}
 
 		for _, routeTest := range testspec.spec.RouteTests {
 			prefix := routeTest.GetInput().GetReachPrefix()
@@ -156,18 +179,18 @@ func testPolicyAux(t *testing.T, testspec PolicyTestCase, installPolicyAfterRout
 					return !ok
 				})
 				if _, ok := w.Await(t); !ok {
-					t.Errorf("prefix %q was not rejected within timeout.", prefix)
+					t.Errorf("prefix %q (%s) was not rejected within timeout.", prefix, routeTest.GetDescription())
 				} else {
-					t.Logf("prefix %q was successfully rejected from DUT2's AdjRibOutPost within timeout.", prefix)
+					t.Logf("prefix %q (%s) was successfully rejected from DUT2's AdjRibOutPost within timeout.", prefix, routeTest.GetDescription())
 				}
 				w = Watch(t, dut3, v4uni.Neighbor(dut2spec.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), rejectTimeout, func(val *ygnmi.Value[string]) bool {
 					_, ok := val.Val()
 					return !ok
 				})
 				if _, ok := w.Await(t); !ok {
-					t.Errorf("prefix %q was not withdrawn from DUT3 within timeout.", prefix)
+					t.Errorf("prefix %q (%s) was not withdrawn from DUT3 within timeout.", prefix, routeTest.GetDescription())
 				} else {
-					t.Logf("prefix %q was successfully not seen at DUT3's AdjRibInPre within timeout.", prefix)
+					t.Logf("prefix %q (%s) was successfully not seen at DUT3's AdjRibInPre within timeout.", prefix, routeTest.GetDescription())
 				}
 			}
 		}

bgp/tests/local_tests/session_establish_test.go

@@ -168,6 +168,7 @@ func newLemming(t *testing.T, dev DeviceSpec, connectedIntfs []*AddIntfAction) (
 }
 
 func establishSessionPair(t *testing.T, dut1, dut2 *ygnmi.Client, spec1, spec2 DeviceSpec) {
+	t.Helper()
 	dutConf := bgpWithNbr(spec1.AS, spec1.RouterID, &oc.NetworkInstance_Protocol_Bgp_Neighbor{
 		PeerAs:          ygot.Uint32(spec2.AS),
 		NeighborAddress: ygot.String(spec2.RouterID),
@@ -181,8 +182,13 @@ func establishSessionPair(t *testing.T, dut1, dut2 *ygnmi.Client, spec1, spec2 D
 	Update(t, dut1, bgp.BGPPath.Config(), dutConf)
 	Update(t, dut2, bgp.BGPPath.Config(), dut2Conf)
 
-	nbrPath := bgp.BGPPath.Neighbor(spec2.RouterID)
-	Await(t, dut1, nbrPath.SessionState().State(), oc.Bgp_Neighbor_SessionState_ESTABLISHED)
+	awaitSessionEstablished(t, dut1, dut2, spec1, spec2)
+}
+
+func awaitSessionEstablished(t *testing.T, dut1, dut2 *ygnmi.Client, spec1, spec2 DeviceSpec) {
+	t.Helper()
+	Await(t, dut1, bgp.BGPPath.Neighbor(spec2.RouterID).SessionState().State(), oc.Bgp_Neighbor_SessionState_ESTABLISHED)
+	Await(t, dut2, bgp.BGPPath.Neighbor(spec1.RouterID).SessionState().State(), oc.Bgp_Neighbor_SessionState_ESTABLISHED)
 }
 
 func TestSessionEstablish(t *testing.T) {

bgp/tests/local_tests/ygnmi_test.go

@@ -19,6 +19,7 @@ package local_test
 import (
 	"context"
 	"errors"
+	"fmt"
 	"testing"
 	"time"
 
@@ -110,6 +111,19 @@ func Await[T any](t testing.TB, c *ygnmi.Client, q ygnmi.SingletonQuery[T], val
 	return v
 }
 
+// AwaitWithErr is the same as Await except an error is returned.
+//
+// Its purpose is to add a better error message.
+func AwaitWithErr[T any](c *ygnmi.Client, q ygnmi.SingletonQuery[T], val T) (*ygnmi.Value[T], error) {
+	ctx, cancel := context.WithTimeout(context.Background(), awaitTimeLimit)
+	defer cancel()
+	v, err := ygnmi.Await(ctx, c, q, val)
+	if err != nil {
+		return v, fmt.Errorf("Await(t) on %v at %v: %v", c, q, err)
+	}
+	return v, nil
+}
+
 type watchAwaiter[T any] interface {
 	Await() (*ygnmi.Value[T], error)
 }

dataplane/forwarding/util/stats/stats.go

@@ -94,6 +94,7 @@ func (s *Stats) Update(id int, value int64) error {
 func (s *Stats) GetAll() map[int]int64 {
 	r := make(map[int]int64)
 	for id, stat := range s.statMap {
+		stat := stat
 		r[id] = atomic.LoadInt64(&stat.value)
 	}
 	return r