community.network icon indicating copy to clipboard operation
community.network copied to clipboard

icx_vlan - add support for stacked switches

Open ratneshnagori opened this issue 3 years ago • 6 comments

SUMMARY

icx_vlan.py at the moment is used to manage VLANs on Ruckus ICX 7000 series switches.

Currently, it doesn't support vlan management when switches are stacked into a single unit.

This pull request is to enable icx_vlan.py to manage vlans when we have the stack of switches.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

icx_vlan.py - changes to enable module to manage vlans on stacked switches.

ADDITIONAL INFORMATION

The problem is because of hardcoding of STACK/MODULE to 1/1 at below lines -

https://github.com/ansible-collections/community.network/blob/fa9911db5354d9bd373956df969b7fd0e6ca6f35/plugins/modules/network/icx/icx_vlan.py#L537 https://github.com/ansible-collections/community.network/blob/fa9911db5354d9bd373956df969b7fd0e6ca6f35/plugins/modules/network/icx/icx_vlan.py#L560 https://github.com/ansible-collections/community.network/blob/fa9911db5354d9bd373956df969b7fd0e6ca6f35/plugins/modules/network/icx/icx_vlan.py#L618 https://github.com/ansible-collections/community.network/blob/fa9911db5354d9bd373956df969b7fd0e6ca6f35/plugins/modules/network/icx/icx_vlan.py#L626


ratneshnagori avatar Jun 06 '21 09:06 ratneshnagori

@ratneshnagori hi, thanks for the PR!

Could you please:

  1. Add a changelog fragment https://docs.ansible.com/ansible/devel/community/development_process.html#creating-a-changelog-fragment
  2. Fix sanity and unit test failings (I re-run the tests. When they finish, if there's a red cross on some, you could see what's wrong through pushing a corresponding "Details" link).

Thank you!

Andersson007 avatar Jul 13 '21 09:07 Andersson007

@Andersson007 Hi,

I am trying to fix the failures I see in unit tests. However, I have noticed that the icx_vlan.py will fail if we have multiple range of interfaces under a vlan.

On a ruckus switch, you can have multiple ranges of interfaces under same vlan -

vlan 100 name ABC by port ethe 1/1/14 to 1/1/15 ethe 1/1/17 to 1/1/18

This will result into incorrecr value of high coming from function extract_list_from_interface(interface) - https://github.com/ansible-collections/community.network/blob/main/plugins/modules/network/icx/icx_vlan.py#L371

For the above config, when I ran function, it gave me output as low = 14 and high = 15. Value of low is correct but not high.

Regex here - https://github.com/ansible-collections/community.network/blob/main/plugins/modules/network/icx/icx_vlan.py#L374, only looks for first occurrence of the pattern.

Let me know your thoughts. I will work towards getting the correct result from the function.

ratneshnagori avatar Jul 21 '21 12:07 ratneshnagori

@ratneshnagori thanks for working on this! Unfortunately I can't provide any ideas on this case because I'm not a network specialist. Try to use your judgement how to solve it more gracefully and safely. Our main goal is to not break things that worked before. After CI is green and the changelog is added, we could look for network folks to review the PR.

I have an idea but maybe it's not applicable and stupid. So if it's not applicable, feel free to ignore. The idea is to use a special boolean option, e.g. called stack_of_switches with default no. If a user use yes, you could change the behavior of the module, e.g. place the related code to separate functions, etc. not changing the existing code much. And you could cover those function with separate tests. It's just an idea, if not applicable, feel free to ignore.

Thank you

Andersson007 avatar Jul 23 '21 07:07 Andersson007

hi @ratneshnagori - I just pushed a commit to revert some of the style differences to make the diff easier to read. However, feel free to remove it. :)

dericcrago avatar Nov 11 '21 19:11 dericcrago

cc @Commscope @sushma-alethea click here for bot help

ansibullbot avatar Nov 17 '21 19:11 ansibullbot

hi @ratneshnagori, are you still working on this?

dericcrago avatar Dec 08 '21 20:12 dericcrago