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

[master][ethtool][chassis] Install ethtool to localhost #18241

Merged
merged 4 commits into from
May 18, 2024

Conversation

mlok-nokia
Copy link
Contributor

Why I did it

Platform Nokia-IXR7250E chassis requires the ethtool to be available in host to disable the Midplane interface autoneg for Linecard-Supervisor midplane communication.

Work item tracking
  • Microsoft ADO (number only):

How I did it

Modify the build_debian.sh to Install the ethtool in host.

How to verify it

Running the new images, verify the ethool is present in /sbin directory

dmin@ixre-cpm-chassis15:~$ ls /sbin/ethtool 
/sbin/ethtool

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@mlok-nokia mlok-nokia requested a review from lguohan as a code owner March 3, 2024 20:25
@mlok-nokia
Copy link
Contributor Author

@rlhui @judyjoseph This PR installs ethtool in host for midplane commuincation setup. Please review.

@mlok-nokia mlok-nokia changed the title [ethtool][chassis] Install ethtool to host [ethtool][chassis] Install ethtool to localhost Mar 6, 2024
@mlok-nokia mlok-nokia changed the title [ethtool][chassis] Install ethtool to localhost [master][ethtool][chassis] Install ethtool to localhost Mar 6, 2024
@lguohan
Copy link
Collaborator

lguohan commented Mar 8, 2024

i remember we removed ethtool on the host and use ethtool in the pmon. can you check the commit history to check why we did that?

@judyjoseph
Copy link
Contributor

@mlok-nokia the ethtool is not present even in 202205 branch of build_debian.sh. Were we installing it via sonic-platform https://github.com/nokia/sonic-platform/blob/e82caa40d4eec59e574bd4acffb0e92fd7187da4/debian/rules#L43 earlier ?

@mlok-nokia
Copy link
Contributor Author

@mlok-nokia the ethtool is not present even in 202205 branch of build_debian.sh. Were we installing it via sonic-platform https://github.com/nokia/sonic-platform/blob/e82caa40d4eec59e574bd4acffb0e92fd7187da4/debian/rules#L43 earlier ?

In 202205 branch, ethtool is built as ethtool_5.9-1_amd64.deb package by the common image build with source code in subdirectory src/ethtool. What you highlight in Nokia platform is that Nokia platform dpkg and copies it to the Nokia folder /opt/srlinux/bin if the ethtoolxxx_amd64.deb exists.

In the current master branch, ethtool is no longer built with the source code and it is installed in the PMON docker by PR# #15856. It appears to be he didn't know that ethtool is needed to be installed in localhost for chassis midplane interface configuration.

@mlok-nokia
Copy link
Contributor Author

mlok-nokia commented Mar 13, 2024

i remember we removed ethtool on the host and use ethtool in the pmon. can you check the commit history to check why we did that?

PR #15856 moved the ethtool to PMON without much description. Based on my underdstanding, it could be needed in the PMON docker too.

@mlok-nokia
Copy link
Contributor Author

@lguohan Please help to review and merge this PR. Thanks

@lguohan
Copy link
Collaborator

lguohan commented Mar 27, 2024

@mlok-nokia , #15856 just install ethtool directly instead of building from source. there is some other PR that move ethtool into the PMON docker. can you check which PR is that and we can check with the owner on this

@mlok-nokia
Copy link
Contributor Author

mlok-nokia commented Mar 27, 2024

@mlok-nokia , #15856 just install ethtool directly instead of building from source. there is some other PR that move ethtool into the PMON docker. can you check which PR is that and we can check with the owner on this

@lguohan It is done in the same PR. This PR adds the "ethtool" to docker/docker-platform-monitor/Dockerfile.j2 to install the ethtool to PMON docker. And removes the rules/ethtool.mk to remove the build from source code.

@mlok-nokia
Copy link
Contributor Author

@k-v1 For chassis, ethtool is required to be installed in localhost. It is required to be localhost to setup midplane interfaces that happens before PMON is running. Is there any reason why it has been moved to PMON?

