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

[Mellanox] Use MAC from EEPROM for PortChannels and VLAN Interfaces #1793

Merged
merged 5 commits into from
Jul 23, 2018

Conversation

andriymoroz-mlnx
Copy link
Collaborator

@andriymoroz-mlnx andriymoroz-mlnx commented Jun 14, 2018

Signed-off-by: Andriy Moroz c_andriym@mellanox.com

- What I did
Updated teams start script to pass MAC from EEPROM to the portchannels config template

- How I did it
updated start.sh which runs in teamd container

- How to verify it
Start SONiC and make sure MACs on PortChannel (and members) or Vlan interface are similar to one returned by command
od -vt x1 -An /sys/bus/i2c/devices/8-0051/eeprom | xargs printf "0x%s " | xargs printf "%02x:" | awk 'BEGIN { FS=":"; i=8+1+2+1} {while(i<NF) {type=$i; len=("0x"$(i+1));if(type!="24") {i=i+2+len} else {print substr($0, (i+1)3+1, len3-1); break}}}'

some bits in last byte may differ

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
@andriymoroz-mlnx
Copy link
Collaborator Author

need to do some tests to implement the same for VLAN interfaces...

if [ "$SONIC_ASIC_TYPE" == "mellanox" ]; then
MAC_ADDRESS=$(od -vt x1 -An /sys/bus/i2c/devices/8-0051/eeprom | xargs printf "0x%s " | xargs printf "%02x:" | awk 'BEGIN { FS=":"; i=8+1+2+1} {while(i<NF) {type=$i; len=("0x"$(i+1));if(type!="24") {i=i+2+len} else {print substr($0, (i+1)*3+1, len*3-1); break}}}')
else
MAC_ADDRESS=$(ip link show eth0 | grep ether | awk '{print $2}')
Copy link
Contributor

Choose a reason for hiding this comment

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

cat /sys/class/net/eth0/address

Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

use decode-syseeprom -m

MAC_ADDRESS=$(ip link show eth0 | grep ether | awk '{print $2}')

if [ "$SONIC_ASIC_TYPE" == "mellanox" ]; then
MAC_ADDRESS=$(od -vt x1 -An /sys/bus/i2c/devices/8-0051/eeprom | xargs printf "0x%s " | xargs printf "%02x:" | awk 'BEGIN { FS=":"; i=8+1+2+1} {while(i<NF) {type=$i; len=("0x"$(i+1));if(type!="24") {i=i+2+len} else {print substr($0, (i+1)*3+1, len*3-1); break}}}')
Copy link
Collaborator

Choose a reason for hiding this comment

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

sudo decode-syseeprom -m to figure out the base mac?

Copy link
Collaborator Author

@andriymoroz-mlnx andriymoroz-mlnx Jun 20, 2018

Choose a reason for hiding this comment

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

it is not available inside docker container, checking why...
UPD:
they have their own /usr/bin
we share only:
-v /etc/sonic:/etc/sonic:ro
-v /var/run/redis:/var/run/redis:rw
-v /usr/share/sonic/device/$PLATFORM:/usr/share/sonic/platform:ro
-v /usr/share/sonic/device/$PLATFORM/$HWSKU:/usr/share/sonic/hwsku:ro \

Will affect MAC for VLAN interfaces

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

if (version_info['asic_type'] == 'mellanox'):
eeprom_file = "/sys/bus/i2c/devices/8-0051/eeprom"
if (os.access(eeprom_file, os.R_OK)):
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 21, 2018

Choose a reason for hiding this comment

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

if (os.access(eeprom_file, os.R_OK)): [](start = 8, length = 37)

else, it is an error. #Closed

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
else:
get_mac_cmd = "ip link show eth0 | grep ether | awk '{print $2}'"

proc = subprocess.Popen(get_mac_cmd, shell=True, stdout=subprocess.PIPE)
(mac, err) = proc.communicate()
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 27, 2018

Choose a reason for hiding this comment

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

err [](start = 10, length = 3)

Handle error before using mac. #Closed

@@ -6,6 +6,10 @@ Requires=database.service

[Service]
Type=oneshot
{% if sonic_asic_platform == 'mellanox' -%}
EnvironmentFile=/host/machine.conf
ExecStartPre=/bin/bash -c "/usr/share/sonic/device/$onie_platform/hw-management start"
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 27, 2018

Choose a reason for hiding this comment

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

hw-management [](start = 66, length = 13)

