Hi Deepak,
I have approved the blueprint (nice work!).
Raja
From: Deepak Tiwari <deepak.tiw...@aricent.com>
Date: Wednesday, April 19, 2017 at 9:57 AM
To: Rajagopalan Sivaramakrishnan <r...@juniper.net>,
"dev@lists.opencontrail.org" <dev@lists.opencontrail.org>
Subject: Re: [opencontrail-dev] Contrailen 171 - vrouter vifdump mirroring
enhancement for bond-slave-links
Hi Raja,
Thanks for reviewing the changes. I have accepted and incorporated all your
comments. Please have a look:-
https://review.opencontrail.org/#/c/30107/5
Br, Deepak
________________________________
From: Rajagopalan Sivaramakrishnan <r...@juniper.net>
Sent: Wednesday, April 19, 2017 11:16:28 AM
To: Deepak Tiwari; dev@lists.opencontrail.org
Subject: Re: [opencontrail-dev] Contrailen 171 - vrouter vifdump mirroring
enhancement for bond-slave-links
Hi Deepak,
Looks good. I have a few minor comments in the review – can you please take
a look?
Thanks,
Raja
From: Deepak Tiwari <deepak.tiw...@aricent.com>
Date: Tuesday, April 18, 2017 at 1:30 AM
To: Rajagopalan Sivaramakrishnan <r...@juniper.net>,
"dev@lists.opencontrail.org" <dev@lists.opencontrail.org>
Subject: Re: [opencontrail-dev] Contrailen 171 - vrouter vifdump mirroring
enhancement for bond-slave-links
Hi Raja,
I have incorporated your comments in the feature-spec. Can you please verify it
and if you find it ok then please mark the review completed (i.e. code review
+1)
https://review.opencontrail.org/#/c/30107/4
Br, Deepak
________________________________
From: Rajagopalan Sivaramakrishnan <r...@juniper.net>
Sent: Thursday, April 13, 2017 6:04:17 AM
To: Deepak Tiwari; dev@lists.opencontrail.org
Subject: Re: [opencontrail-dev] Contrailen 171 - vrouter vifdump mirroring
enhancement for bond-slave-links
Hi Deepak,
Thanks for sending the diff. Please see inline (grep for >>>).
Raja
From: Deepak Tiwari <deepak.tiw...@aricent.com>
Date: Wednesday, April 12, 2017 at 1:15 AM
To: Rajagopalan Sivaramakrishnan <r...@juniper.net>,
"dev@lists.opencontrail.org" <dev@lists.opencontrail.org>
Subject: RE: [opencontrail-dev] Contrailen 171 - vrouter vifdump mirroring
enhancement for bond-slave-links
Hi Raja,
Thanks a lot for taking out time to review the blueprint/spec. I am ok with
your suggested approach. However just to explain the reasoning behind the
approach of showing member links in "vif --list" I am providing few points
below. Please go through these once and then if you still feel we should not
display slave in "vif--list" then I will change the spec accordingly.
>>> From the blueprint, I got the impression that the intent was to actually
>>> create the vifs for slave interfaces. But, from the diff, it looks like the
>>> plan is to only display the slave interfaces in the “vif –get” and “vif
>>> –list” output. I think that should be fine. However, the format of the
>>> output for the slave interfaces should be identical to what is printed for
>>> the physical interface i.e PCI id should be correct (not the port id of the
>>> slave in DPDK) and the stats should correctly reflect the slave interface
>>> stats. Also, commands such as “vif –get 0 –rate” and “vif –list –rate”
>>> should reflect slave stats rates correctly and per-core stats should be
>>> displayed if –core is used with the –rate option.
Secondly, I would like this change to be part of ongoing r4.1 of Opencontrail.
Can you please let me know what is the deadline till which the changes are
being accepted for r4.1. Is there some place where I can find the Opencontrail
release schedule, in terms of cut-off dates for design, code commits etc. ?
>>> The 4.1 dates are not finalized yet, but I think the code-complete date
>>> will be in June/July. The blueprints will need to be approved before that,
>>> which should not be a problem for this feature. FYI, we will be moving to
>>> DPDK 17.02 in 4.1, so that might affect your changes.
Review comments
==========================
Comment:1
“Line 38: 0
Would prefer to not display this in "vif --list" output. The unique identifier
of a member of a bond interface is its PCI id. We can add an option to vifdump
(say "-p") that specifies this PCI id. So, the command would be something like
"vifdump -i 0 -p 0004:00:00", where 0 is the vif index of the bond. vifdump can
send this PCI id encoded as an integer using the "--pmd" option to "vif"
command. This will allow vrouter to identify which member link to monitor (by
searching for its PCI id in the list of devices). “
1. Although the user who created the bond-interface and added slaves to it
knows the PCI-id of slave interfaces however once we have VRouter-DPDK running,
if some other user needs to use vifdump utility then he may find it difficult
to find those PCI-ids of member links because he will not be able to see it via
either "ifconfig", "vif--list" or "lspci". Showing slaves/members in "vif
--list" will be handy for a end-user
2. Comparing it with "ifconfig" and "tcpdump" for kernel interfaces, the bond
as well as its members are visible as output of "ifconfig" command. So if we
display slaves in "vif --list" output then it is aligned to "ifconfig"
behaviour as well
3. In case, you are apprehensive that the approach to show slaves in "vif
--list" would be more complex and would require more code changes, then I would
like to assure you that this is not the case. I have briefly explained the
approach below. In fact the code changes have already been done and I would
request you to please have a look
Code-changes:
https://github.com/deepak-dt/contrail-vrouter/commit/38db2603e09f91a71571633d823b8bb47ace1755
1. Show slave interfaces with their display name codifying the physical eth
port, i.e. vif0/1/slave2
(where, 1 is bond vif-id while 2 is eth-port of slave interface)
2. User will use this name "vif0/1/slave2" to start monitoring for the
slave, i.e.
- vifdump -i vif0/1/slave2 [tcpdump_arguments]
>>> Currently, “vifdump –i 0” works. So, “vifdump –i 0/slave1” should be
>>> sufficient.
3. vifdump will send following command to vif :-
- sudo vif --add mon1/slave2 --type monitoring --vif 1 --id 4351
>/dev/null
4. vif will send "vif_name" as "mon1/slave2" to vrouter
5. Based on the substring "slave" in "vif_name", vrouter
(vr_dpdk_interface.c) will get the slave's eth-port from it.
Comment:2
“We might need some change in vifdump to allow monitoring of multiple members
of a bond simultaneously (see generation of MON_VIF_ID).”
In current implementation ‘vifdump’ generates the MON_VIF_ID in following way—
- Initialize MON_VIF_ID from the other side of interface-id range, i.e.
MON_VIF_ID=$((${VR_MAX_INTERFACES} - ${MON_IF_ID} - 1))
Where,
a. #define VR_MAX_INTERFACES (256 + 4096)
b. MON_IF_ID = VIF_ID of monitored interface; if slave is being
monitored this will have VIF_ID of bond
- From this starting value of MON_VIF_ID, it then keeps on traversing in
reverse direction (subtracting 1 each time) until it finds a free VIF_ID
In my view, this existing approach should work even in our case as well? For
ex. if [Mon_vif_id=X] is being used for slave-1 then for slave-2 it will select
[mon_vif_id=X-1] ? Is my understanding correct or am I missing something here?
>>> Agree. It should be fine.
"DISCLAIMER: This message is proprietary to Aricent and is intended solely for
the use of the individual to whom it is addressed. It may contain privileged or
confidential information and should not be circulated or used for any purpose
other than for what it is intended. If you have received this message in error,
please notify the originator immediately. If you are not the intended
recipient, you are notified that you are strictly prohibited from using,
copying, altering, or disclosing the contents of this message. Aricent accepts
no responsibility for loss or damage arising from the use of the information
transmitted by this email including damage from virus."
"DISCLAIMER: This message is proprietary to Aricent and is intended solely for
the use of the individual to whom it is addressed. It may contain privileged or
confidential information and should not be circulated or used for any purpose
other than for what it is intended. If you have received this message in error,
please notify the originator immediately. If you are not the intended
recipient, you are notified that you are strictly prohibited from using,
copying, altering, or disclosing the contents of this message. Aricent accepts
no responsibility for loss or damage arising from the use of the information
transmitted by this email including damage from virus."
_______________________________________________
Dev mailing list
Dev@lists.opencontrail.org
http://lists.opencontrail.org/mailman/listinfo/dev_lists.opencontrail.org
