Skip to content

Conversation

@crc-kt
Copy link
Contributor

@crc-kt crc-kt commented Jul 14, 2025

Test log: https://partnerissuetracker.corp.google.com/issues/415458482 (contains logs for Arista vendor)

@crc-kt crc-kt requested a review from a team as a code owner July 14, 2025 01:48
@ram-mac
Copy link
Contributor

ram-mac commented Jul 22, 2025

@crc-kt - Have we validated the PR against any vendor so far? If so can you please share the pass log?
I tried to validating this PR against Cisco and it fails.

@crc-kt
Copy link
Contributor Author

crc-kt commented Jul 30, 2025

@crc-kt - Have we validated the PR against any vendor so far? If so can you please share the pass log? I tried to validating this PR against Cisco and it fails.

Our main setup is Arista based, I will share the logs for that. Did not test on Cisco.

@crc-kt
Copy link
Contributor Author

crc-kt commented Jul 30, 2025

@crc-kt - Have we validated the PR against any vendor so far? If so can you please share the pass log? I tried to validating this PR against Cisco and it fails.

Please see test log here:
https://partnerissuetracker.corp.google.com/issues/415458482 (contains logs for Arista vendor)

@coveralls
Copy link

coveralls commented Aug 12, 2025

Pull Request Test Coverage Report for Build 17149918806

Details

  • 0 of 13 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 12.696%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/cfgplugins/bgp_policy.go 0 13 0.0%
Totals Coverage Status
Change from base Build 17148847807: -0.01%
Covered Lines: 2111
Relevant Lines: 16627

💛 - Coveralls

otgAdvertisedRoutesv6Net = []string{"2001:db8:300:100::0", "2001:db8:300:101::0", "2001:db8:400:100::1", "2001:db8:400:101::1"}
otgDeniedRoutesv6Net = []string{"2001:db8:400:100::1", "2001:db8:400:101::1"}
otgAdvertisedRoutesv4Prefix = []uint32{32, 32, 32, 32}
otgAdvertisedRoutesv6Prefix = []uint32{127, 127, 127, 127}
Copy link
Contributor

Choose a reason for hiding this comment

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

@crc-kt - i see in the README we have advertised routes mask length as /127 and denied routes mask length as /128
Here we are using /127 only for all routes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