Is updategraph a good location for hw-management? It seems has nothing todo with graph. @taoyl-ms #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Qi. I don't see the reason to put hw-management here, and I don't see the need to change this into a j2 file either. Platform information can also be tested during runtime.

Choose a reason for hiding this comment

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

we need drivers to access eeprom
j2 to avoid this code for others

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not create another service for this?
And, you can create a shell script to enable hw-management in which you can test platform information run time. It is much easier to follow than a build-time j2 rendering.

Copy link
Collaborator

Choose a reason for hiding this comment

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

in current case, when will hw-management be started? Maybe hw-management itself should be a service and make it start early.

@qiluo-msft qiluo-msft requested a review from taoyl-ms June 27, 2018 16:37
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

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

As comments

if [ "$SONIC_ASIC_TYPE" == "mellanox" ]; then
MAC_ADDRESS=$(sonic-cfggen -d -v DEVICE_METADATA.localhost.mac)
else
MAC_ADDRESS=$(ip link show eth0 | grep ether | awk '{print $2}')
Copy link
Contributor

Choose a reason for hiding this comment

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

MAC_ADDRESS=$(</sys/class/net/eth0/address)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the case under "else" getting MAC address from the "ip link show eth0" is the old code and used for other platforms
I can change it to what you suggested, but I am not able to test it on non-Mellanox boxes...

@@ -155,7 +155,7 @@ sudo LANG=C chroot $FILESYSTEM_ROOT systemctl enable hostname-config.service
sudo cp $IMAGE_CONFIGS/hostname/hostname-config.sh $FILESYSTEM_ROOT/usr/bin/

# Copy updategraph script and service file
sudo cp $IMAGE_CONFIGS/updategraph/updategraph.service $FILESYSTEM_ROOT/etc/systemd/system/
j2 files/build_templates/updategraph.service.j2 | sudo tee $FILESYSTEM_ROOT/etc/systemd/system/updategraph.service
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to copy everything to the screen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to get it in build log, I think
seen it in some other place
suggest to remove tee?

if (version_info['asic_type'] == 'mellanox'):
get_mac_cmd = "sudo decode-syseeprom -m"
else:
get_mac_cmd = "ip link show eth0 | grep ether | awk '{print $2}'"
Copy link
Contributor

Choose a reason for hiding this comment

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

read a file "/sys/class/net/eth0/address"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

like above - not sure about other platforms

MAC_ADDRESS=$(ip link show eth0 | grep ether | awk '{print $2}')

if [ "$SONIC_ASIC_TYPE" == "mellanox" ]; then
MAC_ADDRESS=$(sonic-cfggen -d -v DEVICE_METADATA.localhost.mac)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do sudo decode-syseeprom -m here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unfortunately no, it is not available in the containers...

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

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

Discussed offline. Let's keep the original PR.

@qiluo-msft
Copy link
Collaborator

git clone https://github.com/Mellanox/hw-mgmt/ -b $(MLNX_HW_MANAGEMENT_VERSION) hw-management

This is a suggestion. Since the code is owned by Mellanox, why not commit the patch there?


Refers to: platform/mellanox/hw-management/Makefile:9 in 048db4d. [](commit_id = 048db4d, deletion_comment = False)

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

Unblock, with suggestion.

@lguohan lguohan merged commit dadc17d into sonic-net:master Jul 23, 2018
lguohan pushed a commit that referenced this pull request Oct 13, 2018
…1793)

* Use MAC from EEPROM for PortChannels

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

* Use MAC from EEPROM in DEVICE_METADATA

Will affect MAC for VLAN interfaces

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

* Get MAC via decode-syseeprom

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

* hw-management is now a service

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

* Add error handling for MAC fetch process

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
@@ -481,6 +481,8 @@ $(addprefix $(TARGET_PATH)/, $(SONIC_INSTALLERS)) : $(TARGET_PATH)/% : \
j2 -f env files/initramfs-tools/union-mount.j2 onie-image.conf > files/initramfs-tools/union-mount
j2 -f env files/initramfs-tools/arista-convertfs.j2 onie-image.conf > files/initramfs-tools/arista-convertfs

j2 files/build_templates/updategraph.service.j2 > updategraph.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line necessary? The same operation is being done in sonic_debian_extension.j2 above and there the resulting file is getting copied to its proper final destination. Why is this file getting written to the repo root? It doesn't appear to be used anywhere.

theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants