diff mbox series

[ovs-dev,v2] vtep: set is-vtep to chassis's other_config if absent

Message ID 20220131173457.64391-1-odivlad@gmail.com
State Accepted
Headers show
Series [ovs-dev,v2] vtep: set is-vtep to chassis's other_config if absent | expand

Checks

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

Commit Message

Vladislav Odintsov Jan. 31, 2022, 5:34 p.m. UTC 
After commit [0] the mandatory vtep chassis's option 'is-vtep' has appeared.
The upgrade scenario for ovn-controller-vtep was not supported:
'is-vtep' option was set only on vtep chassis creation. If chassis was
created prior to a new codebase, it was left intact and HW VTEP connectivity
was broken.
This commit fixes such scenario and now 'is-vtep' is set for vtep chassis
other_config if was not set yet.
This patch also adds a related test.

0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998

Fixes: 1c360bbd911c ("Fix basic multicast flows for vxlan (non-vtep) tunnels")
Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
---
 controller-vtep/gateway.c    | 5 +++++
 tests/ovn-controller-vtep.at | 8 ++++++++
 2 files changed, 13 insertions(+)

Comments

Dumitru Ceara Feb. 1, 2022, 8:33 a.m. UTC | #1 
On 1/31/22 18:34, Vladislav Odintsov wrote:
> After commit [0] the mandatory vtep chassis's option 'is-vtep' has appeared.
> The upgrade scenario for ovn-controller-vtep was not supported:
> 'is-vtep' option was set only on vtep chassis creation. If chassis was
> created prior to a new codebase, it was left intact and HW VTEP connectivity
> was broken.
> This commit fixes such scenario and now 'is-vtep' is set for vtep chassis
> other_config if was not set yet.
> This patch also adds a related test.
> 
> 0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998
> 
> Fixes: 1c360bbd911c ("Fix basic multicast flows for vxlan (non-vtep) tunnels")
> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
> ---

Thanks for the follow up, still looks good to me!

>  controller-vtep/gateway.c    | 5 +++++
>  tests/ovn-controller-vtep.at | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/controller-vtep/gateway.c b/controller-vtep/gateway.c
> index 288772dc4..9fbfc0337 100644
> --- a/controller-vtep/gateway.c
> +++ b/controller-vtep/gateway.c
> @@ -22,6 +22,7 @@
>  #include "lib/util.h"
>  #include "openvswitch/vlog.h"
>  #include "lib/ovn-sb-idl.h"
> +#include "smap.h"
>  #include "vtep/vtep-idl.h"
>  #include "ovn-controller-vtep.h"
>  
> @@ -117,6 +118,10 @@ revalidate_gateway(struct controller_vtep_ctx *ctx)
>                                                                    "false");
>                  sbrec_encap_set_options(chassis_rec->encaps[0], &options);
>              }
> +            if (!smap_get_bool(&chassis_rec->other_config, "is-vtep", false)) {
> +                const struct smap oc = SMAP_CONST1(&oc, "is-vtep", "true");
> +                sbrec_chassis_set_other_config(chassis_rec, &oc);
> +            }
>          } else {
>              if (gw_node) {
>                  VLOG_WARN("Chassis for VTEP physical switch (%s) disappears, "
> diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
> index b76fa5cef..08e1d13e7 100644
> --- a/tests/ovn-controller-vtep.at
> +++ b/tests/ovn-controller-vtep.at
> @@ -102,6 +102,14 @@ Chassis br-vtep
>          options: {csum="false"}
>  ])
>  
> +# checks is-vtep option is in place, remove, check again
> +AT_CHECK([ovn-sbctl get chassis br-vtep other_config:is-vtep], [0], [dnl
> +"true"
> +])
> +
> +check ovn-sbctl remove chassis br-vtep other_config is-vtep
> +OVS_WAIT_UNTIL([ovn-sbctl get chassis br-vtep other_config:is-vtep])
> +

I was tempted to comment to use fetch_column/wait_column here but we
don't do that in most of the ovn-controller-vtep.at file.  So, I think
it's fine, we might as well be consistent and tackle this change at a
later time.

>  # deletes the chassis via ovn-sbctl and check that it is readded back
>  # with the log.
>  AT_CHECK([ovn-sbctl chassis-del br-vtep])

Regards,
Dumitru
Odintsov Vladislav Feb. 1, 2022, 8:46 a.m. UTC | #2 
Thanks Dumitru.

I agree, that chaning one line in a file doesn’t help much :)
This work should be done at the whole file someday.

Regards,
Vladislav Odintsov

On 1 Feb 2022, at 11:33, Dumitru Ceara <dceara@redhat.com<mailto:dceara@redhat.com>> wrote:

On 1/31/22 18:34, Vladislav Odintsov wrote:
After commit [0] the mandatory vtep chassis's option 'is-vtep' has appeared.
The upgrade scenario for ovn-controller-vtep was not supported:
'is-vtep' option was set only on vtep chassis creation. If chassis was
created prior to a new codebase, it was left intact and HW VTEP connectivity
was broken.
This commit fixes such scenario and now 'is-vtep' is set for vtep chassis
other_config if was not set yet.
This patch also adds a related test.

0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998

Fixes: 1c360bbd911c ("Fix basic multicast flows for vxlan (non-vtep) tunnels")
Signed-off-by: Vladislav Odintsov <odivlad@gmail.com<mailto:odivlad@gmail.com>>
Acked-by: Dumitru Ceara <dceara@redhat.com<mailto:dceara@redhat.com>>
---

Thanks for the follow up, still looks good to me!

controller-vtep/gateway.c    | 5 +++++
tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at> | 8 ++++++++
2 files changed, 13 insertions(+)

diff --git a/controller-vtep/gateway.c b/controller-vtep/gateway.c
index 288772dc4..9fbfc0337 100644
--- a/controller-vtep/gateway.c
+++ b/controller-vtep/gateway.c
@@ -22,6 +22,7 @@
#include "lib/util.h"
#include "openvswitch/vlog.h"
#include "lib/ovn-sb-idl.h"
+#include "smap.h"
#include "vtep/vtep-idl.h"
#include "ovn-controller-vtep.h"

@@ -117,6 +118,10 @@ revalidate_gateway(struct controller_vtep_ctx *ctx)
                                                                  "false");
                sbrec_encap_set_options(chassis_rec->encaps[0], &options);
            }
+            if (!smap_get_bool(&chassis_rec->other_config, "is-vtep", false)) {
+                const struct smap oc = SMAP_CONST1(&oc, "is-vtep", "true");
+                sbrec_chassis_set_other_config(chassis_rec, &oc);
+            }
        } else {
            if (gw_node) {
                VLOG_WARN("Chassis for VTEP physical switch (%s) disappears, "
diff --git a/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at> b/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at>
index b76fa5cef..08e1d13e7 100644
--- a/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at>
+++ b/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at>
@@ -102,6 +102,14 @@ Chassis br-vtep
        options: {csum="false"}
])

+# checks is-vtep option is in place, remove, check again
+AT_CHECK([ovn-sbctl get chassis br-vtep other_config:is-vtep], [0], [dnl
+"true"
+])
+
+check ovn-sbctl remove chassis br-vtep other_config is-vtep
+OVS_WAIT_UNTIL([ovn-sbctl get chassis br-vtep other_config:is-vtep])
+

I was tempted to comment to use fetch_column/wait_column here but we
don't do that in most of the ovn-controller-vtep.at<http://ovn-controller-vtep.at/> file.  So, I think
it's fine, we might as well be consistent and tackle this change at a
later time.

# deletes the chassis via ovn-sbctl and check that it is readded back
# with the log.
AT_CHECK([ovn-sbctl chassis-del br-vtep])

Regards,
Dumitru
Mark Michelson Feb. 1, 2022, 4:23 p.m. UTC | #3 
Thanks, folks.

I pushed the changes to main and to branch-21.12.

On 2/1/22 03:46, Odintsov Vladislav wrote:
> Thanks Dumitru.
> 
> I agree, that chaning one line in a file doesn’t help much :)
> This work should be done at the whole file someday.
> 
> Regards,
> Vladislav Odintsov
> 
> On 1 Feb 2022, at 11:33, Dumitru Ceara <dceara@redhat.com<mailto:dceara@redhat.com>> wrote:
> 
> On 1/31/22 18:34, Vladislav Odintsov wrote:
> After commit [0] the mandatory vtep chassis's option 'is-vtep' has appeared.
> The upgrade scenario for ovn-controller-vtep was not supported:
> 'is-vtep' option was set only on vtep chassis creation. If chassis was
> created prior to a new codebase, it was left intact and HW VTEP connectivity
> was broken.
> This commit fixes such scenario and now 'is-vtep' is set for vtep chassis
> other_config if was not set yet.
> This patch also adds a related test.
> 
> 0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998
> 
> Fixes: 1c360bbd911c ("Fix basic multicast flows for vxlan (non-vtep) tunnels")
> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com<mailto:odivlad@gmail.com>>
> Acked-by: Dumitru Ceara <dceara@redhat.com<mailto:dceara@redhat.com>>
> ---
> 
> Thanks for the follow up, still looks good to me!
> 
> controller-vtep/gateway.c    | 5 +++++
> tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at> | 8 ++++++++
> 2 files changed, 13 insertions(+)
> 
> diff --git a/controller-vtep/gateway.c b/controller-vtep/gateway.c
> index 288772dc4..9fbfc0337 100644
> --- a/controller-vtep/gateway.c
> +++ b/controller-vtep/gateway.c
> @@ -22,6 +22,7 @@
> #include "lib/util.h"
> #include "openvswitch/vlog.h"
> #include "lib/ovn-sb-idl.h"
> +#include "smap.h"
> #include "vtep/vtep-idl.h"
> #include "ovn-controller-vtep.h"
> 
> @@ -117,6 +118,10 @@ revalidate_gateway(struct controller_vtep_ctx *ctx)
>                                                                    "false");
>                  sbrec_encap_set_options(chassis_rec->encaps[0], &options);
>              }
> +            if (!smap_get_bool(&chassis_rec->other_config, "is-vtep", false)) {
> +                const struct smap oc = SMAP_CONST1(&oc, "is-vtep", "true");
> +                sbrec_chassis_set_other_config(chassis_rec, &oc);
> +            }
>          } else {
>              if (gw_node) {
>                  VLOG_WARN("Chassis for VTEP physical switch (%s) disappears, "
> diff --git a/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at> b/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at>
> index b76fa5cef..08e1d13e7 100644
> --- a/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at>
> +++ b/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at>
> @@ -102,6 +102,14 @@ Chassis br-vtep
>          options: {csum="false"}
> ])
> 
> +# checks is-vtep option is in place, remove, check again
> +AT_CHECK([ovn-sbctl get chassis br-vtep other_config:is-vtep], [0], [dnl
> +"true"
> +])
> +
> +check ovn-sbctl remove chassis br-vtep other_config is-vtep
> +OVS_WAIT_UNTIL([ovn-sbctl get chassis br-vtep other_config:is-vtep])
> +
> 
> I was tempted to comment to use fetch_column/wait_column here but we
> don't do that in most of the ovn-controller-vtep.at<http://ovn-controller-vtep.at/> file.  So, I think
> it's fine, we might as well be consistent and tackle this change at a
> later time.
> 
> # deletes the chassis via ovn-sbctl and check that it is readded back
> # with the log.
> AT_CHECK([ovn-sbctl chassis-del br-vtep])
> 
> Regards,
> Dumitru
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Odintsov Vladislav Feb. 1, 2022, 4:26 p.m. UTC | #4 
Thanks Mark.

Regards,
Vladislav Odintsov

On 1 Feb 2022, at 19:23, Mark Michelson <mmichels@redhat.com<mailto:mmichels@redhat.com>> wrote:

Thanks, folks.

I pushed the changes to main and to branch-21.12.

On 2/1/22 03:46, Odintsov Vladislav wrote:
Thanks Dumitru.
I agree, that chaning one line in a file doesn’t help much :)
This work should be done at the whole file someday.
Regards,
Vladislav Odintsov
On 1 Feb 2022, at 11:33, Dumitru Ceara <dceara@redhat.com<mailto:dceara@redhat.com><mailto:dceara@redhat.com>> wrote:
On 1/31/22 18:34, Vladislav Odintsov wrote:
After commit [0] the mandatory vtep chassis's option 'is-vtep' has appeared.
The upgrade scenario for ovn-controller-vtep was not supported:
'is-vtep' option was set only on vtep chassis creation. If chassis was
created prior to a new codebase, it was left intact and HW VTEP connectivity
was broken.
This commit fixes such scenario and now 'is-vtep' is set for vtep chassis
other_config if was not set yet.
This patch also adds a related test.
0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998
Fixes: 1c360bbd911c ("Fix basic multicast flows for vxlan (non-vtep) tunnels")
Signed-off-by: Vladislav Odintsov <odivlad@gmail.com<mailto:odivlad@gmail.com><mailto:odivlad@gmail.com>>
Acked-by: Dumitru Ceara <dceara@redhat.com<mailto:dceara@redhat.com><mailto:dceara@redhat.com>>
---
Thanks for the follow up, still looks good to me!
controller-vtep/gateway.c    | 5 +++++
tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at><http://ovn-controller-vtep.at> | 8 ++++++++
2 files changed, 13 insertions(+)
diff --git a/controller-vtep/gateway.c b/controller-vtep/gateway.c
index 288772dc4..9fbfc0337 100644
--- a/controller-vtep/gateway.c
+++ b/controller-vtep/gateway.c
@@ -22,6 +22,7 @@
#include "lib/util.h"
#include "openvswitch/vlog.h"
#include "lib/ovn-sb-idl.h"
+#include "smap.h"
#include "vtep/vtep-idl.h"
#include "ovn-controller-vtep.h"
@@ -117,6 +118,10 @@ revalidate_gateway(struct controller_vtep_ctx *ctx)
                                                                  "false");
                sbrec_encap_set_options(chassis_rec->encaps[0], &options);
            }
+            if (!smap_get_bool(&chassis_rec->other_config, "is-vtep", false)) {
+                const struct smap oc = SMAP_CONST1(&oc, "is-vtep", "true");
+                sbrec_chassis_set_other_config(chassis_rec, &oc);
+            }
        } else {
            if (gw_node) {
                VLOG_WARN("Chassis for VTEP physical switch (%s) disappears, "
diff --git a/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at><http://ovn-controller-vtep.at> b/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at><http://ovn-controller-vtep.at>
index b76fa5cef..08e1d13e7 100644
--- a/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at><http://ovn-controller-vtep.at>
+++ b/tests/ovn-controller-vtep.at<http://ovn-controller-vtep.at><http://ovn-controller-vtep.at>
@@ -102,6 +102,14 @@ Chassis br-vtep
        options: {csum="false"}
])
+# checks is-vtep option is in place, remove, check again
+AT_CHECK([ovn-sbctl get chassis br-vtep other_config:is-vtep], [0], [dnl
+"true"
+])
+
+check ovn-sbctl remove chassis br-vtep other_config is-vtep
+OVS_WAIT_UNTIL([ovn-sbctl get chassis br-vtep other_config:is-vtep])
+
I was tempted to comment to use fetch_column/wait_column here but we
don't do that in most of the ovn-controller-vtep.at<http://ovn-controller-vtep.at><http://ovn-controller-vtep.at/> file.  So, I think
it's fine, we might as well be consistent and tackle this change at a
later time.
# deletes the chassis via ovn-sbctl and check that it is readded back
# with the log.
AT_CHECK([ovn-sbctl chassis-del br-vtep])
Regards,
Dumitru
diff mbox series

Patch

diff --git a/controller-vtep/gateway.c b/controller-vtep/gateway.c
index 288772dc4..9fbfc0337 100644
--- a/controller-vtep/gateway.c
+++ b/controller-vtep/gateway.c
@@ -22,6 +22,7 @@ 
 #include "lib/util.h"
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
+#include "smap.h"
 #include "vtep/vtep-idl.h"
 #include "ovn-controller-vtep.h"
 
@@ -117,6 +118,10 @@  revalidate_gateway(struct controller_vtep_ctx *ctx)
                                                                   "false");
                 sbrec_encap_set_options(chassis_rec->encaps[0], &options);
             }
+            if (!smap_get_bool(&chassis_rec->other_config, "is-vtep", false)) {
+                const struct smap oc = SMAP_CONST1(&oc, "is-vtep", "true");
+                sbrec_chassis_set_other_config(chassis_rec, &oc);
+            }
         } else {
             if (gw_node) {
                 VLOG_WARN("Chassis for VTEP physical switch (%s) disappears, "
diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
index b76fa5cef..08e1d13e7 100644
--- a/tests/ovn-controller-vtep.at
+++ b/tests/ovn-controller-vtep.at
@@ -102,6 +102,14 @@  Chassis br-vtep
         options: {csum="false"}
 ])
 
+# checks is-vtep option is in place, remove, check again
+AT_CHECK([ovn-sbctl get chassis br-vtep other_config:is-vtep], [0], [dnl
+"true"
+])
+
+check ovn-sbctl remove chassis br-vtep other_config is-vtep
+OVS_WAIT_UNTIL([ovn-sbctl get chassis br-vtep other_config:is-vtep])
+
 # deletes the chassis via ovn-sbctl and check that it is readded back
 # with the log.
 AT_CHECK([ovn-sbctl chassis-del br-vtep])