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

Support more bond options #452

Merged
merged 1 commit into from Feb 11, 2022
Merged

Support more bond options #452

merged 1 commit into from Feb 11, 2022

Conversation

liangwen12year
Copy link
Collaborator

In order to enable user to flexibly control the network transmission
over the bonded interface, support all the bond options which are
currently supported by NetworkManager.

Signed-off-by: Wen Liang liangwen12year@gmail.com

@liangwen12year
Copy link
Collaborator Author

I will add the documentation soon.

@liangwen12year liangwen12year changed the title Support more bond options [Draft]Support more bond options Feb 3, 2022
@coveralls
Copy link

coveralls commented Feb 3, 2022 •
edited

Pull Request Test Coverage Report for Build 1831504275

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 466 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.7%) to 42.279%
Files with Coverage Reduction New Missed Lines %
/home/runner/work/network/network/module_utils/network_lsr/argument_validator.py 73 84.22%
/home/runner/work/network/network/library/network_connections.py 393 20.91%
Totals Coverage Status
Change from base Build 1831490288: 0.7%
Covered Lines: 1191
Relevant Lines: 2817
💛 - Coveralls

@liangwen12year liangwen12year force-pushed the bond_options branch 11 times, most recently from 1e044b4 to 8ec0a4d Compare February 7, 2022 02:23
@liangwen12year liangwen12year changed the title [Draft]Support more bond options Support more bond options Feb 8, 2022
@liangwen12year liangwen12year force-pushed the bond_options branch 2 times, most recently from 3b44abc to 0185582 Compare February 8, 2022 04:18
ffmancera
ffmancera requested changes Feb 8, 2022
View reviewed changes
Copy link
Collaborator

@ffmancera ffmancera left a comment

Choose a reason for hiding this comment

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

The code looks quite good! I made some comments, thanks!

module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved
module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved
module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved
@liangwen12year
Copy link
Collaborator Author

[citest bad]

thom311
thom311 reviewed Feb 9, 2022
View reviewed changes
module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved
module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved
module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved
module_utils/network_lsr/argument_validator.py Show resolved Hide resolved
@liangwen12year liangwen12year force-pushed the bond_options branch 6 times, most recently from 6b6832e to bbfeb98 Compare February 9, 2022 17:13
thom311
thom311 previously approved these changes Feb 9, 2022
View reviewed changes
@ffmancera
Copy link
Collaborator

[citest bad]

ffmancera
ffmancera requested changes Feb 10, 2022
View reviewed changes
Copy link
Collaborator

@ffmancera ffmancera left a comment

Choose a reason for hiding this comment

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

This is breaking bond with initscripts because it is validating the properties incorrectly

MSG:

error: connection[0]: initscripts only supports the mode and miimon bond options. All the other bond options are not supported by initscripts.
META:

ffmancera
ffmancera previously approved these changes Feb 10, 2022
View reviewed changes
Copy link
Collaborator

@ffmancera ffmancera left a comment

Choose a reason for hiding this comment

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

The new code LGTM!

thom311
thom311 previously approved these changes Feb 10, 2022
View reviewed changes
thom311
thom311 previously approved these changes Feb 11, 2022
View reviewed changes
tyll
tyll requested changes Feb 11, 2022
View reviewed changes
Copy link
Member

@tyll tyll left a comment

Choose a reason for hiding this comment

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

There are too many mentions of "slave" in the README. Please remove them.

tyll
tyll reviewed Feb 11, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
tyll
tyll reviewed Feb 11, 2022
View reviewed changes
Copy link
Member

@tyll tyll left a comment

Choose a reason for hiding this comment

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

There is once instance left

README.md Outdated Show resolved Hide resolved
@liangwen12year @tyll
Support more bond options
c326433 
In order to enable user to flexibly control the network transmission
over the bonded interface, support all the bond options which are
currently supported by NetworkManager.

Signed-off-by: Wen Liang <liangwen12year@gmail.com>
@tyll tyll merged commit 59be618 into linux-system-roles:main Feb 11, 2022
@tyll tyll linked an issue Feb 14, 2022 that may be closed by this pull request
An option to configure an active-backup bond device #448
Closed
@liangwen12year liangwen12year linked an issue Feb 14, 2022 that may be closed by this pull request
LACP (802.3ad) Support lacp_rate #439
Closed
@liangwen12year liangwen12year mentioned this pull request Feb 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyll tyll tyll approved these changes

@thom311 thom311 thom311 left review comments

@ffmancera ffmancera ffmancera left review comments

Assignees
No one assigned
Labels
Wait_Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

An option to configure an active-backup bond device LACP (802.3ad) Support lacp_rate
5 participants
@liangwen12year @coveralls @ffmancera @tyll @thom311