Skip to content

Conversation

@wenovus
Copy link
Contributor

@wenovus wenovus commented May 19, 2023

  • Convert OC policies to GoBGP policies in configuration in the reconciler.
  • Add BGP policy test spec. This is adapted from an existing one.
  • Add prefix-list policy test.

* Convert OC policies to GoBGP policies in configuration in the
  reconciler.
* Add BGP policy test spec. This is adapted from an existing one.
* Add prefix-list policy test.
@wenovus wenovus requested a review from DanG100 May 19, 2023 03:25

syntax = "proto3";

package policy_validation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is reason with a proto and not just a struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is another BGP policy testing framework that the user of this is looking at that currently uses this as the test interface. I'm simply using it here for uniformity and so I don't need to re-invent this. If there is no need for using a proto we can later change it, but it doesn't seem to have any harm.

Base automatically changed from bgpconfig-refactor to main May 22, 2023 17:22
@wenovus wenovus requested a review from DanG100 May 22, 2023 17:32
@github-actions
Copy link

github-actions bot commented May 22, 2023

Pull Request Test Coverage Report for Build 5050896170

  • 193 of 210 (91.9%) changed or added relevant lines in 5 files are covered.
  • 68 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.09%) to 2.483%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bgp/gobgp.go 161 166 96.99%
gnmi/gnmi.go 4 9 44.44%
bgp/ocgobgp.go 21 28 75.0%
Files with Coverage Reduction New Missed Lines %
dataplane/internal/engine/engine.go 29 0%
gnmi/oc/networkinstance/networkinstance-0.go 39 0.27%
Totals Coverage Status
Change from base Build 5048647217: 0.09%
Covered Lines: 13112
Relevant Lines: 528058

💛 - Coveralls

@wenovus
Copy link
Contributor Author

wenovus commented May 22, 2023

There are some strange non-deterministic errors appearing:

Locally running make test-race, this happened once:

--- FAIL: TestBGPGUEPolicy (20.45s)                                                                                                                                                                                                                                                                   
    --- FAIL: TestBGPGUEPolicy/v6 (10.20s)                                                                                                                                                                                                                                                            
        --- FAIL: TestBGPGUEPolicy/v6/Add_Policy (0.00s)                                                                                                                                                                                                                                              
            server_test.go:1950: Got duplicate route key:                                                                                          
                First: &{RouteKey:{Prefix:2001::1400:0:0:0/72 NIName:DEFAULT} Nexthops:map[{NextHopSummary:{Weight:0 Address:2001::c0a8:12a:0:0 NetworkInstance:DEFAULT} Port:{Name:eth0 Index:0 Subinterface:0} GUEHeaders:{GUEPolicy:{dstPortv4:0 dstPortv6:8 srcIP4:[0 0 0 0] srcIP6:[32 1 0 0 0 0 
0 0 42 42 42 42 0 0 0 0]} dstIP4:[0 0 0 0] dstIP6:[32 1 0 0 0 0 0 0 192 168 1 42 0 0 0 0] isV6:true}}:true]}                                                                                                                                                                                          
                Duplicate: &{RouteKey:{Prefix:2001::1400:0:0:0/72 NIName:DEFAULT} Nexthops:map[{NextHopSummary:{Weight:0 Address:2001::c0a8:12a:0:0 NetworkInstance:DEFAULT} Port:{Name:eth0 Index:0 Subinterface:0} GUEHeaders:{GUEPolicy:{dstPortv4:0 dstPortv6:8 srcIP4:[0 0 0 0] srcIP6:[32 1 0 0 
0 0 0 0 42 42 42 42 0 0 0 0]} dstIP4:[0 0 0 0] dstIP6:[32 1 0 0 0 0 0 0 192 168 1 42 0 0 0 0] isV6:true}}:true]}

With GitHub actions I've commented where bgp/tests/local_tests is failing due to a stale cache update -- I have no idea how that could happen since all gNMI operations for these tests happen sequentially, and tests are also run sequentially since they're in the same package: https://github.com/openconfig/lemming/actions/runs/5050247039/jobs/9060715556

Lastly there was a recursive cmp transformer failure that I also have no idea happened: https://github.com/openconfig/lemming/actions/runs/5050317179/jobs/9060884938

This is just a note for the future if these errors occur to help with debugging.

@wenovus wenovus merged commit 66d1c51 into main May 22, 2023
@wenovus wenovus deleted the prefix-list-test branch May 22, 2023 22:59
@wenovus wenovus mentioned this pull request May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants