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

[intfsorch]:add support to change rif mac address #814

Merged
merged 25 commits into from
Feb 12, 2020

Conversation

shine4chen
Copy link
Contributor

What I did
add support to change rif mac address

Why I did it
some apps such as mclag or vrrp need to change rif mac address

How I verified it
test it on nephos lab

Details if related

@shine4chen shine4chen changed the title Rif change mac [intfsorch]:add support to change rif mac address Mar 11, 2019
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

"mac_addr" is not a field currently in the INTF_TABLE/INTERFACE_TABLE. This needs to be defined in the schema and also require corresponding change in interface manager

orchagent/intfsorch.cpp Outdated Show resolved Hide resolved
orchagent/intfsorch.cpp Outdated Show resolved Hide resolved
@lguohan
Copy link
Contributor

lguohan commented Apr 21, 2019

need virtual switch test.

@lguohan
Copy link
Contributor

lguohan commented Apr 21, 2019

@prsunny , I assume this can only be on rif without ip address. We can not set mac address for interface entry with IP address since we only create one rif for all ip addresses on that rif?

@jianjundong
Copy link

@prsunny , I assume this can only be on rif without ip address. We can not set mac address for interface entry with IP address since we only create one rif for all ip addresses on that rif?

In mclag, the ip address of mclag enabled portchannel of two peers must be the same, thus the mac address of rifs also must be the same.

@jianjundong
Copy link

"mac_addr" is not a field currently in the INTF_TABLE/INTERFACE_TABLE. This needs to be defined in the schema and also require corresponding change in interface manager

'mac_addr' is set as a field of INTF_TABLE in APP_DB currently. It is set by iccpd through mclagsyncd. This field may also be set as a field of INTERFACE_TABLE in CFG_DB and processed by intfmgrd? Thanks.

@shine4chen
Copy link
Contributor Author

need virtual switch test.

sure, we will add it soon

@shine4chen
Copy link
Contributor Author

need virtual switch test.

@lguohan Can you give us example or URL how to connect virtual switch test to specific PR

Signed-off-by: leo.li <leo.li@nephosinc.com>
@shine4chen
Copy link
Contributor Author

@lguohan even for the interface associated with the ip address we still can/need to change its src-mac.

@shine4chen
Copy link
Contributor Author

@prsunny Per our discussion on VRF, src-mac attribute should be put on {INTERFACE:Ethernetx} entry. I suggest we merge this PR to VRF code. How about your opinion?

@lguohan
Copy link
Contributor

lguohan commented Jun 4, 2019

@shine4chen , thanks for adding vs test, but the added two tests are failing, can you investigate the failure.

Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

please check the comments.

one more question, will you support remove the source MAC address once it is set?

orchagent/intfsorch.cpp Outdated Show resolved Hide resolved
orchagent/intfsorch.cpp Show resolved Hide resolved
tests/test_intf_mac.py Outdated Show resolved Hide resolved
tests/test_intf_mac.py Outdated Show resolved Hide resolved
tests/test_intf_mac.py Outdated Show resolved Hide resolved
tests/test_intf_mac.py Outdated Show resolved Hide resolved
tests/test_intf_mac.py Outdated Show resolved Hide resolved
tests/test_intf_mac.py Show resolved Hide resolved
Signed-off-by: leo.li <leo.li@nephosinc.com>
@leoli-nps
Copy link
Contributor

The vstest has been updated, please review. Thanks!

@shine4chen
Copy link
Contributor Author

@stcheng @prsunny we have refined code according to your suggestion. Pls help to review it. Thanks.

shine.chen added 2 commits June 17, 2019 19:51
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
@prsunny
Copy link
Collaborator

prsunny commented Jul 8, 2019

@prsunny Per our discussion on VRF, src-mac attribute should be put on {INTERFACE:Ethernetx} entry. I suggest we merge this PR to VRF code. How about your opinion?

@shine4chen , yes I agree. I think you've made the changes based on this assumption currently correct?. If so, the change must be in doIntfGeneralTask and not in doIntfAddrTask in intfmgr.cpp. Can you please check?

tests/test_intf_mac.py Outdated Show resolved Hide resolved
tests/test_intf_mac.py Outdated Show resolved Hide resolved
Signed-off-by: leo.li <leo.li@nephosinc.com>
@shine4chen
Copy link
Contributor Author

@prsunny Per our discussion on VRF, src-mac attribute should be put on {INTERFACE:Ethernetx} entry. I suggest we merge this PR to VRF code. How about your opinion?

@shine4chen , yes I agree. I think you've made the changes based on this assumption currently correct?. If so, the change must be in doIntfGeneralTask and not in doIntfAddrTask in intfmgr.cpp. Can you please check?
@prsunny yes, We will move the change to doIntfGeneralTask function.

prsunny
prsunny previously approved these changes Nov 22, 2019
@prsunny
Copy link
Collaborator

prsunny commented Nov 22, 2019

retest this please

1 similar comment
@shine4chen
Copy link
Contributor Author

retest this please

stcheng
stcheng previously approved these changes Nov 26, 2019
@shine4chen
Copy link
Contributor Author

retest this please

2 similar comments
@shine4chen
Copy link
Contributor Author

retest this please

@shine4chen
Copy link
Contributor Author

retest this please

cfgmgr/intfmgr.cpp Show resolved Hide resolved
orchagent/intfsorch.cpp Outdated Show resolved Hide resolved
@shine4chen shine4chen force-pushed the rif-change-mac branch 3 times, most recently from 39e087f to 7143b94 Compare December 1, 2019 06:54
orchagent/intfsorch.cpp Outdated Show resolved Hide resolved
Signed-off-by: shine.chen <shine.chen@mediatek.com>
@shine4chen
Copy link
Contributor Author

retest this please

2 similar comments
@shine4chen
Copy link
Contributor Author

retest this please

@shine4chen
Copy link
Contributor Author

retest this please

@shine4chen shine4chen dismissed stale reviews from stcheng, prsunny, and adyeung via 7283a4c February 3, 2020 03:14
@shine4chen
Copy link
Contributor Author

shine4chen commented Feb 3, 2020

@stcheng @prsunny @adyeung I resolved some conflict. It seems system automatically dismissed your approve. Would you please to review and approve it?

@prsunny prsunny merged commit 853d822 into sonic-net:master Feb 12, 2020
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants