diff mbox series

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

Message ID 20200520155207.9575-1-dalvarez@redhat.com
State New
Headers show
Series [ovs-dev] ovs-ctl: Don't set hostname as external-id | expand

Commit Message

Daniel Alvarez Sanchez May 20, 2020, 3:52 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 leave this untouched 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 of OVN, if the external-id doesn't
exist, it'll default to its hostname so nothing should get broken by
this change.

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

Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
---
 utilities/ovs-ctl.in | 12 ------------
 1 file changed, 12 deletions(-)

--

Comments

Flavio Fernandes May 20, 2020, 8:21 p.m. UTC | #1
Acked-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> 


> On May 20, 2020, at 11:52 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 leave this untouched 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 of OVN, if the external-id doesn't
> exist, it'll default to its hostname so nothing should get broken by
> this change.
> 
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
> 
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> ---
> utilities/ovs-ctl.in | 12 ------------
> 1 file changed, 12 deletions(-)
> 
> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> index 8c5cd7032..87fc4934f 100644
> --- a/utilities/ovs-ctl.in
> +++ b/utilities/ovs-ctl.in
> @@ -35,17 +35,6 @@ insert_mod_if_required () {
>     ovs_kmod_ctl insert
> }
> 
> -set_hostname () {
> -    # 'hostname -f' needs network connectivity to work.  So we should
> -    # call this only after ovs-vswitchd is running.
> -    if test X$FULL_HOSTNAME = Xyes; then
> -        hn="$(hostname -f)" || hn="$(uname -n)"
> -    else
> -        hn="$(uname -n)"
> -    fi
> -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> -}
> -
> set_system_ids () {
>     set ovs_vsctl set Open_vSwitch .
> 
> @@ -225,7 +214,6 @@ start_forwarding () {
>     if test X"$OVS_VSWITCHD" = Xyes; then
>         do_start_forwarding || return 1
>     fi
> -    set_hostname &
>     return 0
> }
> 
> --
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Han Zhou May 21, 2020, 7:21 a.m. UTC | #2
On Wed, May 20, 2020 at 8:52 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 leave this untouched 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 of OVN, if the external-id doesn't
> exist, it'll default to its hostname so nothing should get broken by
> this change.
>
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
>
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> ---
>  utilities/ovs-ctl.in | 12 ------------
>  1 file changed, 12 deletions(-)
>
> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> index 8c5cd7032..87fc4934f 100644
> --- a/utilities/ovs-ctl.in
> +++ b/utilities/ovs-ctl.in
> @@ -35,17 +35,6 @@ insert_mod_if_required () {
>      ovs_kmod_ctl insert
>  }
>
> -set_hostname () {
> -    # 'hostname -f' needs network connectivity to work.  So we should
> -    # call this only after ovs-vswitchd is running.
> -    if test X$FULL_HOSTNAME = Xyes; then
> -        hn="$(hostname -f)" || hn="$(uname -n)"
> -    else
> -        hn="$(uname -n)"
> -    fi
> -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> -}
> -
>  set_system_ids () {
>      set ovs_vsctl set Open_vSwitch .
>
> @@ -225,7 +214,6 @@ start_forwarding () {
>      if test X"$OVS_VSWITCHD" = Xyes; then
>          do_start_forwarding || return 1
>      fi
> -    set_hostname &
>      return 0
>  }
>
> --
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Hi Daniel,

I believe there are OpenStack based OVN users already depends on this. (And
we had also add the --no-full-hostname option so that it will set the short
name, so that it matches with openstack's "requested-chassis" setting which
uses nova compute node name.) For the scenarios that this behavior is not
desired, I think it is better to add a new option to override it, such as
"--no-hostname", so that existing environment won't get impacted. What do
you think?

Thanks,
Han
Daniel Alvarez Sanchez May 21, 2020, 10:18 a.m. UTC | #3
On Thu, May 21, 2020 at 9:22 AM Han Zhou <hzhou@ovn.org> wrote:
>
>
>
> On Wed, May 20, 2020 at 8:52 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 leave this untouched 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 of OVN, if the external-id doesn't
> > exist, it'll default to its hostname so nothing should get broken by
> > this change.
> >
> > [0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
> >
> > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> > ---
> >  utilities/ovs-ctl.in | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> > index 8c5cd7032..87fc4934f 100644
> > --- a/utilities/ovs-ctl.in
> > +++ b/utilities/ovs-ctl.in
> > @@ -35,17 +35,6 @@ insert_mod_if_required () {
> >      ovs_kmod_ctl insert
> >  }
> >
> > -set_hostname () {
> > -    # 'hostname -f' needs network connectivity to work.  So we should
> > -    # call this only after ovs-vswitchd is running.
> > -    if test X$FULL_HOSTNAME = Xyes; then
> > -        hn="$(hostname -f)" || hn="$(uname -n)"
> > -    else
> > -        hn="$(uname -n)"
> > -    fi
> > -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> > -}
> > -
> >  set_system_ids () {
> >      set ovs_vsctl set Open_vSwitch .
> >
> > @@ -225,7 +214,6 @@ start_forwarding () {
> >      if test X"$OVS_VSWITCHD" = Xyes; then
> >          do_start_forwarding || return 1
> >      fi
> > -    set_hostname &
> >      return 0
> >  }
> >
> > --
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Hi Daniel,
>
> I believe there are OpenStack based OVN users already depends on this. (And we had also add the --no-full-hostname option so that it will set the short name, so that it matches with openstack's "requested-chassis" setting which uses nova compute node name.) For the scenarios that this behavior is not desired, I think it is better to add a new option to override it, such as "--no-hostname", so that existing environment won't get impacted. What do you think?

Hi Han, thanks a lot for your feedback!
I thought of adding a --no-hostname option. However I still see that
this patch has value. Let me try to explain.

Existing OpenStack deployments will have their 'requested-chassis' set
to the current name of the hosts at the time the VMs were created.
This hostname may or may not match the machine hostname as it's a
string consumed from the external_ids, hence external and expected to
be set by CMS (for example, it's configurable by puppet at OpenStack
deployment time).
Now let's imagine you restart one node so ovs-ctl will overwrite
whatever the CMS relied upon and set it to the machine hostname. From
this time on, the workloads on that VM will not be claimed by OVN and
are left inaccessible until manual intervention happens.

I think it's fundamentally wrong for OVS (ovs-ctl in this case) to set
an external id itself as its default behavior (as they're meant for
external entities IIUC). Can you please elaborate on how existing
environments can get impacted by this change? Also keep in mind that
if the external_id is not set, ovn-controller is taking the hostname
of the machine as the default [0]. I might be overlooking something,
for sure.

Thanks again!
Daniel

[0] https://github.com/ovn-org/ovn/blob/master/controller/chassis.c#L133

>
> Thanks,
> Han
Han Zhou May 22, 2020, 12:17 a.m. UTC | #4
On Thu, May 21, 2020 at 3:19 AM Daniel Alvarez Sanchez <dalvarez@redhat.com>
wrote:
>
> On Thu, May 21, 2020 at 9:22 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> >
> >
> > On Wed, May 20, 2020 at 8:52 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 leave this untouched 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 of OVN, if the external-id doesn't
> > > exist, it'll default to its hostname so nothing should get broken by
> > > this change.
> > >
> > > [0]
https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
> > >
> > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> > > ---
> > >  utilities/ovs-ctl.in | 12 ------------
> > >  1 file changed, 12 deletions(-)
> > >
> > > diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> > > index 8c5cd7032..87fc4934f 100644
> > > --- a/utilities/ovs-ctl.in
> > > +++ b/utilities/ovs-ctl.in
> > > @@ -35,17 +35,6 @@ insert_mod_if_required () {
> > >      ovs_kmod_ctl insert
> > >  }
> > >
> > > -set_hostname () {
> > > -    # 'hostname -f' needs network connectivity to work.  So we should
> > > -    # call this only after ovs-vswitchd is running.
> > > -    if test X$FULL_HOSTNAME = Xyes; then
> > > -        hn="$(hostname -f)" || hn="$(uname -n)"
> > > -    else
> > > -        hn="$(uname -n)"
> > > -    fi
> > > -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> > > -}
> > > -
> > >  set_system_ids () {
> > >      set ovs_vsctl set Open_vSwitch .
> > >
> > > @@ -225,7 +214,6 @@ start_forwarding () {
> > >      if test X"$OVS_VSWITCHD" = Xyes; then
> > >          do_start_forwarding || return 1
> > >      fi
> > > -    set_hostname &
> > >      return 0
> > >  }
> > >
> > > --
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > Hi Daniel,
> >
> > I believe there are OpenStack based OVN users already depends on this.
(And we had also add the --no-full-hostname option so that it will set the
short name, so that it matches with openstack's "requested-chassis" setting
which uses nova compute node name.) For the scenarios that this behavior is
not desired, I think it is better to add a new option to override it, such
as "--no-hostname", so that existing environment won't get impacted. What
do you think?
>
> Hi Han, thanks a lot for your feedback!
> I thought of adding a --no-hostname option. However I still see that
> this patch has value. Let me try to explain.
>
> Existing OpenStack deployments will have their 'requested-chassis' set
> to the current name of the hosts at the time the VMs were created.
> This hostname may or may not match the machine hostname as it's a
> string consumed from the external_ids, hence external and expected to
> be set by CMS (for example, it's configurable by puppet at OpenStack
> deployment time).
> Now let's imagine you restart one node so ovs-ctl will overwrite
> whatever the CMS relied upon and set it to the machine hostname. From
> this time on, the workloads on that VM will not be claimed by OVN and
> are left inaccessible until manual intervention happens.
>
> I think it's fundamentally wrong for OVS (ovs-ctl in this case) to set
> an external id itself as its default behavior (as they're meant for
> external entities IIUC). Can you please elaborate on how existing
> environments can get impacted by this change? Also keep in mind that
> if the external_id is not set, ovn-controller is taking the hostname
> of the machine as the default [0]. I might be overlooking something,
> for sure.
>
> Thanks again!
> Daniel
>
> [0] https://github.com/ovn-org/ovn/blob/master/controller/chassis.c#L133
>
> >
> > Thanks,
> > Han
>

Hi Daniel, I understand the problem scenario you mentioned, and I agree
maybe it wasn't a good idea to add the external-ids:hostname by default. My
only concern is that it is already there and deployment may depend on it.
For example, if it is removed, a new HV onboarding will have no hostname,
and the --no-full-hostname setting doesn't take any effect anymore, so as
you mentioned the default behavior is to get the hostname by
ovn-controller. Since ovn-controller has no idea is the short or long name
should be used, it may be just the long name, which doesn't match with the
OpenStack nova convention that uses short name as the compute node name,
which is used as "requested-chassis" for the logical switch ports by
networking-ovn.

Maybe it is very rare situation that would get impacted by this change, so
I don't have any strong objection of removing it completely, but just to
point out that possible impact. If you believe removing it is the right
way, please also remove the --no-full-hostname option.

Thanks,
Han
Daniel Alvarez Sanchez May 22, 2020, 11:02 a.m. UTC | #5
On Fri, May 22, 2020 at 2:17 AM Han Zhou <hzhou@ovn.org> wrote:
>
>
>
> On Thu, May 21, 2020 at 3:19 AM Daniel Alvarez Sanchez <dalvarez@redhat.com> wrote:
> >
> > On Thu, May 21, 2020 at 9:22 AM Han Zhou <hzhou@ovn.org> wrote:
> > >
> > >
> > >
> > > On Wed, May 20, 2020 at 8:52 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 leave this untouched 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 of OVN, if the external-id doesn't
> > > > exist, it'll default to its hostname so nothing should get broken by
> > > > this change.
> > > >
> > > > [0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
> > > >
> > > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> > > > ---
> > > >  utilities/ovs-ctl.in | 12 ------------
> > > >  1 file changed, 12 deletions(-)
> > > >
> > > > diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> > > > index 8c5cd7032..87fc4934f 100644
> > > > --- a/utilities/ovs-ctl.in
> > > > +++ b/utilities/ovs-ctl.in
> > > > @@ -35,17 +35,6 @@ insert_mod_if_required () {
> > > >      ovs_kmod_ctl insert
> > > >  }
> > > >
> > > > -set_hostname () {
> > > > -    # 'hostname -f' needs network connectivity to work.  So we should
> > > > -    # call this only after ovs-vswitchd is running.
> > > > -    if test X$FULL_HOSTNAME = Xyes; then
> > > > -        hn="$(hostname -f)" || hn="$(uname -n)"
> > > > -    else
> > > > -        hn="$(uname -n)"
> > > > -    fi
> > > > -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> > > > -}
> > > > -
> > > >  set_system_ids () {
> > > >      set ovs_vsctl set Open_vSwitch .
> > > >
> > > > @@ -225,7 +214,6 @@ start_forwarding () {
> > > >      if test X"$OVS_VSWITCHD" = Xyes; then
> > > >          do_start_forwarding || return 1
> > > >      fi
> > > > -    set_hostname &
> > > >      return 0
> > > >  }
> > > >
> > > > --
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > > Hi Daniel,
> > >
> > > I believe there are OpenStack based OVN users already depends on this. (And we had also add the --no-full-hostname option so that it will set the short name, so that it matches with openstack's "requested-chassis" setting which uses nova compute node name.) For the scenarios that this behavior is not desired, I think it is better to add a new option to override it, such as "--no-hostname", so that existing environment won't get impacted. What do you think?
> >
> > Hi Han, thanks a lot for your feedback!
> > I thought of adding a --no-hostname option. However I still see that
> > this patch has value. Let me try to explain.
> >
> > Existing OpenStack deployments will have their 'requested-chassis' set
> > to the current name of the hosts at the time the VMs were created.
> > This hostname may or may not match the machine hostname as it's a
> > string consumed from the external_ids, hence external and expected to
> > be set by CMS (for example, it's configurable by puppet at OpenStack
> > deployment time).
> > Now let's imagine you restart one node so ovs-ctl will overwrite
> > whatever the CMS relied upon and set it to the machine hostname. From
> > this time on, the workloads on that VM will not be claimed by OVN and
> > are left inaccessible until manual intervention happens.
> >
> > I think it's fundamentally wrong for OVS (ovs-ctl in this case) to set
> > an external id itself as its default behavior (as they're meant for
> > external entities IIUC). Can you please elaborate on how existing
> > environments can get impacted by this change? Also keep in mind that
> > if the external_id is not set, ovn-controller is taking the hostname
> > of the machine as the default [0]. I might be overlooking something,
> > for sure.
> >
> > Thanks again!
> > Daniel
> >
> > [0] https://github.com/ovn-org/ovn/blob/master/controller/chassis.c#L133
> >
> > >
> > > Thanks,
> > > Han
> >
>
> Hi Daniel, I understand the problem scenario you mentioned, and I agree maybe it wasn't a good idea to add the external-ids:hostname by default. My only concern is that it is already there and deployment may depend on it. For example, if it is removed, a new HV onboarding will have no hostname, and the --no-full-hostname setting doesn't take any effect anymore, so as you mentioned the default behavior is to get the hostname by ovn-controller. Since ovn-controller has no idea is the short or long name should be used, it may be just the long name, which doesn't match with the OpenStack nova convention that uses short name as the compute node name, which is used as "requested-chassis" for the logical switch ports by networking-ovn.
>
> Maybe it is very rare situation that would get impacted by this change, so I don't have any strong objection of removing it completely, but just to point out that possible impact. If you believe removing it is the right way, please also remove the --no-full-hostname option.

Oh I see now. Thanks for clarifying :)

We can wait for other opinions as well. In the case that we're against
my approach here, please let me know if this would fit better (also,
please note that I'd be changing the systemd files so we'll be
impacted anyways on RHEL systems, so I'd be leaning more towards
removing it completely):

diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
index ff43dae96..054bd669a 100644
--- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
+++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
@@ -21,12 +21,12 @@ ExecStartPre=-/bin/sh -c '/usr/bin/chown
:$${OVS_USER_ID##*:} /dev/hugepages'
 ExecStartPre=-/usr/bin/chmod 0775 /dev/hugepages
 @end_dpdk@
 ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
-          --no-ovsdb-server --no-monitor --system-id=random \
+          --no-ovsdb-server --no-monitor --system-id=random --no-hostname \
           ${OVS_USER_OPT} \
           start $OPTIONS
 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 \
+          --no-monitor --system-id=random --no-hostname \
           ${OVS_USER_OPT} \
           restart $OPTIONS
 TimeoutSec=300
diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
b/rhel/usr_lib_systemd_system_ovsdb-server.service
index 4c170c09b..562b2c167 100644
--- a/rhel/usr_lib_systemd_system_ovsdb-server.service
+++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
@@ -16,10 +16,10 @@ ExecStartPre=/bin/sh -c 'rm -f
/run/openvswitch.useropts; /usr/bin/echo "OVS_USE
 ExecStartPre=/bin/sh -c 'if [ "$${OVS_USER_ID/:*/}" != "root" ]; then
/usr/bin/echo "OVS_USER_OPT=--ovs-user=${OVS_USER_ID}" >>
/run/openvswitch.useropts; fi'
 EnvironmentFile=-/run/openvswitch.useropts
 ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
-          --no-ovs-vswitchd --no-monitor --system-id=random \
+          --no-ovs-vswitchd --no-monitor --system-id=random --no-hostname \
           ${OVS_USER_OPT} \
           start $OPTIONS
 ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop
 ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd \
            ${OVS_USER_OPT} \
-           --no-monitor restart $OPTIONS
+           --no-monitor --no-hostname restart $OPTIONS
diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index 8c5cd7032..9ad5d84a6 100644
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -36,6 +36,10 @@ insert_mod_if_required () {
 }

 set_hostname () {
+    if test X"$HOSTNAME" = Xno; then
+        return 0
+    fi
+
     # 'hostname -f' needs network connectivity to work.  So we should
     # call this only after ovs-vswitchd is running.
     if test X$FULL_HOSTNAME = Xyes; then
@@ -311,6 +315,7 @@ enable_protocol () {
 set_defaults () {
     SYSTEM_ID=

+    HOSTNAME=yes
     FULL_HOSTNAME=yes

     DELETE_BRIDGES=no
@@ -404,6 +409,7 @@ Less important options for "start", "restart" and
"force-reload-kmod":
   --no-mlockall                  do not lock all of ovs-vswitchd into memory
   --ovsdb-server-priority=NICE   set ovsdb-server's niceness
(default: $OVSDB_SERVER_PRIORITY)
   --ovs-vswitchd-priority=NICE   set ovs-vswitchd's niceness
(default: $OVS_VSWITCHD_PRIORITY)
+  --no-hostname                  do not set external-id:hostname
   --no-full-hostname             set short hostname instead of full hostname

 Debugging options for "start", "restart" and "force-reload-kmod":


Please, let me know what you think.
Thanks a lot again
Daniel

>
> Thanks,
> Han
Terry Wilson May 22, 2020, 9:27 p.m. UTC | #6
On Wed, May 20, 2020 at 10:52 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.
>
If the problem is just ovs-ctl *overwriting* an existing entity then can we
just change

-    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
>

to ovs_vsctl add Open_vSwitch . external_ids:hostname="$hn"

since add doesn't overwrite existing values if the key is set[1].

[1] From man ovs-vsctl
      [--if-exists] add table record column [key=]value...
              Adds the specified value or key-value pair to column in
record in table.  If column is a map, then key is required, otherwise it is
 prohib‐
              ited.  If key already exists in a map column, then the
current value is not replaced (use the set command to replace an existing
value).

              Without --if-exists, it is an error if record does not
exist.  With --if-exists, this command does nothing if record does not
exist.

Terry
Daniel Alvarez Sanchez May 23, 2020, 7:06 a.m. UTC | #7
Thanks a lot Terry!


> On 22 May 2020, at 23:28, Terry Wilson <twilson@redhat.com> wrote:
> 
> 
> 
> 
>> On Wed, May 20, 2020 at 10:52 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.
> If the problem is just ovs-ctl *overwriting* an existing entity then can we just change
> 
>> -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> 
> to ovs_vsctl add Open_vSwitch . external_ids:hostname="$hn" 
> 
> since add doesn't overwrite existing values if the key is set[1].

This sounds great to me!
Han, it looks like Terry’s suggestion would work for the HV onboarding scenario that you mentioned and also fit in the model where an external entity decides what name to give to a particular HV. What do you think?
> 
> [1] From man ovs-vsctl
>       [--if-exists] add table record column [key=]value...
>               Adds the specified value or key-value pair to column in record in table.  If column is a map, then key is required, otherwise it is  prohib‐
>               ited.  If key already exists in a map column, then the current value is not replaced (use the set command to replace an existing value).
> 
>               Without --if-exists, it is an error if record does not exist.  With --if-exists, this command does nothing if record does not exist.
> 
> Terry
Han Zhou May 23, 2020, 6:37 p.m. UTC | #8
On Sat, May 23, 2020 at 12:06 AM Daniel Alvarez <dalvarez@redhat.com> wrote:
>
>
> Thanks a lot Terry!
>
>
> > On 22 May 2020, at 23:28, Terry Wilson <twilson@redhat.com> wrote:
> >
> > 
> >
> >
> >> On Wed, May 20, 2020 at 10:52 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.
> > If the problem is just ovs-ctl *overwriting* an existing entity then
can we just change
> >
> >> -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> >
> > to ovs_vsctl add Open_vSwitch . external_ids:hostname="$hn"
> >
> > since add doesn't overwrite existing values if the key is set[1].
>
> This sounds great to me!
> Han, it looks like Terry’s suggestion would work for the HV onboarding
scenario that you mentioned and also fit in the model where an external
entity decides what name to give to a particular HV. What do you think?

Yes, SGTM, too :)

> >
> > [1] From man ovs-vsctl
> >       [--if-exists] add table record column [key=]value...
> >               Adds the specified value or key-value pair to column in
record in table.  If column is a map, then key is required, otherwise it is
 prohib‐
> >               ited.  If key already exists in a map column, then the
current value is not replaced (use the set command to replace an existing
value).
> >
> >               Without --if-exists, it is an error if record does not
exist.  With --if-exists, this command does nothing if record does not
exist.
> >
> > Terry
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Daniel Alvarez Sanchez June 2, 2020, 11:05 a.m. UTC | #9
Thanks a lot!
I proposed the new approach at:

https://patchwork.ozlabs.org/project/openvswitch/patch/20200525152821.19838-1-dalvarez@redhat.com/


On Sat, May 23, 2020 at 8:38 PM Han Zhou <zhouhan@gmail.com> wrote:
>
>
>
> On Sat, May 23, 2020 at 12:06 AM Daniel Alvarez <dalvarez@redhat.com> wrote:
> >
> >
> > Thanks a lot Terry!
> >
> >
> > > On 22 May 2020, at 23:28, Terry Wilson <twilson@redhat.com> wrote:
> > >
> > > 
> > >
> > >
> > >> On Wed, May 20, 2020 at 10:52 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.
> > > If the problem is just ovs-ctl *overwriting* an existing entity then can we just change
> > >
> > >> -    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> > >
> > > to ovs_vsctl add Open_vSwitch . external_ids:hostname="$hn"
> > >
> > > since add doesn't overwrite existing values if the key is set[1].
> >
> > This sounds great to me!
> > Han, it looks like Terry’s suggestion would work for the HV onboarding scenario that you mentioned and also fit in the model where an external entity decides what name to give to a particular HV. What do you think?
>
> Yes, SGTM, too :)
>
> > >
> > > [1] From man ovs-vsctl
> > >       [--if-exists] add table record column [key=]value...
> > >               Adds the specified value or key-value pair to column in record in table.  If column is a map, then key is required, otherwise it is  prohib‐
> > >               ited.  If key already exists in a map column, then the current value is not replaced (use the set command to replace an existing value).
> > >
> > >               Without --if-exists, it is an error if record does not exist.  With --if-exists, this command does nothing if record does not exist.
> > >
> > > Terry
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index 8c5cd7032..87fc4934f 100644
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -35,17 +35,6 @@  insert_mod_if_required () {
     ovs_kmod_ctl insert
 }

-set_hostname () {
-    # 'hostname -f' needs network connectivity to work.  So we should
-    # call this only after ovs-vswitchd is running.
-    if test X$FULL_HOSTNAME = Xyes; then
-        hn="$(hostname -f)" || hn="$(uname -n)"
-    else
-        hn="$(uname -n)"
-    fi
-    ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
-}
-
 set_system_ids () {
     set ovs_vsctl set Open_vSwitch .

@@ -225,7 +214,6 @@  start_forwarding () {
     if test X"$OVS_VSWITCHD" = Xyes; then
         do_start_forwarding || return 1
     fi
-    set_hostname &
     return 0
 }