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 |
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 |
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
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
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 >
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 --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])