[ovs-dev,v1] rhel: retain OVS_CTL_OPTS for systemd service files
diff mbox series

Message ID 1536954563-103152-1-git-send-email-martinxu9.ovs@gmail.com
State New
Headers show
Series
  • [ovs-dev,v1] rhel: retain OVS_CTL_OPTS for systemd service files
Related show

Commit Message

Martin Xu Sept. 14, 2018, 7:49 p.m. UTC
OVS init.d script calls ovs-ctl with $OVS_CTL_OPTS defined in the
config file. This variable is replaced by OPTIONS in systemd service
files. This patch addes $OVS_CTL_OPTS back to be passed along with $OPTIONS
for backward compatibility.

VMware-BZ: #2036847

Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com>
CC: Aaron Conole <aconole@redhat.com>
---
 rhel/usr_lib_systemd_system_ovs-vswitchd.service.in           | 4 ++--
 rhel/usr_lib_systemd_system_ovsdb-server.service              | 4 ++--
 rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template | 5 +++++
 3 files changed, 9 insertions(+), 4 deletions(-)

Comments

Gregory Rose Jan. 22, 2019, 10:08 p.m. UTC | #1
Aaron,

This patch seems to have fallen through the cracks.  Would you mind 
giving this a review?  Martin is no
longer with VMware so I will be following up on this patch.

Thanks,

- Greg


On 9/14/2018 12:49 PM, Martin Xu wrote:
> OVS init.d script calls ovs-ctl with $OVS_CTL_OPTS defined in the
> config file. This variable is replaced by OPTIONS in systemd service
> files. This patch addes $OVS_CTL_OPTS back to be passed along with $OPTIONS
> for backward compatibility.
>
> VMware-BZ: #2036847
>
> Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com>
> CC: Aaron Conole <aconole@redhat.com>
> ---
>   rhel/usr_lib_systemd_system_ovs-vswitchd.service.in           | 4 ++--
>   rhel/usr_lib_systemd_system_ovsdb-server.service              | 4 ++--
>   rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template | 5 +++++
>   3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
> index 11b34c6..e4dbdf3 100644
> --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
> @@ -21,10 +21,10 @@ ExecStartPre=-/usr/bin/chmod 0775 /dev/hugepages
>   ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>             --no-ovsdb-server --no-monitor --system-id=random \
>             ${OVSUSER} \
> -          start $OPTIONS
> +          start $OPTIONS $OVS_CTL_OPTS
>   ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server stop
>   ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server \
>             --no-monitor --system-id=random \
>             ${OVSUSER} \
> -          restart $OPTIONS
> +          restart $OPTIONS $OVS_CTL_OPTS
>   TimeoutSec=300
> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service b/rhel/usr_lib_systemd_system_ovsdb-server.service
> index 70da1ec..09f946b 100644
> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
> @@ -16,10 +16,10 @@ EnvironmentFile=-/run/openvswitch/useropts
>   ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>             --no-ovs-vswitchd --no-monitor --system-id=random \
>             ${OVSUSER} \
> -          start $OPTIONS
> +          start $OPTIONS $OVS_CTL_OPTS
>   ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop
>   ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd \
>              ${OVSUSER} \
> -           --no-monitor restart $OPTIONS
> +           --no-monitor restart $OPTIONS $OVS_CTL_OPTS
>   RuntimeDirectory=openvswitch
>   RuntimeDirectoryMode=0755
> diff --git a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
> index 9364454..0ce5b6b 100644
> --- a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
> +++ b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
> @@ -23,5 +23,10 @@
>   #
>   OPTIONS=""
>   
> +# OVS_CTL_OPTS: Extra options to pass along with OPTIONS to ovs-ctl.
> +# This flag is retained for backward compatibility. For example,
> +# user can specify --ovs-vswitchd-wrapper=valgrind.
> +# OVS_CTL_OPTS=
> +
>   # Uncomment and set the OVS User/Group value
>   #OVS_USER_ID="openvswitch:openvswitch"
Gregory Rose Jan. 30, 2019, 7:06 p.m. UTC | #2
Pinging Aaron once again?

- Greg

On 1/22/2019 2:08 PM, Gregory Rose wrote:
>
> Aaron,
>
> This patch seems to have fallen through the cracks.  Would you mind 
> giving this a review?  Martin is no
> longer with VMware so I will be following up on this patch.
>
> Thanks,
>
> - Greg
>
>
> On 9/14/2018 12:49 PM, Martin Xu wrote:
>> OVS init.d script calls ovs-ctl with $OVS_CTL_OPTS defined in the
>> config file. This variable is replaced by OPTIONS in systemd service
>> files. This patch addes $OVS_CTL_OPTS back to be passed along with 
>> $OPTIONS
>> for backward compatibility.
>>
>> VMware-BZ: #2036847
>>
>> Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com>
>> CC: Aaron Conole <aconole@redhat.com>
>> ---
>>   rhel/usr_lib_systemd_system_ovs-vswitchd.service.in | 4 ++--
>>   rhel/usr_lib_systemd_system_ovsdb-server.service | 4 ++--
>>   rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template | 5 
>> +++++
>>   3 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in 
>> b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>> index 11b34c6..e4dbdf3 100644
>> --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>> @@ -21,10 +21,10 @@ ExecStartPre=-/usr/bin/chmod 0775 /dev/hugepages
>>   ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>>             --no-ovsdb-server --no-monitor --system-id=random \
>>             ${OVSUSER} \
>> -          start $OPTIONS
>> +          start $OPTIONS $OVS_CTL_OPTS
>>   ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server stop
>>   ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server \
>>             --no-monitor --system-id=random \
>>             ${OVSUSER} \
>> -          restart $OPTIONS
>> +          restart $OPTIONS $OVS_CTL_OPTS
>>   TimeoutSec=300
>> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service 
>> b/rhel/usr_lib_systemd_system_ovsdb-server.service
>> index 70da1ec..09f946b 100644
>> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
>> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
>> @@ -16,10 +16,10 @@ EnvironmentFile=-/run/openvswitch/useropts
>>   ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>>             --no-ovs-vswitchd --no-monitor --system-id=random \
>>             ${OVSUSER} \
>> -          start $OPTIONS
>> +          start $OPTIONS $OVS_CTL_OPTS
>>   ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop
>>   ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd \
>>              ${OVSUSER} \
>> -           --no-monitor restart $OPTIONS
>> +           --no-monitor restart $OPTIONS $OVS_CTL_OPTS
>>   RuntimeDirectory=openvswitch
>>   RuntimeDirectoryMode=0755
>> diff --git 
>> a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template 
>> b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
>> index 9364454..0ce5b6b 100644
>> --- a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
>> +++ b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
>> @@ -23,5 +23,10 @@
>>   #
>>   OPTIONS=""
>>   +# OVS_CTL_OPTS: Extra options to pass along with OPTIONS to ovs-ctl.
>> +# This flag is retained for backward compatibility. For example,
>> +# user can specify --ovs-vswitchd-wrapper=valgrind.
>> +# OVS_CTL_OPTS=
>> +
>>   # Uncomment and set the OVS User/Group value
>>   #OVS_USER_ID="openvswitch:openvswitch"
>
Aaron Conole Jan. 30, 2019, 7:19 p.m. UTC | #3
Gregory Rose <gvrose8192@gmail.com> writes:

> Pinging Aaron once again?

Whoops - completely missed this.  I'll take a look.

> - Greg
>
> On 1/22/2019 2:08 PM, Gregory Rose wrote:
>>
>> Aaron,
>>
>> This patch seems to have fallen through the cracks.  Would you mind
>> giving this a review?  Martin is no
>> longer with VMware so I will be following up on this patch.
>>
>> Thanks,
>>
>> - Greg
>>
>>
>> On 9/14/2018 12:49 PM, Martin Xu wrote:
>>> OVS init.d script calls ovs-ctl with $OVS_CTL_OPTS defined in the
>>> config file. This variable is replaced by OPTIONS in systemd service
>>> files. This patch addes $OVS_CTL_OPTS back to be passed along with
>>> $OPTIONS
>>> for backward compatibility.
>>>
>>> VMware-BZ: #2036847
>>>
>>> Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com>
>>> CC: Aaron Conole <aconole@redhat.com>
>>> ---
>>>   rhel/usr_lib_systemd_system_ovs-vswitchd.service.in | 4 ++--
>>>   rhel/usr_lib_systemd_system_ovsdb-server.service | 4 ++--
>>>   rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template | 5
>>> +++++
>>>   3 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>>> b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>>> index 11b34c6..e4dbdf3 100644
>>> --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>>> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>>> @@ -21,10 +21,10 @@ ExecStartPre=-/usr/bin/chmod 0775 /dev/hugepages
>>>   ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>>>             --no-ovsdb-server --no-monitor --system-id=random \
>>>             ${OVSUSER} \
>>> -          start $OPTIONS
>>> +          start $OPTIONS $OVS_CTL_OPTS
>>>   ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server stop
>>>   ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server \
>>>             --no-monitor --system-id=random \
>>>             ${OVSUSER} \
>>> -          restart $OPTIONS
>>> +          restart $OPTIONS $OVS_CTL_OPTS
>>>   TimeoutSec=300
>>> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> b/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> index 70da1ec..09f946b 100644
>>> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> @@ -16,10 +16,10 @@ EnvironmentFile=-/run/openvswitch/useropts
>>>   ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>>>             --no-ovs-vswitchd --no-monitor --system-id=random \
>>>             ${OVSUSER} \
>>> -          start $OPTIONS
>>> +          start $OPTIONS $OVS_CTL_OPTS
>>>   ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop
>>>   ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd \
>>>              ${OVSUSER} \
>>> -           --no-monitor restart $OPTIONS
>>> +           --no-monitor restart $OPTIONS $OVS_CTL_OPTS
>>>   RuntimeDirectory=openvswitch
>>>   RuntimeDirectoryMode=0755
>>> diff --git
>>> a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
>>> b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
>>> index 9364454..0ce5b6b 100644
>>> --- a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
>>> +++ b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
>>> @@ -23,5 +23,10 @@
>>>   #
>>>   OPTIONS=""
>>>   +# OVS_CTL_OPTS: Extra options to pass along with OPTIONS to ovs-ctl.
>>> +# This flag is retained for backward compatibility. For example,
>>> +# user can specify --ovs-vswitchd-wrapper=valgrind.
>>> +# OVS_CTL_OPTS=
>>> +
>>>   # Uncomment and set the OVS User/Group value
>>>   #OVS_USER_ID="openvswitch:openvswitch"
>>
Aaron Conole Jan. 31, 2019, 6:38 p.m. UTC | #4
Martin Xu <martinxu9.ovs@gmail.com> writes:

> OVS init.d script calls ovs-ctl with $OVS_CTL_OPTS defined in the
> config file. This variable is replaced by OPTIONS in systemd service
> files. This patch addes $OVS_CTL_OPTS back to be passed along with $OPTIONS
> for backward compatibility.
>
> VMware-BZ: #2036847
>
> Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com>
> CC: Aaron Conole <aconole@redhat.com>
> ---

I'm not sure why there should be two variables in the sysconfig file for
this.  The following would preserve the old and not introduce a new
variable (I think.. it's completely untested).  I guess this is because
the debian-distro openvswitch-switch.template file doesn't match the
rhel-distro template file, and we want to make a common set of systemd
scripts? Otherwise I don't see what the purpose is - what is the
migration path that this is addressing?

---
diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service b/rhel/usr_lib_systemd_system_ovsdb-server.service
index 09f946bb1..660ec75ef 100644
--- a/rhel/usr_lib_systemd_system_ovsdb-server.service
+++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
@@ -12,6 +12,7 @@ EnvironmentFile=/etc/openvswitch/default.conf
 EnvironmentFile=-/etc/sysconfig/openvswitch
 ExecStartPre=/usr/bin/chown ${OVS_USER_ID} /var/run/openvswitch /var/log/openvswitch
 ExecStartPre=/bin/sh -c 'rm -f /run/openvswitch/useropts; if [ "$${OVS_USER_ID/:*/}" != "root" ]; then /usr/bin/echo "OVSUSER=--ovs-user=${OVS_USER_ID}" > /run/openvswitch/useropts; fi'
+ExecStartPre=/bin/sh -c 'if [ "${OVS_CTL_OPTS}" != "" -a "${OPTIONS}" == "" ]; then /usr/bin/echo "OPTIONS=\"${OPTIONS} ${OVS_CTL_OPTS}\"" >> /run/openvswitch/useropts; fi'
 EnvironmentFile=-/run/openvswitch/useropts
 ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
           --no-ovs-vswitchd --no-monitor --system-id=random \
---
Gregory Rose Feb. 5, 2019, 9:19 p.m. UTC | #5
On 1/31/2019 10:38 AM, Aaron Conole wrote:
> Martin Xu <martinxu9.ovs@gmail.com> writes:
>
>> OVS init.d script calls ovs-ctl with $OVS_CTL_OPTS defined in the
>> config file. This variable is replaced by OPTIONS in systemd service
>> files. This patch addes $OVS_CTL_OPTS back to be passed along with $OPTIONS
>> for backward compatibility.
>>
>> VMware-BZ: #2036847
>>
>> Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com>
>> CC: Aaron Conole <aconole@redhat.com>
>> ---
> I'm not sure why there should be two variables in the sysconfig file for
> this.  The following would preserve the old and not introduce a new
> variable (I think.. it's completely untested).  I guess this is because
> the debian-distro openvswitch-switch.template file doesn't match the
> rhel-distro template file, and we want to make a common set of systemd
> scripts? Otherwise I don't see what the purpose is - what is the
> migration path that this is addressing?

Aaron,

I owe you a response on this and will get to it but some fires need 
putting out at the moment.

Thanks,

- Greg

>
> ---
> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service b/rhel/usr_lib_systemd_system_ovsdb-server.service
> index 09f946bb1..660ec75ef 100644
> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
> @@ -12,6 +12,7 @@ EnvironmentFile=/etc/openvswitch/default.conf
>   EnvironmentFile=-/etc/sysconfig/openvswitch
>   ExecStartPre=/usr/bin/chown ${OVS_USER_ID} /var/run/openvswitch /var/log/openvswitch
>   ExecStartPre=/bin/sh -c 'rm -f /run/openvswitch/useropts; if [ "$${OVS_USER_ID/:*/}" != "root" ]; then /usr/bin/echo "OVSUSER=--ovs-user=${OVS_USER_ID}" > /run/openvswitch/useropts; fi'
> +ExecStartPre=/bin/sh -c 'if [ "${OVS_CTL_OPTS}" != "" -a "${OPTIONS}" == "" ]; then /usr/bin/echo "OPTIONS=\"${OPTIONS} ${OVS_CTL_OPTS}\"" >> /run/openvswitch/useropts; fi'
>   EnvironmentFile=-/run/openvswitch/useropts
>   ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>             --no-ovs-vswitchd --no-monitor --system-id=random \
> ---

Patch
diff mbox series

diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
index 11b34c6..e4dbdf3 100644
--- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
+++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
@@ -21,10 +21,10 @@  ExecStartPre=-/usr/bin/chmod 0775 /dev/hugepages
 ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
           --no-ovsdb-server --no-monitor --system-id=random \
           ${OVSUSER} \
-          start $OPTIONS
+          start $OPTIONS $OVS_CTL_OPTS
 ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server stop
 ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovsdb-server \
           --no-monitor --system-id=random \
           ${OVSUSER} \
-          restart $OPTIONS
+          restart $OPTIONS $OVS_CTL_OPTS
 TimeoutSec=300
diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service b/rhel/usr_lib_systemd_system_ovsdb-server.service
index 70da1ec..09f946b 100644
--- a/rhel/usr_lib_systemd_system_ovsdb-server.service
+++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
@@ -16,10 +16,10 @@  EnvironmentFile=-/run/openvswitch/useropts
 ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
           --no-ovs-vswitchd --no-monitor --system-id=random \
           ${OVSUSER} \
-          start $OPTIONS
+          start $OPTIONS $OVS_CTL_OPTS
 ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop
 ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd \
            ${OVSUSER} \
-           --no-monitor restart $OPTIONS
+           --no-monitor restart $OPTIONS $OVS_CTL_OPTS
 RuntimeDirectory=openvswitch
 RuntimeDirectoryMode=0755
diff --git a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
index 9364454..0ce5b6b 100644
--- a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
+++ b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template
@@ -23,5 +23,10 @@ 
 #
 OPTIONS=""
 
+# OVS_CTL_OPTS: Extra options to pass along with OPTIONS to ovs-ctl.
+# This flag is retained for backward compatibility. For example,
+# user can specify --ovs-vswitchd-wrapper=valgrind.
+# OVS_CTL_OPTS=
+
 # Uncomment and set the OVS User/Group value
 #OVS_USER_ID="openvswitch:openvswitch"