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

icx_vlan - add support for stacked switches #279

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ratneshnagori
Copy link

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 -

have_interfaces.append('ethernet 1/1/{0}'.format(low))

have_tagged.append('ethernet 1/1/{0}'.format(low))

ports.append('ethernet 1/1/' + str(port))

ports.append('ethernet 1/1/' + str(port))


@Andersson007
Copy link
Contributor

@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!

@ratneshnagori
Copy link
Author

@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.

@Andersson007
Copy link
Contributor

@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

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module module needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) labels Nov 10, 2021
@dericcrago
Copy link
Contributor

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. :)

@ansibullbot
Copy link
Collaborator

@dericcrago
Copy link
Contributor

hi @ratneshnagori, are you still working on this?

@ansibullbot ansibullbot added stale_ci CI is older than 7 days, rerun before merging and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants