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

Add support for ETHTOOL Ring option in Network Role #355

Merged
merged 1 commit into from
May 11, 2021
Merged

Add support for ETHTOOL Ring option in Network Role #355

merged 1 commit into from
May 11, 2021

Conversation

liangwen12year
Copy link
Collaborator

Signed-off-by: Wen Liang [email protected]

@tyll tyll added the Wait_Submitter The submitter needs to do something before this can move on label Feb 22, 2021
@richm richm mentioned this pull request Mar 12, 2021
Merged
@richm
Copy link
Contributor

richm commented Mar 12, 2021

please rebase to latest main branch

@richm
Copy link
Contributor

richm commented Mar 16, 2021

If this PR is for a new feature, could you add something to the README.md to describe the new feature?

ffmancera
ffmancera requested changes Apr 10, 2021
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.

Hi! Thanks for this work. Please, could check why the CI is failing? In addition, it would be nice to have a detailed description in the commit message.

Thank you!

examples/ethtool_setring.yml Outdated Show resolved Hide resolved
library/network_connections.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
module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved
module_utils/network_lsr/nm_provider.py Outdated Show resolved Hide resolved
module_utils/network_lsr/nm_provider.py Outdated Show resolved Hide resolved
@tyll
Copy link
Member

tyll commented Apr 16, 2021

@liangwen12year this should be rather easy to add but the PR is very outdated. It seems to contain changes that were already added.

@liangwen12year liangwen12year force-pushed the issue113 branch 8 times, most recently from 55382d8 to 698d723 Compare April 16, 2021 16:00
@lgtm-com
Copy link

lgtm-com bot commented Apr 16, 2021

This pull request introduces 1 alert when merging 698d723 into c2e83b1 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Apr 16, 2021

This pull request introduces 1 alert when merging b80917c into c2e83b1 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@liangwen12year liangwen12year force-pushed the issue113 branch 6 times, most recently from 32e3adb to 3ccda62 Compare April 16, 2021 18:04
@liangwen12year liangwen12year changed the title Add support for ethtool set-ring settings in Network Role Add support for ETHTOOL Ring option in Network Role Apr 16, 2021
@liangwen12year liangwen12year added Wait_Review and removed Wait_Submitter The submitter needs to do something before this can move on labels Apr 16, 2021
@liangwen12year
Copy link
Collaborator Author

liangwen12year commented Apr 16, 2021 •
edited
Loading

Hi! Thanks for this work. Please, could check why the CI is failing? In addition, it would be nice to have a detailed description in the commit message.

Thank you!

Detailed description added, PR was rebased, conflict resolved, CI failure fixed. Thanks!

@liangwen12year liangwen12year force-pushed the issue113 branch 2 times, most recently from 2a05a17 to 398ccf1 Compare May 2, 2021 13:32
@liangwen12year liangwen12year removed the Wait_Submitter The submitter needs to do something before this can move on label May 2, 2021
@jharuda
Copy link
Contributor

jharuda commented May 3, 2021

[citest pending]

@coveralls
Copy link

coveralls commented May 4, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 828078048

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 563 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.06%) to 39.6%
Files with Coverage Reduction New Missed Lines %
/home/runner/work/network/network/module_utils/network_lsr/nm_provider.py 3 84.21%
/home/runner/work/network/network/module_utils/network_lsr/argument_validator.py 43 83.76%
/home/runner/work/network/network/library/network_connections.py 517 21.26%
Totals Coverage Status
Change from base Build 824567280: 0.06%
Covered Lines: 1009
Relevant Lines: 2548
💛 - Coveralls

@richm
Copy link
Contributor

richm commented May 4, 2021

[citest pending]

@richm
Copy link
Contributor

richm commented May 4, 2021

[citest bad]

@richm
Copy link
Contributor

richm commented May 4, 2021

[citest pending]

ffmancera
ffmancera reviewed May 5, 2021
View reviewed changes
README.md Show resolved Hide resolved
ffmancera
ffmancera approved these changes May 5, 2021
View reviewed changes
Copy link
Collaborator

@ffmancera ffmancera left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Other than my nit, it looks good. Thanks! Please, could you include more information on the commit message. One ethtool ring example would be great, thanks!

@ffmancera ffmancera added Wait_Submitter The submitter needs to do something before this can move on and removed Wait_Review labels May 5, 2021
@liangwen12year
Copy link
Collaborator Author

Other than my nit, it looks good. Thanks! Please, could you include more information on the commit message. One ethtool ring example would be great, thanks!

Added ring example to the commit message.Thanks.

@liangwen12year liangwen12year added Wait_Review and removed Wait_Submitter The submitter needs to do something before this can move on labels May 6, 2021
@liangwen12year
Add support for ETHTOOL Ring option
6e36cc0 
ETHTOOL Ring option is not supported by NetworkManager until
NM 1.25.2. Currently, ETHTOOL Ring option is not suppored by
Network role, so enable the support for ETHTOOL Ring option.

Configure ethtool ring option via:

	```yaml
            network_connections:
              - name: testnic1
                type: ethernet
                state: up
                ip:
                  dhcp4: no
                  auto6: no
                ethtool:
                  ring:
                    rx: 128
                    rx_jumbo: 128
                    rx_mini: 128
                    tx: 128
	```

Signed-off-by: Wen Liang <[email protected]>
@tyll tyll enabled auto-merge (rebase) May 10, 2021 12:53
@ffmancera
Copy link
Collaborator

[citest bad]

@tyll tyll merged commit ae2d60a into linux-system-roles:main May 11, 2021
@tyll tyll linked an issue Oct 18, 2021 that may be closed by this pull request
Support ethtool -G|--set-ring options #113
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thom311 thom311 thom311 left review comments

@tyll tyll tyll left review comments

@richm richm richm left review comments

@ffmancera ffmancera ffmancera approved these changes

@cathay4t cathay4t cathay4t approved these changes

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

Successfully merging this pull request may close these issues.

Support ethtool -G|--set-ring options
8 participants
@liangwen12year @richm @tyll @jharuda @coveralls @ffmancera @cathay4t @thom311