| Message ID | 20181114153053.13420-1-aserdean@ovn.org |
|---|---|
| State | Accepted |
| Delegated to: | Ben Pfaff |
| Headers | show |
| Series | [ovs-dev] Tests: Fix testing bridge - add port after stopping controller on Windows | expand |
Thanks for fixing this. The files normally get deleted when the agent closes. Is this a regression or just test related? Ack'ing the fix. Acked-by: Sairam Venugopal <vsairam@vmware.com> On 11/14/18, 7:31 AM, "ovs-dev-bounces@openvswitch.org on behalf of Alin Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of aserdean@ovn.org> wrote: On Windows the file which is used for the named pipe connection (`punix:file`) is not deleted when the process is closed. Try to delete the `controller` file and fail if we can't (on Windows you can't delete a file if there still an opened handle to it). Also add a check to see if the `ovs-testcontroller` was successfully started. Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- tests/bridge.at | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/bridge.at b/tests/bridge.at index ee398bdb1..b94afc194 100644 --- a/tests/bridge.at +++ b/tests/bridge.at @@ -84,7 +84,7 @@ AT_SETUP([bridge - add port after stopping controller]) OVS_VSWITCHD_START dnl Start ovs-testcontroller -ovs-testcontroller --detach punix:controller --pidfile=ovs-testcontroller.pid +AT_CHECK([ovs-testcontroller --detach punix:controller --pidfile], [0], [ignore]) OVS_WAIT_UNTIL([test -e controller]) AT_CHECK([ovs-vsctl set-controller br0 unix:controller]) @@ -93,6 +93,9 @@ AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], [ignore]) # Now kill the ovs-testcontroller kill `cat ovs-testcontroller.pid` +if test "$IS_WIN32" = "yes"; then + AT_CHECK([rm controller], [0], [ignore]) +fi OVS_WAIT_UNTIL([! test -e controller]) AT_CHECK([ovs-vsctl --no-wait add-port br0 p2 -- set Interface p2 type=internal], [0], [ignore]) AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], [ignore]) -- 2.16.1.windows.1 _______________________________________________ dev mailing list dev@openvswitch.org https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cvsairam%40vmware.com%7C074d45f6e70d4431f7a708d64a4639a6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636778062866824077&sdata=EPht5myv2MmYwaY49hSYegAfewIVjkbvNCTf5YBsYJM%3D&reserved=0
I have opened an issue for it: https://github.com/openvswitch/ovs-issues/issues/165 It's not test specific unfortunately. The file is put up for unlink on exit, but I'm guessing someone still has an opened handle at that point. I'm applying this patch for the moment and following up with the actual fix for the userspace. Thanks for the ack. Alin. > -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele Sairam Venugopal > Trimis: Wednesday, November 14, 2018 10:19 PM > Către: Alin Gabriel Serdean <aserdean@ovn.org>; dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] Tests: Fix testing bridge - add port after > stopping controller on Windows > > Thanks for fixing this. The files normally get deleted when the agent closes. Is > this a regression or just test related? Ack'ing the fix. > > Acked-by: Sairam Venugopal <vsairam@vmware.com> > > On 11/14/18, 7:31 AM, "ovs-dev-bounces@openvswitch.org on behalf of Alin > Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of > aserdean@ovn.org> wrote: > > On Windows the file which is used for the named pipe connection > (`punix:file`) > is not deleted when the process is closed. > > Try to delete the `controller` file and fail if we can't (on Windows you can't > delete a file if there still an opened handle to it). > > Also add a check to see if the `ovs-testcontroller` was successfully started. > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > --- > tests/bridge.at | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/tests/bridge.at b/tests/bridge.at > index ee398bdb1..b94afc194 100644 > --- a/tests/bridge.at > +++ b/tests/bridge.at > @@ -84,7 +84,7 @@ AT_SETUP([bridge - add port after stopping > controller]) > OVS_VSWITCHD_START > > dnl Start ovs-testcontroller > -ovs-testcontroller --detach punix:controller --pidfile=ovs- > testcontroller.pid > +AT_CHECK([ovs-testcontroller --detach punix:controller --pidfile], [0], > [ignore]) > OVS_WAIT_UNTIL([test -e controller]) > > AT_CHECK([ovs-vsctl set-controller br0 unix:controller]) > @@ -93,6 +93,9 @@ AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], > [ignore]) > > # Now kill the ovs-testcontroller > kill `cat ovs-testcontroller.pid` > +if test "$IS_WIN32" = "yes"; then > + AT_CHECK([rm controller], [0], [ignore]) > +fi > OVS_WAIT_UNTIL([! test -e controller]) > AT_CHECK([ovs-vsctl --no-wait add-port br0 p2 -- set Interface p2 > type=internal], [0], [ignore]) > AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], [ignore]) > -- > 2.16.1.windows.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.o > penvswitch.org%2Fmailman%2Flistinfo%2Fovs- > dev&data=02%7C01%7Cvsairam%40vmware.com%7C074d45f6e70d4431 > f7a708d64a4639a6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6 > 36778062866824077&sdata=EPht5myv2MmYwaY49hSYegAfewIVjkbvNC > Tf5YBsYJM%3D&reserved=0 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Applied on master, branch-2.10, branch-2.9. -- Alin. > -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele aserdean@ovn.org > Trimis: Thursday, November 15, 2018 3:23 PM > Către: 'Sairam Venugopal' <vsairam@vmware.com>; 'Alin Gabriel Serdean' > <aserdean@ovn.org>; dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] Tests: Fix testing bridge - add port after > stopping controller on Windows > > I have opened an issue for it: https://github.com/openvswitch/ovs- > issues/issues/165 > > It's not test specific unfortunately. The file is put up for unlink on exit, but I'm > guessing someone still has an opened handle at that point. > > I'm applying this patch for the moment and following up with the actual fix > for the userspace. > > Thanks for the ack. > > Alin. > > > -----Mesaj original----- > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > bounces@openvswitch.org> În numele Sairam Venugopal > > Trimis: Wednesday, November 14, 2018 10:19 PM > > Către: Alin Gabriel Serdean <aserdean@ovn.org>; dev@openvswitch.org > > Subiect: Re: [ovs-dev] [PATCH] Tests: Fix testing bridge - add port > > after stopping controller on Windows > > > > Thanks for fixing this. The files normally get deleted when the agent > > closes. Is this a regression or just test related? Ack'ing the fix. > > > > Acked-by: Sairam Venugopal <vsairam@vmware.com> > > > > On 11/14/18, 7:31 AM, "ovs-dev-bounces@openvswitch.org on behalf of > > Alin Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of > > aserdean@ovn.org> wrote: > > > > On Windows the file which is used for the named pipe connection > > (`punix:file`) > > is not deleted when the process is closed. > > > > Try to delete the `controller` file and fail if we can't (on Windows you > can't > > delete a file if there still an opened handle to it). > > > > Also add a check to see if the `ovs-testcontroller` was successfully > started. > > > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > > --- > > tests/bridge.at | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/tests/bridge.at b/tests/bridge.at > > index ee398bdb1..b94afc194 100644 > > --- a/tests/bridge.at > > +++ b/tests/bridge.at > > @@ -84,7 +84,7 @@ AT_SETUP([bridge - add port after stopping > > controller]) > > OVS_VSWITCHD_START > > > > dnl Start ovs-testcontroller > > -ovs-testcontroller --detach punix:controller --pidfile=ovs- > > testcontroller.pid > > +AT_CHECK([ovs-testcontroller --detach punix:controller > > --pidfile], [0], > > [ignore]) > > OVS_WAIT_UNTIL([test -e controller]) > > > > AT_CHECK([ovs-vsctl set-controller br0 unix:controller]) > > @@ -93,6 +93,9 @@ AT_CHECK([ovs-appctl -t ovs-vswitchd version], > > [0], > > [ignore]) > > > > # Now kill the ovs-testcontroller > > kill `cat ovs-testcontroller.pid` > > +if test "$IS_WIN32" = "yes"; then > > + AT_CHECK([rm controller], [0], [ignore]) > > +fi > > OVS_WAIT_UNTIL([! test -e controller]) > > AT_CHECK([ovs-vsctl --no-wait add-port br0 p2 -- set Interface p2 > > type=internal], [0], [ignore]) > > AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], [ignore]) > > -- > > 2.16.1.windows.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. > > o > > penvswitch.org%2Fmailman%2Flistinfo%2Fovs- > > > dev&data=02%7C01%7Cvsairam%40vmware.com%7C074d45f6e70d4431 > > > f7a708d64a4639a6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6 > > > 36778062866824077&sdata=EPht5myv2MmYwaY49hSYegAfewIVjkbvNC > > Tf5YBsYJM%3D&reserved=0 > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Nov 15, 2018 at 03:23:28PM +0200, aserdean@ovn.org wrote: > I have opened an issue for it: https://github.com/openvswitch/ovs-issues/issues/165 > > It's not test specific unfortunately. The file is put up for unlink on exit, but I'm guessing > someone still has an opened handle at that point. > > I'm applying this patch for the moment and following up with the actual fix for the > userspace. Unix has similar-sounding issues with dangling Unix domain sockets. On Unix, the customary way to solve it is to delete an existing socket before trying to create one with the same name. You can see that in make_unix_socket() in socket-util-unix.c: if (bind_path) { char linkname[MAX_UN_LEN + 1]; struct sockaddr_un un; socklen_t un_len; int dirfd; if (unlink(bind_path) && errno != ENOENT) { VLOG_WARN("unlinking \"%s\": %s\n", bind_path, ovs_strerror(errno)); } fatal_signal_add_file_to_unlink(bind_path); If that approach is appropriate under Windows as well, it might be implemented something like this: diff --git a/lib/stream-windows.c b/lib/stream-windows.c index 34bc610b6f49..b027e48b4f8d 100644 --- a/lib/stream-windows.c +++ b/lib/stream-windows.c @@ -616,6 +616,11 @@ pwindows_open(const char *name OVS_UNUSED, char *suffix, path = xstrdup(suffix); } + /* Remove any existing named pipe. */ + if (remove(bind_path) && errno != ENOENT) { + VLOG_WARN("removing \"%s\": %s\n", bind_path, ovs_strerror(errno)); + } + /* Try to create a file under the path location. */ FILE *file = fopen(path, "w"); if (!file) {
Thanks for the suggestion Ben. I was thinking about changing the regular open to a CreateFile on Windows with the flag: ` FILE_FLAG_DELETE_ON_CLOSE 0x04000000 The file is to be deleted immediately after all of its handles are closed, which includes the specified handle and any other open or duplicated handles. If there are existing open handles to a file, the call fails unless they were all opened with the FILE_SHARE_DELETE share mode. Subsequent open requests for the file fail, unless the FILE_SHARE_DELETE share mode is specified.` https://docs.microsoft.com/en-us/windows/desktop/api/FileAPI/nf-fileapi-createfilea But both can work. > -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele Ben Pfaff > Trimis: Thursday, November 15, 2018 6:11 PM > Către: aserdean@ovn.org > Cc: dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] Tests: Fix testing bridge - add port after > stopping controller on Windows > > On Thu, Nov 15, 2018 at 03:23:28PM +0200, aserdean@ovn.org wrote: > > I have opened an issue for it: > > https://github.com/openvswitch/ovs-issues/issues/165 > > > > It's not test specific unfortunately. The file is put up for unlink on > > exit, but I'm guessing someone still has an opened handle at that point. > > > > I'm applying this patch for the moment and following up with the > > actual fix for the userspace. > > Unix has similar-sounding issues with dangling Unix domain sockets. On > Unix, the customary way to solve it is to delete an existing socket before > trying to create one with the same name. You can see that in > make_unix_socket() in socket-util-unix.c: > > if (bind_path) { > char linkname[MAX_UN_LEN + 1]; > struct sockaddr_un un; > socklen_t un_len; > int dirfd; > > if (unlink(bind_path) && errno != ENOENT) { > VLOG_WARN("unlinking \"%s\": %s\n", > bind_path, ovs_strerror(errno)); > } > fatal_signal_add_file_to_unlink(bind_path); > > If that approach is appropriate under Windows as well, it might be > implemented something like this: > diff --git a/lib/stream-windows.c b/lib/stream-windows.c index > 34bc610b6f49..b027e48b4f8d 100644 > --- a/lib/stream-windows.c > +++ b/lib/stream-windows.c > @@ -616,6 +616,11 @@ pwindows_open(const char *name OVS_UNUSED, > char *suffix, > path = xstrdup(suffix); > } > > + /* Remove any existing named pipe. */ > + if (remove(bind_path) && errno != ENOENT) { > + VLOG_WARN("removing \"%s\": %s\n", bind_path, > ovs_strerror(errno)); > + } > + > /* Try to create a file under the path location. */ > FILE *file = fopen(path, "w"); > if (!file) { > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
That makes sense. I wish Unix had a delete-on-close flag, it would save a lot of trouble sometimes. On Thu, Nov 15, 2018 at 04:22:59PM +0000, Alin Serdean wrote: > Thanks for the suggestion Ben. > > I was thinking about changing the regular open to a CreateFile on > Windows with the flag: > ` > FILE_FLAG_DELETE_ON_CLOSE > 0x04000000 > > The file is to be deleted immediately after all of its handles are closed, > which includes the specified handle and any other open or > duplicated handles. > > If there are existing open handles to a file, the call fails unless > they were all opened with the FILE_SHARE_DELETE share mode. > > Subsequent open requests for the file fail, unless the > FILE_SHARE_DELETE share mode is specified.` > > https://docs.microsoft.com/en-us/windows/desktop/api/FileAPI/nf-fileapi-createfilea > > But both can work. > > > -----Mesaj original----- > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > bounces@openvswitch.org> În numele Ben Pfaff > > Trimis: Thursday, November 15, 2018 6:11 PM > > Către: aserdean@ovn.org > > Cc: dev@openvswitch.org > > Subiect: Re: [ovs-dev] [PATCH] Tests: Fix testing bridge - add port after > > stopping controller on Windows > > > > On Thu, Nov 15, 2018 at 03:23:28PM +0200, aserdean@ovn.org wrote: > > > I have opened an issue for it: > > > https://github.com/openvswitch/ovs-issues/issues/165 > > > > > > It's not test specific unfortunately. The file is put up for unlink on > > > exit, but I'm guessing someone still has an opened handle at that point. > > > > > > I'm applying this patch for the moment and following up with the > > > actual fix for the userspace. > > > > Unix has similar-sounding issues with dangling Unix domain sockets. On > > Unix, the customary way to solve it is to delete an existing socket before > > trying to create one with the same name. You can see that in > > make_unix_socket() in socket-util-unix.c: > > > > if (bind_path) { > > char linkname[MAX_UN_LEN + 1]; > > struct sockaddr_un un; > > socklen_t un_len; > > int dirfd; > > > > if (unlink(bind_path) && errno != ENOENT) { > > VLOG_WARN("unlinking \"%s\": %s\n", > > bind_path, ovs_strerror(errno)); > > } > > fatal_signal_add_file_to_unlink(bind_path); > > > > If that approach is appropriate under Windows as well, it might be > > implemented something like this: > > diff --git a/lib/stream-windows.c b/lib/stream-windows.c index > > 34bc610b6f49..b027e48b4f8d 100644 > > --- a/lib/stream-windows.c > > +++ b/lib/stream-windows.c > > @@ -616,6 +616,11 @@ pwindows_open(const char *name OVS_UNUSED, > > char *suffix, > > path = xstrdup(suffix); > > } > > > > + /* Remove any existing named pipe. */ > > + if (remove(bind_path) && errno != ENOENT) { > > + VLOG_WARN("removing \"%s\": %s\n", bind_path, > > ovs_strerror(errno)); > > + } > > + > > /* Try to create a file under the path location. */ > > FILE *file = fopen(path, "w"); > > if (!file) { > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/tests/bridge.at b/tests/bridge.at index ee398bdb1..b94afc194 100644 --- a/tests/bridge.at +++ b/tests/bridge.at @@ -84,7 +84,7 @@ AT_SETUP([bridge - add port after stopping controller]) OVS_VSWITCHD_START dnl Start ovs-testcontroller -ovs-testcontroller --detach punix:controller --pidfile=ovs-testcontroller.pid +AT_CHECK([ovs-testcontroller --detach punix:controller --pidfile], [0], [ignore]) OVS_WAIT_UNTIL([test -e controller]) AT_CHECK([ovs-vsctl set-controller br0 unix:controller]) @@ -93,6 +93,9 @@ AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], [ignore]) # Now kill the ovs-testcontroller kill `cat ovs-testcontroller.pid` +if test "$IS_WIN32" = "yes"; then + AT_CHECK([rm controller], [0], [ignore]) +fi OVS_WAIT_UNTIL([! test -e controller]) AT_CHECK([ovs-vsctl --no-wait add-port br0 p2 -- set Interface p2 type=internal], [0], [ignore]) AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], [ignore])
On Windows the file which is used for the named pipe connection (`punix:file`) is not deleted when the process is closed. Try to delete the `controller` file and fail if we can't (on Windows you can't delete a file if there still an opened handle to it). Also add a check to see if the `ovs-testcontroller` was successfully started. Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- tests/bridge.at | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)