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

vxlan: T6401: Avoid calling get_vxlan_vni_filter() unless we need it #3573

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

talmakion
Copy link
Contributor

@talmakion talmakion commented Jun 1, 2024

Change Summary

bridge vni show dev vxlanX will exit with an error if no VNI filters are installed, but the getter is used even when we haven't installed any.

This fix avoids fetching a list of VNI filters unless we know we've created some.

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)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

Component(s) name

  • vxlan

Proposed changes

I've got 2 fix attempts for this, the first one had get_vxlan_vni_filter() check the return code from bridge and return an empty list on error. This is the second, which simply avoids calling get_vxlan_vni_filter() if vni-filter is not enabled.

There only appears to be one caller into get_vxlan_vni_filter(), most accessors in vyos.utils don't bother wrapping errors, so I've switched to this solution.

How to test

vyos@TEST-VYOS-RIGHT# compare
[interfaces]
+ bridge br0 {
+     enable-vlan
+     member {
+         interface vxlan0 {
+         }
+     }
+ }
+ dummy dum0 {
+     address "1.1.1.1/32"
+ }
+ vxlan vxlan0 {
+     parameters {
+         external
+     }
+     source-address "1.1.1.1"
+     vlan-to-vni 10 {
+         vni "10"
+     }
+ }

[edit]
vyos@TEST-VYOS-RIGHT# commit
[edit]
vyos@TEST-VYOS-RIGHT# bridge vni show dev vxlan0
dev               vni                group/remote
RTNETLINK answers: Invalid argument
Dump ternminated
[edit]
vyos@TEST-VYOS-RIGHT# delete interfaces vxlan vxlan0 vlan-to-vni 
[edit]
vyos@TEST-VYOS-RIGHT# commit
[edit]
vyos@TEST-VYOS-RIGHT# rollback-soft 1
Rollback diff has been applied.
Use "compare" to review the changes or "commit" to apply them.
[edit]
vyos@TEST-VYOS-RIGHT# compare
[interfaces vxlan vxlan0]
+ vlan-to-vni 10 {
+     vni "10"
+ }

[edit]
vyos@TEST-VYOS-RIGHT# set interfaces vxlan vxlan0 parameters vni-filter 
[edit]
vyos@TEST-VYOS-RIGHT# commit
[edit]
vyos@TEST-VYOS-RIGHT# bridge vni show dev vxlan0
dev               vni                group/remote
vxlan0            10                 
[edit]
vyos@TEST-VYOS-RIGHT# delete interfaces vxlan vxlan0 vlan-to-vni 
[edit]
vyos@TEST-VYOS-RIGHT# commit
[edit]
vyos@TEST-VYOS-RIGHT# bridge vni show dev vxlan0
dev               vni                group/remote
[edit]
vyos@TEST-VYOS-RIGHT#

bridge does seem to react differently depending on what is configured.

Smoketest result

