| Message ID | 20220531182927.58-1-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] datapath-windows: Detect CT timeout. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/31/22 20:29, William Tu wrote: > CT timeout is not supported in Windows datapath, but > currently it incorrectly reports to ovs-vswitchd as supported blow > "system@ovs-system: Datapath supports timeout policy in conntrack > action". The patches detects it and returns not support. > > Cc: Alin-Gabriel Serdean <aserdean@ovn.org> > Cc: Wilson Peng <pweisong@vmware.com> > Signed-off-by: William Tu <u9012063@gmail.com> > --- > datapath-windows/ovsext/Conntrack.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c > index 471bf961b..b33676cc1 100644 > --- a/datapath-windows/ovsext/Conntrack.c > +++ b/datapath-windows/ovsext/Conntrack.c > @@ -1436,6 +1436,9 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx, > } > } > break; > + case OVS_CT_ATTR_TIMEOUT: > + return NDIS_STATUS_NOT_SUPPORTED; > + break; 'break;' here should not be necessary, I guess. > default: > OVS_LOG_TRACE("Invalid netlink attr type: %u", NlAttrType(ctAttr)); > break; Similar to the other patch, shouldn't the return just be part of the 'default' case instead of a 'break'. Skipping unknown attributes doesn't seem correct. Best regards, Ilya Maximets.
On Tue, May 31, 2022 at 12:09 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 5/31/22 20:29, William Tu wrote: > > CT timeout is not supported in Windows datapath, but > > currently it incorrectly reports to ovs-vswitchd as supported blow > > "system@ovs-system: Datapath supports timeout policy in conntrack > > action". The patches detects it and returns not support. > > > > Cc: Alin-Gabriel Serdean <aserdean@ovn.org> > > Cc: Wilson Peng <pweisong@vmware.com> > > Signed-off-by: William Tu <u9012063@gmail.com> > > --- > > datapath-windows/ovsext/Conntrack.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c > > index 471bf961b..b33676cc1 100644 > > --- a/datapath-windows/ovsext/Conntrack.c > > +++ b/datapath-windows/ovsext/Conntrack.c > > @@ -1436,6 +1436,9 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx, > > } > > } > > break; > > + case OVS_CT_ATTR_TIMEOUT: > > + return NDIS_STATUS_NOT_SUPPORTED; > > + break; > > 'break;' here should not be necessary, I guess. > > > default: > > OVS_LOG_TRACE("Invalid netlink attr type: %u", NlAttrType(ctAttr)); > > break; > > Similar to the other patch, shouldn't the return just be > part of the 'default' case instead of a 'break'. > Skipping unknown attributes doesn't seem correct. > Thanks for taking a look. yes, we should log it with the attribute info and return. I think the controller today should probe datapath features in ovsdb (ex: ovs-vsctl list-dp-cap) and based on the result to program each ovs-vswitchd, on Windows or Linux. Unfortunately the "ovs-vsctl list-dp-cap" is also broken in Windows, I will fix it too. Regards, William
-----Original Message----- From: William Tu <u9012063@gmail.com> Sent: Wednesday, June 1, 2022 2:13 AM To: Ilya Maximets <i.maximets@ovn.org> Cc: <dev@openvswitch.org> <dev@openvswitch.org>; Alin Gabriel Serdean <aserdean@ovn.org> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Detect CT timeout. On Tue, May 31, 2022 at 12:09 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 5/31/22 20:29, William Tu wrote: > > CT timeout is not supported in Windows datapath, but currently it > > incorrectly reports to ovs-vswitchd as supported blow > > "system@ovs-system: Datapath supports timeout policy in conntrack > > action". The patches detects it and returns not support. > > > > Cc: Alin-Gabriel Serdean <aserdean@ovn.org> > > Cc: Wilson Peng <pweisong@vmware.com> > > Signed-off-by: William Tu <u9012063@gmail.com> > > --- > > datapath-windows/ovsext/Conntrack.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/datapath-windows/ovsext/Conntrack.c > > b/datapath-windows/ovsext/Conntrack.c > > index 471bf961b..b33676cc1 100644 > > --- a/datapath-windows/ovsext/Conntrack.c > > +++ b/datapath-windows/ovsext/Conntrack.c > > @@ -1436,6 +1436,9 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx, > > } > > } > > break; > > + case OVS_CT_ATTR_TIMEOUT: > > + return NDIS_STATUS_NOT_SUPPORTED; > > + break; > > 'break;' here should not be necessary, I guess. > > > default: > > OVS_LOG_TRACE("Invalid netlink attr type: %u", NlAttrType(ctAttr)); > > break; > > Similar to the other patch, shouldn't the return just be part of the > 'default' case instead of a 'break'. > Skipping unknown attributes doesn't seem correct. > Thanks for taking a look. yes, we should log it with the attribute info and return. [Alin] FWIW the skip is due to the fact that not all the actions were implemented and skipping those which we didn't knew what to do with them was the next best thing. That's the story behind it 😊. @William Tu We use to add code here OvsProbeSupportedFeature (https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Flow.c#L3270-L3281) to probe features supported by the windows datapath. That returns an encapsulated netlink message back to vswitchd. Let me know if you me to write a patch for it. I think the controller today should probe datapath features in ovsdb (ex: ovs-vsctl list-dp-cap) and based on the result to program each ovs-vswitchd, on Windows or Linux. Unfortunately the "ovs-vsctl list-dp-cap" is also broken in Windows, I will fix it too. [Alin] I'm not familiar with that command. When was it added? Regards, William
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 471bf961b..b33676cc1 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -1436,6 +1436,9 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx, } } break; + case OVS_CT_ATTR_TIMEOUT: + return NDIS_STATUS_NOT_SUPPORTED; + break; default: OVS_LOG_TRACE("Invalid netlink attr type: %u", NlAttrType(ctAttr)); break;
CT timeout is not supported in Windows datapath, but currently it incorrectly reports to ovs-vswitchd as supported blow "system@ovs-system: Datapath supports timeout policy in conntrack action". The patches detects it and returns not support. Cc: Alin-Gabriel Serdean <aserdean@ovn.org> Cc: Wilson Peng <pweisong@vmware.com> Signed-off-by: William Tu <u9012063@gmail.com> --- datapath-windows/ovsext/Conntrack.c | 3 +++ 1 file changed, 3 insertions(+)