Open
Conversation
azryve
force-pushed
the
fix/cisco-trunk-vlanmask-patching
branch
from
b6641c7 to
52dd429
Compare
azryve
force-pushed
the
fix/cisco-trunk-vlanmask-patching
branch
3 times, most recently
from
3ed90de to
a136687
Compare
azryve
force-pushed
the
fix/cisco-trunk-vlanmask-patching
branch
2 times, most recently
from
7f15679 to
48a9d3b
Compare
Stuff in swtrunk() does not seem to be reqired at all. By default vlan mask is "allowed all". When you run "allowed vlan 10,20,30" it replaces current mask. This is exactly a behaviour that default patching assumes.
After playing around with config I finally realised the reason for custom logic. There are cases when allowed vlans are splitted in a following manner: switchport trunk allowed vlan 10,20,30,40,50,60,70,80,90,100,110,120,130,140 switchport trunk allowed vlan add 150,160,170 This in turn requires a custom logic. So I reverted swtrunk() back. Still the old beheviour is obviously broken so I spent a day checking behaviour with minimal changes. Summary of changes in cisco/vlandb.py: - refactored tenary if ... else - the code is complicated enought as is - fix "no" from "switchport trunk allowed vlan remove" - its a direct cmd - issue "switchport trunk allowed vlan none" if no old mask existed - by default all vlans are allowed - issue "no switchport trunk allowed vlan" if no new mask added - this returns state to a default
azryve
force-pushed
the
fix/cisco-trunk-vlanmask-patching
branch
from
48a9d3b to
8ed6732
Compare
azryve
requested review from
anteron1992 and
grigory51
and removed request for
Tishka17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This MR fixes custom logic
swtrunk()forswitchport trunk allowedcommand.Added vlandb tests just to make sure that changes does not affect other configuration block.
Current patching logic of
swtrunk()seems to be actually broken.By default on cisco ios when you enable
switchport mode trunkall vlan ids are permitted.AFAIK it has always been the case.
On setting mask on iface in a default state
swtrunk()logic issuesswitchport trunk allowed vlan add ...which is ignored by the device. Again because all vlans are allowed by default.On removal it issues an invalid
no switchport trunk allowed vlan remove ...which instead removes entire allowed vlans config. Because it gets interpreted asno switchport trunk allowed vlan.So summary of changes in
cisco/vlandb.py:allowed vlantestcases were validated on:PS: I am still not sure if we need this add/remove complexity but I spent too much time on this already and too afraid to touch it at this point.