-
Notifications
You must be signed in to change notification settings - Fork 50
Adding property - fabric forwarding mode anycast-gateway, to Cisco_Interface #215
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
Changes from all commits
171ee07
1136f44
b8f40cc
6342900
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,7 +131,7 @@ def anycast_gateway_mac | |
| def anycast_gateway_mac=(mac_addr) | ||
| fail TypeError unless mac_addr.is_a?(String) | ||
|
|
||
| Feature.nv_overlay_evpn_enable unless Feature.nv_overlay_evpn_enabled? | ||
| Feature.nv_overlay_evpn_enable | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? |
||
| if mac_addr == default_anycast_gateway_mac | ||
| state = 'no' | ||
| mac_addr = '' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| require_relative 'ciscotest' | ||
| require_relative '../lib/cisco_node_utils/acl' | ||
| require_relative '../lib/cisco_node_utils/interface' | ||
| require_relative '../lib/cisco_node_utils/overlay_global' | ||
|
|
||
| include Cisco | ||
|
|
||
|
|
@@ -958,6 +959,42 @@ def test_interface_ipv4_arp_timeout | |
| assert_raises(RuntimeError) { nonvlanint.ipv4_arp_timeout = 300 } | ||
| end | ||
|
|
||
| def test_interface_fabric_forwarding_anycast_gateway | ||
| # Setup | ||
| config('no interface vlan11') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using the Update: Thinking more... using the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignored |
||
| int = Interface.new('vlan11') | ||
| foo = OverlayGlobal.new | ||
| foo.anycast_gateway_mac = '1223.3445.5668' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explicitly configuring 'feature fabric forwarding' isn't needed for VxlanGlobal/OverlayGlobal - unless it's needed for the interface property, please remove it. These three lines should just become:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @glennmatthews It is needed in vs. the behavior on
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @smigopal I spoke with @glennmatthews and he clarified that his point is that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mikewiebe : Awesome. Just to verify, this means I wouldn't need to enable the feature using "config('feature fabric forwarding')". Just the setter should suffice = "OverlayGlobal.new.anycast_gateway_mac = '1223.3445.5668'"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignored. |
||
|
|
||
| # 1. Testing default | ||
| int.fabric_forwarding_anycast_gateway = | ||
| int.default_fabric_forwarding_anycast_gateway | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer removing these two lines - have the test below be for verifying that the default state of a newly created Vlan matches the expected default value, rather than explicitly setting it to default. |
||
| assert_equal(int.default_fabric_forwarding_anycast_gateway, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use the more specific
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eh? assert_equal is the correct thing to use here since you're comparing against the default value. However, the line above should be: int.fabric_forwarding_anycast_gateway = int.default_fabric_forwarding_anycast_gateway
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, my mistake.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| int.fabric_forwarding_anycast_gateway) | ||
|
|
||
| # 2. Testing non-default:true | ||
| int.fabric_forwarding_anycast_gateway = true | ||
| assert(int.fabric_forwarding_anycast_gateway) | ||
|
|
||
| # 3. Setting back to false | ||
| int.fabric_forwarding_anycast_gateway = false | ||
| refute(int.fabric_forwarding_anycast_gateway) | ||
|
|
||
| # 4. Removing fabric forwarding anycast gateway mac | ||
| foo.anycast_gateway_mac = foo.default_anycast_gateway_mac | ||
| assert_raises(RuntimeError) { int.fabric_forwarding_anycast_gateway = true } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presumably we already have similar tests for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but this is testing the specific error case in the interface code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, just making sure we don't have any unnecessary overlap.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignored |
||
|
|
||
| # 5. Removing feature fabric forwarding | ||
| config('no feature fabric forwarding') | ||
| assert_raises(RuntimeError) { int.fabric_forwarding_anycast_gateway = true } | ||
|
|
||
| # 6. Attempting to configure on a non-vlan interface | ||
| nonvlanint = create_interface | ||
| assert_raises(RuntimeError) do | ||
| nonvlanint.fabric_forwarding_anycast_gateway = true | ||
| end | ||
| end | ||
|
|
||
| def test_interface_ipv4_proxy_arp | ||
| interface = create_interface | ||
| interface.switchport_mode = :disabled | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the CHANGELOG to include this property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done