mgmt_vrf_namespace: management vrf using namespace solution#422
mgmt_vrf_namespace: management vrf using namespace solution#422kannankvs wants to merge 1 commit intosonic-net:masterfrom kannankvs:mgmt_vrf_namespace
Conversation
| def vrf_add_management_vrf(): | ||
| """Enable management vrf""" | ||
|
|
||
| cmd = 'sonic-cfggen -d --var-json "MGMT_VRF_CONFIG"' |
There was a problem hiding this comment.
Is it possible to read this address from the config DB?
Why do you read using sonic-cfggen, but write directly to DB?
There was a problem hiding this comment.
@pavel-shirshov : Addressed your comment in #431. Instead of using cfggen to read, it now reads from config DB.
|
|
||
| cmd = 'sonic-cfggen -d --var-json "MGMT_VRF_CONFIG"' | ||
| p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| res = p.communicate() |
There was a problem hiding this comment.
I'd extract subprocess.Popen chunk as a function, otherwise you have a lot of duplicate code
There was a problem hiding this comment.
@pavel-shirshov : Addressed your comment in #431. Instead of using cfggen to read, it now reads from config DB and hence there is no duplicate usage of Popen chunk.
| @click.pass_context | ||
| def vrf_add (ctx, vrfname): | ||
| """VRF ADD""" | ||
| if vrfname == 'mgmt' or vrfname == 'management': |
There was a problem hiding this comment.
why two VRF names? Do we need to choose one only? Like 'mgmt' you defined in the help?
There was a problem hiding this comment.
@pavel-shirshov : Replied to your comment in #431. Please check and followup in that PR.
| if address in network: | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
return address in network ?
There was a problem hiding this comment.
@pavel-shirshov : Replied to your comment in #431. Please check and followup in that PR.
|
|
||
| @ip.command() | ||
| @click.argument("ip_addr", metavar="<ip_addr>", required=True) | ||
| @click.argument('gw1', metavar='<default gateway IP address>', required=False) |
There was a problem hiding this comment.
should we use gw here? Why gw1?
There was a problem hiding this comment.
@pavel-shirshov : Addressed your comment in #431. Changed gw1 to gw.
|
|
||
| if interface_name.startswith("Ethernet"): | ||
| config_db.set_entry("INTERFACE", (interface_name, ip_addr), {"NULL": "NULL"}) | ||
| elif interface_name.startswith("eth0"): |
There was a problem hiding this comment.
It could match interface 'eth01'. I think it's better to compare to 'eth' here
There was a problem hiding this comment.
@pavel-shirshov : Addressed your comment in #431. Changed "startswith eth0" and checked for exact match with "eth0".
|
|
||
| if interface_name.startswith("Ethernet"): | ||
| config_db.set_entry("INTERFACE", (interface_name, ip_addr), None) | ||
| elif interface_name.startswith("eth0"): |
There was a problem hiding this comment.
@pavel-shirshov : Addressed your comment in #431. Changed "startswith eth0" and checked for exact match with "eth0".
|
I am not sure we should use namespace problem to address management vrf in sonic. |
|
Since we faced some issues in raising pull requests on top of this pull request, we shall cancel this PR. New PR #431 is raised. Once if it is decided to use namespace solution for management VRF, all the comments given in this PR will be addressed as part of PR#431. This PR#422 is cancelled. Similarly the related sonic-buildimage PR 'sonic-net/sonic-buildimage#2405' is also cancelled. |
4 participants
- What I did
Added support for management VRF using namespace solution.
Requirements that are covered are explained in the design document.
- How I did it
Added config commands and show commands to enable/disable the management VRF. On enabling, it creates the management namespace, attached eth0 to management namespace, creates the required iptables rules and restarts the networking service.
Detailed design is explained in the design document.
- How to verify it
Use the following commands to enable/disable mgmt vrf and test the features.
config vrf add mgmt
config vrf del mgmt
config interface eth0 ip add ip/mask gatewayIP
Ex: config interface eth0 ip add 10.16.206.11/24 10.16.206.1
- Description for the changelog