sonic-buildimage icon indicating copy to clipboard operation
sonic-buildimage copied to clipboard

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

Open mlok-nokia opened this issue 11 months ago • 22 comments

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 avatar Mar 03 '24 20:03 mlok-nokia

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

mlok-nokia avatar Mar 03 '24 20:03 mlok-nokia

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?

lguohan avatar Mar 08 '24 16:03 lguohan

@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 ?

judyjoseph avatar Mar 11 '24 23:03 judyjoseph

@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# https://github.com/sonic-net/sonic-buildimage/pull/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 avatar Mar 13 '24 00:03 mlok-nokia

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 https://github.com/sonic-net/sonic-buildimage/pull/15856 moved the ethtool to PMON without much description. Based on my underdstanding, it could be needed in the PMON docker too.

mlok-nokia avatar Mar 13 '24 00:03 mlok-nokia

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

mlok-nokia avatar Mar 27 '24 14:03 mlok-nokia

@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 avatar Mar 27 '24 16:03 lguohan

@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 avatar Mar 27 '24 17:03 mlok-nokia

@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?

mlok-nokia avatar Mar 27 '24 19:03 mlok-nokia

@mlok-nokia

  1. ethtool for base_image (apt-get from debian repo) was added to build_debian.sh in #1644
  2. ethtool for docker-pmon Dockerfile (apt-get from debian repo) was added in #2943
  3. In #5725 there were mutliple changes:
  • build ethtool 5.9 from source (because ethtool from debian buster has version 4.19)
  • don't install ethtool directly to the base image but instead use strange solution with copying the binary file (https://github.com/sonic-net/sonic-buildimage/pull/5725#issuecomment-759014651)
  1. As I understand, 19 Apr 2023 you added installation of ethtool for base image in debian/rules (unexpected place, BTW)
  2. In #15856 I partially reverted #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).

k-v1 avatar Mar 27 '24 20:03 k-v1

@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 avatar Mar 27 '24 21:03 prgeor

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

lguohan avatar Mar 27 '24 23:03 lguohan

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

dgsudharsan avatar Mar 28 '24 00:03 dgsudharsan

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

mlok-nokia avatar Mar 29 '24 13:03 mlok-nokia

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.

keboliu avatar Mar 31 '24 03:03 keboliu

@keboliu Thank you for your clarification.

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

  1. In #1644 ethtool was added as a debian package to the host system by @lguohan
  2. In #2943 ethtool was added as a debian package to the docker-pmon by @keboliu
  3. After #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 #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 avatar Mar 31 '24 11:03 k-v1

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 #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

k-v1 avatar Mar 31 '24 12:03 k-v1

/azpw ms_conflict

liushilongbuaa avatar Apr 01 '24 11:04 liushilongbuaa

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 avatar Apr 08 '24 18:04 mlok-nokia

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 avatar Apr 08 '24 18:04 mlok-nokia

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

mlok-nokia avatar Apr 24 '24 15:04 mlok-nokia

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

rlhui avatar May 01 '24 17:05 rlhui

@prgeor, please help review and signoff this PR

arlakshm avatar May 08 '24 17:05 arlakshm

/azpw ms_conflict

judyjoseph avatar May 15 '24 17:05 judyjoseph

/azpw ms_conflict

lguohan avatar May 18 '24 19:05 lguohan