// bgpCreateNbr creates a BGP object with neighbors pointing to ate and returns bgp object.
func bgpCreateNbr(t *testing.T, bgpParams *bgpTestParams, dut *ondatra.DUTDevice) *oc.NetworkInstance_Protocol {
t.Helper()
Copy link
Contributor

Choose a reason for hiding this comment

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

@crc-kt - Can we use cfgplugins to create a new bgp session? sample below

For eg:
bs := cfgplugins.NewBGPSession(t, cfgplugins.PortCount2, nil)

https://github.com/openconfig/featureprofiles/blob/3bb2a9387cfc0dab163e458dc87f94351a187bc7/feature/bgp/policybase/otg_tests/aspath_test/aspath_test.go#L221C1-L222C1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use NewBGPSession upon test creation but failed (different errors), so I had to resort to doing it the way I saw it done in multiple other tests. If you want, I can try to refactor the BGP code but this comes second to new test development so it will probably have delays. Your call.

@crc-kt crc-kt requested a review from a team as a code owner August 19, 2025 15:20
@crc-kt crc-kt requested a review from ram-mac August 19, 2025 15:22
@ram-mac
Copy link
Contributor

ram-mac commented Aug 20, 2025

@crc-kt - I ran this test on my setup and the test is actually failing. Have you run this test with the recent changes and working for you?

[third_party/openconfig/featureprofiles/internal/helpers/helpers.go:136](https://cs.corp.google.com/piper///depot/google3/third_party/openconfig/featureprofiles/internal/helpers/helpers.go?l=136&ws=rmachat/1370&snapshot=36): gnmiClient.Set() with unexpected error: rpc error: code = InvalidArgument desc = error on request {
        update: {
          path: {
            origin: "cli"
          }
          val: {
            ascii_val: "\nip as-path access list aclRegexAsPathAllowed permit ^65000$\nip as-path access list aclRegexAsPathAllowed deny .*\nroute-map FILTER-IN 10\nmatch as-path aclRegexAsPathAllowed\n"
          }
        }
        }: failed to apply: Incomplete token (at token 2: 'access'): CLI command 3 of 8 'ip as-path access list aclRegexAsPathAllowed permit ^65000$' failed: invalid command

@ram-mac
Copy link
Contributor

ram-mac commented Aug 20, 2025

The fail logs are now attached to 415458482

@crc-kt
Copy link
Contributor Author

crc-kt commented Aug 20, 2025

@crc-kt - I ran this test on my setup and the test is actually failing. Have you run this test with the recent changes and working for you?

[third_party/openconfig/featureprofiles/internal/helpers/helpers.go:136](https://cs.corp.google.com/piper///depot/google3/third_party/openconfig/featureprofiles/internal/helpers/helpers.go?l=136&ws=rmachat/1370&snapshot=36): gnmiClient.Set() with unexpected error: rpc error: code = InvalidArgument desc = error on request {
        update: {
          path: {
            origin: "cli"
          }
          val: {
            ascii_val: "\nip as-path access list aclRegexAsPathAllowed permit ^65000$\nip as-path access list aclRegexAsPathAllowed deny .*\nroute-map FILTER-IN 10\nmatch as-path aclRegexAsPathAllowed\n"
          }
        }
        }: failed to apply: Incomplete token (at token 2: 'access'): CLI command 3 of 8 'ip as-path access list aclRegexAsPathAllowed permit ^65000$' failed: invalid command

Did you use an Arista device? I will re-run and upload the log.

@crc-kt
Copy link
Contributor Author

crc-kt commented Aug 20, 2025

The fail logs are now attached to 415458482

PASS log attached after typo fix

@ram-mac
Copy link
Contributor

ram-mac commented Aug 21, 2025

The fail logs are now attached to 415458482

PASS log attached after typo fix

What changed? can you please mention in this PR?

@crc-kt
Copy link
Contributor Author

crc-kt commented Aug 21, 2025

The fail logs are now attached to 415458482

PASS log attached after typo fix

What changed? can you please mention in this PR?

The CLI deviation (for the Arista vendor) for setting the BGP AS Path Set filter had some typos, namely "access list" instead of "access-list" and an extra sequence number for the route-map name where there should have been none (artifacts that somehow crept back in the final commit without my notice). Moreover, since I was playing around with cfgplugins.NewBGPSession that didn't work properly for me, the active (OTG) and passive (DUT) BGP transport modes were mixed at the peer group and AF level. Corrected all of these and uploaded the pass log.

@ram-mac
Copy link
Contributor

ram-mac commented Aug 21, 2025

With the latest changes, i see the test is now passing. Will attach the logs to the bug. 415458482

--- PASS: TestBgpImportExportPolicy (215.77s)
    --- PASS: TestBgpImportExportPolicy/###_RT-1.64.1_Verify_BGP_Peering_without_policy (73.39s)
    --- PASS: TestBgpImportExportPolicy/###_RT-1.64.2_Test_Export_Policy_(Prefix-list_based) (64.17s)
    --- PASS: TestBgpImportExportPolicy/###_RT-1.64.3_Test_Import_Policy_(AS-Path_based) (58.25s)

Copy link
Contributor

@ram-mac ram-mac left a comment

Choose a reason for hiding this comment

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

LGTM

@ram-mac
Copy link
Contributor

ram-mac commented Aug 21, 2025

@crc-kt - Please fix the Static analysis for this PR

@crc-kt
Copy link
Contributor Author

crc-kt commented Aug 21, 2025

@crc-kt - Please fix the Static analysis for this PR

Done.
Same language that complains about a few bytes of extra memory consumption takes 5 minutes for a local staticcheck and eats through hdd space like crazy, way to go, Go! In mari aquam quaeris.

@ram-mac
Copy link
Contributor

ram-mac commented Aug 21, 2025

@crc-kt - Please fix the Static analysis for this PR

Done. Same language that complains about a few bytes of extra memory consumption takes 5 minutes for a local staticcheck and eats through hdd space like crazy, way to go, Go! In mari aquam quaeris.

This PR fails the rebase check. Can you please try to rebase the PR and repush once again?

@crc-kt
Copy link
Contributor Author

crc-kt commented Aug 22, 2025

@crc-kt - Please fix the Static analysis for this PR

Done. Same language that complains about a few bytes of extra memory consumption takes 5 minutes for a local staticcheck and eats through hdd space like crazy, way to go, Go! In mari aquam quaeris.

This PR fails the rebase check. Can you please try to rebase the PR and repush once again?

Done. Please rerun the checks from your side

@ram-mac ram-mac merged commit 634c13f into openconfig:main Aug 22, 2025
14 checks passed
nsadhasivam pushed a commit to nsadhasivam/featureprofiles that referenced this pull request Sep 3, 2025
* test rt1.64

* rt-1.64 address PR comments

* final rt-1.64 test pr comments

* remove unused vars for staticcheck otg-rt-1.64
nsadhasivam pushed a commit to nsadhasivam/featureprofiles that referenced this pull request Oct 26, 2025
* test rt1.64

* rt-1.64 address PR comments

* final rt-1.64 test pr comments

* remove unused vars for staticcheck otg-rt-1.64
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.

4 participants