| Message ID | 20221219161814.55422-6-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drop sampling: Fixes and optimizations | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
Bleep bloop. Greetings Adrian Moreno, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: sha1 information is lacking or useless (controller/ovn-controller.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 controller: only sample flow if Collector Set exists When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
The code change on its own is fine, so Acked-by: Mark Michelson <mmichels@redhat.com> I have a note down below. On 12/19/22 11:18, Adrian Moreno wrote: > Adding a OFPACT_SAMPLE action to a flow is useless (and even detrimental > in terms of performance) if a Flow_Sample_Collector_Set row does not > exist with the correspondent id. The sample (i.e: upcall) would take > place but ovs-vswitchd would not have a target IPFIX collector to send > the sample to. > > In order to be able to control what Chassis perform sampling without > incurring in any performance cost if not needed, this patch makes the > controller only encode the OFPACT_SAMPLE if there's a valid > Flow_Sample_Collector_Set configured on that chassis. > > In order to do that, a new input is added to the en_lflow_output engine > node that is associated with the OVSDB table. But since the information > has to be available in the lib/actions layer, an intermediate helper > struct is added (in lib/ovn-util.{h,c}) to help store and query the > configured collector set ids instead of using the idl directly. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > controller/lflow.c | 1 + > controller/lflow.h | 8 ++++-- > controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++++++++++- > include/ovn/actions.h | 4 +++ > lib/actions.c | 9 +++++- > lib/ovn-util.c | 51 +++++++++++++++++++++++++++++++++ > lib/ovn-util.h | 27 +++++++++++++++--- > tests/ovn-performance.at | 24 ++++++++++++++++ > tests/ovn.at | 8 ++++++ > tests/test-ovn.c | 9 ++++++ > 10 files changed, 188 insertions(+), 9 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index bb47bb0c7..d2d0a6d1c 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -911,6 +911,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > .is_switch = ldp->is_switch, > .group_table = l_ctx_out->group_table, > .meter_table = l_ctx_out->meter_table, > + .collector_ids = l_ctx_in->collector_ids, > .lflow_uuid = lflow->header_.uuid, > .dp_key = ldp->datapath->tunnel_key, > > diff --git a/controller/lflow.h b/controller/lflow.h > index 9e8f9afd3..9bb61c039 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -43,11 +43,12 @@ > #include "openvswitch/uuid.h" > #include "openvswitch/list.h" > > -struct ovn_extend_table; > -struct ovsdb_idl_index; > -struct ovn_desired_flow_table; > struct hmap; > struct hmap_node; > +struct ovn_desired_flow_table; > +struct ovn_extend_table; > +struct ovsdb_idl_index; > +struct ovsrec_flow_sample_collector_set_table; > struct sbrec_chassis; > struct sbrec_load_balancer; > struct sbrec_logical_flow_table; > @@ -114,6 +115,7 @@ struct lflow_ctx_in { > const struct hmap *dhcpv6_opts; > const struct controller_event_options *controller_event_opts; > const struct smap *template_vars; > + const struct flow_collector_ids *collector_ids; > bool lb_hairpin_use_ct_mark; > }; > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 88eeee2d9..09f843541 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -988,6 +988,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_private_key); > ovsdb_idl_add_table(ovs_idl, &ovsrec_table_datapath); > ovsdb_idl_add_column(ovs_idl, &ovsrec_datapath_col_capabilities); > + ovsdb_idl_add_table(ovs_idl, &ovsrec_table_flow_sample_collector_set); > + > chassis_register_ovs_idl(ovs_idl); > encaps_register_ovs_idl(ovs_idl); > binding_register_ovs_idl(ovs_idl); > @@ -1004,6 +1006,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids); > + ovsdb_idl_track_add_column(ovs_idl, > + &ovsrec_flow_sample_collector_set_col_bridge); > + ovsdb_idl_track_add_column(ovs_idl, > + &ovsrec_flow_sample_collector_set_col_id); > } > > #define SB_NODES \ > @@ -1042,7 +1048,8 @@ enum sb_engine_node { > OVS_NODE(bridge, "bridge") \ > OVS_NODE(port, "port") \ > OVS_NODE(interface, "interface") \ > - OVS_NODE(qos, "qos") > + OVS_NODE(qos, "qos") \ > + OVS_NODE(flow_sample_collector_set, "flow_sample_collector_set") > > enum ovs_engine_node { > #define OVS_NODE(NAME, NAME_STR) OVS_##NAME, > @@ -2813,6 +2820,9 @@ struct ed_type_lflow_output { > > /* Fixed controller_event supported options. */ > struct controller_event_options controller_event_opts; > + > + /* Configured Flow Sample Collector Sets. */ > + struct flow_collector_ids collector_ids; > }; > > static void > @@ -2951,6 +2961,7 @@ init_lflow_ctx(struct engine_node *node, > l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts; > l_ctx_in->controller_event_opts = &fo->controller_event_opts; > l_ctx_in->template_vars = &template_vars->local_templates; > + l_ctx_in->collector_ids = &fo->collector_ids; > > l_ctx_out->flow_table = &fo->flow_table; > l_ctx_out->group_table = &fo->group_table; > @@ -2980,6 +2991,7 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, > data->hd.pool = id_pool_create(1, UINT32_MAX - 1); > nd_ra_opts_init(&data->nd_ra_opts); > controller_event_opts_init(&data->controller_event_opts); > + flow_collector_ids_init(&data->collector_ids); > return data; > } > > @@ -3006,6 +3018,7 @@ en_lflow_output_cleanup(void *data) > id_pool_destroy(flow_output_data->hd.pool); > nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); > controller_event_opts_destroy(&flow_output_data->controller_event_opts); > + flow_collector_ids_destroy(&flow_output_data->collector_ids); > } > > static void > @@ -3075,6 +3088,45 @@ lflow_output_sb_logical_flow_handler(struct engine_node *node, void *data) > return handled; > } > > +static bool > +lflow_output_flow_sample_collector_set_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + const struct ovsrec_flow_sample_collector_set_table *flow_collector_table = > + EN_OVSDB_GET(engine_get_input("OVS_flow_sample_collector_set", node)); > + const struct ovsrec_open_vswitch_table *ovs_table = > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > + const struct ovsrec_bridge_table *bridge_table = > + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > + > + const struct ovsrec_open_vswitch *cfg = > + ovsrec_open_vswitch_table_first(ovs_table); > + if (!cfg) { > + return true; > + } > + > + const struct ovsrec_bridge *br_int; > + br_int = get_bridge(bridge_table, br_int_name(cfg)); > + if (!br_int) { > + return true; > + } > + > + const struct ovsrec_flow_sample_collector_set *set; > + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH_TRACKED (set, > + flow_collector_table) { > + if (set->bridge == br_int) { > + struct ed_type_lflow_output *lfo = data; > + flow_collector_ids_clear(&lfo->collector_ids); > + flow_collector_ids_init_from_table(&lfo->collector_ids, > + flow_collector_table); > + return false; > + } > + } > + > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > static bool > lflow_output_sb_mac_binding_handler(struct engine_node *node, void *data) > { > @@ -4179,6 +4231,8 @@ main(int argc, char *argv[]) > > engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL); > engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL); > + engine_add_input(&en_lflow_output, &en_ovs_flow_sample_collector_set, > + lflow_output_flow_sample_collector_set_handler); > > engine_add_input(&en_lflow_output, &en_sb_mac_binding, > lflow_output_sb_mac_binding_handler); > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index a56351081..ed166cdd0 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -33,6 +33,7 @@ struct ofpbuf; > struct shash; > struct simap; > struct ovn_extend_table; > +struct collector_set_ids; > > /* List of OVN logical actions. > * > @@ -814,6 +815,9 @@ struct ovnact_encode_params { > /* A struct to figure out the meter_id for meter actions. */ > struct ovn_extend_table *meter_table; > > + /* A struct to lookup Flow_Sample_Collector_Set ids */ > + const struct flow_collector_ids *collector_ids; > + > /* The logical flow uuid that drove this action. */ > struct uuid lflow_uuid; > > diff --git a/lib/actions.c b/lib/actions.c > index 47ec654e1..523a51890 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -4337,7 +4337,14 @@ encode_SAMPLE(const struct ovnact_sample *sample, > const struct ovnact_encode_params *ep, > struct ofpbuf *ofpacts) > { > - struct ofpact_sample *os = ofpact_put_SAMPLE(ofpacts); > + struct ofpact_sample *os; > + if (!ep->collector_ids || > + !flow_collector_ids_lookup(ep->collector_ids, > + sample->collector_set_id)) { > + return; > + } > + > + os = ofpact_put_SAMPLE(ofpacts); > os->probability = sample->probability; > os->collector_set_id = sample->collector_set_id; > os->obs_domain_id = > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index 86b98acf7..670c7e2ad 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -965,3 +965,54 @@ lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family) > { > return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); > } > + > +void flow_collector_ids_init(struct flow_collector_ids *ids) > +{ > + ovs_list_init(&ids->list); > +} > + > +void flow_collector_ids_init_from_table(struct flow_collector_ids *ids, > + const struct ovsrec_flow_sample_collector_set_table *table) > +{ > + flow_collector_ids_init(ids); > + const struct ovsrec_flow_sample_collector_set *ovs_collector_set; > + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH (ovs_collector_set, > + table) { > + flow_collector_ids_add(ids, ovs_collector_set->id); > + } > +} > + > +void flow_collector_ids_add(struct flow_collector_ids *ids, > + uint64_t id) > +{ > + struct flow_collector_id *collector = xzalloc(sizeof *collector); > + collector->id = id; > + ovs_list_push_back(&ids->list, &collector->node); > +} > + > +bool flow_collector_ids_lookup(const struct flow_collector_ids *ids, > + uint32_t id) > +{ > + struct flow_collector_id *collector; > + LIST_FOR_EACH (collector, node, &ids->list) { > + if (collector->id == id) { > + return true; > + } > + } > + return false; > +} > + > +void flow_collector_ids_destroy(struct flow_collector_ids *ids) > +{ > + struct flow_collector_id *collector; > + LIST_FOR_EACH_SAFE (collector, node, &ids->list) { > + ovs_list_remove(&collector->node); > + free(collector); > + } > +} > + > +void flow_collector_ids_clear(struct flow_collector_ids *ids) > +{ > + flow_collector_ids_destroy(ids); > + flow_collector_ids_init(ids); > +} > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 809ff1d36..a60e65ca3 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -28,15 +28,16 @@ > #define ROUTE_ORIGIN_CONNECTED "connected" > #define ROUTE_ORIGIN_STATIC "static" > > +struct eth_addr; > struct nbrec_logical_router_port; > +struct ovsrec_flow_sample_collector_set_table; > +struct sbrec_datapath_binding; > struct sbrec_logical_flow; > +struct sbrec_port_binding; > +struct smap; > struct svec; > struct uuid; > -struct eth_addr; > -struct sbrec_port_binding; > -struct sbrec_datapath_binding; > struct unixctl_conn; > -struct smap; > > struct ipv4_netaddr { > ovs_be32 addr; /* 192.168.10.123 */ > @@ -318,4 +319,22 @@ int64_t daemon_startup_ts(void); > char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); > char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); > > +/* flow_collector_ids is a helper struct used to store and lookup > + * Flow_Sample_Collector_Set ids. */ > +struct flow_collector_id { > + struct ovs_list node; /* In flow_collector_ids->list */ > + uint64_t id; > +}; > +struct flow_collector_ids { > + struct ovs_list list; > +}; > +void flow_collector_ids_init(struct flow_collector_ids *); > +void flow_collector_ids_init_from_table(struct flow_collector_ids *, > + const struct ovsrec_flow_sample_collector_set_table *); > +void flow_collector_ids_add(struct flow_collector_ids *, uint64_t); > +bool flow_collector_ids_lookup(const struct flow_collector_ids *, uint32_t); > +void flow_collector_ids_destroy(struct flow_collector_ids *); > +void flow_collector_ids_clear(struct flow_collector_ids *); OVS provides an id_pool structure that might be a good fit here. It would save you from having to write most of the avove functions. You'd still need flow_collector_ids_init_from_table(), but the rest could be removed. The only drawbacks id_pool has are: 1) It uses 32-bit IDs instead of 64-bit IDs. 2) It doesn't provide a clear() function, only a destroy() function. If you think that id_pool would fit here, it could reduce the patch size some. However, if the above issues are dealbreakers, then stick with what you've already written. > + > + > #endif /* OVN_UTIL_H */ > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at > index db0e5272e..f3143409e 100644 > --- a/tests/ovn-performance.at > +++ b/tests/ovn-performance.at > @@ -586,6 +586,30 @@ OVN_CONTROLLER_EXPECT_NO_HIT( > [ovn-nbctl --wait=hv meter-del meter0] > ) > > +OVN_CONTROLLER_EXPECT_HIT( > + [hv1], [lflow_run], > + [as hv1 ovs-vsctl --id=@br get Bridge br-int -- \ > + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ > + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] > +) > + > +OVN_CONTROLLER_EXPECT_NO_HIT( > + [hv3], [lflow_run], > + [as hv2 ovs-vsctl --id=@br get Bridge br-int -- \ > + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ > + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] > +) > + > +OVN_CONTROLLER_EXPECT_NO_HIT( > + [hv1], [lflow_run], > + [as hv1 ovs-vsctl set IPFIX . targets=\"192.168.1.2\"] > +) > + > +OVN_CONTROLLER_EXPECT_HIT( > + [hv1], [lflow_run], > + [as hv1 ovs-vsctl destroy Flow_Sample_Collector_Set .] > +) > + > for i in 1 2; do > j=$((i%2 + 1)) > lp=lp$i > diff --git a/tests/ovn.at b/tests/ovn.at > index 4953bee49..2c3762e63 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -2171,6 +2171,10 @@ sample(probability=10); > formats as sample(probability=10,collector_set=0,obs_domain=0,obs_point=0); > encodes as sample(probability=10,collector_set_id=0,obs_domain_id=11259375,obs_point_id=0) > > +# sample with a collector_set_id that is not configured in the chassis. > +sample(probability=100,collector_set=999,obs_domain=0,obs_point=1000); > + encodes as drop > + > sample(probability=0,collector_set=200,obs_domain=0,obs_point=1000); > probability must be greater than zero > > @@ -33245,6 +33249,10 @@ check_sample_drops() { > # Take match part of flows that contain "drop". > drop_matches="$(grep 'drop' oflows_nosample | grep -oP 'table=\d*, priority=.* ')" > > + ovs-vsctl --id=@br get Bridge br-int -- \ > + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ > + create Flow_Sample_Collector_Set bridge=@br id=123 ipfix=@i > + > check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123" \ > -- set NB_Global . options:debug_drop_domain_id="1" > check ovn-nbctl --wait=hv sync > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index 64b0a2e20..d58350dcf 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -34,6 +34,7 @@ > #include "ovn/logical-fields.h" > #include "lib/ovn-l7.h" > #include "lib/extend-table.h" > +#include "lib/ovn-util.h" > #include "ovs-thread.h" > #include "ovstest.h" > #include "openvswitch/shash.h" > @@ -1302,6 +1303,12 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) > struct ovn_extend_table meter_table; > ovn_extend_table_init(&meter_table); > > + /* Initialize collector sets. */ > + struct flow_collector_ids collector_ids; > + flow_collector_ids_init(&collector_ids); > + flow_collector_ids_add(&collector_ids, 0); > + flow_collector_ids_add(&collector_ids, 200); > + > simap_init(&ports); > simap_put(&ports, "eth0", 5); > simap_put(&ports, "eth1", 6); > @@ -1348,6 +1355,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) > .is_switch = true, > .group_table = &group_table, > .meter_table = &meter_table, > + .collector_ids = &collector_ids, > > .pipeline = OVNACT_P_INGRESS, > .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE, > @@ -1434,6 +1442,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) > smap_destroy(&template_vars); > ovn_extend_table_destroy(&group_table); > ovn_extend_table_destroy(&meter_table); > + flow_collector_ids_destroy(&collector_ids); > exit(ok ? EXIT_SUCCESS : EXIT_FAILURE); > } >
On 1/6/23 20:15, Mark Michelson wrote: > The code change on its own is fine, so > > Acked-by: Mark Michelson <mmichels@redhat.com> > > I have a note down below. > > On 12/19/22 11:18, Adrian Moreno wrote: >> Adding a OFPACT_SAMPLE action to a flow is useless (and even detrimental >> in terms of performance) if a Flow_Sample_Collector_Set row does not >> exist with the correspondent id. The sample (i.e: upcall) would take >> place but ovs-vswitchd would not have a target IPFIX collector to send >> the sample to. >> >> In order to be able to control what Chassis perform sampling without >> incurring in any performance cost if not needed, this patch makes the >> controller only encode the OFPACT_SAMPLE if there's a valid >> Flow_Sample_Collector_Set configured on that chassis. >> >> In order to do that, a new input is added to the en_lflow_output engine >> node that is associated with the OVSDB table. But since the information >> has to be available in the lib/actions layer, an intermediate helper >> struct is added (in lib/ovn-util.{h,c}) to help store and query the >> configured collector set ids instead of using the idl directly. >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> controller/lflow.c | 1 + >> controller/lflow.h | 8 ++++-- >> controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++++++++++- >> include/ovn/actions.h | 4 +++ >> lib/actions.c | 9 +++++- >> lib/ovn-util.c | 51 +++++++++++++++++++++++++++++++++ >> lib/ovn-util.h | 27 +++++++++++++++--- >> tests/ovn-performance.at | 24 ++++++++++++++++ >> tests/ovn.at | 8 ++++++ >> tests/test-ovn.c | 9 ++++++ >> 10 files changed, 188 insertions(+), 9 deletions(-) >> >> diff --git a/controller/lflow.c b/controller/lflow.c >> index bb47bb0c7..d2d0a6d1c 100644 >> --- a/controller/lflow.c >> +++ b/controller/lflow.c >> @@ -911,6 +911,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow >> *lflow, >> .is_switch = ldp->is_switch, >> .group_table = l_ctx_out->group_table, >> .meter_table = l_ctx_out->meter_table, >> + .collector_ids = l_ctx_in->collector_ids, >> .lflow_uuid = lflow->header_.uuid, >> .dp_key = ldp->datapath->tunnel_key, >> diff --git a/controller/lflow.h b/controller/lflow.h >> index 9e8f9afd3..9bb61c039 100644 >> --- a/controller/lflow.h >> +++ b/controller/lflow.h >> @@ -43,11 +43,12 @@ >> #include "openvswitch/uuid.h" >> #include "openvswitch/list.h" >> -struct ovn_extend_table; >> -struct ovsdb_idl_index; >> -struct ovn_desired_flow_table; >> struct hmap; >> struct hmap_node; >> +struct ovn_desired_flow_table; >> +struct ovn_extend_table; >> +struct ovsdb_idl_index; >> +struct ovsrec_flow_sample_collector_set_table; >> struct sbrec_chassis; >> struct sbrec_load_balancer; >> struct sbrec_logical_flow_table; >> @@ -114,6 +115,7 @@ struct lflow_ctx_in { >> const struct hmap *dhcpv6_opts; >> const struct controller_event_options *controller_event_opts; >> const struct smap *template_vars; >> + const struct flow_collector_ids *collector_ids; >> bool lb_hairpin_use_ct_mark; >> }; >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 88eeee2d9..09f843541 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -988,6 +988,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) >> ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_private_key); >> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_datapath); >> ovsdb_idl_add_column(ovs_idl, &ovsrec_datapath_col_capabilities); >> + ovsdb_idl_add_table(ovs_idl, &ovsrec_table_flow_sample_collector_set); >> + >> chassis_register_ovs_idl(ovs_idl); >> encaps_register_ovs_idl(ovs_idl); >> binding_register_ovs_idl(ovs_idl); >> @@ -1004,6 +1006,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) >> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name); >> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces); >> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids); >> + ovsdb_idl_track_add_column(ovs_idl, >> + &ovsrec_flow_sample_collector_set_col_bridge); >> + ovsdb_idl_track_add_column(ovs_idl, >> + &ovsrec_flow_sample_collector_set_col_id); >> } >> #define SB_NODES \ >> @@ -1042,7 +1048,8 @@ enum sb_engine_node { >> OVS_NODE(bridge, "bridge") \ >> OVS_NODE(port, "port") \ >> OVS_NODE(interface, "interface") \ >> - OVS_NODE(qos, "qos") >> + OVS_NODE(qos, "qos") \ >> + OVS_NODE(flow_sample_collector_set, "flow_sample_collector_set") >> enum ovs_engine_node { >> #define OVS_NODE(NAME, NAME_STR) OVS_##NAME, >> @@ -2813,6 +2820,9 @@ struct ed_type_lflow_output { >> /* Fixed controller_event supported options. */ >> struct controller_event_options controller_event_opts; >> + >> + /* Configured Flow Sample Collector Sets. */ >> + struct flow_collector_ids collector_ids; >> }; >> static void >> @@ -2951,6 +2961,7 @@ init_lflow_ctx(struct engine_node *node, >> l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts; >> l_ctx_in->controller_event_opts = &fo->controller_event_opts; >> l_ctx_in->template_vars = &template_vars->local_templates; >> + l_ctx_in->collector_ids = &fo->collector_ids; >> l_ctx_out->flow_table = &fo->flow_table; >> l_ctx_out->group_table = &fo->group_table; >> @@ -2980,6 +2991,7 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, >> data->hd.pool = id_pool_create(1, UINT32_MAX - 1); >> nd_ra_opts_init(&data->nd_ra_opts); >> controller_event_opts_init(&data->controller_event_opts); >> + flow_collector_ids_init(&data->collector_ids); >> return data; >> } >> @@ -3006,6 +3018,7 @@ en_lflow_output_cleanup(void *data) >> id_pool_destroy(flow_output_data->hd.pool); >> nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); >> controller_event_opts_destroy(&flow_output_data->controller_event_opts); >> + flow_collector_ids_destroy(&flow_output_data->collector_ids); >> } >> static void >> @@ -3075,6 +3088,45 @@ lflow_output_sb_logical_flow_handler(struct engine_node >> *node, void *data) >> return handled; >> } >> +static bool >> +lflow_output_flow_sample_collector_set_handler(struct engine_node *node, >> + void *data OVS_UNUSED) >> +{ >> + const struct ovsrec_flow_sample_collector_set_table *flow_collector_table = >> + EN_OVSDB_GET(engine_get_input("OVS_flow_sample_collector_set", node)); >> + const struct ovsrec_open_vswitch_table *ovs_table = >> + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); >> + const struct ovsrec_bridge_table *bridge_table = >> + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); >> + >> + const struct ovsrec_open_vswitch *cfg = >> + ovsrec_open_vswitch_table_first(ovs_table); >> + if (!cfg) { >> + return true; >> + } >> + >> + const struct ovsrec_bridge *br_int; >> + br_int = get_bridge(bridge_table, br_int_name(cfg)); >> + if (!br_int) { >> + return true; >> + } >> + >> + const struct ovsrec_flow_sample_collector_set *set; >> + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH_TRACKED (set, >> + flow_collector_table) { >> + if (set->bridge == br_int) { >> + struct ed_type_lflow_output *lfo = data; >> + flow_collector_ids_clear(&lfo->collector_ids); >> + flow_collector_ids_init_from_table(&lfo->collector_ids, >> + flow_collector_table); >> + return false; >> + } >> + } >> + >> + engine_set_node_state(node, EN_UPDATED); >> + return true; >> +} >> + >> static bool >> lflow_output_sb_mac_binding_handler(struct engine_node *node, void *data) >> { >> @@ -4179,6 +4231,8 @@ main(int argc, char *argv[]) >> engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL); >> engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL); >> + engine_add_input(&en_lflow_output, &en_ovs_flow_sample_collector_set, >> + lflow_output_flow_sample_collector_set_handler); >> engine_add_input(&en_lflow_output, &en_sb_mac_binding, >> lflow_output_sb_mac_binding_handler); >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index a56351081..ed166cdd0 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -33,6 +33,7 @@ struct ofpbuf; >> struct shash; >> struct simap; >> struct ovn_extend_table; >> +struct collector_set_ids; >> /* List of OVN logical actions. >> * >> @@ -814,6 +815,9 @@ struct ovnact_encode_params { >> /* A struct to figure out the meter_id for meter actions. */ >> struct ovn_extend_table *meter_table; >> + /* A struct to lookup Flow_Sample_Collector_Set ids */ >> + const struct flow_collector_ids *collector_ids; >> + >> /* The logical flow uuid that drove this action. */ >> struct uuid lflow_uuid; >> diff --git a/lib/actions.c b/lib/actions.c >> index 47ec654e1..523a51890 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -4337,7 +4337,14 @@ encode_SAMPLE(const struct ovnact_sample *sample, >> const struct ovnact_encode_params *ep, >> struct ofpbuf *ofpacts) >> { >> - struct ofpact_sample *os = ofpact_put_SAMPLE(ofpacts); >> + struct ofpact_sample *os; >> + if (!ep->collector_ids || >> + !flow_collector_ids_lookup(ep->collector_ids, >> + sample->collector_set_id)) { >> + return; >> + } >> + >> + os = ofpact_put_SAMPLE(ofpacts); >> os->probability = sample->probability; >> os->collector_set_id = sample->collector_set_id; >> os->obs_domain_id = >> diff --git a/lib/ovn-util.c b/lib/ovn-util.c >> index 86b98acf7..670c7e2ad 100644 >> --- a/lib/ovn-util.c >> +++ b/lib/ovn-util.c >> @@ -965,3 +965,54 @@ lr_lb_address_set_ref(uint32_t lr_tunnel_key, int >> addr_family) >> { >> return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); >> } >> + >> +void flow_collector_ids_init(struct flow_collector_ids *ids) >> +{ >> + ovs_list_init(&ids->list); >> +} >> + >> +void flow_collector_ids_init_from_table(struct flow_collector_ids *ids, >> + const struct ovsrec_flow_sample_collector_set_table *table) >> +{ >> + flow_collector_ids_init(ids); >> + const struct ovsrec_flow_sample_collector_set *ovs_collector_set; >> + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH (ovs_collector_set, >> + table) { >> + flow_collector_ids_add(ids, ovs_collector_set->id); >> + } >> +} >> + >> +void flow_collector_ids_add(struct flow_collector_ids *ids, >> + uint64_t id) >> +{ >> + struct flow_collector_id *collector = xzalloc(sizeof *collector); >> + collector->id = id; >> + ovs_list_push_back(&ids->list, &collector->node); >> +} >> + >> +bool flow_collector_ids_lookup(const struct flow_collector_ids *ids, >> + uint32_t id) >> +{ >> + struct flow_collector_id *collector; >> + LIST_FOR_EACH (collector, node, &ids->list) { >> + if (collector->id == id) { >> + return true; >> + } >> + } >> + return false; >> +} >> + >> +void flow_collector_ids_destroy(struct flow_collector_ids *ids) >> +{ >> + struct flow_collector_id *collector; >> + LIST_FOR_EACH_SAFE (collector, node, &ids->list) { >> + ovs_list_remove(&collector->node); >> + free(collector); >> + } >> +} >> + >> +void flow_collector_ids_clear(struct flow_collector_ids *ids) >> +{ >> + flow_collector_ids_destroy(ids); >> + flow_collector_ids_init(ids); >> +} >> diff --git a/lib/ovn-util.h b/lib/ovn-util.h >> index 809ff1d36..a60e65ca3 100644 >> --- a/lib/ovn-util.h >> +++ b/lib/ovn-util.h >> @@ -28,15 +28,16 @@ >> #define ROUTE_ORIGIN_CONNECTED "connected" >> #define ROUTE_ORIGIN_STATIC "static" >> +struct eth_addr; >> struct nbrec_logical_router_port; >> +struct ovsrec_flow_sample_collector_set_table; >> +struct sbrec_datapath_binding; >> struct sbrec_logical_flow; >> +struct sbrec_port_binding; >> +struct smap; >> struct svec; >> struct uuid; >> -struct eth_addr; >> -struct sbrec_port_binding; >> -struct sbrec_datapath_binding; >> struct unixctl_conn; >> -struct smap; >> struct ipv4_netaddr { >> ovs_be32 addr; /* 192.168.10.123 */ >> @@ -318,4 +319,22 @@ int64_t daemon_startup_ts(void); >> char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); >> char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); >> +/* flow_collector_ids is a helper struct used to store and lookup >> + * Flow_Sample_Collector_Set ids. */ >> +struct flow_collector_id { >> + struct ovs_list node; /* In flow_collector_ids->list */ >> + uint64_t id; >> +}; >> +struct flow_collector_ids { >> + struct ovs_list list; >> +}; >> +void flow_collector_ids_init(struct flow_collector_ids *); >> +void flow_collector_ids_init_from_table(struct flow_collector_ids *, >> + const struct ovsrec_flow_sample_collector_set_table *); >> +void flow_collector_ids_add(struct flow_collector_ids *, uint64_t); >> +bool flow_collector_ids_lookup(const struct flow_collector_ids *, uint32_t); >> +void flow_collector_ids_destroy(struct flow_collector_ids *); >> +void flow_collector_ids_clear(struct flow_collector_ids *); > > OVS provides an id_pool structure that might be a good fit here. It would save > you from having to write most of the avove functions. You'd still need > flow_collector_ids_init_from_table(), but the rest could be removed. The only > drawbacks id_pool has are: > > 1) It uses 32-bit IDs instead of 64-bit IDs. > 2) It doesn't provide a clear() function, only a destroy() function. > > If you think that id_pool would fit here, it could reduce the patch size some. > However, if the above issues are dealbreakers, then stick with what you've > already written. > Thanks for the pointer Mark! Although the id at the OVSDB level is 64bit, at the action level it's 32bit so I think it does fit. I won't be ussing the allocation part of id_pool but it'll definitely make this patch smaller and simpler. >> + >> + >> #endif /* OVN_UTIL_H */ >> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at >> index db0e5272e..f3143409e 100644 >> --- a/tests/ovn-performance.at >> +++ b/tests/ovn-performance.at >> @@ -586,6 +586,30 @@ OVN_CONTROLLER_EXPECT_NO_HIT( >> [ovn-nbctl --wait=hv meter-del meter0] >> ) >> +OVN_CONTROLLER_EXPECT_HIT( >> + [hv1], [lflow_run], >> + [as hv1 ovs-vsctl --id=@br get Bridge br-int -- \ >> + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ >> + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] >> +) >> + >> +OVN_CONTROLLER_EXPECT_NO_HIT( >> + [hv3], [lflow_run], >> + [as hv2 ovs-vsctl --id=@br get Bridge br-int -- \ >> + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ >> + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] >> +) >> + >> +OVN_CONTROLLER_EXPECT_NO_HIT( >> + [hv1], [lflow_run], >> + [as hv1 ovs-vsctl set IPFIX . targets=\"192.168.1.2\"] >> +) >> + >> +OVN_CONTROLLER_EXPECT_HIT( >> + [hv1], [lflow_run], >> + [as hv1 ovs-vsctl destroy Flow_Sample_Collector_Set .] >> +) >> + >> for i in 1 2; do >> j=$((i%2 + 1)) >> lp=lp$i >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 4953bee49..2c3762e63 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -2171,6 +2171,10 @@ sample(probability=10); >> formats as sample(probability=10,collector_set=0,obs_domain=0,obs_point=0); >> encodes as >> sample(probability=10,collector_set_id=0,obs_domain_id=11259375,obs_point_id=0) >> +# sample with a collector_set_id that is not configured in the chassis. >> +sample(probability=100,collector_set=999,obs_domain=0,obs_point=1000); >> + encodes as drop >> + >> sample(probability=0,collector_set=200,obs_domain=0,obs_point=1000); >> probability must be greater than zero >> @@ -33245,6 +33249,10 @@ check_sample_drops() { >> # Take match part of flows that contain "drop". >> drop_matches="$(grep 'drop' oflows_nosample | grep -oP 'table=\d*, >> priority=.* ')" >> + ovs-vsctl --id=@br get Bridge br-int -- \ >> + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ >> + create Flow_Sample_Collector_Set bridge=@br id=123 ipfix=@i >> + >> check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123" \ >> -- set NB_Global . options:debug_drop_domain_id="1" >> check ovn-nbctl --wait=hv sync >> diff --git a/tests/test-ovn.c b/tests/test-ovn.c >> index 64b0a2e20..d58350dcf 100644 >> --- a/tests/test-ovn.c >> +++ b/tests/test-ovn.c >> @@ -34,6 +34,7 @@ >> #include "ovn/logical-fields.h" >> #include "lib/ovn-l7.h" >> #include "lib/extend-table.h" >> +#include "lib/ovn-util.h" >> #include "ovs-thread.h" >> #include "ovstest.h" >> #include "openvswitch/shash.h" >> @@ -1302,6 +1303,12 @@ test_parse_actions(struct ovs_cmdl_context *ctx >> OVS_UNUSED) >> struct ovn_extend_table meter_table; >> ovn_extend_table_init(&meter_table); >> + /* Initialize collector sets. */ >> + struct flow_collector_ids collector_ids; >> + flow_collector_ids_init(&collector_ids); >> + flow_collector_ids_add(&collector_ids, 0); >> + flow_collector_ids_add(&collector_ids, 200); >> + >> simap_init(&ports); >> simap_put(&ports, "eth0", 5); >> simap_put(&ports, "eth1", 6); >> @@ -1348,6 +1355,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) >> .is_switch = true, >> .group_table = &group_table, >> .meter_table = &meter_table, >> + .collector_ids = &collector_ids, >> .pipeline = OVNACT_P_INGRESS, >> .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE, >> @@ -1434,6 +1442,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) >> smap_destroy(&template_vars); >> ovn_extend_table_destroy(&group_table); >> ovn_extend_table_destroy(&meter_table); >> + flow_collector_ids_destroy(&collector_ids); >> exit(ok ? EXIT_SUCCESS : EXIT_FAILURE); >> } >> >
On 1/13/23 10:30, Adrian Moreno wrote: > > > On 1/6/23 20:15, Mark Michelson wrote: >> The code change on its own is fine, so >> >> Acked-by: Mark Michelson <mmichels@redhat.com> >> >> I have a note down below. >> >> On 12/19/22 11:18, Adrian Moreno wrote: >>> Adding a OFPACT_SAMPLE action to a flow is useless (and even detrimental >>> in terms of performance) if a Flow_Sample_Collector_Set row does not >>> exist with the correspondent id. The sample (i.e: upcall) would take >>> place but ovs-vswitchd would not have a target IPFIX collector to send >>> the sample to. >>> >>> In order to be able to control what Chassis perform sampling without >>> incurring in any performance cost if not needed, this patch makes the >>> controller only encode the OFPACT_SAMPLE if there's a valid >>> Flow_Sample_Collector_Set configured on that chassis. >>> >>> In order to do that, a new input is added to the en_lflow_output engine >>> node that is associated with the OVSDB table. But since the information >>> has to be available in the lib/actions layer, an intermediate helper >>> struct is added (in lib/ovn-util.{h,c}) to help store and query the >>> configured collector set ids instead of using the idl directly. >>> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>> --- >>> controller/lflow.c | 1 + >>> controller/lflow.h | 8 ++++-- >>> controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++++++++++- >>> include/ovn/actions.h | 4 +++ >>> lib/actions.c | 9 +++++- >>> lib/ovn-util.c | 51 +++++++++++++++++++++++++++++++++ >>> lib/ovn-util.h | 27 +++++++++++++++--- >>> tests/ovn-performance.at | 24 ++++++++++++++++ >>> tests/ovn.at | 8 ++++++ >>> tests/test-ovn.c | 9 ++++++ >>> 10 files changed, 188 insertions(+), 9 deletions(-) >>> >>> diff --git a/controller/lflow.c b/controller/lflow.c >>> index bb47bb0c7..d2d0a6d1c 100644 >>> --- a/controller/lflow.c >>> +++ b/controller/lflow.c >>> @@ -911,6 +911,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow >>> *lflow, >>> .is_switch = ldp->is_switch, >>> .group_table = l_ctx_out->group_table, >>> .meter_table = l_ctx_out->meter_table, >>> + .collector_ids = l_ctx_in->collector_ids, >>> .lflow_uuid = lflow->header_.uuid, >>> .dp_key = ldp->datapath->tunnel_key, >>> diff --git a/controller/lflow.h b/controller/lflow.h >>> index 9e8f9afd3..9bb61c039 100644 >>> --- a/controller/lflow.h >>> +++ b/controller/lflow.h >>> @@ -43,11 +43,12 @@ >>> #include "openvswitch/uuid.h" >>> #include "openvswitch/list.h" >>> -struct ovn_extend_table; >>> -struct ovsdb_idl_index; >>> -struct ovn_desired_flow_table; >>> struct hmap; >>> struct hmap_node; >>> +struct ovn_desired_flow_table; >>> +struct ovn_extend_table; >>> +struct ovsdb_idl_index; >>> +struct ovsrec_flow_sample_collector_set_table; >>> struct sbrec_chassis; >>> struct sbrec_load_balancer; >>> struct sbrec_logical_flow_table; >>> @@ -114,6 +115,7 @@ struct lflow_ctx_in { >>> const struct hmap *dhcpv6_opts; >>> const struct controller_event_options *controller_event_opts; >>> const struct smap *template_vars; >>> + const struct flow_collector_ids *collector_ids; >>> bool lb_hairpin_use_ct_mark; >>> }; >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 88eeee2d9..09f843541 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -988,6 +988,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) >>> ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_private_key); >>> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_datapath); >>> ovsdb_idl_add_column(ovs_idl, &ovsrec_datapath_col_capabilities); >>> + ovsdb_idl_add_table(ovs_idl, &ovsrec_table_flow_sample_collector_set); >>> + >>> chassis_register_ovs_idl(ovs_idl); >>> encaps_register_ovs_idl(ovs_idl); >>> binding_register_ovs_idl(ovs_idl); >>> @@ -1004,6 +1006,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) >>> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name); >>> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces); >>> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids); >>> + ovsdb_idl_track_add_column(ovs_idl, >>> + &ovsrec_flow_sample_collector_set_col_bridge); >>> + ovsdb_idl_track_add_column(ovs_idl, >>> + &ovsrec_flow_sample_collector_set_col_id); >>> } >>> #define SB_NODES \ >>> @@ -1042,7 +1048,8 @@ enum sb_engine_node { >>> OVS_NODE(bridge, "bridge") \ >>> OVS_NODE(port, "port") \ >>> OVS_NODE(interface, "interface") \ >>> - OVS_NODE(qos, "qos") >>> + OVS_NODE(qos, "qos") \ >>> + OVS_NODE(flow_sample_collector_set, "flow_sample_collector_set") >>> enum ovs_engine_node { >>> #define OVS_NODE(NAME, NAME_STR) OVS_##NAME, >>> @@ -2813,6 +2820,9 @@ struct ed_type_lflow_output { >>> /* Fixed controller_event supported options. */ >>> struct controller_event_options controller_event_opts; >>> + >>> + /* Configured Flow Sample Collector Sets. */ >>> + struct flow_collector_ids collector_ids; >>> }; >>> static void >>> @@ -2951,6 +2961,7 @@ init_lflow_ctx(struct engine_node *node, >>> l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts; >>> l_ctx_in->controller_event_opts = &fo->controller_event_opts; >>> l_ctx_in->template_vars = &template_vars->local_templates; >>> + l_ctx_in->collector_ids = &fo->collector_ids; >>> l_ctx_out->flow_table = &fo->flow_table; >>> l_ctx_out->group_table = &fo->group_table; >>> @@ -2980,6 +2991,7 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, >>> data->hd.pool = id_pool_create(1, UINT32_MAX - 1); >>> nd_ra_opts_init(&data->nd_ra_opts); >>> controller_event_opts_init(&data->controller_event_opts); >>> + flow_collector_ids_init(&data->collector_ids); >>> return data; >>> } >>> @@ -3006,6 +3018,7 @@ en_lflow_output_cleanup(void *data) >>> id_pool_destroy(flow_output_data->hd.pool); >>> nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); >>> controller_event_opts_destroy(&flow_output_data->controller_event_opts); >>> + flow_collector_ids_destroy(&flow_output_data->collector_ids); >>> } >>> static void >>> @@ -3075,6 +3088,45 @@ lflow_output_sb_logical_flow_handler(struct >>> engine_node *node, void *data) >>> return handled; >>> } >>> +static bool >>> +lflow_output_flow_sample_collector_set_handler(struct engine_node *node, >>> + void *data OVS_UNUSED) >>> +{ >>> + const struct ovsrec_flow_sample_collector_set_table *flow_collector_table = >>> + EN_OVSDB_GET(engine_get_input("OVS_flow_sample_collector_set", node)); >>> + const struct ovsrec_open_vswitch_table *ovs_table = >>> + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); >>> + const struct ovsrec_bridge_table *bridge_table = >>> + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); >>> + >>> + const struct ovsrec_open_vswitch *cfg = >>> + ovsrec_open_vswitch_table_first(ovs_table); >>> + if (!cfg) { >>> + return true; >>> + } >>> + >>> + const struct ovsrec_bridge *br_int; >>> + br_int = get_bridge(bridge_table, br_int_name(cfg)); >>> + if (!br_int) { >>> + return true; >>> + } >>> + >>> + const struct ovsrec_flow_sample_collector_set *set; >>> + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH_TRACKED (set, >>> + flow_collector_table) { >>> + if (set->bridge == br_int) { >>> + struct ed_type_lflow_output *lfo = data; >>> + flow_collector_ids_clear(&lfo->collector_ids); >>> + flow_collector_ids_init_from_table(&lfo->collector_ids, >>> + flow_collector_table); >>> + return false; >>> + } >>> + } >>> + >>> + engine_set_node_state(node, EN_UPDATED); >>> + return true; >>> +} >>> + >>> static bool >>> lflow_output_sb_mac_binding_handler(struct engine_node *node, void *data) >>> { >>> @@ -4179,6 +4231,8 @@ main(int argc, char *argv[]) >>> engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL); >>> engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL); >>> + engine_add_input(&en_lflow_output, &en_ovs_flow_sample_collector_set, >>> + lflow_output_flow_sample_collector_set_handler); >>> engine_add_input(&en_lflow_output, &en_sb_mac_binding, >>> lflow_output_sb_mac_binding_handler); >>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >>> index a56351081..ed166cdd0 100644 >>> --- a/include/ovn/actions.h >>> +++ b/include/ovn/actions.h >>> @@ -33,6 +33,7 @@ struct ofpbuf; >>> struct shash; >>> struct simap; >>> struct ovn_extend_table; >>> +struct collector_set_ids; >>> /* List of OVN logical actions. >>> * >>> @@ -814,6 +815,9 @@ struct ovnact_encode_params { >>> /* A struct to figure out the meter_id for meter actions. */ >>> struct ovn_extend_table *meter_table; >>> + /* A struct to lookup Flow_Sample_Collector_Set ids */ >>> + const struct flow_collector_ids *collector_ids; >>> + >>> /* The logical flow uuid that drove this action. */ >>> struct uuid lflow_uuid; >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 47ec654e1..523a51890 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -4337,7 +4337,14 @@ encode_SAMPLE(const struct ovnact_sample *sample, >>> const struct ovnact_encode_params *ep, >>> struct ofpbuf *ofpacts) >>> { >>> - struct ofpact_sample *os = ofpact_put_SAMPLE(ofpacts); >>> + struct ofpact_sample *os; >>> + if (!ep->collector_ids || >>> + !flow_collector_ids_lookup(ep->collector_ids, >>> + sample->collector_set_id)) { >>> + return; >>> + } >>> + >>> + os = ofpact_put_SAMPLE(ofpacts); >>> os->probability = sample->probability; >>> os->collector_set_id = sample->collector_set_id; >>> os->obs_domain_id = >>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c >>> index 86b98acf7..670c7e2ad 100644 >>> --- a/lib/ovn-util.c >>> +++ b/lib/ovn-util.c >>> @@ -965,3 +965,54 @@ lr_lb_address_set_ref(uint32_t lr_tunnel_key, int >>> addr_family) >>> { >>> return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); >>> } >>> + >>> +void flow_collector_ids_init(struct flow_collector_ids *ids) >>> +{ >>> + ovs_list_init(&ids->list); >>> +} >>> + >>> +void flow_collector_ids_init_from_table(struct flow_collector_ids *ids, >>> + const struct ovsrec_flow_sample_collector_set_table *table) >>> +{ >>> + flow_collector_ids_init(ids); >>> + const struct ovsrec_flow_sample_collector_set *ovs_collector_set; >>> + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH (ovs_collector_set, >>> + table) { >>> + flow_collector_ids_add(ids, ovs_collector_set->id); >>> + } >>> +} >>> + >>> +void flow_collector_ids_add(struct flow_collector_ids *ids, >>> + uint64_t id) >>> +{ >>> + struct flow_collector_id *collector = xzalloc(sizeof *collector); >>> + collector->id = id; >>> + ovs_list_push_back(&ids->list, &collector->node); >>> +} >>> + >>> +bool flow_collector_ids_lookup(const struct flow_collector_ids *ids, >>> + uint32_t id) >>> +{ >>> + struct flow_collector_id *collector; >>> + LIST_FOR_EACH (collector, node, &ids->list) { >>> + if (collector->id == id) { >>> + return true; >>> + } >>> + } >>> + return false; >>> +} >>> + >>> +void flow_collector_ids_destroy(struct flow_collector_ids *ids) >>> +{ >>> + struct flow_collector_id *collector; >>> + LIST_FOR_EACH_SAFE (collector, node, &ids->list) { >>> + ovs_list_remove(&collector->node); >>> + free(collector); >>> + } >>> +} >>> + >>> +void flow_collector_ids_clear(struct flow_collector_ids *ids) >>> +{ >>> + flow_collector_ids_destroy(ids); >>> + flow_collector_ids_init(ids); >>> +} >>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h >>> index 809ff1d36..a60e65ca3 100644 >>> --- a/lib/ovn-util.h >>> +++ b/lib/ovn-util.h >>> @@ -28,15 +28,16 @@ >>> #define ROUTE_ORIGIN_CONNECTED "connected" >>> #define ROUTE_ORIGIN_STATIC "static" >>> +struct eth_addr; >>> struct nbrec_logical_router_port; >>> +struct ovsrec_flow_sample_collector_set_table; >>> +struct sbrec_datapath_binding; >>> struct sbrec_logical_flow; >>> +struct sbrec_port_binding; >>> +struct smap; >>> struct svec; >>> struct uuid; >>> -struct eth_addr; >>> -struct sbrec_port_binding; >>> -struct sbrec_datapath_binding; >>> struct unixctl_conn; >>> -struct smap; >>> struct ipv4_netaddr { >>> ovs_be32 addr; /* 192.168.10.123 */ >>> @@ -318,4 +319,22 @@ int64_t daemon_startup_ts(void); >>> char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); >>> char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); >>> +/* flow_collector_ids is a helper struct used to store and lookup >>> + * Flow_Sample_Collector_Set ids. */ >>> +struct flow_collector_id { >>> + struct ovs_list node; /* In flow_collector_ids->list */ >>> + uint64_t id; >>> +}; >>> +struct flow_collector_ids { >>> + struct ovs_list list; >>> +}; >>> +void flow_collector_ids_init(struct flow_collector_ids *); >>> +void flow_collector_ids_init_from_table(struct flow_collector_ids *, >>> + const struct ovsrec_flow_sample_collector_set_table *); >>> +void flow_collector_ids_add(struct flow_collector_ids *, uint64_t); >>> +bool flow_collector_ids_lookup(const struct flow_collector_ids *, uint32_t); >>> +void flow_collector_ids_destroy(struct flow_collector_ids *); >>> +void flow_collector_ids_clear(struct flow_collector_ids *); >> >> OVS provides an id_pool structure that might be a good fit here. It would save >> you from having to write most of the avove functions. You'd still need >> flow_collector_ids_init_from_table(), but the rest could be removed. The only >> drawbacks id_pool has are: >> >> 1) It uses 32-bit IDs instead of 64-bit IDs. >> 2) It doesn't provide a clear() function, only a destroy() function. >> >> If you think that id_pool would fit here, it could reduce the patch size some. >> However, if the above issues are dealbreakers, then stick with what you've >> already written. >> > > Thanks for the pointer Mark! Although the id at the OVSDB level is 64bit, at the > action level it's 32bit so I think it does fit. I won't be ussing the allocation > part of id_pool but it'll definitely make this patch smaller and simpler. > I gave it another look and it doesn't seem such a drop-in placement. id_pool_add() is not exposed in the API, so it seems ids can only be requested to the pool which will give them back sequentially. In this case the values that we insert come from the OVSDB configuration so I don't think it'll work. Do you know of any other way to avoid the creation of another object (even if it's a small one like this)? >>> + >>> + >>> #endif /* OVN_UTIL_H */ >>> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at >>> index db0e5272e..f3143409e 100644 >>> --- a/tests/ovn-performance.at >>> +++ b/tests/ovn-performance.at >>> @@ -586,6 +586,30 @@ OVN_CONTROLLER_EXPECT_NO_HIT( >>> [ovn-nbctl --wait=hv meter-del meter0] >>> ) >>> +OVN_CONTROLLER_EXPECT_HIT( >>> + [hv1], [lflow_run], >>> + [as hv1 ovs-vsctl --id=@br get Bridge br-int -- \ >>> + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ >>> + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] >>> +) >>> + >>> +OVN_CONTROLLER_EXPECT_NO_HIT( >>> + [hv3], [lflow_run], >>> + [as hv2 ovs-vsctl --id=@br get Bridge br-int -- \ >>> + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ >>> + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] >>> +) >>> + >>> +OVN_CONTROLLER_EXPECT_NO_HIT( >>> + [hv1], [lflow_run], >>> + [as hv1 ovs-vsctl set IPFIX . targets=\"192.168.1.2\"] >>> +) >>> + >>> +OVN_CONTROLLER_EXPECT_HIT( >>> + [hv1], [lflow_run], >>> + [as hv1 ovs-vsctl destroy Flow_Sample_Collector_Set .] >>> +) >>> + >>> for i in 1 2; do >>> j=$((i%2 + 1)) >>> lp=lp$i >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index 4953bee49..2c3762e63 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -2171,6 +2171,10 @@ sample(probability=10); >>> formats as >>> sample(probability=10,collector_set=0,obs_domain=0,obs_point=0); >>> encodes as >>> sample(probability=10,collector_set_id=0,obs_domain_id=11259375,obs_point_id=0) >>> +# sample with a collector_set_id that is not configured in the chassis. >>> +sample(probability=100,collector_set=999,obs_domain=0,obs_point=1000); >>> + encodes as drop >>> + >>> sample(probability=0,collector_set=200,obs_domain=0,obs_point=1000); >>> probability must be greater than zero >>> @@ -33245,6 +33249,10 @@ check_sample_drops() { >>> # Take match part of flows that contain "drop". >>> drop_matches="$(grep 'drop' oflows_nosample | grep -oP 'table=\d*, >>> priority=.* ')" >>> + ovs-vsctl --id=@br get Bridge br-int -- \ >>> + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ >>> + create Flow_Sample_Collector_Set bridge=@br id=123 ipfix=@i >>> + >>> check ovn-nbctl -- set NB_Global . >>> options:debug_drop_collector_set="123" \ >>> -- set NB_Global . options:debug_drop_domain_id="1" >>> check ovn-nbctl --wait=hv sync >>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c >>> index 64b0a2e20..d58350dcf 100644 >>> --- a/tests/test-ovn.c >>> +++ b/tests/test-ovn.c >>> @@ -34,6 +34,7 @@ >>> #include "ovn/logical-fields.h" >>> #include "lib/ovn-l7.h" >>> #include "lib/extend-table.h" >>> +#include "lib/ovn-util.h" >>> #include "ovs-thread.h" >>> #include "ovstest.h" >>> #include "openvswitch/shash.h" >>> @@ -1302,6 +1303,12 @@ test_parse_actions(struct ovs_cmdl_context *ctx >>> OVS_UNUSED) >>> struct ovn_extend_table meter_table; >>> ovn_extend_table_init(&meter_table); >>> + /* Initialize collector sets. */ >>> + struct flow_collector_ids collector_ids; >>> + flow_collector_ids_init(&collector_ids); >>> + flow_collector_ids_add(&collector_ids, 0); >>> + flow_collector_ids_add(&collector_ids, 200); >>> + >>> simap_init(&ports); >>> simap_put(&ports, "eth0", 5); >>> simap_put(&ports, "eth1", 6); >>> @@ -1348,6 +1355,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx >>> OVS_UNUSED) >>> .is_switch = true, >>> .group_table = &group_table, >>> .meter_table = &meter_table, >>> + .collector_ids = &collector_ids, >>> .pipeline = OVNACT_P_INGRESS, >>> .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE, >>> @@ -1434,6 +1442,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx >>> OVS_UNUSED) >>> smap_destroy(&template_vars); >>> ovn_extend_table_destroy(&group_table); >>> ovn_extend_table_destroy(&meter_table); >>> + flow_collector_ids_destroy(&collector_ids); >>> exit(ok ? EXIT_SUCCESS : EXIT_FAILURE); >>> } >>> >> >
diff --git a/controller/lflow.c b/controller/lflow.c index bb47bb0c7..d2d0a6d1c 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -911,6 +911,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, .is_switch = ldp->is_switch, .group_table = l_ctx_out->group_table, .meter_table = l_ctx_out->meter_table, + .collector_ids = l_ctx_in->collector_ids, .lflow_uuid = lflow->header_.uuid, .dp_key = ldp->datapath->tunnel_key, diff --git a/controller/lflow.h b/controller/lflow.h index 9e8f9afd3..9bb61c039 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -43,11 +43,12 @@ #include "openvswitch/uuid.h" #include "openvswitch/list.h" -struct ovn_extend_table; -struct ovsdb_idl_index; -struct ovn_desired_flow_table; struct hmap; struct hmap_node; +struct ovn_desired_flow_table; +struct ovn_extend_table; +struct ovsdb_idl_index; +struct ovsrec_flow_sample_collector_set_table; struct sbrec_chassis; struct sbrec_load_balancer; struct sbrec_logical_flow_table; @@ -114,6 +115,7 @@ struct lflow_ctx_in { const struct hmap *dhcpv6_opts; const struct controller_event_options *controller_event_opts; const struct smap *template_vars; + const struct flow_collector_ids *collector_ids; bool lb_hairpin_use_ct_mark; }; diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 88eeee2d9..09f843541 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -988,6 +988,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_private_key); ovsdb_idl_add_table(ovs_idl, &ovsrec_table_datapath); ovsdb_idl_add_column(ovs_idl, &ovsrec_datapath_col_capabilities); + ovsdb_idl_add_table(ovs_idl, &ovsrec_table_flow_sample_collector_set); + chassis_register_ovs_idl(ovs_idl); encaps_register_ovs_idl(ovs_idl); binding_register_ovs_idl(ovs_idl); @@ -1004,6 +1006,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name); ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces); ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids); + ovsdb_idl_track_add_column(ovs_idl, + &ovsrec_flow_sample_collector_set_col_bridge); + ovsdb_idl_track_add_column(ovs_idl, + &ovsrec_flow_sample_collector_set_col_id); } #define SB_NODES \ @@ -1042,7 +1048,8 @@ enum sb_engine_node { OVS_NODE(bridge, "bridge") \ OVS_NODE(port, "port") \ OVS_NODE(interface, "interface") \ - OVS_NODE(qos, "qos") + OVS_NODE(qos, "qos") \ + OVS_NODE(flow_sample_collector_set, "flow_sample_collector_set") enum ovs_engine_node { #define OVS_NODE(NAME, NAME_STR) OVS_##NAME, @@ -2813,6 +2820,9 @@ struct ed_type_lflow_output { /* Fixed controller_event supported options. */ struct controller_event_options controller_event_opts; + + /* Configured Flow Sample Collector Sets. */ + struct flow_collector_ids collector_ids; }; static void @@ -2951,6 +2961,7 @@ init_lflow_ctx(struct engine_node *node, l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts; l_ctx_in->controller_event_opts = &fo->controller_event_opts; l_ctx_in->template_vars = &template_vars->local_templates; + l_ctx_in->collector_ids = &fo->collector_ids; l_ctx_out->flow_table = &fo->flow_table; l_ctx_out->group_table = &fo->group_table; @@ -2980,6 +2991,7 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, data->hd.pool = id_pool_create(1, UINT32_MAX - 1); nd_ra_opts_init(&data->nd_ra_opts); controller_event_opts_init(&data->controller_event_opts); + flow_collector_ids_init(&data->collector_ids); return data; } @@ -3006,6 +3018,7 @@ en_lflow_output_cleanup(void *data) id_pool_destroy(flow_output_data->hd.pool); nd_ra_opts_destroy(&flow_output_data->nd_ra_opts); controller_event_opts_destroy(&flow_output_data->controller_event_opts); + flow_collector_ids_destroy(&flow_output_data->collector_ids); } static void @@ -3075,6 +3088,45 @@ lflow_output_sb_logical_flow_handler(struct engine_node *node, void *data) return handled; } +static bool +lflow_output_flow_sample_collector_set_handler(struct engine_node *node, + void *data OVS_UNUSED) +{ + const struct ovsrec_flow_sample_collector_set_table *flow_collector_table = + EN_OVSDB_GET(engine_get_input("OVS_flow_sample_collector_set", node)); + const struct ovsrec_open_vswitch_table *ovs_table = + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); + const struct ovsrec_bridge_table *bridge_table = + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); + + const struct ovsrec_open_vswitch *cfg = + ovsrec_open_vswitch_table_first(ovs_table); + if (!cfg) { + return true; + } + + const struct ovsrec_bridge *br_int; + br_int = get_bridge(bridge_table, br_int_name(cfg)); + if (!br_int) { + return true; + } + + const struct ovsrec_flow_sample_collector_set *set; + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH_TRACKED (set, + flow_collector_table) { + if (set->bridge == br_int) { + struct ed_type_lflow_output *lfo = data; + flow_collector_ids_clear(&lfo->collector_ids); + flow_collector_ids_init_from_table(&lfo->collector_ids, + flow_collector_table); + return false; + } + } + + engine_set_node_state(node, EN_UPDATED); + return true; +} + static bool lflow_output_sb_mac_binding_handler(struct engine_node *node, void *data) { @@ -4179,6 +4231,8 @@ main(int argc, char *argv[]) engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL); engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL); + engine_add_input(&en_lflow_output, &en_ovs_flow_sample_collector_set, + lflow_output_flow_sample_collector_set_handler); engine_add_input(&en_lflow_output, &en_sb_mac_binding, lflow_output_sb_mac_binding_handler); diff --git a/include/ovn/actions.h b/include/ovn/actions.h index a56351081..ed166cdd0 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -33,6 +33,7 @@ struct ofpbuf; struct shash; struct simap; struct ovn_extend_table; +struct collector_set_ids; /* List of OVN logical actions. * @@ -814,6 +815,9 @@ struct ovnact_encode_params { /* A struct to figure out the meter_id for meter actions. */ struct ovn_extend_table *meter_table; + /* A struct to lookup Flow_Sample_Collector_Set ids */ + const struct flow_collector_ids *collector_ids; + /* The logical flow uuid that drove this action. */ struct uuid lflow_uuid; diff --git a/lib/actions.c b/lib/actions.c index 47ec654e1..523a51890 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -4337,7 +4337,14 @@ encode_SAMPLE(const struct ovnact_sample *sample, const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts) { - struct ofpact_sample *os = ofpact_put_SAMPLE(ofpacts); + struct ofpact_sample *os; + if (!ep->collector_ids || + !flow_collector_ids_lookup(ep->collector_ids, + sample->collector_set_id)) { + return; + } + + os = ofpact_put_SAMPLE(ofpacts); os->probability = sample->probability; os->collector_set_id = sample->collector_set_id; os->obs_domain_id = diff --git a/lib/ovn-util.c b/lib/ovn-util.c index 86b98acf7..670c7e2ad 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -965,3 +965,54 @@ lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family) { return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); } + +void flow_collector_ids_init(struct flow_collector_ids *ids) +{ + ovs_list_init(&ids->list); +} + +void flow_collector_ids_init_from_table(struct flow_collector_ids *ids, + const struct ovsrec_flow_sample_collector_set_table *table) +{ + flow_collector_ids_init(ids); + const struct ovsrec_flow_sample_collector_set *ovs_collector_set; + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH (ovs_collector_set, + table) { + flow_collector_ids_add(ids, ovs_collector_set->id); + } +} + +void flow_collector_ids_add(struct flow_collector_ids *ids, + uint64_t id) +{ + struct flow_collector_id *collector = xzalloc(sizeof *collector); + collector->id = id; + ovs_list_push_back(&ids->list, &collector->node); +} + +bool flow_collector_ids_lookup(const struct flow_collector_ids *ids, + uint32_t id) +{ + struct flow_collector_id *collector; + LIST_FOR_EACH (collector, node, &ids->list) { + if (collector->id == id) { + return true; + } + } + return false; +} + +void flow_collector_ids_destroy(struct flow_collector_ids *ids) +{ + struct flow_collector_id *collector; + LIST_FOR_EACH_SAFE (collector, node, &ids->list) { + ovs_list_remove(&collector->node); + free(collector); + } +} + +void flow_collector_ids_clear(struct flow_collector_ids *ids) +{ + flow_collector_ids_destroy(ids); + flow_collector_ids_init(ids); +} diff --git a/lib/ovn-util.h b/lib/ovn-util.h index 809ff1d36..a60e65ca3 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -28,15 +28,16 @@ #define ROUTE_ORIGIN_CONNECTED "connected" #define ROUTE_ORIGIN_STATIC "static" +struct eth_addr; struct nbrec_logical_router_port; +struct ovsrec_flow_sample_collector_set_table; +struct sbrec_datapath_binding; struct sbrec_logical_flow; +struct sbrec_port_binding; +struct smap; struct svec; struct uuid; -struct eth_addr; -struct sbrec_port_binding; -struct sbrec_datapath_binding; struct unixctl_conn; -struct smap; struct ipv4_netaddr { ovs_be32 addr; /* 192.168.10.123 */ @@ -318,4 +319,22 @@ int64_t daemon_startup_ts(void); char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); +/* flow_collector_ids is a helper struct used to store and lookup + * Flow_Sample_Collector_Set ids. */ +struct flow_collector_id { + struct ovs_list node; /* In flow_collector_ids->list */ + uint64_t id; +}; +struct flow_collector_ids { + struct ovs_list list; +}; +void flow_collector_ids_init(struct flow_collector_ids *); +void flow_collector_ids_init_from_table(struct flow_collector_ids *, + const struct ovsrec_flow_sample_collector_set_table *); +void flow_collector_ids_add(struct flow_collector_ids *, uint64_t); +bool flow_collector_ids_lookup(const struct flow_collector_ids *, uint32_t); +void flow_collector_ids_destroy(struct flow_collector_ids *); +void flow_collector_ids_clear(struct flow_collector_ids *); + + #endif /* OVN_UTIL_H */ diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at index db0e5272e..f3143409e 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -586,6 +586,30 @@ OVN_CONTROLLER_EXPECT_NO_HIT( [ovn-nbctl --wait=hv meter-del meter0] ) +OVN_CONTROLLER_EXPECT_HIT( + [hv1], [lflow_run], + [as hv1 ovs-vsctl --id=@br get Bridge br-int -- \ + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] +) + +OVN_CONTROLLER_EXPECT_NO_HIT( + [hv3], [lflow_run], + [as hv2 ovs-vsctl --id=@br get Bridge br-int -- \ + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ + create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i] +) + +OVN_CONTROLLER_EXPECT_NO_HIT( + [hv1], [lflow_run], + [as hv1 ovs-vsctl set IPFIX . targets=\"192.168.1.2\"] +) + +OVN_CONTROLLER_EXPECT_HIT( + [hv1], [lflow_run], + [as hv1 ovs-vsctl destroy Flow_Sample_Collector_Set .] +) + for i in 1 2; do j=$((i%2 + 1)) lp=lp$i diff --git a/tests/ovn.at b/tests/ovn.at index 4953bee49..2c3762e63 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -2171,6 +2171,10 @@ sample(probability=10); formats as sample(probability=10,collector_set=0,obs_domain=0,obs_point=0); encodes as sample(probability=10,collector_set_id=0,obs_domain_id=11259375,obs_point_id=0) +# sample with a collector_set_id that is not configured in the chassis. +sample(probability=100,collector_set=999,obs_domain=0,obs_point=1000); + encodes as drop + sample(probability=0,collector_set=200,obs_domain=0,obs_point=1000); probability must be greater than zero @@ -33245,6 +33249,10 @@ check_sample_drops() { # Take match part of flows that contain "drop". drop_matches="$(grep 'drop' oflows_nosample | grep -oP 'table=\d*, priority=.* ')" + ovs-vsctl --id=@br get Bridge br-int -- \ + --id=@i create IPFIX targets=\"192.168.1.1\" -- \ + create Flow_Sample_Collector_Set bridge=@br id=123 ipfix=@i + check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123" \ -- set NB_Global . options:debug_drop_domain_id="1" check ovn-nbctl --wait=hv sync diff --git a/tests/test-ovn.c b/tests/test-ovn.c index 64b0a2e20..d58350dcf 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -34,6 +34,7 @@ #include "ovn/logical-fields.h" #include "lib/ovn-l7.h" #include "lib/extend-table.h" +#include "lib/ovn-util.h" #include "ovs-thread.h" #include "ovstest.h" #include "openvswitch/shash.h" @@ -1302,6 +1303,12 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) struct ovn_extend_table meter_table; ovn_extend_table_init(&meter_table); + /* Initialize collector sets. */ + struct flow_collector_ids collector_ids; + flow_collector_ids_init(&collector_ids); + flow_collector_ids_add(&collector_ids, 0); + flow_collector_ids_add(&collector_ids, 200); + simap_init(&ports); simap_put(&ports, "eth0", 5); simap_put(&ports, "eth1", 6); @@ -1348,6 +1355,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) .is_switch = true, .group_table = &group_table, .meter_table = &meter_table, + .collector_ids = &collector_ids, .pipeline = OVNACT_P_INGRESS, .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE, @@ -1434,6 +1442,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED) smap_destroy(&template_vars); ovn_extend_table_destroy(&group_table); ovn_extend_table_destroy(&meter_table); + flow_collector_ids_destroy(&collector_ids); exit(ok ? EXIT_SUCCESS : EXIT_FAILURE); }
Adding a OFPACT_SAMPLE action to a flow is useless (and even detrimental in terms of performance) if a Flow_Sample_Collector_Set row does not exist with the correspondent id. The sample (i.e: upcall) would take place but ovs-vswitchd would not have a target IPFIX collector to send the sample to. In order to be able to control what Chassis perform sampling without incurring in any performance cost if not needed, this patch makes the controller only encode the OFPACT_SAMPLE if there's a valid Flow_Sample_Collector_Set configured on that chassis. In order to do that, a new input is added to the en_lflow_output engine node that is associated with the OVSDB table. But since the information has to be available in the lib/actions layer, an intermediate helper struct is added (in lib/ovn-util.{h,c}) to help store and query the configured collector set ids instead of using the idl directly. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- controller/lflow.c | 1 + controller/lflow.h | 8 ++++-- controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++++++++++- include/ovn/actions.h | 4 +++ lib/actions.c | 9 +++++- lib/ovn-util.c | 51 +++++++++++++++++++++++++++++++++ lib/ovn-util.h | 27 +++++++++++++++--- tests/ovn-performance.at | 24 ++++++++++++++++ tests/ovn.at | 8 ++++++ tests/test-ovn.c | 9 ++++++ 10 files changed, 188 insertions(+), 9 deletions(-)