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

Module rh_ip support ethtool set channels #59134

Merged

Conversation

msciciel
Copy link
Contributor

@msciciel msciciel commented Dec 14, 2020

What does this PR do?

Introduce ability to control network interface channels using rh_ip module and network.managed state.

What issues does this PR fix or reference?

Fixes: #59147

Previous Behavior

It was not possible to control network interface channels. Salt ethtool module do not support channels too because it use python ethtool module which lacks support for channels.

New Behavior

Allow to control network interface channels. On some systems it is useful to manually control channel in order to achieve proper performance.

Merge requirements satisfied?

It's hard to write test for this because all other ethtool related options in rh_ip module do not have tests.

Commits signed with GPG?

No

@msciciel msciciel changed the base branch from master to develop December 14, 2020 15:57
@msciciel msciciel changed the title WIP: Module rh ip support ethtool set channels WIP: Module rh_ip support ethtool set channels Dec 15, 2020
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch 2 times, most recently from ba09bc6 to 4fd9cac Compare December 15, 2020 15:19
@msciciel msciciel marked this pull request as ready for review December 15, 2020 15:40
@msciciel msciciel requested a review from a team as a code owner December 15, 2020 15:40
@msciciel msciciel requested review from cmcmarrow and removed request for a team December 15, 2020 15:40
@msciciel msciciel changed the title WIP: Module rh_ip support ethtool set channels Module rh_ip support ethtool set channels Dec 15, 2020
@msciciel msciciel changed the base branch from develop to master December 16, 2020 09:45
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch from 4fd9cac to 19674de Compare December 16, 2020 09:59
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch from d4dddec to 7cdbff1 Compare January 8, 2021 09:24
@msciciel msciciel marked this pull request as draft January 8, 2021 09:31
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch 2 times, most recently from 50d53b1 to f329b0e Compare January 8, 2021 10:14
@msciciel
Copy link
Contributor Author

msciciel commented Jan 8, 2021

@cmcmarrow may I ask for some hint? I've changed my code to disappear from diff in Pre-Commit / Run Pre-Commit Against Salt (pull_request) test but it still fails and I have no clue why.

@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch from 828256e to 29364b6 Compare January 8, 2021 10:44
@msciciel msciciel marked this pull request as ready for review January 18, 2021 08:40
@msciciel msciciel changed the title Module rh_ip support ethtool set channels WIP: Module rh_ip support ethtool set channels Jan 18, 2021
@msciciel msciciel changed the title WIP: Module rh_ip support ethtool set channels Draft: Module rh_ip support ethtool set channels Jan 18, 2021
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch 3 times, most recently from 3da2ef1 to 044c62a Compare January 18, 2021 16:07
@msciciel msciciel changed the title Draft: Module rh_ip support ethtool set channels Module rh_ip support ethtool set channels Jan 18, 2021
@sagetherage
Copy link
Contributor

@cmcmarrow may I ask for some hint? I've changed my code to disappear from diff in Pre-Commit / Run Pre-Commit Against Salt (pull_request) test but it still fails and I have no clue why.

I apologize I missed this - I will get someone to help you with the dreaded pre-commit. @s0undt3ch @waynew can one of you help with pre-commit failures, here?

@ScriptAutomate
Copy link
Contributor

ScriptAutomate commented Jan 20, 2021

@msciciel For setting up pre-commit, check the following guidance:

In the above link, depending on how your dev environment is setup, you'll see how you need to install pre-commit via pip, and then run pre-commit install in the root of the salt repo fork that you have locally.

Once you've followed those directions, you need to run the following to apply any fixes or guidance against the two files you are modifying:

pre-commit run --files salt/modules/rh_ip.py salt/states/network.py

After this command runs and applies changes, now create a new git commit that is taking all of the newly applied changes.

In the future, pre-commit will handle changes automatically without you having to run this command because it will run before any git commit is able to complete. :)

If you run into any more problems around pre-commit, feel free to ask. Additional assistance can be found on our Slack channel, if needed, also:

