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.

Sign up for GitHub

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

Jump to bottom

Network: support nmstate bridge STP options #283

Closed
wants to merge 15 commits into from
Closed

Network: support nmstate bridge STP options #283

wants to merge 15 commits into from

Conversation

erav
Copy link
Member

@erav erav commented Jul 14, 2022 •
edited
Loading

step1: cleanup
step 2: resolve https://bugzilla.redhat.com/2108974

@erav erav requested a review from almusil as a code owner July 14, 2022 20:22
@erav erav requested a review from mz-pdm July 14, 2022 20:22
@erav erav marked this pull request as draft July 14, 2022 20:23
mz-pdm
mz-pdm reviewed Jul 15, 2022
View reviewed changes
Copy link
Member

@mz-pdm mz-pdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup. Please make sure that it works (tests pass at minimum) after each of the commits.

lib/vdsm/network/ipwrapper.py Outdated Show resolved Hide resolved
lib/vdsm/network/netinfo/bonding.py Show resolved Hide resolved
@erav erav force-pushed the nmstate-for-bridge-options branch 7 times, most recently from 16f6088 to 85d42b6 Compare July 18, 2022 05:03
almusil
almusil requested changes Jul 18, 2022
View reviewed changes
Copy link
Member

@almusil almusil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The improvements are nice overall, I left some comments. However I would disagree that it solves #270, the module was not visited by this PR (maybe I am looking at this too early). Also please consider changing the title of the PR, this pretty big refactor that is addressing multiple things not only bridge options.

lib/vdsm/network/link/setup.py Outdated Show resolved Hide resolved
lib/vdsm/network/ipwrapper.py Outdated Show resolved Hide resolved
lib/vdsm/network/initializer.py Show resolved Hide resolved
lib/vdsm/network/netinfo/cache.py Outdated Show resolved Hide resolved
lib/vdsm/network/netinfo/cache.py Outdated Show resolved Hide resolved
tests/network/unit/netinfo_test.py Outdated Show resolved Hide resolved
@erav erav changed the title Nmstate for bridge options Network: refactoring in preparation for issue #270 Jul 24, 2022
@erav erav changed the title Network: refactoring in preparation for issue #270 Network: refactoring in preparation for issue 270 Jul 24, 2022
@erav erav force-pushed the nmstate-for-bridge-options branch 4 times, most recently from 0fb0bdb to e371515 Compare July 25, 2022 20:31
@erav erav changed the title Network: refactoring in preparation for issue 270 Network: refactoring Jul 26, 2022
@erav erav added the network label Jul 26, 2022
@erav
Copy link
Member Author

erav commented Jul 26, 2022

/ost

@erav
Copy link
Member Author

erav commented Jul 26, 2022

/ost network-suite-master

@erav
Copy link
Member Author

erav commented Jul 26, 2022

/ost network-suite-master

passed

@erav
Copy link
Member Author

erav commented Jul 26, 2022

/ost basic-suite-master

mz-pdm
mz-pdm reviewed Jul 26, 2022
View reviewed changes
Copy link
Member

@mz-pdm mz-pdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing, otherwise it looks good to me.

lib/vdsm/network/ipwrapper.py Outdated Show resolved Hide resolved
@erav erav force-pushed the nmstate-for-bridge-options branch from e371515 to 811e147 Compare July 26, 2022 15:08
@erav erav added the cleanup Code change keeping current behavior label Jul 26, 2022
@erav erav force-pushed the nmstate-for-bridge-options branch from 811e147 to 317e324 Compare July 27, 2022 20:08
@erav
Copy link
Member Author

erav commented Jul 28, 2022

leaving unmerged until 4.5.2 is out to avoid risk of regression

@mz-pdm
Copy link
Member

mz-pdm commented Jul 29, 2022

OK, looks good, let's wait until 4.5.2 branch is created.

erav added 4 commits August 1, 2022 22:26
@erav
network: encapsulate ipwrapper artifacts
5db598a 
Change-Id: I18fcc7c8d1120893b185f058668eec12ce7b70f3
Signed-off-by: Eitan Raviv <[email protected]>
@erav
network: remove masking imports
99c7231 
'import as' makes it difficult to follow where code is called from
because of the heavy reuse of the same names for different concepts.

Change-Id: Idfed823e0821ec4e6f055880efabb6046042cc69

Signed-off-by: Eitan Raviv <[email protected]>
Change-Id: I367f7cd0a50a11ff80754f2b0c8aa6fcf1ae90ad
@erav
network: remove unused code and redundant indirections
a39f69c 
Change-Id: Ic16c346f02f9c5ee481c227391b7c7c21e713ab5

Signed-off-by: Eitan Raviv <[email protected]>
Change-Id: Id2868b93e83de729f9680380250fb3c499d74e25
@erav
network: consolidate functionality into ipwrapper
f08562d 
- fix leakage of internals of ipwrapper
- enhance possibility of reuse

