| Message ID | 1500229621-18299-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Joe Stringer |
| Headers | show |
Same thing here. Not a full review. Please, use 'system-traffic' for these patches as a prefix. Details are in previous e-mail: https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335751.html Best regards, Ilya Maximets.
On Mon, Jul 17, 2017 at 10:03:47AM +0300, Ilya Maximets wrote: > Same thing here. Not a full review. > Please, use 'system-traffic' for these patches as a prefix. I'm seeing a lot more focus on area prefixes than I expect. These are supposed to be helpful to the reader, but I don't think it's worth obsessing over them. (Of course, when they diverge from the customary ones, it can mean that a patch doesn't get reviewed expeditiously, but I don't think that's the problem here.)
On 16 July 2017 at 11:27, Darrell Ball <dlu998@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@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.
Please don’t update the prefix -----Original Message----- From: <ovs-dev-bounces@openvswitch.org> on behalf of Joe Stringer <joe@ovn.org> Date: Thursday, July 20, 2017 at 11:15 AM To: Darrell Ball <dlu998@gmail.com> Cc: ovs dev <dev@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@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@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@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=
On 20 July 2017 at 11:17, Darrell Ball <dball@vmware.com> wrote:
> Please don’t update the prefix
OK. As discussed offline, I'll wait for Darrell to respin this series.
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
/////////////////////
commit 9d3e0e5c196c0a91ea23d8d9254b1487cb58b58e
Author: Jarno Rajahalme <jarno@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@ovn.org>
Acked-by: Joe Stringer <joe@ovn.org>
/////////////////////
commit d0e4206230b31ab8dde44b6e8896c10b6317b1a8
Author: Jarno Rajahalme <jarno@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@ovn.org>
Acked-by: Joe Stringer joe@ovn.org
/////////////////////
commit 2fa3e06d35988ee24ce1cc0f62ccceb3862038a1
Author: Jarno Rajahalme <jarno@ovn.org>
Date: Wed Nov 25 16:04:59 2015 -0800
system-tests: Add IPv6 FTP system test.
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Joe Stringer <joe@ovn.org>
/////////////////////
commit e5cf8cce275934549ee1b1ed41d60d5b6ce7918d
Author: Daniele Di Proietto <diproiettod@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@vmware.com>
Acked-by: Joe Stringer joe@ovn.org
/////////////////////
commit e0b9270136465f1c5379c274ca41081980bcd076
Author: Daniele Di Proietto <diproiettod@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@vmware.com>
Acked-by: Joe Stringer <joe@ovn.org>
Acked-by: Flavio Leitner fbl@sysclose.org
/////////////////////
commit 6cfa8ec3e3c6bfdda3982e50721549d7d7853a91
Author: Jarno Rajahalme <jarno@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@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
/////////////////////
-----Original Message-----
From: Darrell Ball <dball@vmware.com>
Date: Thursday, July 20, 2017 at 11:17 AM
To: Joe Stringer <joe@ovn.org>, Darrell Ball <dlu998@gmail.com>
Cc: ovs dev <dev@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@openvswitch.org> on behalf of Joe Stringer <joe@ovn.org>
Date: Thursday, July 20, 2017 at 11:15 AM
To: Darrell Ball <dlu998@gmail.com>
Cc: ovs dev <dev@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@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@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@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=
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@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@ovn.org> > Acked-by: Joe Stringer <joe@ovn.org> > > > ///////////////////// > > commit d0e4206230b31ab8dde44b6e8896c10b6317b1a8 > Author: Jarno Rajahalme <jarno@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@ovn.org> > Acked-by: Joe Stringer joe@ovn.org > > ///////////////////// > > commit 2fa3e06d35988ee24ce1cc0f62ccceb3862038a1 > Author: Jarno Rajahalme <jarno@ovn.org> > Date: Wed Nov 25 16:04:59 2015 -0800 > > system-tests: Add IPv6 FTP system test. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Acked-by: Joe Stringer <joe@ovn.org> > > > ///////////////////// > > commit e5cf8cce275934549ee1b1ed41d60d5b6ce7918d > Author: Daniele Di Proietto <diproiettod@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@vmware.com> > Acked-by: Joe Stringer joe@ovn.org > > ///////////////////// > > commit e0b9270136465f1c5379c274ca41081980bcd076 > Author: Daniele Di Proietto <diproiettod@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@vmware.com> > Acked-by: Joe Stringer <joe@ovn.org> > Acked-by: Flavio Leitner fbl@sysclose.org > > > ///////////////////// > > commit 6cfa8ec3e3c6bfdda3982e50721549d7d7853a91 > Author: Jarno Rajahalme <jarno@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@ovn.org> > Acked-by: Ben Pfaff <blp@ovn.org> > > ///////////////////// > > > > > > > > -----Original Message----- > From: Darrell Ball <dball@vmware.com> > Date: Thursday, July 20, 2017 at 11:17 AM > To: Joe Stringer <joe@ovn.org>, Darrell Ball <dlu998@gmail.com> > Cc: ovs dev <dev@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@openvswitch.org> on behalf of Joe Stringer <joe@ovn.org> > Date: Thursday, July 20, 2017 at 11:15 AM > To: Darrell Ball <dlu998@gmail.com> > Cc: ovs dev <dev@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@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@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@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= > > > >
-----Original Message----- From: Ilya Maximets <i.maximets@samsung.com> Date: Friday, July 21, 2017 at 5:24 AM To: Darrell Ball <dball@vmware.com>, Joe Stringer <joe@ovn.org> Cc: ovs dev <dev@openvswitch.org>, Ben Pfaff <blp@ovn.org> Subject: Re: [ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries. 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. Really, so that would mean as long as only one file exists in an area, that the associated naming used in ‘Area’ prefixes is flexible, per your rules, right ? You seem to be on my side of the discussion now. 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. You just a few lines above stated that directory name (eg Tests) for ‘Area’ prefixes is ok. Are not directory names even a wider scope than a few files and even more ambiguous. Directories have many files covering many different parts of the code, like ‘tests’, ‘datapath’ and ‘windows-datapath’. So, you are saying “One file is ok, many different files are ok, but 4 similar files are bad” ?, I see. 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. That’s why we have documentation (submitting-patches.rst) allowing for flexibility in such cases. Folder is one option in naming per the documentation. In this case, there is no userspace datapath folder, that is where ‘Area’ comes into play – there is no folder, but we can still use the appropriate ‘Area’ name. 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. That would mean the existing history we have, for the examples stated already, are a problem and you seem to have no problem tracing the git history even when ‘tests’ and ‘System-tests’ are used in place of ‘System traffic’, as you mentioned above. So where is the problem ? Once again, It's only my opinion and you're free to disagree. Best regards, Ilya Maximets. > ///////////////////// > > commit 9d3e0e5c196c0a91ea23d8d9254b1487cb58b58e > Author: Jarno Rajahalme <jarno@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@ovn.org> > Acked-by: Joe Stringer <joe@ovn.org> > > > ///////////////////// > > commit d0e4206230b31ab8dde44b6e8896c10b6317b1a8 > Author: Jarno Rajahalme <jarno@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@ovn.org> > Acked-by: Joe Stringer joe@ovn.org > > ///////////////////// > > commit 2fa3e06d35988ee24ce1cc0f62ccceb3862038a1 > Author: Jarno Rajahalme <jarno@ovn.org> > Date: Wed Nov 25 16:04:59 2015 -0800 > > system-tests: Add IPv6 FTP system test. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Acked-by: Joe Stringer <joe@ovn.org> > > > ///////////////////// > > commit e5cf8cce275934549ee1b1ed41d60d5b6ce7918d > Author: Daniele Di Proietto <diproiettod@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@vmware.com> > Acked-by: Joe Stringer joe@ovn.org > > ///////////////////// > > commit e0b9270136465f1c5379c274ca41081980bcd076 > Author: Daniele Di Proietto <diproiettod@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@vmware.com> > Acked-by: Joe Stringer <joe@ovn.org> > Acked-by: Flavio Leitner fbl@sysclose.org > > > ///////////////////// > > commit 6cfa8ec3e3c6bfdda3982e50721549d7d7853a91 > Author: Jarno Rajahalme <jarno@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@ovn.org> > Acked-by: Ben Pfaff <blp@ovn.org> > > ///////////////////// > > > > > > > > -----Original Message----- > From: Darrell Ball <dball@vmware.com> > Date: Thursday, July 20, 2017 at 11:17 AM > To: Joe Stringer <joe@ovn.org>, Darrell Ball <dlu998@gmail.com> > Cc: ovs dev <dev@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@openvswitch.org> on behalf of Joe Stringer <joe@ovn.org> > Date: Thursday, July 20, 2017 at 11:15 AM > To: Darrell Ball <dlu998@gmail.com> > Cc: ovs dev <dev@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@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@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@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= > > > >
On 21 July 2017 at 07:52, Darrell Ball <dball@vmware.com> wrote: > > > -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Date: Friday, July 21, 2017 at 5:24 AM > To: Darrell Ball <dball@vmware.com>, Joe Stringer <joe@ovn.org> > Cc: ovs dev <dev@openvswitch.org>, Ben Pfaff <blp@ovn.org> > Subject: Re: [ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries. > > 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. Hi Darrell, It seems like we got off on the wrong foot on this one. You had asked me to look at the patches, so I fetched them, looked at the somewhat trivial feedback, applied that locally and tested the patches. Given that it seemed like I had done all the work locally that you would have done if you respin a v2, I figured that I would propose to just push them as-is from my local tree. To be explicit about the changes I had made, I responded on the list to highlight the changes. This gave you a chance to review what I had done, which you have done. Clearly this topic is important enough to you that you preferred I don't apply the patches as I had prepared them - so I backed off to allow you to send a v2. > > > > 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. > > Really, so that would mean as long as only one file exists in an area, that the > associated naming used in ‘Area’ prefixes is flexible, per your rules, right ? > You seem to be on my side of the discussion now. > > > 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. > > You just a few lines above stated that directory name (eg Tests) for ‘Area’ prefixes is ok. > Are not directory names even a wider scope than a few files and even more ambiguous. > Directories have many files covering many different parts of the code, like ‘tests’, ‘datapath’ and ‘windows-datapath’. > So, you are saying “One file is ok, many different files are ok, but 4 similar files are bad” ?, I see. I believe that the reason this is flexible is that sometimes patches change files across multiple systems. That's actually the trickiest thing to pick a name for, and occasionally patches end up omitting the area entirely for this reason. However when it comes to a patch changing a single file, typically the simplest way to pick a name is to take the filename. > 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. > > That would mean the existing history we have, for the examples stated already, > are a problem and you seem to have no problem tracing the git > history even when ‘tests’ and ‘System-tests’ are used in place of ‘System traffic’, as you > mentioned above. So where is the problem ? I can see cases where I've been interested in listing all patches relevant to an area, where filtering to specific files is not an appropriate solution. In this case, the fewer possible different areas I need to consider, the easier it is to script against the git logs. For this kind of use case, improving consistency is the most useful thing. If one label was used consistently at some point in history, then you can consider everything up until that point trivially. If it changes at some point in the history, then obviously I have to go and update my filter to include the new one as well. We can't do much about that. However, for incoming patches going forward, ideally we don't move towards a situation with several different ways to refer to changes to particular areas as this will complicate this kind of usage. This is, for instance, why I prefer that patches consistently use all lower case (unless the filename has capitals); consistently use dashes rather than spaces and so on. Cheers, Joe
On 21 July 2017 at 11:16, Joe Stringer <joe@ovn.org> wrote: > On 21 July 2017 at 07:52, Darrell Ball <dball@vmware.com> wrote: >> >> >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Date: Friday, July 21, 2017 at 5:24 AM >> To: Darrell Ball <dball@vmware.com>, Joe Stringer <joe@ovn.org> >> Cc: ovs dev <dev@openvswitch.org>, Ben Pfaff <blp@ovn.org> >> Subject: Re: [ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries. >> >> 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. > > Hi Darrell, > > It seems like we got off on the wrong foot on this one. You had asked > me to look at the patches, so I fetched them, looked at the somewhat > trivial feedback, applied that locally and tested the patches. Given > that it seemed like I had done all the work locally that you would > have done if you respin a v2, I figured that I would propose to just > push them as-is from my local tree. To be explicit about the changes I > had made, I responded on the list to highlight the changes. This gave > you a chance to review what I had done, which you have done. Clearly > this topic is important enough to you that you preferred I don't apply > the patches as I had prepared them - so I backed off to allow you to > send a v2. In the spirit of moving these useful patches past the bikeshed and into master where we'll now see less frequent failures, I went ahead and applied the v2 as-is. Cheers, Joe
diff --git a/tests/system-traffic.at b/tests/system-traffic.at index b2393f5..1ebf928 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2678,7 +2678,7 @@ dnl HTTP requests from p0->p1 should work fine. OVS_START_L7([at_ns1], [http]) NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 5 -T 1 --retry-connrefused -v -o wget0.log]) -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/'], [0], [dnl +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/' | uniq], [0], [dnl tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.2XX,sport=<cleared>,dport=<cleared>),zone=1,protoinfo=(state=<cleared>) ]) @@ -2777,7 +2777,7 @@ dnl HTTP requests from p0->p1 should work fine. OVS_START_L7([at_ns1], [http]) NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 5 -T 1 --retry-connrefused -v -o wget0.log]) -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/'], [0], [dnl +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/' | uniq], [0], [dnl tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.2XX,sport=<cleared>,dport=<cleared>),zone=1,protoinfo=(state=<cleared>) ]) @@ -2830,7 +2830,7 @@ dnl HTTP requests from p0->p1 should work fine. OVS_START_L7([at_ns1], [http]) NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 5 -T 1 --retry-connrefused -v -o wget0.log]) -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/'], [0], [dnl +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/' | uniq], [0], [dnl tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.2XX,sport=<cleared>,dport=<cleared>),zone=1,protoinfo=(state=<cleared>) ])
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@gmail.com> --- tests/system-traffic.at | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)