[ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries.

[Ilya Maximets]

On 21.07.2017 05:40, Darrell Ball wrote:
> The discussion about the ‘Area’ prefix has come up again, even after Ben had commented about it
> and after I had pointed folks to the submitting-patches.rst, which allows flexibility in choosing an
> ‘Area’ prefix by the patch submitter.
> 
> In this thread, it was again suggested that the use of ‘Area’ prefix ‘System Tests’ needs to change to ‘System-traffic’
> Below, I list some the history regarding previous commits of system-traffic.at as a single patch.
> Different people have had different preferences and those seem to have been tolerated in the past.
> 
> Hence, I would like know what has changed recently such that the documented
> (submitting-patches.rst) and historical flexibility (see previous patches) regarding the
> ‘Area’ prefix is no longer tolerated ?
> 
> My suggestion is that we don’t continue along these new lines and rather stay flexible, as the 
> work will be more productive in such an environment.
> 
> Darrell

Hi Darrell.

Looks like I should say something because I raised this issue.
First of all I want to say that it's my own opinion and you're
completely free to disagree with it, but I need to clarify my
position.

About system-traffic.at related patches:

You mentioned below commits which has 'tests' and 'system-tests'
prefixes. 'tests' is fine, because it is the folder name. Maybe
authors should be more specific with patches where only one file
changed, but it doesn't really matter.
>From the other side 'system-tests' is not the name of file or
folder, it's the area. But if you'll go back to the history,
all of these patches was committed when where was only one file
responsible for system tests (one patch is an exception introduced
after appearing of system-ovn.at). So it was, actually, fine
at the time of submission.

Today we have at least 4 types of system tests and it'll be nice
to have more detailed information directly in subject instead of
looking to the patch itself.

I personally don't like capital letters in area, except for
cases where capital letter is in filename.

About datapaths:

Previously you mentioned that you're using area 'Userspace Datapath'
to be consistent with policies used for kernel and windows datapaths.
But this statement is not right because 'datapath' is the name
of folder and 'datapath-windows' is a name of folder too, but
there is no such folder for useerspace datapath.

Additionally, this mailing list is actually not the primary place
for reviewing patches for kernel datapath. They are here only for
information and backporting. 'datapath-windows' prefix needed to
filter patches targeted for windows because there are only few
persons who works on that and able to review and test.
So, the main areas for patches in this mailing list are general
management code, userspace actions and userspace datapath.
Userspace datapath contains too many files/modules to not
mention them in subject line. So, if you're submitting patch
with 'conntrack' prefix, everybody knows that it's all about
connection tracking in userspace. 

Beside all of that: isn't it a good habit to use most commonly
used prefixes like 'system-traffic' or 'conntrack' instead of
making the new one?
If everybody will use their own preferable prefixes, git history
will become a total mess. And that is the main concern.

Once again, It's only my opinion and you're free to disagree.

Best regards, Ilya Maximets.

> /////////////////////
> 
> commit 9d3e0e5c196c0a91ea23d8d9254b1487cb58b58e
> Author: Jarno Rajahalme <jarno at ovn.org>
> Date:   Wed Mar 8 17:18:23 2017 -0800
> 
>     tests: Add an FTP test without conntrack.
>     
>     If FTP tests with conntrack fail, it is informative to know if the
>     problem is with the FTP client and/or server, or with conntrack
>     itself.
>     
>     Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
>     Acked-by: Joe Stringer <joe at ovn.org>
> 
> 
> /////////////////////
> 
> commit d0e4206230b31ab8dde44b6e8896c10b6317b1a8
> Author: Jarno Rajahalme <jarno at ovn.org>
> Date:   Fri Mar 10 16:10:41 2017 -0800
> 
>     tests: ICMP related to original direction test.
>     
>     Normally ICMP responses are in the reply direction of a conntrack
>     entry.  This test exercises an ICMP response to the original direction
>     of the conntrack entry.
>     
>     Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
>     Acked-by: Joe Stringer joe at ovn.org
> 
> /////////////////////
> 
> commit 2fa3e06d35988ee24ce1cc0f62ccceb3862038a1
> Author: Jarno Rajahalme <jarno at ovn.org>
> Date:   Wed Nov 25 16:04:59 2015 -0800
> 
>     system-tests: Add IPv6 FTP system test.
>     
>     Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
>     Acked-by: Joe Stringer <joe at ovn.org>
> 
> 
> /////////////////////
> 
> commit e5cf8cce275934549ee1b1ed41d60d5b6ce7918d
> Author: Daniele Di Proietto <diproiettod at vmware.com>
> Date:   Mon Apr 25 19:06:40 2016 -0700
> 
>     system-tests: Add ping through conntrack test.
>     
>     Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>     Acked-by: Joe Stringer joe at ovn.org
> 
> /////////////////////
> 
> commit e0b9270136465f1c5379c274ca41081980bcd076
> Author: Daniele Di Proietto <diproiettod at vmware.com>
> Date:   Mon Apr 11 14:02:10 2016 -0700
> 
>     system-tests: Add tcp simple test.
>     
>     Useful to test the datapath ability to forward tcp packets without the
>     complexity of connection tracking.
>     
>     Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>     Acked-by: Joe Stringer <joe at ovn.org>
>     Acked-by: Flavio Leitner fbl at sysclose.org
> 
> 
> /////////////////////
> 
> commit 6cfa8ec3e3c6bfdda3982e50721549d7d7853a91
> Author: Jarno Rajahalme <jarno at ovn.org>
> Date:   Tue Nov 24 13:33:22 2015 -0800
> 
>     system-tests: Use '--bundle'
>     
>     Use OpenFlow bundles for setting up flow tables.  This has the benefit
>     that when debugging test failures, no packet gets processed by
>     partially set-up flow table, which may seem confusing.
>     
>     Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
>     Acked-by: Ben Pfaff <blp at ovn.org>
> 
> /////////////////////
> 
> 
> 
> 
> 
> 
> 
> -----Original Message-----
> From: Darrell Ball <dball at vmware.com>
> Date: Thursday, July 20, 2017 at 11:17 AM
> To: Joe Stringer <joe at ovn.org>, Darrell Ball <dlu998 at gmail.com>
> Cc: ovs dev <dev at openvswitch.org>
> Subject: Re: [ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries.
> 
>     Please don’t update the prefix
>     
>     -----Original Message-----
>     From: <ovs-dev-bounces at openvswitch.org> on behalf of Joe Stringer <joe at ovn.org>
>     Date: Thursday, July 20, 2017 at 11:15 AM
>     To: Darrell Ball <dlu998 at gmail.com>
>     Cc: ovs dev <dev at openvswitch.org>
>     Subject: Re: [ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries.
>     
>         On 16 July 2017 at 11:27, Darrell Ball <dlu998 at gmail.com> wrote:
>         > Three of the SNAT tests allow for wget retries, which occasionally
>         > happen.  However, these tests did not allow for SNAT address
>         > variability for the retries, which is now tolerated.
>         >
>         > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>         
>         Thanks for the patch, I noticed this behaviour occasionally myself but
>         hadn't dug into it yet.
>         
>         Per Ilya's request, I updated the prefix. Will apply to master shortly.
>         _______________________________________________
>         dev mailing list
>         dev at openvswitch.org
>         https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=bMwLwPwNQR3XbY6KCxI54HZPRTBWPcrSF4_ikMQj44Q&s=9m-0DFAgXSLjCzA6DDzcTgCX-Evii2MGmQsDncTuikM&e= 
>         
>     
>     
>