| Message ID | 20220124113008.20761-1-odivlad@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [ovs-dev] 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 | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 1/24/22 12:30, 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. > > 0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998 > > Fixes: 1c360bbd911cab9fadd6df8cd528d992ffa7a998 (Fix basic multicast flows for vxlan (non-vtep) tunnels) > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- Looks good to me, thanks! Acked-by: Dumitru Ceara <dceara@redhat.com> One nit though: it would be nice to have a test for this, e.g, list the chassis check that is-vtep is set, remove is-vtep, wait until ovn-controller-vtep readds it. What do you think? Regards, Dumitru
On 1/28/22 18:04, Dumitru Ceara wrote: > On 1/24/22 12:30, 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. >> >> 0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998 >> >> Fixes: 1c360bbd911cab9fadd6df8cd528d992ffa7a998 (Fix basic multicast flows for vxlan (non-vtep) tunnels) Another tiny nit, this should be: Fixes: 1c360bbd911c ("Fix basic multicast flows for vxlan (non-vtep) tunnels") Generated with: git log -1 --pretty=format:"Fixes: %h (\"%s\")" --abbrev=12 1c360bbd911cab9fadd6df8cd528d992ffa7a998 >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >> --- > > Looks good to me, thanks! > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > One nit though: it would be nice to have a test for this, e.g, list the > chassis check that is-vtep is set, remove is-vtep, wait until > ovn-controller-vtep readds it. What do you think? > > Regards, > Dumitru >
Hi Dumitru, thanks for the review. I’ve addressed requested changes in v2: https://patchwork.ozlabs.org/project/ovn/patch/20220131173457.64391-1-odivlad@gmail.com/ Regards, Vladislav Odintsov > On 28 Jan 2022, at 20:06, Dumitru Ceara <dceara@redhat.com> wrote: > > On 1/28/22 18:04, Dumitru Ceara wrote: >> On 1/24/22 12:30, 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. >>> >>> 0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998 >>> >>> Fixes: 1c360bbd911cab9fadd6df8cd528d992ffa7a998 (Fix basic multicast flows for vxlan (non-vtep) tunnels) > > Another tiny nit, this should be: > > Fixes: 1c360bbd911c ("Fix basic multicast flows for vxlan (non-vtep) tunnels") > > Generated with: > > git log -1 --pretty=format:"Fixes: %h (\"%s\")" --abbrev=12 1c360bbd911cab9fadd6df8cd528d992ffa7a998 > >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >>> --- >> >> Looks good to me, thanks! >> >> Acked-by: Dumitru Ceara <dceara@redhat.com> >> >> One nit though: it would be nice to have a test for this, e.g, list the >> chassis check that is-vtep is set, remove is-vtep, wait until >> ovn-controller-vtep readds it. What do you think? >> >> 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, "
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. 0: https://github.com/ovn-org/ovn/commit/1c360bbd911cab9fadd6df8cd528d992ffa7a998 Fixes: 1c360bbd911cab9fadd6df8cd528d992ffa7a998 (Fix basic multicast flows for vxlan (non-vtep) tunnels) Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- After this patch is accepted to main branch, it should be considered to be backported to branch-21.12, as the problem is shown originally in that version. --- controller-vtep/gateway.c | 5 +++++ 1 file changed, 5 insertions(+)