$ /usr/libexec/vyos/tests/smoke/cli/test_interfaces_vxlan.py 
test_add_multiple_ip_addresses (__main__.VXLANInterfaceTest.test_add_multiple_ip_addresses) ... ok
test_add_single_ip_address (__main__.VXLANInterfaceTest.test_add_single_ip_address) ... ok
test_dhcp_client_options (__main__.VXLANInterfaceTest.test_dhcp_client_options) ... skipped 'not supported'
test_dhcp_disable_interface (__main__.VXLANInterfaceTest.test_dhcp_disable_interface) ... skipped 'not supported'
test_dhcp_vrf (__main__.VXLANInterfaceTest.test_dhcp_vrf) ... skipped 'not supported'
test_dhcpv6_client_options (__main__.VXLANInterfaceTest.test_dhcpv6_client_options) ... skipped 'not supported'
test_dhcpv6_vrf (__main__.VXLANInterfaceTest.test_dhcpv6_vrf) ... skipped 'not supported'
test_dhcpv6pd_auto_sla_id (__main__.VXLANInterfaceTest.test_dhcpv6pd_auto_sla_id) ... skipped 'not supported'
test_dhcpv6pd_manual_sla_id (__main__.VXLANInterfaceTest.test_dhcpv6pd_manual_sla_id) ... skipped 'not supported'
test_interface_description (__main__.VXLANInterfaceTest.test_interface_description) ... ok
test_interface_disable (__main__.VXLANInterfaceTest.test_interface_disable) ... ok
test_interface_ip_options (__main__.VXLANInterfaceTest.test_interface_ip_options) ... ok
test_interface_ipv6_options (__main__.VXLANInterfaceTest.test_interface_ipv6_options) ... ok
test_interface_mtu (__main__.VXLANInterfaceTest.test_interface_mtu) ... ok
test_ipv6_link_local_address (__main__.VXLANInterfaceTest.test_ipv6_link_local_address) ... ok
test_mtu_1200_no_ipv6_interface (__main__.VXLANInterfaceTest.test_mtu_1200_no_ipv6_interface) ... ok
test_span_mirror (__main__.VXLANInterfaceTest.test_span_mirror) ... skipped 'not supported'
test_vif_8021q_interfaces (__main__.VXLANInterfaceTest.test_vif_8021q_interfaces) ... skipped 'not supported'
test_vif_8021q_lower_up_down (__main__.VXLANInterfaceTest.test_vif_8021q_lower_up_down) ... skipped 'not supported'
test_vif_8021q_mtu_limits (__main__.VXLANInterfaceTest.test_vif_8021q_mtu_limits) ... skipped 'not supported'
test_vif_8021q_qos_change (__main__.VXLANInterfaceTest.test_vif_8021q_qos_change) ... skipped 'not supported'
test_vif_s_8021ad_vlan_interfaces (__main__.VXLANInterfaceTest.test_vif_s_8021ad_vlan_interfaces) ... skipped 'not supported'
test_vif_s_protocol_change (__main__.VXLANInterfaceTest.test_vif_s_protocol_change) ... skipped 'not supported'
test_vxlan_external (__main__.VXLANInterfaceTest.test_vxlan_external) ... ok
test_vxlan_neighbor_suppress (__main__.VXLANInterfaceTest.test_vxlan_neighbor_suppress) ... ok
test_vxlan_parameters (__main__.VXLANInterfaceTest.test_vxlan_parameters) ... ok
test_vxlan_vlan_vni_mapping (__main__.VXLANInterfaceTest.test_vxlan_vlan_vni_mapping) ... ok
test_vxlan_vni_filter (__main__.VXLANInterfaceTest.test_vxlan_vni_filter) ... ok
test_vxlan_vni_filter_add_remove (__main__.VXLANInterfaceTest.test_vxlan_vni_filter_add_remove) ... ok

----------------------------------------------------------------------
Ran 29 tests in 158.646s

OK (skipped=14)

Checklist:

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

`bridge vni show dev vxlanX` will exit with an error if no VNI filters
are installed, but the getter is used even when we haven't installed any.

This fix avoids fetching a list of VNI filters unless we know we've
created some.
@talmakion
Copy link
Contributor Author

Updated the report

  • Some issues in my custom testing spin prevented vxlan smoketesting, works OK with a nightly + vyos-1x-smoketest installed
  • Re-did the test section with the patch applied in the nightly, noticed I'd copy/pasted the wrong piece last time.

@dmbaturin dmbaturin merged commit 1c57ed8 into vyos:current Jun 6, 2024
8 checks passed
@c-po
Copy link
Member

c-po commented Jun 9, 2024

@Mergifyio backport sagitta

Copy link
Contributor

mergify bot commented Jun 9, 2024

backport sagitta

✅ Backports have been created

c-po added a commit that referenced this pull request Jun 9, 2024
vxlan: T6401: Avoid calling get_vxlan_vni_filter() unless we need it (backport #3573)
@talmakion talmakion deleted the bugfix/T6401-2 branch June 11, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants