[ovs-dev] Tests: Fix testing bridge - add port after stopping controller on Windows
diff mbox series

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
Related show

Commit Message

Alin Gabriel Serdean Nov. 14, 2018, 3:30 p.m. UTC
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(-)

Comments

Sairam Venugopal Nov. 14, 2018, 8:18 p.m. UTC | #1
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&amp;data=02%7C01%7Cvsairam%40vmware.com%7C074d45f6e70d4431f7a708d64a4639a6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636778062866824077&amp;sdata=EPht5myv2MmYwaY49hSYegAfewIVjkbvNCTf5YBsYJM%3D&amp;reserved=0
Alin Gabriel Serdean Nov. 15, 2018, 1:23 p.m. UTC | #2
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&amp;data=02%7C01%7Cvsairam%40vmware.com%7C074d45f6e70d4431
> f7a708d64a4639a6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6
> 36778062866824077&amp;sdata=EPht5myv2MmYwaY49hSYegAfewIVjkbvNC
> Tf5YBsYJM%3D&amp;reserved=0
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Alin Gabriel Serdean Nov. 15, 2018, 1:32 p.m. UTC | #3
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&amp;data=02%7C01%7Cvsairam%40vmware.com%7C074d45f6e70d4431
> >
> f7a708d64a4639a6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6
> >
> 36778062866824077&amp;sdata=EPht5myv2MmYwaY49hSYegAfewIVjkbvNC
> > Tf5YBsYJM%3D&amp;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
Ben Pfaff Nov. 15, 2018, 4:10 p.m. UTC | #4
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) {
Alin Serdean Nov. 15, 2018, 4:22 p.m. UTC | #5
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
Ben Pfaff Nov. 15, 2018, 4:38 p.m. UTC | #6
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

Patch
diff mbox series

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])