-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
move loopback configuration to intfMgrd #3044
Conversation
Merge Azure/sonic-buildimage
Retest this please |
Can you please look at the build failures. You may want to fix this file |
Fix merging error in last commit
Sorry, I made a mistake when merging. |
Modify test sample_output file
what is the pr for intfmgrd to support lo? we need both both into the buildimage at the same time. |
sonic-swss PR: |
@@ -9,13 +9,6 @@ | |||
# The loopback network interface | |||
auto lo | |||
iface lo inet loopback | |||
# Use command 'ip addr list dev lo' to check all addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tylerlinp @prsunny Any reason why the approach used for mgmt i/f being in the vrf can't be followed for lo? Not even sure that lo should be allowed to be in a vrf. How will redis work if lo is in a vrf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tylerlinp @prsunny Any reason why the approach used for mgmt i/f being in the vrf can't be followed for lo? Not even sure that lo should be allowed to be in a vrf. How will redis work if lo is in a vrf?
All loopback interfaces map to device lo, that cause conflict when loopbacks belong to differents vrfs. I think lo shouldn't be add to a vrf, even though kernel don't restrict doing so. Some process which communicates locally may be affected. It is good to add a device for every loopback interface(LoopbackXXX), then lo keep original function. Then LoopbackXXX is add vrf/ip just like other l3 interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand the issue. However, you are taking away the ability to further statically configure lo with this change. The right approach to follow would be to leave lo configuration where it is and make sure other loopbacks don't map to the same loopback device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand the issue. However, you are taking away the ability to further statically configure lo with this change. The right approach to follow would be to leave lo configuration where it is and make sure other loopbacks don't map to the same loopback device.
What configuration do you mean? The ip addresses only config to kernel? If not add to ASIC, I think the addresses except 127.x.x.x & ::1 would be useless. If truely needed, maybe should filter configuration like LOOPBACK_INTERFACE|lo|1.1.1.1/32 and then add to interfaces.
@@ -9,13 +9,6 @@ | |||
# The loopback network interface | |||
auto lo | |||
iface lo inet loopback | |||
# Use command 'ip addr list dev lo' to check all addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand the issue. However, you are taking away the ability to further statically configure lo with this change. The right approach to follow would be to leave lo configuration where it is and make sure other loopbacks don't map to the same loopback device.
@nikos-github This is the loopback device consideration which will add to upcoming vrf-hld-1.2.
The following is the example to configure new loopback interface by using dummy interface ip link add loopback1 type dummy
ip link set loopback1 up
ip link set dev loopback1 master Vrf_blue
ip address add 10.0.2.2/32 dev loopback1
|
retest this please |
1 similar comment
retest this please |
Merge branch 'master' into loopback, remove blank line
@prsunny It is better to merge this PR together with sonic-swss/943. |
Yes @tylerlinp. We need to update swss submodule. Can you update this PR with submodule update? |
@prsunny this PR should be merged since PR 943 has been merged. |
OK, I will do it. |
Ok great, I saw you updated the submodule. I'll close my PR #3720 Submodules update here: 85ff17d - 2019-11-07 : [VRF]: submit vrf feature (#943) [Tyler Li] |
retest this please |
@prsunny It seems that there is problem to build mock_tests when building normal image, not generate Makefile but want to make. |
retest broadcom please |
retest mellanox please |
@tylerlinp , we are working on the build failure |
@tylerlinp , can you update PR with this commit - sonic-net/sonic-swss#1123 ? |
retest broadcom please |
check #3733 |
The commits have been cherry-picked to PR #3733 which has been merged. That's great, so this PR can be closed directlly. |
retest vs please |
Closing in favor of #3733 |
- What I did
Sonic supports multiple loopback interfaces. Each loopback interfaces can belong to different Vrf.
- How I did it
IntfMgrd will take interface-config.service instead to handle loopback interface configuration.
- How to verify it
Pass vrf vs test cases and ansible test cases.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)