| Message ID | 20230918164623.3144813-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] controller: have I+P assigning ct_zones for l3gateway ports | 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 | fail | github build: failed |
On 9/18/23 18:46, Xavier Simonart wrote: > When l3gateway ports get added, ct_zones were assigned during > (ct_zones) recomputes, but not by I+P. > Before this patch, test "Migration of CT zone from UUID to name" > was randomly failing, as ct_zone was not assigned by I+P but > a ct_zone recompute happened most of the time (and hence test succeeded). > Test case has been adapted so that ct_zone recompute usually happens > before adding the sw0-lr0 port. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > controller/ovn-controller.c | 1 + > tests/ovn.at | 6 +++--- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 859d9cab9..e5067ed1b 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2637,6 +2637,7 @@ ct_zones_runtime_data_handler(struct engine_node *node, void *data) > struct tracked_lport *t_lport = shash_node->data; > if (strcmp(t_lport->pb->type, "") > && strcmp(t_lport->pb->type, "localport") > + && strcmp(t_lport->pb->type, "l3gateway") > && strcmp(t_lport->pb->type, "localnet")) { > /* We allocate zone-id's only to VIF, localport, and localnet > * lports. */ Hi Xavier, Why do we need per-port zones for routers? Isn't it a bug that one gets assigned in the "recompute" stage? Unlike switches, routers use 2 conntrack zones, SNAT and DNAT (sometimes merged). The comment here also suggests the same thing. Am I missing something? Thanks, Dumitru > diff --git a/tests/ovn.at b/tests/ovn.at > index ba5ce298a..acf00a335 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -36860,14 +36860,14 @@ check ovn-nbctl ls-add sw0 > check ovn-nbctl lsp-add sw0 sw0-port1 > check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2" > > +ovs-vsctl add-port br-int p1 -- \ > + set Interface p1 external_ids:iface-id=sw0-port1 > + > check ovn-nbctl lsp-add sw0 sw0-lr0 > check ovn-nbctl lsp-set-type sw0-lr0 router > check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 > check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > > -ovs-vsctl add-port br-int p1 -- \ > - set Interface p1 external_ids:iface-id=sw0-port1 > - > check ovn-appctl -t ovn-controller vlog/set dbg:main > > wait_for_ports_up
On 10/9/23 11:15, Dumitru Ceara wrote: > On 9/18/23 18:46, Xavier Simonart wrote: >> When l3gateway ports get added, ct_zones were assigned during >> (ct_zones) recomputes, but not by I+P. >> Before this patch, test "Migration of CT zone from UUID to name" >> was randomly failing, as ct_zone was not assigned by I+P but >> a ct_zone recompute happened most of the time (and hence test succeeded). >> Test case has been adapted so that ct_zone recompute usually happens >> before adding the sw0-lr0 port. >> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >> --- >> controller/ovn-controller.c | 1 + >> tests/ovn.at | 6 +++--- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 859d9cab9..e5067ed1b 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -2637,6 +2637,7 @@ ct_zones_runtime_data_handler(struct engine_node *node, void *data) >> struct tracked_lport *t_lport = shash_node->data; >> if (strcmp(t_lport->pb->type, "") >> && strcmp(t_lport->pb->type, "localport") >> + && strcmp(t_lport->pb->type, "l3gateway") >> && strcmp(t_lport->pb->type, "localnet")) { >> /* We allocate zone-id's only to VIF, localport, and localnet >> * lports. */ > > Hi Xavier, > > Why do we need per-port zones for routers? Isn't it a bug that one gets > assigned in the "recompute" stage? > > Unlike switches, routers use 2 conntrack zones, SNAT and DNAT (sometimes > merged). The comment here also suggests the same thing. > > Am I missing something? Hi Dumitru, Ports of type "l3gateway" are on logical switches, not logical routers. Ports of this type connect to logical routers. This patch isn't adding per-port zones for routers. Instead the patch ensures that all "local" logical switch ports are handled in the I-P case, just like they are in the recompute case. > > Thanks, > Dumitru > >> diff --git a/tests/ovn.at b/tests/ovn.at >> index ba5ce298a..acf00a335 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -36860,14 +36860,14 @@ check ovn-nbctl ls-add sw0 >> check ovn-nbctl lsp-add sw0 sw0-port1 >> check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2" >> >> +ovs-vsctl add-port br-int p1 -- \ >> + set Interface p1 external_ids:iface-id=sw0-port1 >> + >> check ovn-nbctl lsp-add sw0 sw0-lr0 >> check ovn-nbctl lsp-set-type sw0-lr0 router >> check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 >> check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 >> >> -ovs-vsctl add-port br-int p1 -- \ >> - set Interface p1 external_ids:iface-id=sw0-port1 >> - >> check ovn-appctl -t ovn-controller vlog/set dbg:main >> >> wait_for_ports_up > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 10/12/23 23:39, Mark Michelson wrote: > On 10/9/23 11:15, Dumitru Ceara wrote: >> On 9/18/23 18:46, Xavier Simonart wrote: >>> When l3gateway ports get added, ct_zones were assigned during >>> (ct_zones) recomputes, but not by I+P. >>> Before this patch, test "Migration of CT zone from UUID to name" >>> was randomly failing, as ct_zone was not assigned by I+P but >>> a ct_zone recompute happened most of the time (and hence test >>> succeeded). >>> Test case has been adapted so that ct_zone recompute usually happens >>> before adding the sw0-lr0 port. >>> >>> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >>> --- >>> controller/ovn-controller.c | 1 + >>> tests/ovn.at | 6 +++--- >>> 2 files changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 859d9cab9..e5067ed1b 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -2637,6 +2637,7 @@ ct_zones_runtime_data_handler(struct >>> engine_node *node, void *data) >>> struct tracked_lport *t_lport = shash_node->data; >>> if (strcmp(t_lport->pb->type, "") >>> && strcmp(t_lport->pb->type, "localport") >>> + && strcmp(t_lport->pb->type, "l3gateway") >>> && strcmp(t_lport->pb->type, "localnet")) { >>> /* We allocate zone-id's only to VIF, localport, >>> and localnet >>> * lports. */ >> >> Hi Xavier, >> >> Why do we need per-port zones for routers? Isn't it a bug that one gets >> assigned in the "recompute" stage? >> >> Unlike switches, routers use 2 conntrack zones, SNAT and DNAT (sometimes >> merged). The comment here also suggests the same thing. >> >> Am I missing something? > > Hi Dumitru, > Hi Mark, > Ports of type "l3gateway" are on logical switches, not logical routers. > Ports of this type connect to logical routers. This patch isn't adding > per-port zones for routers. Instead the patch ensures that all "local" > logical switch ports are handled in the I-P case, just like they are in > the recompute case. > You're right, I mixed things up, thanks for the clarification! We should probably also update the comment to include l3gateway ports too. Otherwise, the patch looks good to me now. Regards, Dumitru >> >> Thanks, >> Dumitru >> >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index ba5ce298a..acf00a335 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -36860,14 +36860,14 @@ check ovn-nbctl ls-add sw0 >>> check ovn-nbctl lsp-add sw0 sw0-port1 >>> check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 >>> 192.168.0.2" >>> +ovs-vsctl add-port br-int p1 -- \ >>> + set Interface p1 external_ids:iface-id=sw0-port1 >>> + >>> check ovn-nbctl lsp-add sw0 sw0-lr0 >>> check ovn-nbctl lsp-set-type sw0-lr0 router >>> check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 >>> check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 >>> -ovs-vsctl add-port br-int p1 -- \ >>> - set Interface p1 external_ids:iface-id=sw0-port1 >>> - >>> check ovn-appctl -t ovn-controller vlog/set dbg:main >>> wait_for_ports_up >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 859d9cab9..e5067ed1b 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2637,6 +2637,7 @@ ct_zones_runtime_data_handler(struct engine_node *node, void *data) struct tracked_lport *t_lport = shash_node->data; if (strcmp(t_lport->pb->type, "") && strcmp(t_lport->pb->type, "localport") + && strcmp(t_lport->pb->type, "l3gateway") && strcmp(t_lport->pb->type, "localnet")) { /* We allocate zone-id's only to VIF, localport, and localnet * lports. */ diff --git a/tests/ovn.at b/tests/ovn.at index ba5ce298a..acf00a335 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -36860,14 +36860,14 @@ check ovn-nbctl ls-add sw0 check ovn-nbctl lsp-add sw0 sw0-port1 check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2" +ovs-vsctl add-port br-int p1 -- \ + set Interface p1 external_ids:iface-id=sw0-port1 + check ovn-nbctl lsp-add sw0 sw0-lr0 check ovn-nbctl lsp-set-type sw0-lr0 router check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 -ovs-vsctl add-port br-int p1 -- \ - set Interface p1 external_ids:iface-id=sw0-port1 - check ovn-appctl -t ovn-controller vlog/set dbg:main wait_for_ports_up
When l3gateway ports get added, ct_zones were assigned during (ct_zones) recomputes, but not by I+P. Before this patch, test "Migration of CT zone from UUID to name" was randomly failing, as ct_zone was not assigned by I+P but a ct_zone recompute happened most of the time (and hence test succeeded). Test case has been adapted so that ct_zone recompute usually happens before adding the sw0-lr0 port. Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/ovn-controller.c | 1 + tests/ovn.at | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-)