Change-Id: Ia52b5661ec68b6982a03927daea14eb32cd9ece5
Signed-off-by: Eitan Raviv <[email protected]>
@erav erav force-pushed the nmstate-for-bridge-options branch from b1da1bb to 4be2201 Compare August 1, 2022 19:29
@erav
network: remove masking import for sysfs_options
cc553ac 
The masking import add no value, but interfere with the structure of the
code and its readability.

Change-Id: I263aaec49a05205002649bf454034911cfc74610
Signed-off-by: Eitan Raviv <[email protected]>
@erav erav force-pushed the nmstate-for-bridge-options branch from 4be2201 to 99dd58b Compare August 3, 2022 20:35
@erav erav changed the title Network: refactoring Network: support nmstate bridge STP options Aug 3, 2022
@erav erav force-pushed the nmstate-for-bridge-options branch from 99dd58b to d9e24ec Compare August 3, 2022 20:47
@erav erav added bug Issue is a bug or fix for a bug and removed cleanup Code change keeping current behavior labels Aug 3, 2022
almusil
almusil requested changes Aug 4, 2022
View reviewed changes
lib/vdsm/network/link/setup.py Outdated Show resolved Hide resolved
lib/vdsm/network/link/setup.py Outdated Show resolved Hide resolved
lib/vdsm/network/link/setup.py Outdated Show resolved Hide resolved
tests/network/functional/bridge_test.py Show resolved Hide resolved
lib/vdsm/network/link/setup.py Outdated Show resolved Hide resolved
@erav
network: simplify network.link.setup
79ac57b 
Properly reflect what option parsers are doing for readability while
refactoring for reuse, extend-ability.

Change-Id: I734ade2f9265f9be5f473665fb458ab1f7d9b469
Signed-off-by: Eitan Raviv <[email protected]>
@erav erav force-pushed the nmstate-for-bridge-options branch 6 times, most recently from ac2222c to 3c0bd05 Compare August 8, 2022 18:55
@erav erav marked this pull request as ready for review August 8, 2022 19:27
almusil
almusil requested changes Aug 9, 2022
View reviewed changes
lib/vdsm/network/nmstate/bridge_opts.py Outdated Show resolved Hide resolved
lib/vdsm/network/nmstate/bridge_opts.py Outdated Show resolved Hide resolved
lib/vdsm/network/nmstate/bridge_opts.py Outdated Show resolved Hide resolved
lib/vdsm/network/nmstate/bridge_opts.py Outdated Show resolved Hide resolved
lib/vdsm/network/nmstate/bridge_opts.py Outdated Show resolved Hide resolved
lib/vdsm/network/nmstate/bridge_opts.py Outdated Show resolved Hide resolved
@erav
network: network.link.setup - extract bridge options
60f32fc 
Extract the options so the parser can be used with any set of options.

Signed-off-by: Eitan Raviv <[email protected]>
Change-Id: I8a73251969083c25812c8ad534df1dfdf2266645
@erav erav force-pushed the nmstate-for-bridge-options branch 3 times, most recently from 632e649 to ce08adc Compare August 10, 2022 10:12
erav added 3 commits August 10, 2022 13:35
@erav
network: apply stp bridge options with nmstate
ce205c5 
Use nmstate to apply STP options to a bridge.
It is not possible to set the STP state to 'off' and at the same time
pass non-default values for the other STP attributes (max-age,
hello-time, forward-delay).

Bug-Url: https://bugzilla.redhat.com/2108974
Change-Id: I51077ee206bd0b0e2644b4103b37280dbda5cd8f
Signed-off-by: Eitan Raviv <[email protected]>
@erav
network: move bridge option parsing under nmstate
c5b9c24 
Encapsulate bridge options parsing in network/nmstate because it is used
in production code only in network/nmstate.

Signed-off-by: Eitan Raviv <[email protected]>
Change-Id: I0ae4fa94efe4ddd1fd861a1b89170cbb6d703b73
@erav
network: test stp bridge options with nmstate
c83de0c 
Test that nmstate correctly applies STP bridge options passed during
setup networks.
It is not possible to set the STP state to 'off' and at the same time
pass non-default values for the other STP attributes (max-age,
hello-time, forward-delay).

Change-Id: Ie904f9943d9cb8a99718f85ab7c8c9a264b97a39
Bug-Url: https://bugzilla.redhat.com/2108974
Signed-off-by: Eitan Raviv <[email protected]>
@erav erav force-pushed the nmstate-for-bridge-options branch from ce08adc to c83de0c Compare August 10, 2022 10:40
@erav erav closed this Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mz-pdm mz-pdm mz-pdm left review comments

@almusil almusil almusil requested changes

Assignees
No one assigned
Labels
bug Issue is a bug or fix for a bug network
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erav @mz-pdm @almusil