| Message ID | 1461996718-20575-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Apr 29, 2016 at 11:11 PM, William Tu <u9012063@gmail.com> wrote: > This avoids error message "Cannot remove namespace file > "/var/run/netns/at_ns0": No such file or directory", > when at_ns0 does not exist. > Signed-off-by: William Tu <u9012063@gmail.com> > --- > tests/system-common-macros.at | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 2116f1e..ae09e83 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -3,8 +3,10 @@ > # Delete namespaces from the running OS > m4_define([DEL_NAMESPACES], > [m4_foreach([ns], [$@], > - [ip netns del ns > -]) > + [if ip netns list | grep ns > /dev/null; then > + ip netns del ns > + fi > + ]) > ] > ) > Do we want to suppress an error on deletion in general ? Is the problem wherein ADD_NAMESPACES tries to always remove a namespace before adding it ? Is it better to check if ns exists here before calling DEL_NAMESPACES ? m4_define([ADD_NAMESPACES], [m4_foreach([ns], [$@], [DEL_NAMESPACES(ns) AT_CHECK([ip netns add ns || return 77]) on_exit 'DEL_NAMESPACES(ns)' ]) ] ) > > -- > 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Hi Darrel, # Delete namespaces from the running OS >> m4_define([DEL_NAMESPACES], >> [m4_foreach([ns], [$@], >> - [ip netns del ns >> -]) >> + [if ip netns list | grep ns > /dev/null; then >> + ip netns del ns >> + fi >> + ]) >> ] >> ) >> > > Do we want to suppress an error on deletion in general ? > > No, I think it won't suppress errors on deletion. > Is the problem wherein ADD_NAMESPACES tries to always remove > a namespace before adding it ? > Yes. > Is it better to check if ns exists here before calling DEL_NAMESPACES ? > > > yes we could also add check here: > m4_define([ADD_NAMESPACES], > [m4_foreach([ns], [$@], > - [DEL_NAMESPACES(ns) > + [ //check if ns exists > AT_CHECK([ip netns add ns || return 77]) > on_exit 'DEL_NAMESPACES(ns)' > ]) > ] > ) > > Regards, William
On Mon, May 2, 2016 at 8:39 AM, William Tu <u9012063@gmail.com> wrote: > Hi Darrel, > > # Delete namespaces from the running OS >>> m4_define([DEL_NAMESPACES], >>> [m4_foreach([ns], [$@], >>> - [ip netns del ns >>> -]) >>> + [if ip netns list | grep ns > /dev/null; then >>> + ip netns del ns >>> + fi >>> + ]) >>> ] >>> ) >>> >> >> Do we want to suppress an error on deletion in general ? >> >> > No, I think it won't suppress errors on deletion. > Just to be clear, what the comment means is that if: 1) DEL_NAMESPACE is called and there is no such ns, then this may be an error with the surrounding code (i.e. a bug) or maybe the test itself. 2) Hence the above code in DEL_NAMESPACE would make the bug less visible since there would be no visible complaint on trying to delete a ns that does not exist > > >> Is the problem wherein ADD_NAMESPACES tries to always remove >> a namespace before adding it ? >> > > Yes. > > >> Is it better to check if ns exists here before calling DEL_NAMESPACES ? >> >> >> > yes we could also add check here: > > >> m4_define([ADD_NAMESPACES], >> [m4_foreach([ns], [$@], >> > - [DEL_NAMESPACES(ns) >> > + [ //check if ns exists > >> AT_CHECK([ip netns add ns || return 77]) >> on_exit 'DEL_NAMESPACES(ns)' >> ]) >> ] >> ) >> >> Regards, > William > >
Hi Darrell, > > Just to be clear, what the comment means is that if: > 1) DEL_NAMESPACE is called and there is no such ns, then this may be an > error with the surrounding code (i.e. a bug) or maybe the test itself. > > 2) Hence the above code in DEL_NAMESPACE would make the bug less > visible since there would be no visible complaint on trying to delete a ns > that does not exist > > I see your point. Thanks! Regards, William
On 2 May 2016 at 09:04, Darrell Ball <dlu998@gmail.com> wrote: > On Mon, May 2, 2016 at 8:39 AM, William Tu <u9012063@gmail.com> wrote: > >> Hi Darrel, >> >> # Delete namespaces from the running OS >>>> m4_define([DEL_NAMESPACES], >>>> [m4_foreach([ns], [$@], >>>> - [ip netns del ns >>>> -]) >>>> + [if ip netns list | grep ns > /dev/null; then >>>> + ip netns del ns >>>> + fi >>>> + ]) >>>> ] >>>> ) >>>> >>> >>> Do we want to suppress an error on deletion in general ? >>> >>> >> No, I think it won't suppress errors on deletion. >> > > > Just to be clear, what the comment means is that if: > 1) DEL_NAMESPACE is called and there is no such ns, then this may be an > error with the surrounding code (i.e. a bug) or maybe the test itself. > > 2) Hence the above code in DEL_NAMESPACE would make the bug less > visible since there would be no visible complaint on trying to delete a ns > that does not exist I was actually just wondering about why we need DEL_NAMESPACES. Originally, if you did a CTRL+C in the middle of the test, then cleanup would not properly occur so you'd end up with all of these test namespaces still existing. By deleting all of the specified namespaces at the start of ADD_NAMESPACES, it would allow the test to proceed without forcing the user to go through and delete all of the namespaces. However, if we were to queue up namespace deletion using on_exit "ip netns delete foo" immediately after creation, then the above issue should not exist, so maybe we could get rid of DELETE_NAMESPACES? In general I've advocated in the tests that while the test-writer needs to specify things like ADD_NAMESPACES(), those commands will queue up the cleanup to ensure that whether the test passes or fails, the system is left in a tidy state. This means that it is not necessary inside of tests to add the DELETE_NAMESPACES() towards the end (which would only execute if the rest of the test was successful).
On Mon, May 2, 2016 at 11:12 AM, Joe Stringer <joe@ovn.org> wrote: > On 2 May 2016 at 09:04, Darrell Ball <dlu998@gmail.com> wrote: > > On Mon, May 2, 2016 at 8:39 AM, William Tu <u9012063@gmail.com> wrote: > > > >> Hi Darrel, > >> > >> # Delete namespaces from the running OS > >>>> m4_define([DEL_NAMESPACES], > >>>> [m4_foreach([ns], [$@], > >>>> - [ip netns del ns > >>>> -]) > >>>> + [if ip netns list | grep ns > /dev/null; then > >>>> + ip netns del ns > >>>> + fi > >>>> + ]) > >>>> ] > >>>> ) > >>>> > >>> > >>> Do we want to suppress an error on deletion in general ? > >>> > >>> > >> No, I think it won't suppress errors on deletion. > >> > > > > > > Just to be clear, what the comment means is that if: > > 1) DEL_NAMESPACE is called and there is no such ns, then this may be an > > error with the surrounding code (i.e. a bug) or maybe the test itself. > > > > 2) Hence the above code in DEL_NAMESPACE would make the bug less > > visible since there would be no visible complaint on trying to delete a > ns > > that does not exist > > I was actually just wondering about why we need DEL_NAMESPACES. > Originally, if you did a CTRL+C in the middle of the test, then > cleanup would not properly occur so you'd end up with all of these > test namespaces still existing. By deleting all of the specified > namespaces at the start of ADD_NAMESPACES, it would allow the test to > proceed without forcing the user to go through and delete all of the > namespaces. > For the purposes of ADD_NAMESPACES, I agree - DEL_NAMESPACES is not essential. > > However, if we were to queue up namespace deletion using on_exit "ip > netns delete foo" immediately after creation, then the above issue > should not exist, so maybe we could get rid of DELETE_NAMESPACES? > However, testing delete namespaces within the testsuite in other respects (in future) seems useful to catch bugs as opposed to just cleanup. DEL_NAMESPACES seems like one small wrapper that could be used to make this cleaner, although it is not essential. > > In general I've advocated in the tests that while the test-writer > needs to specify things like ADD_NAMESPACES(), those commands will > queue up the cleanup to ensure that whether the test passes or fails, > the system is left in a tidy state. This means that it is not > necessary inside of tests to add the DELETE_NAMESPACES() towards the > end (which would only execute if the rest of the test was successful). >
On 2 May 2016 at 16:57, Darrell Ball <dlu998@gmail.com> wrote: > > > On Mon, May 2, 2016 at 11:12 AM, Joe Stringer <joe@ovn.org> wrote: >> >> On 2 May 2016 at 09:04, Darrell Ball <dlu998@gmail.com> wrote: >> > On Mon, May 2, 2016 at 8:39 AM, William Tu <u9012063@gmail.com> wrote: >> > >> >> Hi Darrel, >> >> >> >> # Delete namespaces from the running OS >> >>>> m4_define([DEL_NAMESPACES], >> >>>> [m4_foreach([ns], [$@], >> >>>> - [ip netns del ns >> >>>> -]) >> >>>> + [if ip netns list | grep ns > /dev/null; then >> >>>> + ip netns del ns >> >>>> + fi >> >>>> + ]) >> >>>> ] >> >>>> ) >> >>>> >> >>> >> >>> Do we want to suppress an error on deletion in general ? >> >>> >> >>> >> >> No, I think it won't suppress errors on deletion. >> >> >> > >> > >> > Just to be clear, what the comment means is that if: >> > 1) DEL_NAMESPACE is called and there is no such ns, then this may be an >> > error with the surrounding code (i.e. a bug) or maybe the test itself. >> > >> > 2) Hence the above code in DEL_NAMESPACE would make the bug less >> > visible since there would be no visible complaint on trying to delete a >> > ns >> > that does not exist >> >> I was actually just wondering about why we need DEL_NAMESPACES. >> Originally, if you did a CTRL+C in the middle of the test, then >> cleanup would not properly occur so you'd end up with all of these >> test namespaces still existing. By deleting all of the specified >> namespaces at the start of ADD_NAMESPACES, it would allow the test to >> proceed without forcing the user to go through and delete all of the >> namespaces. > > > For the purposes of ADD_NAMESPACES, I agree - DEL_NAMESPACES > is not essential. > >> >> >> However, if we were to queue up namespace deletion using on_exit "ip >> netns delete foo" immediately after creation, then the above issue >> should not exist, so maybe we could get rid of DELETE_NAMESPACES? > > > However, testing delete namespaces within the testsuite in other respects > (in future) seems useful to catch bugs as opposed to just cleanup. > DEL_NAMESPACES > seems like one small wrapper that could be used to make this cleaner, > although > it is not essential. If the only argument is regarding some possible future, then I would press to get rid of it. It's not a public API; it's not used like that today; we can always reintroduce it if/when we want to use it that way.
less code -> less bugs On Mon, May 2, 2016 at 5:45 PM, Joe Stringer <joe@ovn.org> wrote: > On 2 May 2016 at 16:57, Darrell Ball <dlu998@gmail.com> wrote: > > > > > > On Mon, May 2, 2016 at 11:12 AM, Joe Stringer <joe@ovn.org> wrote: > >> > >> On 2 May 2016 at 09:04, Darrell Ball <dlu998@gmail.com> wrote: > >> > On Mon, May 2, 2016 at 8:39 AM, William Tu <u9012063@gmail.com> > wrote: > >> > > >> >> Hi Darrel, > >> >> > >> >> # Delete namespaces from the running OS > >> >>>> m4_define([DEL_NAMESPACES], > >> >>>> [m4_foreach([ns], [$@], > >> >>>> - [ip netns del ns > >> >>>> -]) > >> >>>> + [if ip netns list | grep ns > /dev/null; then > >> >>>> + ip netns del ns > >> >>>> + fi > >> >>>> + ]) > >> >>>> ] > >> >>>> ) > >> >>>> > >> >>> > >> >>> Do we want to suppress an error on deletion in general ? > >> >>> > >> >>> > >> >> No, I think it won't suppress errors on deletion. > >> >> > >> > > >> > > >> > Just to be clear, what the comment means is that if: > >> > 1) DEL_NAMESPACE is called and there is no such ns, then this may be > an > >> > error with the surrounding code (i.e. a bug) or maybe the test itself. > >> > > >> > 2) Hence the above code in DEL_NAMESPACE would make the bug less > >> > visible since there would be no visible complaint on trying to delete > a > >> > ns > >> > that does not exist > >> > >> I was actually just wondering about why we need DEL_NAMESPACES. > >> Originally, if you did a CTRL+C in the middle of the test, then > >> cleanup would not properly occur so you'd end up with all of these > >> test namespaces still existing. By deleting all of the specified > >> namespaces at the start of ADD_NAMESPACES, it would allow the test to > >> proceed without forcing the user to go through and delete all of the > >> namespaces. > > > > > > For the purposes of ADD_NAMESPACES, I agree - DEL_NAMESPACES > > is not essential. > > > >> > >> > >> However, if we were to queue up namespace deletion using on_exit "ip > >> netns delete foo" immediately after creation, then the above issue > >> should not exist, so maybe we could get rid of DELETE_NAMESPACES? > > > > > > However, testing delete namespaces within the testsuite in other respects > > (in future) seems useful to catch bugs as opposed to just cleanup. > > DEL_NAMESPACES > > seems like one small wrapper that could be used to make this cleaner, > > although > > it is not essential. > > If the only argument is regarding some possible future, then I would > press to get rid of it. It's not a public API; it's not used like that > today; we can always reintroduce it if/when we want to use it that > way. >
diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at index 2116f1e..ae09e83 100644 --- a/tests/system-common-macros.at +++ b/tests/system-common-macros.at @@ -3,8 +3,10 @@ # Delete namespaces from the running OS m4_define([DEL_NAMESPACES], [m4_foreach([ns], [$@], - [ip netns del ns -]) + [if ip netns list | grep ns > /dev/null; then + ip netns del ns + fi + ]) ] )
This avoids error message "Cannot remove namespace file "/var/run/netns/at_ns0": No such file or directory", when at_ns0 does not exist. Signed-off-by: William Tu <u9012063@gmail.com> --- tests/system-common-macros.at | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)