[ovs-dev] [PATCH net-next v4] net: openvswitch: Check vport netdev name
From: "jun.gu"
Ensure that the provided netdev name is not one of its aliases to
prevent unnecessary creation and destruction of the vport by
ovs-vswitchd.
Signed-off-by: jun.gu
Acked-by: Eelco Chaudron
---
net/openvswitch/vport-netdev.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
index 903537a5da22..618edc346c0f 100644
--- a/net/openvswitch/vport-netdev.c
+++ b/net/openvswitch/vport-netdev.c
@@ -78,7 +78,10 @@ struct vport *ovs_netdev_link(struct vport *vport, const
char *name)
int err;
vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name);
- if (!vport->dev) {
+ /* Ensure that the device exists and that the provided
+* name is not one of its aliases.
+*/
+ if (!vport->dev || strcmp(name, ovs_vport_name(vport))) {
err = -ENODEV;
goto error_free_vport;
}
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Allow DHCP request from the lport if enabled DHCPv4
>
>
>
> On Tue, Apr 16, 2024 at 1:22 AM wrote:
>>
>> From: shylou
>>
>> DHCP for VM fails when removing default security group rules
>> using a CMS like Neutron ML2/OVN [1]. This is because DHCP
>> requests from VMs may be dropped by ACLs. To fix this issue,
>> we add a lflow with a priority of 34000 to allow DHCP requests
>> from the logical port if the CMS has enabled native DHCPv4
>> for this port.
>>
>> [1]https://bugs.launchpad.net/neutron/+bug/1926515
>>
>> Signed-off-by: Xie Liu
>
>
> Thanks for the patch.
>
> I don't think this is the correct fix. If neutron wants to allow DHCP
> overriding any ACL rules,
> it should add a high priority ACL to allow DHCP. What if a user wants to
> add an explicit ACL to
> drop DHCP from certain ports ?
>
> Thanks
> Numan
>
Thanks, Numan
I have a puzzling question: Why would users want to block
DHCP request packets after enabling DHCP for any LSPs ?
They could justly not enable DHCP for them at all, right?
>
>> ---
>> northd/northd.c | 10 ++
>> tests/ovn-northd.at | 5 +
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 2c3560ce2..ca641a19e 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -8414,6 +8414,16 @@ build_dhcpv4_options_flows(struct ovn_port *op,
>> meter_groups),
>>>nbsp->dhcpv4_options->header_,
>>lflow_ref);
>> +/* Add 34000 priority flow to allow DHCP request from the lport
>> + * if the CMS has enabled native DHCPv4 for this lport.
>> + * */
>> +ovn_lflow_add_with_lport_and_hint(lflows, op->od,
>> + S_SWITCH_IN_ACL_EVAL, 34000,
>> + ds_cstr(),
>> + REGBIT_ACL_VERDICT_ALLOW" =
>> 1; next;",
>> + op->key,
>> + >nbsp->header_,
>> + lflow_ref);
>> ds_clear();
>>
>> /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 6fdd761da..7657aefff 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -4897,6 +4897,11 @@ ovn-nbctl --wait=sb lsp-set-dhcpv4-options sw0-port1
>> $CIDR_UUID
>> ovn-sbctl dump-flows sw0 > sw0flows
>> AT_CAPTURE_FILE([sw0flows])
>>
>> +AT_CHECK([grep "_acl_eval" sw0flows | grep sw0-port1 | ovn_strip_lflows],
>> [0], [dnl
>> + table=??(ls_in_acl_eval ), priority=34000, match=(inport ==
>> "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2,
>> 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 &&
>> udp.dst == 67), action=(reg8[[16]] = 1; next;)
>> + table=??(ls_out_acl_eval), priority=34000, match=(outport ==
>> "sw0-port1" && eth.src == c0:ff:ee:00:00:01 && ip4.src == 10.0.0.1 && udp &&
>> udp.src == 67 && udp.dst == 68), action=(reg8[[16]] = 1; next;)
>> +])
>> +
>> AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | ovn_strip_lflows], [0],
>> [dnl
>>table=??(ls_in_dhcp_options ), priority=0, match=(1), action=(next;)
>>table=??(ls_in_dhcp_options ), priority=100 , match=(inport ==
>> "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2,
>> 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 &&
>> udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2,
>> hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router =
>> 10.0.0.1, server_id = 10.0.0.1); next;)
>> --
>> 2.42.0.windows.2
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v4] net: openvswitch: Check vport netdev name
On Thu, 18 Apr 2024 10:32:42 +0800 jun.gu wrote:
> + if ((!vport->dev) || strcmp(name, ovs_vport_name(vport))) {
Please drop the unnecessary brackets.
When you repost, start a new thread, do not post new version
in-reply-to.
--
pw-bot: cr
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel/systemd: Set ovsdb-server timeout to 5 minutes
On 15/04/2024 14:39, Jon Kohler wrote: On Apr 11, 2024, at 9:43 AM, Chris Riches wrote: On 11/04/2024 14:24, Ilya Maximets wrote: On 4/11/24 10:59, Chris Riches wrote: From what we know so far, the DB was full of stale connection-tracking information such as the following: [...] Once the host was recovered by putting in the timeout increase, ovsdb-server successfully started and GCed the database down from 2.4 *GB* to 29 *KB*. Had this happened before the host restart, we would have never seen this problem. But since it seems possible to end up booting with such a large DB, we figured a timeout increase was a sensible measure to take. Uff. Sounds like ovn-controller went off the rails. Normally, ovsdb-server compacts the database once in 10-20 minutes, if the database doubles the size since the previous check. If all the transactions are that small, it would mean ovn-controller made about 10K transactions per second in the 10-20 minutes before the restart. That's huge. I wonder if this can be addressed with a better compaction strategy. Something like forcing compaction if "the database is more than 10 MB and increased 10x" regardless of the time. I'm not sure exactly what the test was doing when this was observed, so I don't know whether that transaction volume is within the realm of possibility or if we're looking at a failure to perform compaction on time. It would be nice to have an enhanced safety-net for DB size, as we were only a few hundred MB away from hitting filesystem space issues as well. Normally, ovsdb-server compacts the database once in 10-20 minutes, if the database doubles the size since the previous check. I presume you mean if it doubled in size since the previous *compaction*? If we only compact when it doubles since the last *check*, then it would be easy for it to slightly-less-than-double every 10-20 minutes and never trigger the compaction while still growing exponentially. I'm happy to discuss compaction approaches (though my expertise is very much in host service management and not OVS itself), but do you think there's merit in having this extended timeout as a backstop too? FWIW, I think we should do both extending the time out and tuning up the compaction, as having a situation where a service can get in an endless loop if for whatever reason it takes too long is problematic. Addressing the root cause (compaction, too many calls, some other bug(s) etc) is good, but extending the timeout seems like an easy backstop. I agree with Jon's assessment - regardless of any action taken on compaction or preventing growth in the first place, we should consider the proposed timeout increase as a backstop against getting stuck in an infinite loop. Ilya (or another maintainer) - can I get an opinion on this? Thanks, Chris ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Allow DHCP request from the lport if enabled DHCPv4
On Tue, Apr 16, 2024 at 1:22 AM wrote:
> From: shylou
>
> DHCP for VM fails when removing default security group rules
> using a CMS like Neutron ML2/OVN [1]. This is because DHCP
> requests from VMs may be dropped by ACLs. To fix this issue,
> we add a lflow with a priority of 34000 to allow DHCP requests
> from the logical port if the CMS has enabled native DHCPv4
> for this port.
>
> [1]https://bugs.launchpad.net/neutron/+bug/1926515
>
> Signed-off-by: Xie Liu
>
Thanks for the patch.
I don't think this is the correct fix. If neutron wants to allow DHCP
overriding any ACL rules,
it should add a high priority ACL to allow DHCP. What if a user wants to
add an explicit ACL to
drop DHCP from certain ports ?
Thanks
Numan
---
> northd/northd.c | 10 ++
> tests/ovn-northd.at | 5 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 2c3560ce2..ca641a19e 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8414,6 +8414,16 @@ build_dhcpv4_options_flows(struct ovn_port *op,
> meter_groups),
>>nbsp->dhcpv4_options->header_,
>lflow_ref);
> +/* Add 34000 priority flow to allow DHCP request from the
> lport
> + * if the CMS has enabled native DHCPv4 for this lport.
> + * */
> +ovn_lflow_add_with_lport_and_hint(lflows, op->od,
> + S_SWITCH_IN_ACL_EVAL, 34000,
> + ds_cstr(),
> + REGBIT_ACL_VERDICT_ALLOW" =
> 1; next;",
> + op->key,
> + >nbsp->header_,
> + lflow_ref);
> ds_clear();
>
> /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6fdd761da..7657aefff 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -4897,6 +4897,11 @@ ovn-nbctl --wait=sb lsp-set-dhcpv4-options
> sw0-port1 $CIDR_UUID
> ovn-sbctl dump-flows sw0 > sw0flows
> AT_CAPTURE_FILE([sw0flows])
>
> +AT_CHECK([grep "_acl_eval" sw0flows | grep sw0-port1 | ovn_strip_lflows],
> [0], [dnl
> + table=??(ls_in_acl_eval ), priority=34000, match=(inport ==
> "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2,
> 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 &&
> udp.dst == 67), action=(reg8[[16]] = 1; next;)
> + table=??(ls_out_acl_eval), priority=34000, match=(outport ==
> "sw0-port1" && eth.src == c0:ff:ee:00:00:01 && ip4.src == 10.0.0.1 && udp
> && udp.src == 67 && udp.dst == 68), action=(reg8[[16]] = 1; next;)
> +])
> +
> AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | ovn_strip_lflows], [0],
> [dnl
>table=??(ls_in_dhcp_options ), priority=0, match=(1), action=(next;)
>table=??(ls_in_dhcp_options ), priority=100 , match=(inport ==
> "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2,
> 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 &&
> udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2,
> hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router =
> 10.0.0.1, server_id = 10.0.0.1); next;)
> --
> 2.42.0.windows.2
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 1/2] ci: Make sure that multinode test runs on correct branch.
On Tue, Apr 16, 2024 at 3:28 PM Mark Michelson wrote:
> This looks good to me. Thanks Ales.
>
> Acked-by: Mark Michelson
>
Thanks. I applied this patch to the main.
@Mark - I'm sorry. I missed adding your Ack to this patch.
Numan
> On 4/10/24 09:29, Ales Musil wrote:
> > The ovn-fake-multinode workflow can be triggered manually,
> > however the definition didn't respect the branch for the manual
> > run and always used main branch. Make sure that the correct
> > branch is used for the ovn-fake-multinode workflow.
> >
> > Fixes: 033f5bebf94d ("CI: Add a couple of periodic jobs using
> ovn-fake-multinode.")
> > Signed-off-by: Ales Musil
> > ---
> > .github/workflows/ovn-fake-multinode-tests.yml | 15 +++
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/.github/workflows/ovn-fake-multinode-tests.yml
> b/.github/workflows/ovn-fake-multinode-tests.yml
> > index 79b6c4253..f097ee9a1 100644
> > --- a/.github/workflows/ovn-fake-multinode-tests.yml
> > +++ b/.github/workflows/ovn-fake-multinode-tests.yml
> > @@ -17,7 +17,7 @@ jobs:
> > strategy:
> > matrix:
> > cfg:
> > -- { branch: "main" }
> > +- { branch: "${{ github.ref_name }}" }
> > - { branch: "branch-22.03" }
> > env:
> > RUNC_CMD: podman
> > @@ -47,7 +47,7 @@ jobs:
> > with:
> > path: 'ovn-fake-multinode/ovn'
> > submodules: recursive
> > -ref: ${{ matrix.cfg.branch }}
> > +ref: "${{ matrix.cfg.branch }}"
> >
> > - name: Install dependencies
> > run: |
> > @@ -76,16 +76,15 @@ jobs:
> > fail-fast: false
> > matrix:
> > cfg:
> > -- { branch: "main", testsuiteflags: ""}
> > - { branch: "branch-22.03", testsuiteflags: "-k 'basic test'" }
> > name: multinode tests ${{ join(matrix.cfg.*, ' ') }}
> > env:
> > RUNC_CMD: podman
> > OS_IMAGE: "fedora:37"
> > CENTRAL_IMAGE: "ovn/ovn-multi-node:${{ matrix.cfg.branch }}"
> > - CHASSIS_IMAGE: "ovn/ovn-multi-node:main"
> > - RELAY_IMAGE: "ovn/ovn-multi-node:main"
> > - GW_IMAGE: "ovn/ovn-multi-node:main"
> > + CHASSIS_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
> > + RELAY_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
> > + GW_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
> > # Disable SSL for now. Revisit this if required.
> > ENABLE_SSL: no
> > CC: gcc
> > @@ -114,7 +113,7 @@ jobs:
> >
> > - uses: actions/download-artifact@v4
> > with:
> > -name: test-main-image
> > +name: test-${{ github.ref_name }}-image
> >
> > - uses: actions/download-artifact@v4
> > with:
> > @@ -122,7 +121,7 @@ jobs:
> >
> > - name: Load podman image
> > run: |
> > -sudo podman load --input ovn_main_image.tar
> > +sudo podman load --input ovn_${{ github.ref_name }}_image.tar
> > sudo podman load --input ovn_branch-22.03_image.tar
> >
> > - name: Check out ovn-fake-multi-node
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Fix typo in README.
On 4/18/24 14:10, Dumitru Ceara wrote: > From: Kacper Kamiński > > Submitted-at: https://github.com/ovn-org/ovn/pull/138 > Signed-off-by: Kacper Kamiński > Signed-off-by: Dumitru Ceara > --- Applied to main. I also added Kacper to the AUTHORS list. Best regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] Fix typo in README.
From: Kacper Kamiński Submitted-at: https://github.com/ovn-org/ovn/pull/138 Signed-off-by: Kacper Kamiński Signed-off-by: Dumitru Ceara --- README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.rst b/README.rst index 4b9ea1df91..014d208c07 100644 --- a/README.rst +++ b/README.rst @@ -23,7 +23,7 @@ OVN (Open Virtual Network) is a series of daemons that translates virtual network configuration into OpenFlow, and installs them into Open vSwitch. It is licensed under the open source Apache 2 license. -OVN provides a higher-layer abstraction then Open vSwitch, working with logical +OVN provides a higher-layer abstraction than Open vSwitch, working with logical routers and logical switches, rather than flows. OVN is intended to be used by cloud management software (CMS). For details about the architecture of OVN, see the ovn-architecture manpage. Some high-level features offered by OVN include: -- 2.44.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Ignore log setting extended ack support failed.
On 4/18/24 13:29, Xie Liu wrote: > > @Dumitru > Thanks for your review! > Yeah, I'm OK! > Awesome, I fixed up the git author and pushed this to main. Best regards, Dumitru > At 2024-04-18 19:22:01, "Dumitru Ceara" wrote: >> On 4/16/24 04:57, liuxie...@163.com wrote: >>> From: shylou >>> >>> The test error log 'setting extended ack support failed' in the test >>> results may cause test failures. However, according to the OVS commit[1], >>> this is not a critical issue, so we can safely ignore this log. >>> >>> [1]https://github.com/openvswitch/ovs/commit/812164adefc716dd35c50c2101e8e60a15167635 >>> >>> Signed-off-by: Xie Liu >>> --- >> >> Hi Xie Liu, >> >> Thanks for the patch, it looks good to me. >> >> There's one small thing though: the git email address used to author the >> patch doesn't match the signed-off-by address. >> >> I can change it to Xie Liu when accepting the >> patch if you're OK with that. >> >> Regards, >> Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Ignore log setting extended ack support failed.
@Dumitru Thanks for your review! Yeah, I'm OK! At 2024-04-18 19:22:01, "Dumitru Ceara" wrote: >On 4/16/24 04:57, liuxie...@163.com wrote: >> From: shylou >> >> The test error log 'setting extended ack support failed' in the test >> results may cause test failures. However, according to the OVS commit[1], >> this is not a critical issue, so we can safely ignore this log. >> >> [1]https://github.com/openvswitch/ovs/commit/812164adefc716dd35c50c2101e8e60a15167635 >> >> Signed-off-by: Xie Liu >> --- > >Hi Xie Liu, > >Thanks for the patch, it looks good to me. > >There's one small thing though: the git email address used to author the >patch doesn't match the signed-off-by address. > >I can change it to Xie Liu when accepting the >patch if you're OK with that. > >Regards, >Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Ignore log setting extended ack support failed.
On 4/16/24 04:57, liuxie...@163.com wrote: > From: shylou > > The test error log 'setting extended ack support failed' in the test > results may cause test failures. However, according to the OVS commit[1], > this is not a critical issue, so we can safely ignore this log. > > [1]https://github.com/openvswitch/ovs/commit/812164adefc716dd35c50c2101e8e60a15167635 > > Signed-off-by: Xie Liu > --- Hi Xie Liu, Thanks for the patch, it looks good to me. There's one small thing though: the git email address used to author the patch doesn't match the signed-off-by address. I can change it to Xie Liu when accepting the patch if you're OK with that. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [Patch ovn v3 1/2] actions: New action ct_commit_to_zone.
On Thu, Apr 18, 2024 at 11:45 AM Martin Kalcok
wrote:
> Thanks for another round of review.
>
> On Wed, Apr 17, 2024 at 1:45 PM Ales Musil wrote:
>
>>
>>
>> On Wed, Apr 17, 2024 at 1:05 PM Dumitru Ceara wrote:
>>
>>> On 4/17/24 09:04, Ales Musil wrote:
>>> > On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok <
>>> martin.kal...@canonical.com>
>>> > wrote:
>>> >
>>> >> This change adds a new action 'ct_commit_to_zone' that enables users
>>> to
>>> >> commit
>>> >> the flow into a specific zone in the connection tracker.
>>> >>
>>> >> A new feature flag, OVN_FEATURE_CT_COMMIT_TO_ZONE, is also included to
>>> >> avoid
>>> >> issues during upgrade in case the northd is upgraded to a version that
>>> >> supports
>>> >> this action before the controller is upgraded.
>>> >>
>>> >> Note that this action is meaningful only in the context of Logical
>>> Router
>>> >> datapath. Logical Switch datapath does not use multiple zones and this
>>> >> action
>>> >> falls back to committing the connection into the default zone for the
>>> >> Logical
>>> >> Switch.
>>> >>
>>> >> Signed-off-by: Martin Kalcok
>>> >> ---
>>> >>
>>> >
>>> > Hi Martin,
>>> >
>>>
>>> Hi Martin, Ales,
>>>
>>> > thank you for the followup. I have a couple of comments down below.
>>> >
>>>
>>> I have a few of my own. :)
>>>
>>> >
>>> >> controller/chassis.c | 8 ++
>>> >> include/ovn/actions.h | 1 +
>>> >> include/ovn/features.h| 1 +
>>> >> lib/actions.c | 60
>>> +++
>>> >> northd/en-global-config.c | 11 +++
>>> >> northd/en-global-config.h | 1 +
>>> >> ovn-sb.xml| 24
>>> >> tests/ovn.at | 16 +++
>>> >> utilities/ovn-trace.c | 1 +
>>> >> 9 files changed, 123 insertions(+)
>>> >>
>>> >> diff --git a/controller/chassis.c b/controller/chassis.c
>>> >> index ad75df288..9bb2eba95 100644
>>> >> --- a/controller/chassis.c
>>> >> +++ b/controller/chassis.c
>>> >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct
>>> >> ovs_chassis_cfg *ovs_cfg,
>>> >> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
>>> >> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
>>> >> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
>>> >> +smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
>>> >> }
>>> >>
>>> >> /*
>>> >> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct
>>> >> ovs_chassis_cfg *ovs_cfg,
>>> >> return true;
>>> >> }
>>> >>
>>> >> +if (!smap_get_bool(_rec->other_config,
>>> >> + OVN_FEATURE_CT_COMMIT_TO_ZONE,
>>> >> + false)) {
>>> >> +return true;
>>> >> +}
>>> >> +
>>> >> return false;
>>> >> }
>>> >>
>>> >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported)
>>> >> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
>>> >> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
>>> >> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
>>> >> +sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE);
>>> >> }
>>> >>
>>> >> static void
>>> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>> >> index 8e794450c..4bcd1f58c 100644
>>> >> --- a/include/ovn/actions.h
>>> >> +++ b/include/ovn/actions.h
>>> >> @@ -68,6 +68,7 @@ struct collector_set_ids;
>>> >> OVNACT(CT_NEXT, ovnact_ct_next) \
>>> >> /* CT_COMMIT_V1 is not supported anymore. */ \
>>> >> OVNACT(CT_COMMIT_V2, ovnact_nest)\
>>> >> +OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \
>>> >> OVNACT(CT_DNAT, ovnact_ct_nat) \
>>> >> OVNACT(CT_SNAT, ovnact_ct_nat) \
>>> >> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \
>>> >> diff --git a/include/ovn/features.h b/include/ovn/features.h
>>> >> index 08f1d8288..35a5d8ba0 100644
>>> >> --- a/include/ovn/features.h
>>> >> +++ b/include/ovn/features.h
>>> >> @@ -28,6 +28,7 @@
>>> >> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
>>> >> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
>>> >> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
>>> >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone"
>>> >>
>>> >> /* OVS datapath supported features. Based on availability OVN might
>>> >> generate
>>> >> * different types of openflows.
>>> >> diff --git a/lib/actions.c b/lib/actions.c
>>> >> index 39bb5bc8a..f26817018 100644
>>> >> --- a/lib/actions.c
>>> >> +++ b/lib/actions.c
>>> >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>>> >> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
>>> >> ct = ofpacts->header;
>>> >> ofpact_finish(ofpacts, >ofpact);
>>> >> +}
>>> >> +
>>> >> +static void
>>> >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx)
>>> >> +{
>>> >> +add_prerequisite(ctx, "ip");
>>> >> +
>>> >> +struct ovnact_ct_commit_nat
Re: [ovs-dev] [Patch ovn v3 1/2] actions: New action ct_commit_to_zone.
Thanks for another round of review.
On Wed, Apr 17, 2024 at 1:45 PM Ales Musil wrote:
>
>
> On Wed, Apr 17, 2024 at 1:05 PM Dumitru Ceara wrote:
>
>> On 4/17/24 09:04, Ales Musil wrote:
>> > On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok <
>> martin.kal...@canonical.com>
>> > wrote:
>> >
>> >> This change adds a new action 'ct_commit_to_zone' that enables users to
>> >> commit
>> >> the flow into a specific zone in the connection tracker.
>> >>
>> >> A new feature flag, OVN_FEATURE_CT_COMMIT_TO_ZONE, is also included to
>> >> avoid
>> >> issues during upgrade in case the northd is upgraded to a version that
>> >> supports
>> >> this action before the controller is upgraded.
>> >>
>> >> Note that this action is meaningful only in the context of Logical
>> Router
>> >> datapath. Logical Switch datapath does not use multiple zones and this
>> >> action
>> >> falls back to committing the connection into the default zone for the
>> >> Logical
>> >> Switch.
>> >>
>> >> Signed-off-by: Martin Kalcok
>> >> ---
>> >>
>> >
>> > Hi Martin,
>> >
>>
>> Hi Martin, Ales,
>>
>> > thank you for the followup. I have a couple of comments down below.
>> >
>>
>> I have a few of my own. :)
>>
>> >
>> >> controller/chassis.c | 8 ++
>> >> include/ovn/actions.h | 1 +
>> >> include/ovn/features.h| 1 +
>> >> lib/actions.c | 60 +++
>> >> northd/en-global-config.c | 11 +++
>> >> northd/en-global-config.h | 1 +
>> >> ovn-sb.xml| 24
>> >> tests/ovn.at | 16 +++
>> >> utilities/ovn-trace.c | 1 +
>> >> 9 files changed, 123 insertions(+)
>> >>
>> >> diff --git a/controller/chassis.c b/controller/chassis.c
>> >> index ad75df288..9bb2eba95 100644
>> >> --- a/controller/chassis.c
>> >> +++ b/controller/chassis.c
>> >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct
>> >> ovs_chassis_cfg *ovs_cfg,
>> >> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
>> >> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
>> >> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
>> >> +smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
>> >> }
>> >>
>> >> /*
>> >> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct
>> >> ovs_chassis_cfg *ovs_cfg,
>> >> return true;
>> >> }
>> >>
>> >> +if (!smap_get_bool(_rec->other_config,
>> >> + OVN_FEATURE_CT_COMMIT_TO_ZONE,
>> >> + false)) {
>> >> +return true;
>> >> +}
>> >> +
>> >> return false;
>> >> }
>> >>
>> >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported)
>> >> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
>> >> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
>> >> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
>> >> +sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE);
>> >> }
>> >>
>> >> static void
>> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>> >> index 8e794450c..4bcd1f58c 100644
>> >> --- a/include/ovn/actions.h
>> >> +++ b/include/ovn/actions.h
>> >> @@ -68,6 +68,7 @@ struct collector_set_ids;
>> >> OVNACT(CT_NEXT, ovnact_ct_next) \
>> >> /* CT_COMMIT_V1 is not supported anymore. */ \
>> >> OVNACT(CT_COMMIT_V2, ovnact_nest)\
>> >> +OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \
>> >> OVNACT(CT_DNAT, ovnact_ct_nat) \
>> >> OVNACT(CT_SNAT, ovnact_ct_nat) \
>> >> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \
>> >> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> >> index 08f1d8288..35a5d8ba0 100644
>> >> --- a/include/ovn/features.h
>> >> +++ b/include/ovn/features.h
>> >> @@ -28,6 +28,7 @@
>> >> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
>> >> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
>> >> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
>> >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone"
>> >>
>> >> /* OVS datapath supported features. Based on availability OVN might
>> >> generate
>> >> * different types of openflows.
>> >> diff --git a/lib/actions.c b/lib/actions.c
>> >> index 39bb5bc8a..f26817018 100644
>> >> --- a/lib/actions.c
>> >> +++ b/lib/actions.c
>> >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>> >> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
>> >> ct = ofpacts->header;
>> >> ofpact_finish(ofpacts, >ofpact);
>> >> +}
>> >> +
>> >> +static void
>> >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx)
>> >> +{
>> >> +add_prerequisite(ctx, "ip");
>> >> +
>> >> +struct ovnact_ct_commit_nat *ct_commit =
>> >> +ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts);
>> >>
>> >
>> > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be
>> renamed
>> > if anything with a flag indicating
