diff mbox series

[ovs-dev,v2] controller, controller-vtep: Add --unixctl option.

Message ID 20240801162112.62537-1-roriorden@redhat.com
State Accepted
Headers show
Series [ovs-dev,v2] controller, controller-vtep: Add --unixctl option. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Rosemarie O'Riorden Aug. 1, 2024, 4:21 p.m. UTC
All other OVN daemons have the --unixctl option to specify the control
socket file. It was missing from ovn-controller and ovn-controller-vtep,
which this patch fixes.

Reported-at: https://issues.redhat.com/browse/FDP-714
Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
---
 controller-vtep/ovn-controller-vtep.8.xml |  3 +++
 controller-vtep/ovn-controller-vtep.c     | 11 ++++++++++-
 controller/ovn-controller.8.xml           |  3 ++-
 controller/ovn-controller.c               | 11 ++++++++++-
 4 files changed, 25 insertions(+), 3 deletions(-)

Comments

Rosemarie O'Riorden Aug. 2, 2024, 3:43 p.m. UTC | #1
Recheck-request: github-robot-_ovn-kubernetes

On 8/1/24 12:21 PM, Rosemarie O'Riorden wrote:
> All other OVN daemons have the --unixctl option to specify the control
> socket file. It was missing from ovn-controller and ovn-controller-vtep,
> which this patch fixes.
> 
> Reported-at: https://issues.redhat.com/browse/FDP-714
> Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
> ---
>  controller-vtep/ovn-controller-vtep.8.xml |  3 +++
>  controller-vtep/ovn-controller-vtep.c     | 11 ++++++++++-
>  controller/ovn-controller.8.xml           |  3 ++-
>  controller/ovn-controller.c               | 11 ++++++++++-
>  4 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/controller-vtep/ovn-controller-vtep.8.xml b/controller-vtep/ovn-controller-vtep.8.xml
> index 89acae7ed..026e18d97 100644
> --- a/controller-vtep/ovn-controller-vtep.8.xml
> +++ b/controller-vtep/ovn-controller-vtep.8.xml
> @@ -28,6 +28,9 @@
>      <xi:include href="lib/ssl-bootstrap.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>      <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>  
> +    <h2>Other Options</h2>
> +    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> +
>      <h1>Configuration</h1>
>      <p>
>        <code>ovn-controller-vtep</code> retrieves its configuration
> diff --git a/controller-vtep/ovn-controller-vtep.c b/controller-vtep/ovn-controller-vtep.c
> index 4472e5285..698511482 100644
> --- a/controller-vtep/ovn-controller-vtep.c
> +++ b/controller-vtep/ovn-controller-vtep.c
> @@ -58,6 +58,9 @@ static char *vtep_remote;
>  static char *ovnsb_remote;
>  static char *default_db_;
>  
> +/* --unixctl-path: Path to use for unixctl server socket. */
> +static char *unixctl_path;
> +
>  /* Returns true if the northd internal version stored in SB_Global
>   * and ovn-controller-vtep internal version match.
>   */
> @@ -118,7 +121,7 @@ main(int argc, char *argv[])
>  
>      daemonize_start(false, false);
>  
> -    char *abs_unixctl_path = get_abs_unix_ctl_path(NULL);
> +    char *abs_unixctl_path = get_abs_unix_ctl_path(unixctl_path);
>      retval = unixctl_server_create(abs_unixctl_path, &unixctl);
>      free(abs_unixctl_path);
>  
> @@ -287,6 +290,7 @@ parse_options(int argc, char *argv[])
>          {"vtep-db", required_argument, NULL, 'D'},
>          {"help", no_argument, NULL, 'h'},
>          {"version", no_argument, NULL, 'V'},
> +        {"unixctl", required_argument, NULL, 'u'},
>          VLOG_LONG_OPTIONS,
>          OVN_DAEMON_LONG_OPTIONS,
>          STREAM_SSL_LONG_OPTIONS,
> @@ -322,6 +326,10 @@ parse_options(int argc, char *argv[])
>              ovn_print_version(OFP13_VERSION, OFP13_VERSION);
>              exit(EXIT_SUCCESS);
>  
> +        case 'u':
> +            unixctl_path = optarg;
> +            break;
> +
>          VLOG_OPTION_HANDLERS
>          OVN_DAEMON_OPTION_HANDLERS
>          STREAM_SSL_OPTION_HANDLERS
> @@ -364,6 +372,7 @@ Options:\n\
>                              (default: %s)\n\
>    --ovnsb-db=DATABASE       connect to ovn-sb database at DATABASE\n\
>                              (default: %s)\n\
> +  -u, --unixctl=SOCKET      set control socket name\n\
>    -h, --help                display this help message\n\
>    -o, --options             list available options\n\
>    -V, --version             display version information\n\
> diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
> index faefa77b9..bcae4617e 100644
> --- a/controller/ovn-controller.8.xml
> +++ b/controller/ovn-controller.8.xml
> @@ -47,7 +47,8 @@
>      <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>  
>      <h2>Other Options</h2>
> -
> +    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> +    <h3></h3>
>      <xi:include href="lib/common.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>  
>  
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index f5674184a..936b42636 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -137,6 +137,9 @@ static const char *ssl_private_key_file;
>  static const char *ssl_certificate_file;
>  static const char *ssl_ca_cert_file;
>  
> +/* --unixctl-path: Path to use for unixctl server socket. */
> +static char *unixctl_path;
> +
>  /* By default don't set an upper bound for the lflow cache and enable auto
>   * trimming above 10K logical flows when reducing cache size by 50%.
>   */
> @@ -4835,7 +4838,7 @@ main(int argc, char *argv[])
>  
>      daemonize_start(true, false);
>  
> -    char *abs_unixctl_path = get_abs_unix_ctl_path(NULL);
> +    char *abs_unixctl_path = get_abs_unix_ctl_path(unixctl_path);
>      retval = unixctl_server_create(abs_unixctl_path, &unixctl);
>      free(abs_unixctl_path);
>      if (retval) {
> @@ -5957,6 +5960,7 @@ parse_options(int argc, char *argv[])
>      static struct option long_options[] = {
>          {"help", no_argument, NULL, 'h'},
>          {"version", no_argument, NULL, 'V'},
> +        {"unixctl", required_argument, NULL, 'u'},
>          VLOG_LONG_OPTIONS,
>          OVN_DAEMON_LONG_OPTIONS,
>          STREAM_SSL_LONG_OPTIONS,
> @@ -5986,6 +5990,10 @@ parse_options(int argc, char *argv[])
>              printf("SB DB Schema %s\n", sbrec_get_db_version());
>              exit(EXIT_SUCCESS);
>  
> +        case 'u':
> +            unixctl_path = optarg;
> +            break;
> +
>          VLOG_OPTION_HANDLERS
>          OVN_DAEMON_OPTION_HANDLERS
>  
> @@ -6061,6 +6069,7 @@ usage(void)
>      daemon_usage();
>      vlog_usage();
>      printf("\nOther options:\n"
> +           "  -u, --unixctl=SOCKET    set control socket name\n"
>             "  -n                      custom chassis name\n"
>             "  -h, --help              display this help message\n"
>             "  -V, --version           display version information\n");
Dumitru Ceara Aug. 7, 2024, 9:02 p.m. UTC | #2
On 8/1/24 18:21, Rosemarie O'Riorden wrote:
> All other OVN daemons have the --unixctl option to specify the control
> socket file. It was missing from ovn-controller and ovn-controller-vtep,
> which this patch fixes.
> 
> Reported-at: https://issues.redhat.com/browse/FDP-714
> Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara <dceara@redhat.com>

Regards,
Dumitru
Numan Siddique Aug. 8, 2024, 3:47 a.m. UTC | #3
On Wed, Aug 7, 2024 at 5:02 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 8/1/24 18:21, Rosemarie O'Riorden wrote:
> > All other OVN daemons have the --unixctl option to specify the control
> > socket file. It was missing from ovn-controller and ovn-controller-vtep,
> > which this patch fixes.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-714
> > Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
> > ---
>
> Looks good to me, thanks!
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks for the patch.  I added your name to AUTHORS list and applied
this patch to main.

Numan

>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Dumitru Ceara Aug. 8, 2024, 7:31 a.m. UTC | #4
On 8/8/24 05:47, Numan Siddique wrote:
> On Wed, Aug 7, 2024 at 5:02 PM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> On 8/1/24 18:21, Rosemarie O'Riorden wrote:
>>> All other OVN daemons have the --unixctl option to specify the control
>>> socket file. It was missing from ovn-controller and ovn-controller-vtep,
>>> which this patch fixes.
>>>
>>> Reported-at: https://issues.redhat.com/browse/FDP-714
>>> Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
>>> ---
>>
>> Looks good to me, thanks!
>>
>> Acked-by: Dumitru Ceara <dceara@redhat.com>
> 
> Thanks for the patch.  I added your name to AUTHORS list and applied
> this patch to main.
> 

Thanks, Numan, but did you actually mean to reply to the other patch
from Rosemarie, the one that adds --help and --version?

https://patchwork.ozlabs.org/project/ovn/patch/20240802145404.75625-1-roriorden@redhat.com/

The --unixctl patch should, in theory, be accepted only after branching.

Regards,
Dumitru
Numan Siddique Aug. 8, 2024, 6:19 p.m. UTC | #5
On Thu, Aug 8, 2024 at 3:31 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 8/8/24 05:47, Numan Siddique wrote:
> > On Wed, Aug 7, 2024 at 5:02 PM Dumitru Ceara <dceara@redhat.com> wrote:
> >>
> >> On 8/1/24 18:21, Rosemarie O'Riorden wrote:
> >>> All other OVN daemons have the --unixctl option to specify the control
> >>> socket file. It was missing from ovn-controller and ovn-controller-vtep,
> >>> which this patch fixes.
> >>>
> >>> Reported-at: https://issues.redhat.com/browse/FDP-714
> >>> Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
> >>> ---
> >>
> >> Looks good to me, thanks!
> >>
> >> Acked-by: Dumitru Ceara <dceara@redhat.com>
> >
> > Thanks for the patch.  I added your name to AUTHORS list and applied
> > this patch to main.
> >
>
> Thanks, Numan, but did you actually mean to reply to the other patch
> from Rosemarie, the one that adds --help and --version?
>
> https://patchwork.ozlabs.org/project/ovn/patch/20240802145404.75625-1-roriorden@redhat.com/

You're right.  I actually applied the doc patch.

Thanks
Numan

>
> The --unixctl patch should, in theory, be accepted only after branching.
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Aug. 27, 2024, 3:29 p.m. UTC | #6
On 8/1/24 18:21, Rosemarie O'Riorden wrote:
> All other OVN daemons have the --unixctl option to specify the control
> socket file. It was missing from ovn-controller and ovn-controller-vtep,
> which this patch fixes.
> 
> Reported-at: https://issues.redhat.com/browse/FDP-714
> Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
> ---
>  controller-vtep/ovn-controller-vtep.8.xml |  3 +++
>  controller-vtep/ovn-controller-vtep.c     | 11 ++++++++++-
>  controller/ovn-controller.8.xml           |  3 ++-
>  controller/ovn-controller.c               | 11 ++++++++++-
>  4 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/controller-vtep/ovn-controller-vtep.8.xml b/controller-vtep/ovn-controller-vtep.8.xml
> index 89acae7ed..026e18d97 100644
> --- a/controller-vtep/ovn-controller-vtep.8.xml
> +++ b/controller-vtep/ovn-controller-vtep.8.xml
> @@ -28,6 +28,9 @@
>      <xi:include href="lib/ssl-bootstrap.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>      <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>  
> +    <h2>Other Options</h2>
> +    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> +

This part needs a minor rebase to look the same as a page for ovn-controller
(h3 there is important):

diff --git a/controller-vtep/ovn-controller-vtep.8.xml b/controller-vtep/ovn-controller-vtep.8.xml
index ab9af63b1..6aec43fa7 100644
--- a/controller-vtep/ovn-controller-vtep.8.xml
+++ b/controller-vtep/ovn-controller-vtep.8.xml
@@ -29,6 +29,8 @@
     <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
 
     <h2>Other Options</h2>
+    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
+    <h3></h3>
     <xi:include href="lib/common.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
 
     <h1>Configuration</h1>
--

I guess, maintainers may do that while applying.

Acked-by: Ilya Maximets <i.maximets@ovn.org>
Numan Siddique Sept. 18, 2024, 11:02 p.m. UTC | #7
On Tue, Aug 27, 2024 at 11:30 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 8/1/24 18:21, Rosemarie O'Riorden wrote:
> > All other OVN daemons have the --unixctl option to specify the control
> > socket file. It was missing from ovn-controller and ovn-controller-vtep,
> > which this patch fixes.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-714
> > Signed-off-by: Rosemarie O'Riorden <roriorden@redhat.com>
> > ---
> >  controller-vtep/ovn-controller-vtep.8.xml |  3 +++
> >  controller-vtep/ovn-controller-vtep.c     | 11 ++++++++++-
> >  controller/ovn-controller.8.xml           |  3 ++-
> >  controller/ovn-controller.c               | 11 ++++++++++-
> >  4 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/controller-vtep/ovn-controller-vtep.8.xml b/controller-vtep/ovn-controller-vtep.8.xml
> > index 89acae7ed..026e18d97 100644
> > --- a/controller-vtep/ovn-controller-vtep.8.xml
> > +++ b/controller-vtep/ovn-controller-vtep.8.xml
> > @@ -28,6 +28,9 @@
> >      <xi:include href="lib/ssl-bootstrap.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> >      <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> >
> > +    <h2>Other Options</h2>
> > +    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> > +
>
> This part needs a minor rebase to look the same as a page for ovn-controller
> (h3 there is important):
>
> diff --git a/controller-vtep/ovn-controller-vtep.8.xml b/controller-vtep/ovn-controller-vtep.8.xml
> index ab9af63b1..6aec43fa7 100644
> --- a/controller-vtep/ovn-controller-vtep.8.xml
> +++ b/controller-vtep/ovn-controller-vtep.8.xml
> @@ -29,6 +29,8 @@
>      <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>
>      <h2>Other Options</h2>
> +    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> +    <h3></h3>
>      <xi:include href="lib/common.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>
>      <h1>Configuration</h1>
> --
>
> I guess, maintainers may do that while applying.
>
> Acked-by: Ilya Maximets <i.maximets@ovn.org>

Thanks.  Applied after rebasing.

Numan

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/controller-vtep/ovn-controller-vtep.8.xml b/controller-vtep/ovn-controller-vtep.8.xml
index 89acae7ed..026e18d97 100644
--- a/controller-vtep/ovn-controller-vtep.8.xml
+++ b/controller-vtep/ovn-controller-vtep.8.xml
@@ -28,6 +28,9 @@ 
     <xi:include href="lib/ssl-bootstrap.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
     <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
 
+    <h2>Other Options</h2>
+    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
+
     <h1>Configuration</h1>
     <p>
       <code>ovn-controller-vtep</code> retrieves its configuration
diff --git a/controller-vtep/ovn-controller-vtep.c b/controller-vtep/ovn-controller-vtep.c
index 4472e5285..698511482 100644
--- a/controller-vtep/ovn-controller-vtep.c
+++ b/controller-vtep/ovn-controller-vtep.c
@@ -58,6 +58,9 @@  static char *vtep_remote;
 static char *ovnsb_remote;
 static char *default_db_;
 
+/* --unixctl-path: Path to use for unixctl server socket. */
+static char *unixctl_path;
+
 /* Returns true if the northd internal version stored in SB_Global
  * and ovn-controller-vtep internal version match.
  */
@@ -118,7 +121,7 @@  main(int argc, char *argv[])
 
     daemonize_start(false, false);
 
-    char *abs_unixctl_path = get_abs_unix_ctl_path(NULL);
+    char *abs_unixctl_path = get_abs_unix_ctl_path(unixctl_path);
     retval = unixctl_server_create(abs_unixctl_path, &unixctl);
     free(abs_unixctl_path);
 
@@ -287,6 +290,7 @@  parse_options(int argc, char *argv[])
         {"vtep-db", required_argument, NULL, 'D'},
         {"help", no_argument, NULL, 'h'},
         {"version", no_argument, NULL, 'V'},
+        {"unixctl", required_argument, NULL, 'u'},
         VLOG_LONG_OPTIONS,
         OVN_DAEMON_LONG_OPTIONS,
         STREAM_SSL_LONG_OPTIONS,
@@ -322,6 +326,10 @@  parse_options(int argc, char *argv[])
             ovn_print_version(OFP13_VERSION, OFP13_VERSION);
             exit(EXIT_SUCCESS);
 
+        case 'u':
+            unixctl_path = optarg;
+            break;
+
         VLOG_OPTION_HANDLERS
         OVN_DAEMON_OPTION_HANDLERS
         STREAM_SSL_OPTION_HANDLERS
@@ -364,6 +372,7 @@  Options:\n\
                             (default: %s)\n\
   --ovnsb-db=DATABASE       connect to ovn-sb database at DATABASE\n\
                             (default: %s)\n\
+  -u, --unixctl=SOCKET      set control socket name\n\
   -h, --help                display this help message\n\
   -o, --options             list available options\n\
   -V, --version             display version information\n\
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index faefa77b9..bcae4617e 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -47,7 +47,8 @@ 
     <xi:include href="lib/ssl-peer-ca-cert.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
 
     <h2>Other Options</h2>
-
+    <xi:include href="lib/unixctl.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
+    <h3></h3>
     <xi:include href="lib/common.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
 
 
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index f5674184a..936b42636 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -137,6 +137,9 @@  static const char *ssl_private_key_file;
 static const char *ssl_certificate_file;
 static const char *ssl_ca_cert_file;
 
+/* --unixctl-path: Path to use for unixctl server socket. */
+static char *unixctl_path;
+
 /* By default don't set an upper bound for the lflow cache and enable auto
  * trimming above 10K logical flows when reducing cache size by 50%.
  */
@@ -4835,7 +4838,7 @@  main(int argc, char *argv[])
 
     daemonize_start(true, false);
 
-    char *abs_unixctl_path = get_abs_unix_ctl_path(NULL);
+    char *abs_unixctl_path = get_abs_unix_ctl_path(unixctl_path);
     retval = unixctl_server_create(abs_unixctl_path, &unixctl);
     free(abs_unixctl_path);
     if (retval) {
@@ -5957,6 +5960,7 @@  parse_options(int argc, char *argv[])
     static struct option long_options[] = {
         {"help", no_argument, NULL, 'h'},
         {"version", no_argument, NULL, 'V'},
+        {"unixctl", required_argument, NULL, 'u'},
         VLOG_LONG_OPTIONS,
         OVN_DAEMON_LONG_OPTIONS,
         STREAM_SSL_LONG_OPTIONS,
@@ -5986,6 +5990,10 @@  parse_options(int argc, char *argv[])
             printf("SB DB Schema %s\n", sbrec_get_db_version());
             exit(EXIT_SUCCESS);
 
+        case 'u':
+            unixctl_path = optarg;
+            break;
+
         VLOG_OPTION_HANDLERS
         OVN_DAEMON_OPTION_HANDLERS
 
@@ -6061,6 +6069,7 @@  usage(void)
     daemon_usage();
     vlog_usage();
     printf("\nOther options:\n"
+           "  -u, --unixctl=SOCKET    set control socket name\n"
            "  -n                      custom chassis name\n"
            "  -h, --help              display this help message\n"
            "  -V, --version           display version information\n");