| Message ID | 20210930193507.716589-1-frode.nordahl@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] controller: Allocate zone ids for localport port bindings. | 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 Thu, Sep 30, 2021 at 3:35 PM Frode Nordahl <frode.nordahl@canonical.com> wrote: > > Commit d4bca93 limited ct zone allocation to VIF-ports only, this > breaks OpenStack's use of localport for providing metadata to > instances. > > This patch adds lports of type 'localport' to the list of lport > types to allocate zone ids for. > > No test case is added as I am unable to make the error condition > occur reliably in the sandbox test environment, however I have > confirmed that the error condition is there on an unmodified OVN > installation and that the behavior changes by adding this patch. > Both for a fresh deployment where a modified package is slipped in > prior to first start of OVN and comparison of patched and > non-patched OVN after reboot of a deployed node. > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2021-September/051473.html > Fixes: d4bca93 ("controller: Don't allocate zone ids for non-VIF port bindings. ") > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> Thanks a lot for fixing this issue and thanks to Benjamin Reichel for reporting it. I was able to reproduce locally with the below changes to the test case - "ovn-controller ct zone I-P handling" So I added this test case to this patch and applied to the main branch and branch-21.09. Also updated the commit message accordingly. Hope that's fine with you. Thanks Numan ---diff --git a/tests/ovn.at b/tests/ovn.at index 172b5c713..fc8f31d06 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -28481,6 +28481,9 @@ check as hv1 ovs-vsctl add-port br-int vif11 \ ovn-nbctl ls-add sw0 ovn-nbctl lsp-add sw0 sw0-port1 ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 10.0.0.3" +check ovn-nbctl lsp-add sw0 sw0-port2 +check ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:01 10.0.0.4" +check ovn-nbctl lsp-set-type sw0-port2 localport ovn-nbctl lr-add lr0 ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 @@ -28495,6 +28498,17 @@ AT_CHECK([as hv1 ovn-appctl -t ovn-controller ct-zone-list | \ grep sw0-port1 -c], [0], [1 ]) +check as hv1 ovs-vsctl add-port br-int vif13 \ + -- set interface vif13 external_ids:iface-id=sw0-port2 ofport-request=13 + +ovn-nbctl --wait=hv sync +OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=0 | grep "in_port=13" | wc -l` -eq 1]) + +# There should be ct zone for sw0-port2 (localport). +AT_CHECK([as hv1 ovn-appctl -t ovn-controller ct-zone-list | \ +grep sw0-port2 -c], [0], [1 +]) + # There should be no ct-zone id allocated for sw0-lr0 and lr0-sw0 AT_CHECK([as hv1 ovn-appctl -t ovn-controller ct-zone-list | \ grep sw0-lr0], [1], []) ------------ > --- > controller/ovn-controller.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index a719beb0e..4202f32cc 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1919,8 +1919,9 @@ ct_zones_runtime_data_handler(struct engine_node *node, void *data) > struct shash_node *shash_node; > SHASH_FOR_EACH (shash_node, &tdp->lports) { > struct tracked_lport *t_lport = shash_node->data; > - if (strcmp(t_lport->pb->type, "")) { > - /* We allocate zone-id's only to VIF lports. */ > + if (strcmp(t_lport->pb->type, "") > + && strcmp(t_lport->pb->type, "localport")) { > + /* We allocate zone-id's only to VIF and localport lports. */ > continue; > } > > -- > 2.32.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Oct 1, 2021 at 2:34 AM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Sep 30, 2021 at 3:35 PM Frode Nordahl > <frode.nordahl@canonical.com> wrote: > > > > Commit d4bca93 limited ct zone allocation to VIF-ports only, this > > breaks OpenStack's use of localport for providing metadata to > > instances. > > > > This patch adds lports of type 'localport' to the list of lport > > types to allocate zone ids for. > > > > No test case is added as I am unable to make the error condition > > occur reliably in the sandbox test environment, however I have > > confirmed that the error condition is there on an unmodified OVN > > installation and that the behavior changes by adding this patch. > > Both for a fresh deployment where a modified package is slipped in > > prior to first start of OVN and comparison of patched and > > non-patched OVN after reboot of a deployed node. > > > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2021-September/051473.html > > Fixes: d4bca93 ("controller: Don't allocate zone ids for non-VIF port bindings. ") > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > > Thanks a lot for fixing this issue and thanks to Benjamin Reichel for > reporting it. > > I was able to reproduce locally with the below changes to the test > case - "ovn-controller ct zone I-P handling" > So I added this test case to this patch and applied to the main branch > and branch-21.09. > > Also updated the commit message accordingly. Hope that's fine with you. That's great, thank you for helping with that. Cheers!
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index a719beb0e..4202f32cc 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1919,8 +1919,9 @@ ct_zones_runtime_data_handler(struct engine_node *node, void *data) struct shash_node *shash_node; SHASH_FOR_EACH (shash_node, &tdp->lports) { struct tracked_lport *t_lport = shash_node->data; - if (strcmp(t_lport->pb->type, "")) { - /* We allocate zone-id's only to VIF lports. */ + if (strcmp(t_lport->pb->type, "") + && strcmp(t_lport->pb->type, "localport")) { + /* We allocate zone-id's only to VIF and localport lports. */ continue; }
Commit d4bca93 limited ct zone allocation to VIF-ports only, this breaks OpenStack's use of localport for providing metadata to instances. This patch adds lports of type 'localport' to the list of lport types to allocate zone ids for. No test case is added as I am unable to make the error condition occur reliably in the sandbox test environment, however I have confirmed that the error condition is there on an unmodified OVN installation and that the behavior changes by adding this patch. Both for a fresh deployment where a modified package is slipped in prior to first start of OVN and comparison of patched and non-patched OVN after reboot of a deployed node. Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2021-September/051473.html Fixes: d4bca93 ("controller: Don't allocate zone ids for non-VIF port bindings. ") Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> --- controller/ovn-controller.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)