Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LCP: enable by default route-no-paths #1

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Conversation

sever-sever
Copy link
Member

@sever-sever sever-sever commented Jan 15, 2025

Change Summary

LCP: enable by default route-no-paths

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Other (please describe):

Related Task(s)

Related PR(s)

Proposed changes

How to test

SMOKETEST

vyos@r14:~$ /usr/libexec/vyos/tests/smoke/cli/test_vpp.py
test_01_vpp_basic (__main__.TestVPP.test_01_vpp_basic) ... ok
test_02_vpp_vxlan (__main__.TestVPP.test_02_vpp_vxlan) ... ok
test_03_vpp_gre (__main__.TestVPP.test_03_vpp_gre) ... ok
test_04_vpp_geneve (__main__.TestVPP.test_04_vpp_geneve) ... skipped 'Skipping this test geneve index always is 0'
test_05_vpp_loopback (__main__.TestVPP.test_05_vpp_loopback) ... ok
test_06_vpp_bonding (__main__.TestVPP.test_06_vpp_bonding) ... ok
test_07_vpp_bridge (__main__.TestVPP.test_07_vpp_bridge) ... ok
test_08_vpp_ipip (__main__.TestVPP.test_08_vpp_ipip) ... ok
test_09_vpp_xconnect (__main__.TestVPP.test_09_vpp_xconnect) ... ok

----------------------------------------------------------------------
Ran 9 tests in 129.322s

OK (skipped=1)
vyos@r14:~$ 

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@sever-sever sever-sever requested a review from zdc January 15, 2025 16:05
@sever-sever sever-sever self-assigned this Jan 15, 2025
@sever-sever sever-sever added the enhancement New feature or request label Jan 15, 2025
@sever-sever sever-sever marked this pull request as draft January 15, 2025 21:10
@sever-sever sever-sever force-pushed the sever-sever-lcp branch 3 times, most recently from b5457f1 to f680364 Compare January 16, 2025 09:05
@sever-sever sever-sever marked this pull request as ready for review January 16, 2025 09:14
<properties>
<help>Route no paths</help>
<help>Disable route no paths option</help>
Copy link
Member

Choose a reason for hiding this comment

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

I can't find the docs for that option in the VPP manual and from this help string, I'd never be able to guess what the option actually does.

Do you think it's possible to make it more descriptive?

If you point me at the docs, I can also think about it, once I know what it does. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find the docs for that option in the VPP manual and from this help string, I'd never be able to guess what the option actually does.

It was added from the patch

The most suitable option learn-kernel-routes < enable|disable >

What do you think about it?

Copy link
Member

Choose a reason for hiding this comment

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

learn-kernel-routes sounds good to me! Although we made an agreement long ago that we do not create nodes with arguments like true/false, enable/disable, etc.

If you think the most sensible default is to have the dataplane learn kernel routes, then we should have a valueless node like disable-learning-kernel-routes (or similar, e.g., ignore-kernel-routes).

If you think the default should be not to learn then, we should add a valueless node learn-kernel-routes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

set vpp settings lcp ignore-kernel-routes

@sever-sever sever-sever requested a review from dmbaturin January 27, 2025 09:44
@sever-sever sever-sever merged commit a50a34f into current Jan 27, 2025
2 checks passed
@sever-sever sever-sever deleted the sever-sever-lcp branch January 27, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants