[intfmgrd]: Support loopback #742
Conversation
Signed-off-by: Marian Pritsak <[email protected]>
|
retest this please |
| setIntfVrf(alias, vrf_name); | ||
| m_appIntfTableProducer.set(alias, data); | ||
| // Set Interface VRF except for lo | ||
| if (!is_lo) |
There was a problem hiding this comment.
Why are you excluding the loopbacks?
There was a problem hiding this comment.
Ip address for lo is configured in /etc/network/interfaces
There was a problem hiding this comment.
Not sure I follow your response. It is valid for a loopback to be in a vrf. Why are you excluding the loopbacks?
There was a problem hiding this comment.
@nikos-github , your comment is valid, but currently we do not support to have loopback in the vrf. We can add it in the future PR.
| @@ -173,7 +196,11 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys, | |||
| return false; | |||
| } | |||
|
|
|||
| setIntfIp(alias, "add", ip_prefix.to_string(), ip_prefix.isV4()); | |||
| // Set Interface IP except for lo | |||
| if (!is_lo) | |||
There was a problem hiding this comment.
Please explain how your code is dealing with multiple IP addresses on the loopback which is a valid config and must continue to work.
There was a problem hiding this comment.
What's the issue with multiple IPs?
It's just different keys in APP DB
There was a problem hiding this comment.
Today the loopback in sonic has more than one address. So with your changes post #635, will all the loopback addresses make it to the asic and correctly? I'm just asking you to explain and also test.
| @@ -186,7 +213,11 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys, | |||
| } | |||
| else if (op == DEL_COMMAND) | |||
| { | |||
| setIntfIp(alias, "del", ip_prefix.to_string(), ip_prefix.isV4()); | |||
| // Set Interface IP except for lo | |||
There was a problem hiding this comment.
This set of changes require unit tests.
|
retest this please |
|
Is it expected to support incremental IP config for loopback interface? if yes, lo config should be removed from /etc/network/interfaces and lo needs to be treated as other regular interface as to address config. |
|
@jipanyang Ideally it should be treated like any other interface when it comes to its incremental config, etc. however the loopback interface is kind of special since it's the anchoring point for 127.0.0.1. So I'm not sure it's a simple yes/no answer to your question but certainly today's loopback may have more than one address. |
|
This change indeed solves the issue where ptf cannot ping lo interface. It is to my belief that the issue was introduced by PR #635 |
|
in long run, we should create lo interface and assign IP address to it, so that we can move those interfaces to the vrf, but current we do not support this. |
That's fine. Since we don't currently support the loopback in the vrf, we can exclude it. I just need my other two comments addressed please regarding testing (including unit test) as well as an explanation how all the IP addresses a loopback can have, will make it to the ASIC. |
Signed-off-by: Volodymyr Samotiy <[email protected]>
|
@marian-pritsak , I don't think we need to check for |
|
This change is required by warm reboot test. Let's keep the discussion offline and make changes accordingly. |
Code clean refactor
In #635, the intfmgrd does not read loopback address from config db at all and then it does not configure the loopback address in the asic. The PR fix the issue by reading from the config db. Since loopback address is configured via /etc/network/interface so it is skipped in intfmgrd.
Signed-off-by: Marian Pritsak [email protected]