On 6/25/24 22:53, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: >> On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: >>> On 6/24/24 15:19, Adrián Moreno wrote: >>>> On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: >>>>> On 6/5/24 22:23, Adrian Moreno wrote: >>>>>> Use the newly added emit_sample to implement OpenFlow sample() actions >>>>>> with local sampling configuration. >>>>>> >>>>>> Signed-off-by: Adrian Moreno <amore...@redhat.com> >>>>>> --- >>>>>> ofproto/ofproto-dpif-lsample.c | 17 ++++ >>>>>> ofproto/ofproto-dpif-lsample.h | 6 ++ >>>>>> ofproto/ofproto-dpif-xlate.c | 163 ++++++++++++++++++++++++--------- >>>>>> ofproto/ofproto-dpif-xlate.h | 3 +- >>>>>> ofproto/ofproto-dpif.c | 2 +- >>>>>> 5 files changed, 144 insertions(+), 47 deletions(-) >>>>> >>>>> Not a full review, just a note that this patch needs tests in >>>>> ofproto-dpif.at >>>>> that would check that we generate correct datapath flows. >>>>> >>>> >>>> I thought about that, but ofproto-dpif.at is based on dummy datapat >>>> (userspace) which does not support this action. The configuration will >>>> be ignored and the datapath actions will not be generated. That's why I >>>> relied on system-traffic.at tests. Do you see any way around this? >>> >>> We don't need actual datapath support if we're not sending any actual >>> trafic. You should be able to just turn on the capability and check >>> that ofproto/trace generates correct actions. >>> >>> We test kernel tunnels this way, for example. See the macro >>> OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. >>> >> > > Tried it but it doesn't seem to work. I now remember I hit this issue > originally :-). > We cannot enable a capability beyond the datapath's "boot time" > capabilities. > > If you try to run > "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command > fails with: > "Can not enable features not supported by the datapth" > > The safeguard does make sense in general (prevents users from > knee-capping themselves) but for testing maybe we want to force it? > > OTOH, marking this feature as supported in the userspace datapath would > not be catastrophic, we could just warn the action is not supported > in odp_execute(), or consider VLOG_DBG_RL as current implementation > until we come up with a good one. However, I cannot of a good way of > of properly reporting this to the user beforehand so I think it could > still cause a lot of confusion. > > Thoughts?
Hmm. This command is generally for testing only or experiments. It's not documented and hence we do not support changing datapath features in runtime. It should be fine to add another command like force-dp-features to overcome the restriction. Just make sure that it is not documented and doesn't appear in the list-commands output. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