@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch 3 times, most recently from afd8d8f to cddba61 Compare February 2, 2021 08:33
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch 2 times, most recently from 8e6d6d0 to dde395b Compare February 3, 2021 10:01
@msciciel
Copy link
Contributor Author

msciciel commented Feb 3, 2021

All checks have passed. Please review my contribution and i'm waiting for feedback.

Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

@msciciel Might have missed it, but wondering how the changes for channels are displayed or output, that is, is there any function doing the equivalent of '-l --show-channels', or is there a need ?

@msciciel
Copy link
Contributor Author

msciciel commented Feb 9, 2021

@msciciel Might have missed it, but wondering how the changes for channels are displayed or output, that is, is there any function doing the equivalent of '-l --show-channels', or is there a need ?

Example state output looks like:

----------
          ID: eth0
    Function: network.managed
      Result: True
     Comment: Interface eth0 updated.
     Started: 14:36:08.176177
    Duration: 1830.047 ms
     Changes:
              ----------
              interface:
                  ---
                  +++
                  @@ -3,4 +3,5 @@
                   USERCTL="no"
                   TYPE="Ethernet"
                   ONBOOT="yes"
                  +ETHTOOL_OPTS="-L eth0  rx 4 tx 4 combined 4"
                   NM_CONTROLLED="no"
              status:
                  Interface eth0 restart to validate

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   1.830 s

Example pillar:

# cat /srv/salt/ethtools.sls
eth0:
  network.managed:
  - enabled: True
  - type: eth
  - channels:
      rx: 4
      tx: 4
      combined: 4

To verify that interface is brought up with proper number of channels it is required to use ethtool -l eth0. Unfortunately salt module ethtool lacks such functionality because it is based on python-ethool module which also lacks such feature.

Altering channels number and ring size during interface initialization is safier than on busy system. I've experienced many cases where alteration of these parameter on busy system caused even kernel panic on some specific network card.

@dmurphy18 do I properly addressed your question or You have mean something else?

dmurphy18
dmurphy18 previously approved these changes Feb 9, 2021
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Thanks for the response, looks good

@sagetherage sagetherage added the Aluminium Release Post Mg and Pre Si label Feb 11, 2021
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

This will need test coverage added

@Ch3LL Ch3LL added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Feb 17, 2021
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch 4 times, most recently from a83e80a to 2f35981 Compare February 19, 2021 15:17
@msciciel
Copy link
Contributor Author

@Ch3LL Please verify if provided test is enough.

@msciciel msciciel requested a review from Ch3LL February 19, 2021 18:32
tests/unit/modules/test_rh_ip.py Outdated Show resolved Hide resolved
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch from 2f35981 to a290a83 Compare February 19, 2021 20:15
…ged state

New param channels can be used to configure network interface channels:

  eth0:
    network.managed:
      - enabled: True
      - type: eth
      - proto: static
      - ipaddr: 10.1.0.7
      - netmask: 255.255.255.0
      - gateway: 10.1.0.1
      - channels:
          rx: 4
          tx: 4
          other: 4
          combined: 4

It is passed to ethtool during network interfaces initialization:

  ethtool -L|--set-channels DEVNAME       Set Channels
         [ rx N ]
         [ tx N ]
         [ other N ]
         [ combined N ]
  rx N
    Changes the number of channels with only receive queues.
  tx N
    Changes the number of channels with only transmit queues.
  other N
    Changes the number of channels used only for other purposes
  combined N
    Changes the number of multi-purpose channels.
@msciciel msciciel force-pushed the module-rh-ip_support-ethtool-set-channels branch from a290a83 to 9cea86d Compare February 23, 2021 07:47
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

Apologies I did not notice this on first review, but this will require a changelog entry. I can add it if you are not able to.

@msciciel
Copy link
Contributor Author

Apologies I did not notice this on first review, but this will require a changelog entry. I can add it if you are not able to.

Thanks. Next time I will try to add changelog entry.

@Ch3LL Ch3LL merged commit 822a2e7 into saltstack:master Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] rh_ip module support interface channels management
8 participants