On Wed, Jun 26, 2024 at 11:28:57AM GMT, Ilya Maximets wrote: > 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 <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. > > > > 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. >
I will not documment it but I don't think we should remove from "list-commands" either. And sure, I'll add a scary warning. Thanks. Adrián _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
