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

Incremental config add/del of IPv4/IPv6 addr on Loopback0 intf is not working #767

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Incremental config add/del of IPv4/IPv6 addr on Loopback0 intf is not working #767

wants to merge 1 commit into from

Conversation

kirankella
Copy link
Contributor

@kirankella kirankella commented Jan 24, 2019 •
edited
Loading

In intfmgr, add/del the addresses on "lo" interface too.

What I did
Intfmgr needs to push the address config into Linux for "lo"

Why I did it
The loopback addresses adding via config command are not pushed to Config DB, Linux Stack and to the Hardware

How I verified it

root@sonic:/home/admin# config interface Loopback0 ip add 28.28.28.28/32

root@sonic:/home/admin# ip route show table local | grep "dev 28.28.28.28"
local 28.28.28.28 dev lo proto kernel scope host src 28.28.28.28

root@sonic:/home/admin# config interface Loopback0 ip add 2037::1/128

root@sonic:/home/admin# ip -6 route show | grep 2037::1
unreachable 2037::1 dev lo proto kernel metric 256 error -101 pref medium

root@sonic:/home/admin# show runningconfiguration all | grep Loopback0
"Loopback0|28.28.28.28/32": {},
"Loopback0|2037::1/128": {},

root@sonic:/home/admin# bcmcmd 'l3 defip show'
l3 defip show
Unit 0, Total Number of DEFIP entries: 8192
VRF Net addr Next Hop Mac INTF MODID PORT PRIO CLASS HIT VLAN
.....
514 0 28.28.28.28/32 00:00:00:00:00:00 100003 0 0 0 1 n
.....

root@sonic:/home/admin# bcmcmd 'l3 ip6route show'
l3 ip6route show
Unit 0, Total Number of IPv6 entries: 6144 (IPv6/64 4096, IPv6/128 2048)
Max number of ECMP paths 64
Free IPv6 entries available: 6127 (IPv6/64 4087, IPv6/128 2040)
VRF Net addr Next Hop Mac INTF MODID PORT PRIO CLASS HIT VLAN
....
7 0 2037:0000:0000:0000:0000:0000:0000:0001/128 00:00:00:00:00:00 100003 0 0 0 1 n
....

root@sonic:/home/admin# config interface Loopback0 ip remove 28.28.28.28/32
root@sonic:/home/admin# ip route show table local | grep "28.28.28.28"

root@sonic:/home/admin# config interface Loopback0 ip remove 2037::1/128
root@sonic:/home/admin# ip -6 route show | grep 2037::1
root@sonic:/home/admin#

Details if related
Along with change in cfgmgr/intfmgr.cpp, this issue needed a change in config/main.py in sonic-utilities repo (tracked via PR sonic-net/sonic-utilities#443).

@kirankella
Incremental config add/del of IPv4/Ipv6 addresses on Loopback0 interf…
5457a6d 
…ace is not working

Fix:
   In intfmgr, add/del the addresses on "lo" interface too.
@kirankella kirankella mentioned this pull request Jan 24, 2019
Merged
jipanyang
jipanyang reviewed Jan 24, 2019
View reviewed changes
Copy link
Contributor

@jipanyang jipanyang left a comment

Choose a reason for hiding this comment

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

If Incremental config add/del of IPv4/IPv6 addr on Loopback0 is supported, how to cooperate with /etc/network/interfaces where lo IP config takes effect currently?

cfgmgr/intfmgr.cpp
else
{
setIntfIp("lo", "add", ip_prefix.to_string(), ip_prefix.isV4());
}
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 just remove "if (!is_lo)" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I thought about that :-). But alias value is not 'lo'.
The 'alias' value for loopback interface is "Loopback0" as passed from the config command. The 'is_lo' boolean is set based by comparing the 'alias' name prefix with LOOPBACK_PREFIX (Loopback).
Hence the above change.

@nikos-github
Copy link
Contributor

@lguohan @jipanyang So the decision was made since #742 was merged to support incremental loopback config? Would be nice to understand what the goal is here so that changes can be properly reviewed.

@kirankella
Copy link
Contributor Author

kirankella commented Jan 25, 2019 •
edited
Loading

@lguohan @jipanyang So the decision was made since #742 was merged to support incremental loopback config? Would be nice to understand what the goal is here so that changes can be properly reviewed.

The goal of this PR is to support the incremental config of the lo addresses. As I understand, #742 was done to ensure that loopback addresses are going to asic (from config_db) and not for incremental config support. Right?
Any change in config_db for the loopback addresses are pushed to /etc/network/interfaces only via 'config reload', right? I see they are not pushed to /etc/network/interfaces file via 'config load'.

With this PR change in intfmgr.cpp, I agree we will see 'already exist' errors for the config_db loopback addresses (pushed via /etc/network/interfaces), when they are being configured again from doIntfAddrTask.

Hence to complete the support for the lo incremental config, may be we can:
-- Either remove the lo support from /etc/network/interfaces, like Jipin mentioned in #742 (or)
-- handle the 'already exists' errors for the addresses added in doIntfAddrTask (not just for lo, but for any interface)

@kirankella
Copy link
Contributor Author

@lguohan @jipanyang So the decision was made since #742 was merged to support incremental loopback config? Would be nice to understand what the goal is here so that changes can be properly reviewed.

The goal of this PR is to support the incremental config of the lo addresses. As I understand, #742 was done to ensure that loopback addresses are going to asic (from config_db) and not for incremental config support. Right?
Any change in config_db for the loopback addresses are pushed to /etc/network/interfaces only via 'config reload', right? I see they are not pushed to /etc/network/interfaces file via 'config load'.

With this PR change in intfmgr.cpp, I agree we will see 'already exist' errors for the config_db loopback addresses (pushed via /etc/network/interfaces), when they are being configured again from doIntfAddrTask.

Hence to complete the support for the lo incremental config, may be we can:
-- Either remove the lo support from /etc/network/interfaces, like Jipin mentioned in #742 (or)
-- handle the 'already exists' errors for the addresses added in doIntfAddrTask (not just for lo, but for any interface)

@jipanyang @lguohan @nikos-github Like discussed above, only on bootup I see the "File exists" error when intfmgr is adding loopback addresses (as they were already added via /etc/network/interfaces during bootup). I believe the incremental config is good to have on LOOPBACK interface too for consistency purposes, please suggest how to go about this.
Either remove the LOOPBACK interface logic in ./files/image_config/interfaces/interfaces.j2 or ignore "File Exists" error in intfmgr::setIntfIp() only for "lo" interfaces.

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
@samaity
[sfputil] Add support of platform.json (sonic-net#767)
8affaf7 
Signed-off-by: Sangita Maity <[email protected]>

> This PR is dependent on [sonic-platform-common/pull/72](sonic-net/sonic-platform-common#72)

All three PRs are necessary to run `show interfaces transceiver` command.

1. [sonic-net/sonic-buildimage#3912](sonic-net/sonic-buildimage#3912)
2. [sonic-net/sonic-platform-common#72](sonic-net/sonic-platform-common#72)
3. [sonic-net/sonic-utilities#767](sonic-net/sonic-utilities#767)


**- What I did**
Add support of platform.json in sfputil to get correct output of `show interfaces transceiver`


**- How to verify it**
Check whether all the below-mentioned CLI's are working correctly.
```
Usage: show interfaces transceiver [OPTIONS] COMMAND [ARGS]...

  Show SFP Transceiver information

Options:
  -?, -h, --help  Show this message and exit.

Commands:
  eeprom    Show interface transceiver EEPROM information
  lpmode    Show interface transceiver low-power mode status
  presence  Show interface transceiver presence
```
@prsunny prsunny self-requested a review as a code owner September 2, 2022 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jipanyang jipanyang jipanyang left review comments

@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@kirankella @nikos-github @jipanyang