kuraudo has quit [Remote host closed the connection]
kuraudo has joined #openvswitch
imaximets__ has quit [Ping timeout: 276 seconds]
imaximets__ has joined #openvswitch
imaximets_ has quit [Ping timeout: 252 seconds]
kuraudo has quit [Remote host closed the connection]
jistr has quit [Remote host closed the connection]
jistr has joined #openvswitch
erlon has joined #openvswitch
evplus has joined #openvswitch
kuraudo has joined #openvswitch
ChmEarl has joined #openvswitch
evplus has quit [Quit: Client closed]
imaximets__ has quit [Changing host]
imaximets__ has joined #openvswitch
imaximets__ is now known as imaximets
elvira1 has quit [Ping timeout: 272 seconds]
elvira has joined #openvswitch
elvira has quit [Ping timeout: 276 seconds]
mmichelson has joined #openvswitch
zhouhan has joined #openvswitch
<mmichelson>
Hi everyone. Welcome to the weekly OVN developers' meeting.
<fnordahl>
o/
<erlon>
\o
<zhouhan>
hi
<mmichelson>
Quick note before any updates are given: tomorrow is the scheduled soft freeze for OVN 25.09. If you have any feature patches you want considered for inclusion in 25.09, please get them posted by tomorrow.
<mmichelson>
My update is pretty short. Last week _lore_ and I managed to get the I-P datapath refactor patch posted. I've been doing reviews since then to try to reduce the load once soft freeze hits.
<mmichelson>
That's all from me.
<mmichelson>
Who would like to go next?
zhouhan90 has joined #openvswitch
* zhouhan90
connection broken for a while
<erlon>
Hey folks
<mmichelson>
hi erlon
<erlon>
me and Frode are investigating how to get stateful flows working on the userspace (dpdk)
<zhouhan90>
erlon: for the fragmentation issue with ACL?
<erlon>
currently if you create an ACL in openstack, fragmented packets do not came through
<erlon>
yes
<erlon>
we were able to syntetically reproduce to problem modifying one of the LB tests
zhouhan16 has joined #openvswitch
<mmichelson>
so many zhouhans today :)
<zhouhan16>
mmichelson: my connection sucks today
zhouhan has quit [Ping timeout: 272 seconds]
<erlon>
and noticed that we can areproduce the problem if we add an acl like this:
<amusil>
Well the problem with ACLs is that the match is used specified
<fnordahl>
as in it comes directly from CMS, right?
<amusil>
So we cannot easily replace it with ct_*
zhouhan90 has quit [Ping timeout: 272 seconds]
<amusil>
As it was done for LB
<amusil>
fnordahl Yeah
<fnordahl>
that is more or less our question. do we need to coordinate with CMS to fix this, or can we have northd rewrite the rules?
<amusil>
IMO northd shouldn't rewrite the rules
<erlon>
ow, that is pretty hot
<erlon>
yesterday
<imaximets>
I suppose, either northd would have to replace the matches or this just need to be documented that L4 fields cannot be directly used for stateful ACLs.
<amusil>
The CMS should know the best what it needs to match on
<amusil>
Well for TCP traffic it's not an issue right?
<zhouhan16>
amusil: right, it seems to be an issue only if the first the UDP packet is fragmented
<imaximets>
It is, if the first packet is fragmented for one reason or another. But yes, it is unlikely.
<zhouhan16>
imaximets: ahh, for one reason or another :D
<amusil>
So I guess as workaround they would need to match on ct_* fields risking HW offload breakage
<amusil>
Also in order to match on ct_* fields you need to ensure that the flows matches on ct.trk too
<amusil>
That was the only requirement imaximets right?
<imaximets>
Probably. To be clear, I'm not a fan of having users directly match on low level stuff like ct states...
<amusil>
We can discuss is the replace is acceptable or having a flag that would indicate that we can replace those fields might be an option
<fnordahl>
This is also what I'm thinking, how would a Neutron developer fully know the cirucmstances for when to do what?
<imaximets>
It is tricky and asks from them to know too much about the underlying implementation.
<amusil>
*if the
<zhouhan16>
imaximets: agreed. CMS shouldn't need to care about that low level of implementation
<amusil>
I don;t know if we are actually going into ACLs after recirculation, we might not have the CT state at that point too
<amusil>
So we would require extra stage, extra recirculation and potentially extra performance hit
<amusil>
Sounds a bit like an ACL flag to me
<imaximets>
But there must be ct() recirc anyway for stateful, isn't it? How else you're checking the state?
<zhouhan16>
Yes, for stateful ACLs
<amusil>
You could still have valid stateless ACLs filtering on ports
<imaximets>
Yes, there is no problem with stateless. Only with stateful. FOr stateless you just can't match on L4 at all if you care about fragments.
<zhouhan16>
imaximets: you are saying stateless ACL with L4 match doesn't support fragments :D
<imaximets>
Yep. They are never reassembled and there is no conntrack to get the ct state from.
<zhouhan16>
imaximets: understand, but just not sure if there is a requirement for that. Also, can we say the same thing for stateful ACL requirements? lol
<amusil>
Stateful are slightly different due to the reassemble, so I guess userspace CT is technically a regression in that sense?
<zhouhan16>
amusil: it seems to me the kernel did something it shouldn't, and we happened to rely on that in the past
<imaximets>
No, kernel ct action is not OF-compliant. The fact that kernel reassembles is a bug, but we can't fix it because we can't add another ct implementation.
<imaximets>
What zhouhan16 said.
<fnordahl>
fwiw, I like imaximets's argument of the kernel ct behaving odd here, especially thinking of the reassembly to a arbritrary boundary in the middle of a network
<amusil>
Alright so then we don't have a problem right? Because it shouldn't work then
<fnordahl>
could an apparocah be something like (pseudocode) if (stateful && matches_l4(match) && some_option) { rewrite_match(/* adds ct.new && ct_udp.dst, ct.trk && ct_udp.dst and so on */) } + chassis feature check and so on because we want to backport this
<amusil>
Ah to me this is completely new feature I don't like the sound of backport
<zhouhan16>
fnordahl: I think that "rewirte_match" part is not trivial
<amusil>
You definitely don't want to do it for all ACLs IMO
<amusil>
*all stateful ACLs to be clear
<zhouhan16>
It seems if we support this for stateful ACLs, people might ask the same for stateless ACLs
<fnordahl>
zhouhan16: but that is counter intuitive, how would stateless ACL, which does not track connections, know which fragment belongs to which other fragment?
<amusil>
Then stateless becomes stateful because we need the CT state to make it work
<mmichelson>
It sounds like a hard thing to support though. If the ACL isn't stateful, then how do we know how to handle the fragment?
<mmichelson>
fnordahl, jinx
<fnordahl>
I'm only interested in the stateful case fwiw
<zhouhan16>
fnordahl: ok, makes sense
<amusil>
It's still non-trivial to get right (with HWOL in mind)
<fnordahl>
I agree, and I see that as an argument for ovn to own this and not CMS?
<amusil>
Sure, but probably as a feature
<amusil>
Matching directly could be a workaround for CMS if needed
<zhouhan16>
amusil: the workaround also means no HW offload
<erlon>
do you think we can make that simple enough to make a backport reasonable?
<mmichelson>
Is hw offload expected when using DPDK? Sorry if that's a silly question
<amusil>
Right, but that's kinda the trade-off here, if it's document that's probably fine
<zhouhan16>
mmichelson: yes that's expected
<erlon>
I mean without the hw offload support
<imaximets>
zhouhan16, can't HW match on ct_dst, for example?
<amusil>
I don't think backport is on the table based on the discussion, but the patch/series might prove me wrong
<zhouhan16>
imaximets: at least not now
<imaximets>
ok
<fnordahl>
We would at least need the match fields to be there, could a compromise be backport support for matching on ct_* in OVN, opening for a workaround to be applied in CMS through ACL->mattch, and full ovn-northd match rewrite support a future feature?
<amusil>
fnordahl I guess that could be a possibility yes
<imaximets>
I'd like not to add ct_* matches in ACLs if we don't have them already.
<fnordahl>
It's supported on main, we do it in the test case supporting this discussion
<fnordahl>
(I know main is not released yet)
<amusil>
We do have them, zhouhan16 removed them in his recent series though
<zhouhan16>
yes, I removed them but I can add them back
<amusil>
Yeah we can add them back if that would be enough for the workaround that's fine
<erlon>
@zhouhan16 the one you linked?
<imaximets>
If we don't have them in any released version, I'd like to not have them, i.e. remove. :)
<fnordahl>
would that not also break the lb fragment fix?
<mmichelson>
fnordahl, I think zhouhan16 addresses that in his patch series.
<fnordahl>
ah, ok.
<amusil>
fnordahl Would you mind posting a atch that would add them back with the justification so we could have discussion u/s?
<zhouhan16>
It seems the workaround approach is the least intrusive, although I agree with imaximets that it is not a good interface for CMS :) . No better idea, if this is a must-have requirement
<fnordahl>
we are facing a hard requirement for this, yes
<amusil>
Well if backport is a must, there isn't anything better probably
<zhouhan16>
amusil: my patch is not merged yet, so I can keep them in v3
<amusil>
Eh it's in my backlog to merge :D It will be merged today
<mmichelson>
nice
<zhouhan16>
amusil: :D
<amusil>
Pretty please don't make redo the backlog once again :D
<mmichelson>
OK, it sounds like we have some sort of conclusion on this particular discussion. We'll look for fnordahl's patch to appear soon (hopefully by tomorrow).
<amusil>
I think that one could be considered a bug fix if we want to backport it?
<amusil>
Anyway it's better to have it up before the soft freeze just in case
<mmichelson>
Yeah, this is one of those that could be interpreted either way, depending on how you look at it.
<amusil>
I would also love to hear Dumitru's opinion on this and we can cc him on the patch
<mmichelson>
In the meantime, I want to look at something other than this IRC window :) So I'm going to move the meeting forward and ask if anyone else has an update to give.
<amusil>
I have
<mmichelson>
amusil, go right ahead
<amusil>
I have already mentioned there is a bunch of stuff going to be merged today, then I'll rebase the evpn series and post it for review tomorrow morning
<amusil>
That's it, thanks
<mmichelson>
amusil, great, thanks for the merges!
<mmichelson>
Who wants to go next?
<zhouhan16>
my one line update: I sent v2 for the ct_xxx fix with push/pop encapsulated to the actions
<mmichelson>
zhouhan16, great, thanks! Does anyone else have an update to give?
<imaximets>
No updates from me this week.
<mmichelson>
imaximets, ack
<mmichelson>
OK, I guess that's all for this week. Thank you everyone, and thanks for the good discussion on the impossible issue of fragmentation, userspace ct, and hw offload.
<fnordahl>
\o thanks all, cheers!
<erlon>
\o thanks all
<mmichelson>
bye!
<imaximets>
Thanks! Bye.
mmichelson has quit [Quit: Leaving]
<amusil>
Thanks, bye
amusil has quit [Quit: Client closed]
zhouhan16 has quit [Quit: Client closed]
kuraudo has quit [Remote host closed the connection]