@k-v1
Copy link
Contributor

k-v1 commented Mar 27, 2024

@mlok-nokia

  1. ethtool for base_image (apt-get from debian repo) was added to build_debian.sh in [baseimage]: add screen package #1644
  2. ethtool for docker-pmon Dockerfile (apt-get from debian repo) was added in [Pmon docker]Add ethtool to pmon docker #2943
  3. In Backport ethtool to support QSFP-DD #5725 there were mutliple changes:
  1. As I understand, 19 Apr 2023 you added installation of ethtool for base image in debian/rules (unexpected place, BTW)
  2. In Don't build ethtool from source #15856 I partially reverted Backport ethtool to support QSFP-DD #5725 because we can just install ethtool 5.9 from debian bullseye repository.

I think the better solution now:

  1. Add ethtool installation to build_debian.sh (apt-get install)
  2. Remove this line.
    I'm not sure why it doesn't work.
    Probably because ethtool installation path is changed (/sbin vs /usr/bin or something like this) or maybe conflict between bullseye and bookworm (base system is bookworm but docker-pmon is bullseye).

@prgeor
Copy link
Contributor

prgeor commented Mar 27, 2024

@keboliu ethtool inside PMON came via your platform need, it will now be removed since your platform no longer needs inside PMON. it will be available in host only.

@prgeor prgeor requested review from keboliu and prgeor March 27, 2024 21:08
@lguohan
Copy link
Collaborator

lguohan commented Mar 27, 2024

@dgsudharsan let us know if you have any concern on this one?

@dgsudharsan
Copy link
Collaborator

@keboliu @Junchao-Mellanox Can you please check if we have any concern?

@mlok-nokia
Copy link
Contributor Author

@prgeor @k-v1 @lguohan @dgsudharsan Per discussion, I have updated the PR with the change of removing ethtool from PMON. Please review

@keboliu
Copy link
Collaborator

keboliu commented Mar 31, 2024

I don't have concerns about removing Ethtool from PMON, but please note that there will be an impact on the way using Ethtool from the host side.
Currently, on the host side, the Ethtool command is a wrapper script to call the Ethtool inside PMON, and it doesn't need root privilege, when we install Ethtool on the host side and use the native Ethtool command, it will require root privilege. This could impact the codes that are calling Ethtool and don't have root privileges.

btw, in this PR, I only see Ethtool is removed from PMON but doesn't install it on the host side, as I mentioned, currently from the host side, the Ethtool command is just a wrapper script to the Ethtool inside PMON, after we remove it, we need to install it on the host side, otherwise we will not have Ethtool, I assume this is not the intention.

@k-v1
Copy link
Contributor

k-v1 commented Mar 31, 2024

@keboliu
Thank you for your clarification.

@mlok-nokia @lguohan
So my clarification was not correct.
Correct version:

  1. In [baseimage]: add screen package #1644 ethtool was added as a debian package to the host system by @lguohan
  2. In [Pmon docker]Add ethtool to pmon docker #2943 ethtool was added as a debian package to the docker-pmon by @keboliu
  3. After Backport ethtool to support QSFP-DD #5725 modifications added by @shlomibitton

There are side effects for cmd_wrapper solution:

  • we can call ethtool on host system to modifiy interface settings without root privileges (because docker-pmon is running in privileged mode).
  • we can't call ethtool on host system until docker-pmon is started:
ethtool -h
Error response from daemon: No such container: pmon
  1. Due to above side effect with docker container installation of ethtool deb package on host system has been added for Nokia platform by @mlok-nokia.

  2. In Don't build ethtool from source #15856 I decided to stop building ethtool from source to reduce SONiC build time because this solution was added in 2021 for debian:buster. But in 2023 docker-pmon was based on debian:bullseye (it has version 5.9 in repository).

After my PR has been merged solution for Nokia platform is broken because we don't have ethtool.deb package in target/debs/bullseye but instead just install it from debian mirrors. But I'm not sure it's correct to keep multiple versions of ethtool (cmd_wrapper /usr/bin/ethtool calling ethtool installed in docker-pmon and real binary package in /sbin/ethtool or somewhere else) on host system.

@k-v1
Copy link
Contributor

k-v1 commented Mar 31, 2024

My suggestion it's not correct to call ethtool on host system via cmd_wrapper docker exec (as it's implemented now).
ethtool is a generic and independent utility and we should have possibility to call it even if docker-pmon is not yet running or dead. So I think it's correct solution to install ethtool directly on host system in build_debian.sh and don't use cmd_wrapper.

But need to answer to 2 questions:

  1. Do we also need to install ethtool to docker-pmon (added in [Pmon docker]Add ethtool to pmon docker #2943).
    If yes then we just do 'apt-get install ethtool` in docker-pmon Dockerfile (without any wrappers).

  2. Should we allow to modify interface settings on host system via ethtool without root or sudo?
    If yes then we need to give permissions (via setuid or anything else).
    Otherwise we should check places where we need to call ethtool with root privileges

@liushilongbuaa
Copy link
Contributor

/azpw ms_conflict

Signed-off-by: mlok <marty.lok@nokia.com>
@mlok-nokia
Copy link
Contributor Author

I don't have concerns about removing Ethtool from PMON, but please note that there will be an impact on the way using Ethtool from the host side. Currently, on the host side, the Ethtool command is a wrapper script to call the Ethtool inside PMON, and it doesn't need root privilege, when we install Ethtool on the host side and use the native Ethtool command, it will require root privilege. This could impact the codes that are calling Ethtool and don't have root privileges.

btw, in this PR, I only see Ethtool is removed from PMON but doesn't install it on the host side, as I mentioned, currently from the host side, the Ethtool command is just a wrapper script to the Ethtool inside PMON, after we remove it, we need to install it on the host side, otherwise we will not have Ethtool, I assume this is not the intention.

The modification in build_debian.sh is for installing the ethtool on host.

@mlok-nokia
Copy link
Contributor Author

I don't have concerns about removing Ethtool from PMON, but please note that there will be an impact on the way using Ethtool from the host side. Currently, on the host side, the Ethtool command is a wrapper script to call the Ethtool inside PMON, and it doesn't need root privilege, when we install Ethtool on the host side and use the native Ethtool command, it will require root privilege. This could impact the codes that are calling Ethtool and don't have root privileges.

btw, in this PR, I only see Ethtool is removed from PMON but doesn't install it on the host side, as I mentioned, currently from the host side, the Ethtool command is just a wrapper script to the Ethtool inside PMON, after we remove it, we need to install it on the host side, otherwise we will not have Ethtool, I assume this is not the intention.

The modification on the build_debian.sh is for installing ethtool on host.

@mlok-nokia
Copy link
Contributor Author

@liushilongbuaa @lguohan Could you please follow up with this PR? Anything I need to do? Thanks.

@rlhui
Copy link
Contributor

rlhui commented May 1, 2024

@prgeor would you please review/comment as it's blocking nokia LC platform to come up? thanks

@rlhui rlhui added the P0 Priority of the issue label May 1, 2024
@arlakshm
Copy link
Contributor

arlakshm commented May 8, 2024

@prgeor, please help review and signoff this PR

judyjoseph
judyjoseph previously approved these changes May 10, 2024
@judyjoseph
Copy link
Contributor

/azpw ms_conflict

@lguohan
Copy link
Collaborator

lguohan commented May 18, 2024

/azpw ms_conflict

@lguohan lguohan disabled auto-merge May 18, 2024 22:12
@lguohan lguohan merged commit 1fd869b into sonic-net:master May 18, 2024
19 checks passed
@mlok-nokia mlok-nokia deleted the install_ethtool branch September 27, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 Priority of the issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants