diff mbox series

[ovs-dev] ovs-ctl: Don't overwrite external-id hostname

Message ID 20200525152821.19838-1-dalvarez@redhat.com
State Accepted
Headers show
Series [ovs-dev] ovs-ctl: Don't overwrite external-id hostname | expand

Commit Message

Daniel Alvarez Sanchez May 25, 2020, 3:28 p.m. UTC
ovs-ctl started to add the hostname as external-id [0] at some point.

However, this can be problematic as if it's already set by an external
entity it will get overwritten. In RHEL systems, systemd will invoke
ovs-ctl to start OVS and that will overwrite it to the hostname of the
machine.

For OVN this can have a big impact because if, for whatever reason the
hostname changes and the host gets restarted, ovn-controller won't
claim the ports back leaving the workloads unaccessible.

Also, it makes sense to not overwrite it as 1) it's an external_id,
so it will actually let external entities to configure it (unlike now),
and 2) it's optional. In the case that some systems were relying on
ovs-ctl to set the external-id for the first time (e.g onboarding
of a new hypervisor), this patch is not changing such behavior.

For more details, see discussion at [1].

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
[1] https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370813.html

Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
---
 utilities/ovs-ctl.in | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--

Comments

Han Zhou May 26, 2020, 6:30 a.m. UTC | #1
On Mon, May 25, 2020 at 8:28 AM Daniel Alvarez <dalvarez@redhat.com> wrote:
>
> ovs-ctl started to add the hostname as external-id [0] at some point.
>
> However, this can be problematic as if it's already set by an external
> entity it will get overwritten. In RHEL systems, systemd will invoke
> ovs-ctl to start OVS and that will overwrite it to the hostname of the
> machine.
>
> For OVN this can have a big impact because if, for whatever reason the
> hostname changes and the host gets restarted, ovn-controller won't
> claim the ports back leaving the workloads unaccessible.
>
> Also, it makes sense to not overwrite it as 1) it's an external_id,
> so it will actually let external entities to configure it (unlike now),
> and 2) it's optional. In the case that some systems were relying on
> ovs-ctl to set the external-id for the first time (e.g onboarding
> of a new hypervisor), this patch is not changing such behavior.
>
> For more details, see discussion at [1].
>
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370813.html
>
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> ---
>  utilities/ovs-ctl.in | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> index 8c5cd7032..9be9c9871 100644
> --- a/utilities/ovs-ctl.in
> +++ b/utilities/ovs-ctl.in
> @@ -43,7 +43,8 @@ set_hostname () {
>      else
>          hn="$(uname -n)"
>      fi
> -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> +    # Set the hostname if it wasn't set before
> +    ovs_vsctl add Open_vSwitch . external-ids hostname="$hn"
>  }
>
>  set_system_ids () {
> --
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-by: Han Zhou <hzhou@ovn.org>
Ilya Maximets Jan. 5, 2021, 8:17 p.m. UTC | #2
On 5/26/20 8:30 AM, Han Zhou wrote:
> On Mon, May 25, 2020 at 8:28 AM Daniel Alvarez <dalvarez@redhat.com> wrote:
>>
>> ovs-ctl started to add the hostname as external-id [0] at some point.
>>
>> However, this can be problematic as if it's already set by an external
>> entity it will get overwritten. In RHEL systems, systemd will invoke
>> ovs-ctl to start OVS and that will overwrite it to the hostname of the
>> machine.
>>
>> For OVN this can have a big impact because if, for whatever reason the
>> hostname changes and the host gets restarted, ovn-controller won't
>> claim the ports back leaving the workloads unaccessible.
>>
>> Also, it makes sense to not overwrite it as 1) it's an external_id,
>> so it will actually let external entities to configure it (unlike now),
>> and 2) it's optional. In the case that some systems were relying on
>> ovs-ctl to set the external-id for the first time (e.g onboarding
>> of a new hypervisor), this patch is not changing such behavior.
>>
>> For more details, see discussion at [1].
>>
>> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
>> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370813.html
>>
>> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
>> ---
>>  utilities/ovs-ctl.in | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
>> index 8c5cd7032..9be9c9871 100644
>> --- a/utilities/ovs-ctl.in
>> +++ b/utilities/ovs-ctl.in
>> @@ -43,7 +43,8 @@ set_hostname () {
>>      else
>>          hn="$(uname -n)"
>>      fi
>> -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
>> +    # Set the hostname if it wasn't set before
>> +    ovs_vsctl add Open_vSwitch . external-ids hostname="$hn"
>>  }
>>
>>  set_system_ids () {
>> --
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> Acked-by: Han Zhou <hzhou@ovn.org>

Thanks, Daniel and Han!
Sorry for so long delay.

Applied to master and down to 2.13.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index 8c5cd7032..9be9c9871 100644
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -43,7 +43,8 @@  set_hostname () {
     else
         hn="$(uname -n)"
     fi
-    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
+    # Set the hostname if it wasn't set before
+    ovs_vsctl add Open_vSwitch . external-ids hostname="$hn"
 }

 set_system_ids () {