On 6/26/24 07:14, Adrián Moreno wrote: > On Tue, Jun 25, 2024 at 11:04:42PM GMT, Ilya Maximets wrote: >> 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 <[email protected]> >>>>>>>> --- >>>>>>>> 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. > > Oh! It's true that it's docummented, although it does appear in > "list-commands". I thought we supported disabling features, I guess we > might on a case-by-case basis and through OVSDB. > >> >> 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. >> > > If "set-dp-features" is already an exceptional path, can't we just add a > "--force" option to it?
I guess, it's fine to add a flag. Just don't document it. But, please, add a scary warning in this case saying that unsupported feature is enabled and the behavior is unpredictable from this point on. Tests can ignore the warning. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
