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? Thanks. Adrián _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
