Update RT-2.13: Weighted-ECMP for IS-IS to not also use BGP #3316
Conversation
Pull Request Functional Test Report for #3316 / 3374917Virtual Devices
Hardware Devices
|
Pull Request Test Coverage Report for Build 13047976046Details
💛 - Coveralls |
dplore
changed the title
| @@ -249,17 +220,13 @@ func TestWeightedECMPForISIS(t *testing.T) { | |||
| } | |||
|
|
|||
| top.Flows().Clear() | |||
| if deviations.ISISLoopbackRequired(dut) { | |||
There was a problem hiding this comment.
@ANISH-GOTTAPU , i see this deviation is removed from the code. Can you give reason for this change?
There was a problem hiding this comment.
We have updated the test not to use BGP anymore, so the deviation "ISISLoopbackRequired" is not needed.
| d := &oc.Root{} | ||
| p1 := dut.Port(t, "port1") | ||
|
|
||
| i := d.GetOrCreateInterface(p1.Name()) |
There was a problem hiding this comment.
Here interface leaf is mandatory to be added. Use NewOCInterface for creating the new interface which would configure the right way. Similarly for other aggregate interfaces too.
// NewOCInterface returns a new *oc.Interface configured with these attributes.
func (a *Attributes) NewOCInterface(name string, dut *ondatra.DUTDevice) *oc.Interface {
return a.ConfigOCInterface(&oc.Interface{Name: ygot.String(name)}, dut)
}
There was a problem hiding this comment.
Updated the code to use NewOCInterface
| @@ -503,8 +461,8 @@ func configureDUT(t *testing.T, dut *ondatra.DUTDevice) []string { | |||
| gnmi.BatchDelete(b, gnmi.OC().Interface(aggID).Aggregation().MinLinks().Config()) | |||
| gnmi.BatchReplace(b, gnmi.OC().Interface(aggID).Config(), agg) | |||
|
|
|||
| p1 := dut.Port(t, fmt.Sprintf("port%d", (aggIdx*2)+1)) | |||
| p2 := dut.Port(t, fmt.Sprintf("port%d", (aggIdx*2)+2)) | |||
| p1 := dut.Port(t, fmt.Sprintf("port%d", (aggIdx*2)+2)) | |||
There was a problem hiding this comment.
p1 is already used in this function above. Please use a different variable name here instead of reusing the same variable under the same function.
Instead of p1, p2 you could use something meaningful here above too
There was a problem hiding this comment.
Updated the code accordingly.
The test is passing with KNE environment but had to reduce the traffic rate and adjust tolerance accordingly. Attaching the log for your reference. |
It seems to me that the subtest is failing due to the interface not being shut down due to bizzare handling in that area. I think the baseline behavior of this test should be to shut the DUT port explicitly, that way we get a deterministic behavior whichever type of platform this test is ran on. There's also an implicit assumption that the DUT is directly connected to the ATE/OTG with the current behavior. Thoughts? @ram-mac @rohit-rp I'm just curious to know why this value has a comment that suggests the value should be different based on the environment. |
Currently, OTG does not support IBGP with the same NextHOP across multiple devices. As a result, the DUT will only consider one neighborship. To address this, the test has been modified to use only ISIS, advertising the same route from three ISIS neighbors. The expected behavior is that traffic will load balance based on weights.
I've updated the test to use only the interface on the transmit end, ensuring compatibility with both HW and KNE (SW), as we lack a proto file that supports KNE with LAG on transmit. This change will not affect the weighted-ECMP since the receiving end is LAG only.