| Message ID | 20161210065633.32634-2-blp@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On 9 December 2016 at 22:56, Ben Pfaff <blp@ovn.org> wrote: > This makes it easy to find the logical flow that generated a particular > OpenFlow flow, by running "ovn-sbctl dump-flows <cookie>". > > Later, this can be refined (and automated for "ofproto/trace"), but this > is still a significant advance. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > lib/uuid.c | 22 +++++++- > lib/uuid.h | 3 +- > ovn/controller/lflow.c | 6 +- > ovn/controller/ofctrl.c | 6 +- > ovn/controller/ofctrl.h | 2 +- > ovn/controller/physical.c | 34 ++++++------ > ovn/utilities/ovn-sbctl.8.in | 13 ++++- > ovn/utilities/ovn-sbctl.c | 129 ++++++++++++++++++++++++++++++ > ++++++------- > 8 files changed, 169 insertions(+), 46 deletions(-) > > diff --git a/lib/uuid.c b/lib/uuid.c > index 0f2a58e..bd98d40 100644 > --- a/lib/uuid.c > +++ b/lib/uuid.c > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc. > +/* Copyright (c) 2008, 2009, 2010, 2011, 2013, 2016 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -210,6 +210,26 @@ error: > uuid_zero(uuid); > return false; > } > + > +/* Returns the number of characters at the beginning of 's' that are > valid for > + * a UUID. For example, the "123" at the beginning of "123xyzzy" could > begin a > + * UUID, so uuid_is_partial_string() would return 3; for "xyzzy", this > function > + * would return 0, since "x" can't start a UUID. */ > +int > +uuid_is_partial_string(const char *s) > +{ > + static const char tmpl[UUID_LEN] = "xxxxxxxx-xxxx-xxxx-xxxx- > xxxxxxxxxxxx"; > + size_t i; > + for (i = 0; i < UUID_LEN; i++) { > + if (tmpl[i] == 'x' > + ? hexit_value(s[i]) < 0 > + : s[i] != '-') { > + break; > + } > + } > + return i; > +} > + > > static void > sha1_update_int(struct sha1_ctx *sha1_ctx, uintmax_t x) > diff --git a/lib/uuid.h b/lib/uuid.h > index a5792b0..113574c 100644 > --- a/lib/uuid.h > +++ b/lib/uuid.h > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2008, 2009, 2010 Nicira, Inc. > +/* Copyright (c) 2008, 2009, 2010, 2016 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -61,6 +61,7 @@ bool uuid_is_zero(const struct uuid *); > int uuid_compare_3way(const struct uuid *, const struct uuid *); > bool uuid_from_string(struct uuid *, const char *); > bool uuid_from_string_prefix(struct uuid *, const char *); > +int uuid_is_partial_string(const char *); > void uuid_set_bits_v4(struct uuid *); > > #endif /* uuid.h */ > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > index 4e67365..5f49183 100644 > --- a/ovn/controller/lflow.c > +++ b/ovn/controller/lflow.c > @@ -302,7 +302,7 @@ consider_logical_flow(const struct lport_index *lports, > } > if (!m->n) { > ofctrl_add_flow(flow_table, ptable, lflow->priority, > &m->match, > - &ofpacts); > + lflow->header_.uuid.parts[0], &ofpacts); > } else { > uint64_t conj_stubs[64 / 8]; > struct ofpbuf conj; > @@ -318,7 +318,7 @@ consider_logical_flow(const struct lport_index *lports, > dst->n_clauses = src->n_clauses; > } > ofctrl_add_flow(flow_table, ptable, lflow->priority, > &m->match, > - &conj); > + 0, &conj); > If I understand it right, a logical flow of the form ip4.src == {a, b, c} after it is split into multiple openflow flows won't have a cookie? > ofpbuf_uninit(&conj); > } > } > @@ -386,7 +386,7 @@ consider_neighbor_flow(const struct lport_index > *lports, > uint64_t stub[1024 / 8]; > struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); > put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, &ofpacts); > - ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match, > &ofpacts); > + ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match, 0, > &ofpacts); > ofpbuf_uninit(&ofpacts); > } > > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c > index 1d8fbf3..7449293 100644 > --- a/ovn/controller/ofctrl.c > +++ b/ovn/controller/ofctrl.c > @@ -57,6 +57,7 @@ struct ovn_flow { > /* Data. */ > struct ofpact *ofpacts; > size_t ofpacts_len; > + uint64_t cookie; > }; > > static uint32_t ovn_flow_hash(const struct ovn_flow *); > @@ -602,7 +603,8 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype > type) > void > ofctrl_add_flow(struct hmap *desired_flows, > uint8_t table_id, uint16_t priority, > - const struct match *match, const struct ofpbuf *actions) > + const struct match *match, uint64_t cookie, > + const struct ofpbuf *actions) > { > struct ovn_flow *f = xmalloc(sizeof *f); > f->table_id = table_id; > @@ -611,6 +613,7 @@ ofctrl_add_flow(struct hmap *desired_flows, > f->ofpacts = xmemdup(actions->data, actions->size); > f->ofpacts_len = actions->size; > f->hmap_node.hash = ovn_flow_hash(f); > + f->cookie = cookie; > > if (ovn_flow_lookup(desired_flows, f)) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > @@ -935,6 +938,7 @@ ofctrl_put(struct hmap *flow_table, struct shash > *pending_ct_zones, > .table_id = d->table_id, > .ofpacts = d->ofpacts, > .ofpacts_len = d->ofpacts_len, > + .new_cookie = htonll(d->cookie), > .command = OFPFC_ADD, > }; > add_flow_mod(&fm, &msgs); > diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h > index 5308b61..67d6c59 100644 > --- a/ovn/controller/ofctrl.h > +++ b/ovn/controller/ofctrl.h > @@ -46,7 +46,7 @@ void ofctrl_ct_flush_zone(uint16_t zone_id); > /* Flow table interfaces to the rest of ovn-controller. */ > void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id, > uint16_t priority, const struct match *, > - const struct ofpbuf *ofpacts); > + uint64_t cookie, const struct ofpbuf *ofpacts); > > void ofctrl_flow_table_clear(void); > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index 48adb78..17a4126 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -331,7 +331,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > /* Resubmit to first logical ingress pipeline table. */ > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, > - tag ? 150 : 100, &match, ofpacts_p); > + tag ? 150 : 100, &match, 0, ofpacts_p); > > if (!tag && (!strcmp(binding->type, "localnet") > || !strcmp(binding->type, "l2gateway"))) { > @@ -341,7 +341,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > * action. */ > ofpbuf_pull(ofpacts_p, ofpacts_orig_size); > match_set_dl_tci_masked(&match, 0, htons(VLAN_CFI)); > - ofctrl_add_flow(flow_table, 0, 100, &match, ofpacts_p); > + ofctrl_add_flow(flow_table, 0, 100, &match, 0, ofpacts_p); > } > > /* Table 33, priority 100. > @@ -372,7 +372,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > /* Resubmit to table 34. */ > put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p); > ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, > - &match, ofpacts_p); > + &match, 0, ofpacts_p); > > /* Table 34, Priority 100. > * ======================= > @@ -387,7 +387,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > match_set_reg(&match, MFF_LOG_INPORT - MFF_REG0, port_key); > match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key); > ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 100, > - &match, ofpacts_p); > + &match, 0, ofpacts_p); > > /* Table 64, Priority 100. > * ======================= > @@ -411,7 +411,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > put_resubmit(OFTABLE_LOG_TO_PHY, ofpacts_p); > put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p)); > ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 100, > - &match, ofpacts_p); > + &match, 0, ofpacts_p); > > /* Table 65, Priority 100. > * ======================= > @@ -438,7 +438,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > ofpact_put_STRIP_VLAN(ofpacts_p); > } > ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100, > - &match, ofpacts_p); > + &match, 0, ofpacts_p); > } else if (!tun) { > /* Remote port connected by localnet port */ > /* Table 33, priority 100. > @@ -461,7 +461,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > /* Resubmit to table 33. */ > put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p); > ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, > - &match, ofpacts_p); > + &match, 0, ofpacts_p); > } else { > /* Remote port connected by tunnel */ > > @@ -486,7 +486,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > > /* Resubmit to table 33. */ > put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p); > - ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, > + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, 0, > ofpacts_p); > > > @@ -503,7 +503,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > /* Output to tunnel. */ > ofpact_put_OUTPUT(ofpacts_p)->port = ofport; > ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100, > - &match, ofpacts_p); > + &match, 0, ofpacts_p); > } > } > > @@ -583,7 +583,7 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, ofpacts_p); > > ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, > - &match, ofpacts_p); > + &match, 0, ofpacts_p); > } > > /* Table 32, priority 100. > @@ -621,7 +621,7 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > put_resubmit(OFTABLE_LOCAL_OUTPUT, remote_ofpacts_p); > } > ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100, > - &match, remote_ofpacts_p); > + &match, 0, remote_ofpacts_p); > } > } > sset_destroy(&remote_chassis); > @@ -833,7 +833,8 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > > put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); > > - ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, > &ofpacts); > + ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, 0, > + &ofpacts); > } > > /* Add flows for VXLAN encapsulations. Due to the limited amount of > @@ -865,7 +866,7 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > put_load(1, MFF_LOG_FLAGS, MLF_RCV_FROM_VXLAN_BIT, 1, > &ofpacts); > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts); > > - ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, > + ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, > 0, > &ofpacts); > } > } > @@ -879,7 +880,7 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > match_init_catchall(&match); > ofpbuf_clear(&ofpacts); > put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); > - ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, &match, > &ofpacts); > + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, &match, 0, > &ofpacts); > > /* Table 34, Priority 0. > * ======================= > @@ -893,7 +894,8 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > put_load(0, MFF_REG0 + i, 0, 32, &ofpacts); > } > put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, &ofpacts); > - ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 0, &match, > &ofpacts); > + ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 0, &match, 0, > + &ofpacts); > > /* Table 64, Priority 0. > * ======================= > @@ -903,7 +905,7 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > match_init_catchall(&match); > ofpbuf_clear(&ofpacts); > put_resubmit(OFTABLE_LOG_TO_PHY, &ofpacts); > - ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, &match, &ofpacts); > + ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, &match, 0, > &ofpacts); > > ofpbuf_uninit(&ofpacts); > > diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in > index 458a643..2f274ae 100644 > --- a/ovn/utilities/ovn-sbctl.8.in > +++ b/ovn/utilities/ovn-sbctl.8.in > @@ -159,9 +159,18 @@ to unbind logical port that is not bound has no > effect. > . > .SS "Logical Flow Commands" > . > -.IP "\fBlflow\-list\fR [\fIlogical-datapath\fR]" > +.IP "\fBlflow\-list\fR [\fIlogical-datapath\fR] [\fIlflow\fR...]" > List logical flows. If \fIlogical-datapath\fR is specified, only list > -flows for that logical datapath. > +flows for that logical datapath. The \fIlogical-datapath\fR may be > +given as a UUID or as a datapath name. > +.IP > +If at least one \fIlflow\fR is given, only matching logical flows, if > +any, are listed. Each \fIlflow\fR may be specified as a UUID or the > +first few characters of a UUID, optionally prefixed by \fB0x\fR. > +(Because \fBovn\-controller\fR sets OpenFlow flow cookies to the first > +32 bits of the corresponding logical flow's UUID, this makes it easy > +to look up the logical flow that generated a particular OpenFlow > +flow.) > . > .IP "\fBdump\-flows\fR [\fIlogical-datapath\fR]" > Alias for \fBlflow\-list\fB. > diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c > index 0763b79..f51a2ed 100644 > --- a/ovn/utilities/ovn-sbctl.c > +++ b/ovn/utilities/ovn-sbctl.c > @@ -306,8 +306,8 @@ Port binding commands:\n\ > lsp-unbind PORT reset the port binding of logical port > PORT\n\ > \n\ > Logical flow commands:\n\ > - lflow-list [DATAPATH] List logical flows for all or a single > datapath\n\ > - dump-flows [DATAPATH] Alias for lflow-list\n\ > + lflow-list [DATAPATH] [LFLOW...] List logical flows for DATAPATH\n\ > + dump-flows [DATAPATH] [LFLOW...] Alias for lflow-list\n\ > \n\ > %s\ > \n\ > @@ -687,53 +687,138 @@ lflow_cmp(const void *lf1_, const void *lf2_) > return 0; > } > > +static char * > +parse_partial_uuid(char *s) > +{ > + /* Accept a full or partial UUID. */ > + if (uuid_is_partial_string(s) == strlen(s)) { > + return s; > + } > + > + /* Accept a full or partial UUID prefixed by 0x, since "ovs-ofctl > + * dump-flows" prints cookies prefixed by 0x. */ > + if (s[0] == '0' && (s[1] == 'x' || s[1] == 'X') > + && uuid_is_partial_string(s + 2) == strlen(s + 2)) { > + return s + 2; > + } > + > + /* Not a (partial) UUID. */ > + return NULL; > +} > + > +static bool > +is_partial_uuid_match(const struct uuid *uuid, const char *match) > +{ > + char uuid_s[UUID_LEN + 1]; > + snprintf(uuid_s, sizeof uuid_s, UUID_FMT, UUID_ARGS(uuid)); > + > + return !strncmp(uuid_s, match, strlen(match)); > +} > + > +static const struct sbrec_datapath_binding * > +lookup_datapath(struct ovsdb_idl *idl, const char *s) > +{ > + const struct sbrec_datapath_binding *datapath; > + > + struct uuid uuid; > + if (uuid_from_string(&uuid, s)) { > + datapath = sbrec_datapath_binding_get_for_uuid(idl, &uuid); > + if (datapath) { > + return datapath; > + } > + } > + > + SBREC_DATAPATH_BINDING_FOR_EACH (datapath, idl) { > + const char *name = smap_get(&datapath->external_ids, "name"); > + if (name && !strcmp(name, s)) { > + return datapath; > + } > + } > + > + return NULL; > +} > + > static void > cmd_lflow_list(struct ctl_context *ctx) > { > - const char *datapath = ctx->argc == 2 ? ctx->argv[1] : NULL; > - struct uuid datapath_uuid = { .parts = { 0, }}; > - const struct sbrec_logical_flow **lflows; > - const struct sbrec_logical_flow *lflow; > - size_t n_flows = 0, n_capacity = 64; > + const struct sbrec_datapath_binding *datapath = NULL; > + if (ctx->argc > 1) { > + datapath = lookup_datapath(ctx->idl, ctx->argv[1]); > + if (datapath) { > + ctx->argc--; > + ctx->argv++; > + } > + } > > - if (datapath && !uuid_from_string(&datapath_uuid, datapath)) { > - VLOG_ERR("Invalid format of datapath UUID"); > - return; > + for (size_t i = 1; i < ctx->argc; i++) { > + char *s = parse_partial_uuid(ctx->argv[i]); > + if (!s) { > + ctl_fatal("%s is not a UUID or the beginning of a UUID", > + ctx->argv[i]); > + } > + ctx->argv[i] = s; > } > > - lflows = xmalloc(sizeof *lflows * n_capacity); > + const struct sbrec_logical_flow **lflows = NULL; > + size_t n_flows = 0; > + size_t n_capacity = 0; > + const struct sbrec_logical_flow *lflow; > SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->idl) { > + if (datapath && lflow->logical_datapath != datapath) { > + continue; > + } > + > if (n_flows == n_capacity) { > lflows = x2nrealloc(lflows, &n_capacity, sizeof *lflows); > } > lflows[n_flows] = lflow; > n_flows++; > } > - > qsort(lflows, n_flows, sizeof *lflows, lflow_cmp); > > - const char *cur_pipeline = ""; > - size_t i; > - for (i = 0; i < n_flows; i++) { > + const struct sbrec_logical_flow *prev = NULL; > + for (size_t i = 0; i < n_flows; i++) { > lflow = lflows[i]; > - if (datapath && !uuid_equals(&datapath_uuid, > - &lflow->logical_datapath->header_.uuid)) > { > + > + /* Figure out whether to print this particular flow. By default, > we > + * print all flows, but if any UUIDs were listed on the command > line > + * then we only print the matching ones. */ > + bool include; > + if (ctx->argc > 1) { > + include = false; > + for (size_t j = 1; j < ctx->argc; j++) { > + if (is_partial_uuid_match(&lflow->header_.uuid, > + ctx->argv[j])) { > + include = true; > + break; > + } > + } > + } else { > + include = true; > + } > + if (!include) { > continue; > } > - if (strcmp(cur_pipeline, lflow->pipeline)) { > + > + /* Print a header line for this datapath or pipeline, if we > haven't > + * already done so. */ > + if (!prev > + || prev->logical_datapath != lflow->logical_datapath > + || strcmp(prev->pipeline, lflow->pipeline)) { > printf("Datapath: \"%s\" ("UUID_FMT") Pipeline: %s\n", > smap_get_def(&lflow->logical_datapath->external_ids, > "name", ""), > UUID_ARGS(&lflow->logical_datapath->header_.uuid), > lflow->pipeline); > - cur_pipeline = lflow->pipeline; > } > > + /* Print the flow. */ > printf(" table=%-2" PRId64 "(%-19s), priority=%-5" PRId64 > ", match=(%s), action=(%s)\n", > lflow->table_id, > smap_get_def(&lflow->external_ids, "stage-name", ""), > lflow->priority, lflow->match, lflow->actions); > I wonder whether we should also print extenral_ids:source in the dump-flows output? Acked-by: Gurucharan Shetty <guru@ovn.org> > + prev = lflow; > } > > free(lflows); > @@ -1039,9 +1124,11 @@ static const struct ctl_command_syntax > sbctl_commands[] = { > "--if-exists", RW}, > > /* Logical flow commands */ > - {"lflow-list", 0, 1, "[DATAPATH]", pre_get_info, cmd_lflow_list, NULL, > + {"lflow-list", 0, INT_MAX, "[DATAPATH] [LFLOW...]", > + pre_get_info, cmd_lflow_list, NULL, > "", RO}, > - {"dump-flows", 0, 1, "[DATAPATH]", pre_get_info, cmd_lflow_list, NULL, > + {"dump-flows", 0, INT_MAX, "[DATAPATH] [LFLOW...]", > + pre_get_info, cmd_lflow_list, NULL, > "", RO}, /* Friendly alias for lflow-list */ > > /* SSL commands (To Be Added). */ > -- > 2.10.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Dec 12, 2016 at 12:59:06PM -0800, Guru Shetty wrote: > On 9 December 2016 at 22:56, Ben Pfaff <blp@ovn.org> wrote: > > > This makes it easy to find the logical flow that generated a particular > > OpenFlow flow, by running "ovn-sbctl dump-flows <cookie>". > > > > Later, this can be refined (and automated for "ofproto/trace"), but this > > is still a significant advance. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> ... > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > index 4e67365..5f49183 100644 > > --- a/ovn/controller/lflow.c > > +++ b/ovn/controller/lflow.c > > @@ -302,7 +302,7 @@ consider_logical_flow(const struct lport_index *lports, > > } > > if (!m->n) { > > ofctrl_add_flow(flow_table, ptable, lflow->priority, > > &m->match, > > - &ofpacts); > > + lflow->header_.uuid.parts[0], &ofpacts); > > } else { > > uint64_t conj_stubs[64 / 8]; > > struct ofpbuf conj; > > @@ -318,7 +318,7 @@ consider_logical_flow(const struct lport_index *lports, > > dst->n_clauses = src->n_clauses; > > } > > ofctrl_add_flow(flow_table, ptable, lflow->priority, > > &m->match, > > - &conj); > > + 0, &conj); > > > > If I understand it right, a logical flow of the form ip4.src == {a, b, c} > after it is split into multiple openflow flows won't have a cookie? Such a flow generates OpenFlow flows that look something like this, if necessary (not actually in this case since there's only one field): ip,nw_src=a, actions=conjunction(123) ip,nw_src=b, actions=conjunction(123) ip,nw_src=c, actions=conjunction(123) conj_id=123, actions=...real actions... In such a case, the flows with "conjunction" actions indeed will not get a cookie, but really they can't because these might be shared by multiple logical flows. That is, if another OVN logical flow comes along with ip4.src == {c,d,e} and other criteria, then you'd end up with: ip,nw_src=a, actions=conjunction(123) ip,nw_src=b, actions=conjunction(123) ip,nw_src=c, actions=conjunction(123),conjunction(456) ip,nw_src=d, actions=conjunction(456) ip,nw_src=e, actions=conjunction(456) conj_id=123, actions=...real actions... conj_id=456, actions=...real actions... and so in a case like that, which cookie should get used? The flows with "conjunction" actions won't ever show up in a trace, because they direct packet classification and never get executed like normal actions. The text added to the ovn-trace manpage in patch 2/2 describes this, although it doesn't have an example like above. > > + /* Print the flow. */ > > printf(" table=%-2" PRId64 "(%-19s), priority=%-5" PRId64 > > ", match=(%s), action=(%s)\n", > > lflow->table_id, > > smap_get_def(&lflow->external_ids, "stage-name", ""), > > lflow->priority, lflow->match, lflow->actions); > > > > I wonder whether we should also print extenral_ids:source in the dump-flows > output? Maybe. These lines are already pretty wide though. > Acked-by: Gurucharan Shetty <guru@ovn.org> Thanks! I'm going to ask Justin whether he wants to review this before I push it, because I know that he has opinions on how to provide this extra information.
diff --git a/lib/uuid.c b/lib/uuid.c index 0f2a58e..bd98d40 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc. +/* Copyright (c) 2008, 2009, 2010, 2011, 2013, 2016 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -210,6 +210,26 @@ error: uuid_zero(uuid); return false; } + +/* Returns the number of characters at the beginning of 's' that are valid for + * a UUID. For example, the "123" at the beginning of "123xyzzy" could begin a + * UUID, so uuid_is_partial_string() would return 3; for "xyzzy", this function + * would return 0, since "x" can't start a UUID. */ +int +uuid_is_partial_string(const char *s) +{ + static const char tmpl[UUID_LEN] = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"; + size_t i; + for (i = 0; i < UUID_LEN; i++) { + if (tmpl[i] == 'x' + ? hexit_value(s[i]) < 0 + : s[i] != '-') { + break; + } + } + return i; +} + static void sha1_update_int(struct sha1_ctx *sha1_ctx, uintmax_t x) diff --git a/lib/uuid.h b/lib/uuid.h index a5792b0..113574c 100644 --- a/lib/uuid.h +++ b/lib/uuid.h @@ -1,4 +1,4 @@ -/* Copyright (c) 2008, 2009, 2010 Nicira, Inc. +/* Copyright (c) 2008, 2009, 2010, 2016 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -61,6 +61,7 @@ bool uuid_is_zero(const struct uuid *); int uuid_compare_3way(const struct uuid *, const struct uuid *); bool uuid_from_string(struct uuid *, const char *); bool uuid_from_string_prefix(struct uuid *, const char *); +int uuid_is_partial_string(const char *); void uuid_set_bits_v4(struct uuid *); #endif /* uuid.h */ diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index 4e67365..5f49183 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -302,7 +302,7 @@ consider_logical_flow(const struct lport_index *lports, } if (!m->n) { ofctrl_add_flow(flow_table, ptable, lflow->priority, &m->match, - &ofpacts); + lflow->header_.uuid.parts[0], &ofpacts); } else { uint64_t conj_stubs[64 / 8]; struct ofpbuf conj; @@ -318,7 +318,7 @@ consider_logical_flow(const struct lport_index *lports, dst->n_clauses = src->n_clauses; } ofctrl_add_flow(flow_table, ptable, lflow->priority, &m->match, - &conj); + 0, &conj); ofpbuf_uninit(&conj); } } @@ -386,7 +386,7 @@ consider_neighbor_flow(const struct lport_index *lports, uint64_t stub[1024 / 8]; struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, &ofpacts); - ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match, &ofpacts); + ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match, 0, &ofpacts); ofpbuf_uninit(&ofpacts); } diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c index 1d8fbf3..7449293 100644 --- a/ovn/controller/ofctrl.c +++ b/ovn/controller/ofctrl.c @@ -57,6 +57,7 @@ struct ovn_flow { /* Data. */ struct ofpact *ofpacts; size_t ofpacts_len; + uint64_t cookie; }; static uint32_t ovn_flow_hash(const struct ovn_flow *); @@ -602,7 +603,8 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id, uint16_t priority, - const struct match *match, const struct ofpbuf *actions) + const struct match *match, uint64_t cookie, + const struct ofpbuf *actions) { struct ovn_flow *f = xmalloc(sizeof *f); f->table_id = table_id; @@ -611,6 +613,7 @@ ofctrl_add_flow(struct hmap *desired_flows, f->ofpacts = xmemdup(actions->data, actions->size); f->ofpacts_len = actions->size; f->hmap_node.hash = ovn_flow_hash(f); + f->cookie = cookie; if (ovn_flow_lookup(desired_flows, f)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); @@ -935,6 +938,7 @@ ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones, .table_id = d->table_id, .ofpacts = d->ofpacts, .ofpacts_len = d->ofpacts_len, + .new_cookie = htonll(d->cookie), .command = OFPFC_ADD, }; add_flow_mod(&fm, &msgs); diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h index 5308b61..67d6c59 100644 --- a/ovn/controller/ofctrl.h +++ b/ovn/controller/ofctrl.h @@ -46,7 +46,7 @@ void ofctrl_ct_flush_zone(uint16_t zone_id); /* Flow table interfaces to the rest of ovn-controller. */ void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id, uint16_t priority, const struct match *, - const struct ofpbuf *ofpacts); + uint64_t cookie, const struct ofpbuf *ofpacts); void ofctrl_flow_table_clear(void); diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 48adb78..17a4126 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -331,7 +331,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, /* Resubmit to first logical ingress pipeline table. */ put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, - tag ? 150 : 100, &match, ofpacts_p); + tag ? 150 : 100, &match, 0, ofpacts_p); if (!tag && (!strcmp(binding->type, "localnet") || !strcmp(binding->type, "l2gateway"))) { @@ -341,7 +341,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, * action. */ ofpbuf_pull(ofpacts_p, ofpacts_orig_size); match_set_dl_tci_masked(&match, 0, htons(VLAN_CFI)); - ofctrl_add_flow(flow_table, 0, 100, &match, ofpacts_p); + ofctrl_add_flow(flow_table, 0, 100, &match, 0, ofpacts_p); } /* Table 33, priority 100. @@ -372,7 +372,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, /* Resubmit to table 34. */ put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p); ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, - &match, ofpacts_p); + &match, 0, ofpacts_p); /* Table 34, Priority 100. * ======================= @@ -387,7 +387,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, match_set_reg(&match, MFF_LOG_INPORT - MFF_REG0, port_key); match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key); ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 100, - &match, ofpacts_p); + &match, 0, ofpacts_p); /* Table 64, Priority 100. * ======================= @@ -411,7 +411,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, put_resubmit(OFTABLE_LOG_TO_PHY, ofpacts_p); put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p)); ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 100, - &match, ofpacts_p); + &match, 0, ofpacts_p); /* Table 65, Priority 100. * ======================= @@ -438,7 +438,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, ofpact_put_STRIP_VLAN(ofpacts_p); } ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100, - &match, ofpacts_p); + &match, 0, ofpacts_p); } else if (!tun) { /* Remote port connected by localnet port */ /* Table 33, priority 100. @@ -461,7 +461,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, /* Resubmit to table 33. */ put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p); ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, - &match, ofpacts_p); + &match, 0, ofpacts_p); } else { /* Remote port connected by tunnel */ @@ -486,7 +486,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, /* Resubmit to table 33. */ put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p); - ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, 0, ofpacts_p); @@ -503,7 +503,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, /* Output to tunnel. */ ofpact_put_OUTPUT(ofpacts_p)->port = ofport; ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100, - &match, ofpacts_p); + &match, 0, ofpacts_p); } } @@ -583,7 +583,7 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, ofpacts_p); ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, - &match, ofpacts_p); + &match, 0, ofpacts_p); } /* Table 32, priority 100. @@ -621,7 +621,7 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, put_resubmit(OFTABLE_LOCAL_OUTPUT, remote_ofpacts_p); } ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100, - &match, remote_ofpacts_p); + &match, 0, remote_ofpacts_p); } } sset_destroy(&remote_chassis); @@ -833,7 +833,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); - ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, &ofpacts); + ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, 0, + &ofpacts); } /* Add flows for VXLAN encapsulations. Due to the limited amount of @@ -865,7 +866,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, put_load(1, MFF_LOG_FLAGS, MLF_RCV_FROM_VXLAN_BIT, 1, &ofpacts); put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts); - ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, + ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, 0, &ofpacts); } } @@ -879,7 +880,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, match_init_catchall(&match); ofpbuf_clear(&ofpacts); put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); - ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, &match, &ofpacts); + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, &match, 0, &ofpacts); /* Table 34, Priority 0. * ======================= @@ -893,7 +894,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, put_load(0, MFF_REG0 + i, 0, 32, &ofpacts); } put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, &ofpacts); - ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 0, &match, &ofpacts); + ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 0, &match, 0, + &ofpacts); /* Table 64, Priority 0. * ======================= @@ -903,7 +905,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, match_init_catchall(&match); ofpbuf_clear(&ofpacts); put_resubmit(OFTABLE_LOG_TO_PHY, &ofpacts); - ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, &match, &ofpacts); + ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, &match, 0, &ofpacts); ofpbuf_uninit(&ofpacts); diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in index 458a643..2f274ae 100644 --- a/ovn/utilities/ovn-sbctl.8.in +++ b/ovn/utilities/ovn-sbctl.8.in @@ -159,9 +159,18 @@ to unbind logical port that is not bound has no effect. . .SS "Logical Flow Commands" . -.IP "\fBlflow\-list\fR [\fIlogical-datapath\fR]" +.IP "\fBlflow\-list\fR [\fIlogical-datapath\fR] [\fIlflow\fR...]" List logical flows. If \fIlogical-datapath\fR is specified, only list -flows for that logical datapath. +flows for that logical datapath. The \fIlogical-datapath\fR may be +given as a UUID or as a datapath name. +.IP +If at least one \fIlflow\fR is given, only matching logical flows, if +any, are listed. Each \fIlflow\fR may be specified as a UUID or the +first few characters of a UUID, optionally prefixed by \fB0x\fR. +(Because \fBovn\-controller\fR sets OpenFlow flow cookies to the first +32 bits of the corresponding logical flow's UUID, this makes it easy +to look up the logical flow that generated a particular OpenFlow +flow.) . .IP "\fBdump\-flows\fR [\fIlogical-datapath\fR]" Alias for \fBlflow\-list\fB. diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c index 0763b79..f51a2ed 100644 --- a/ovn/utilities/ovn-sbctl.c +++ b/ovn/utilities/ovn-sbctl.c @@ -306,8 +306,8 @@ Port binding commands:\n\ lsp-unbind PORT reset the port binding of logical port PORT\n\ \n\ Logical flow commands:\n\ - lflow-list [DATAPATH] List logical flows for all or a single datapath\n\ - dump-flows [DATAPATH] Alias for lflow-list\n\ + lflow-list [DATAPATH] [LFLOW...] List logical flows for DATAPATH\n\ + dump-flows [DATAPATH] [LFLOW...] Alias for lflow-list\n\ \n\ %s\ \n\ @@ -687,53 +687,138 @@ lflow_cmp(const void *lf1_, const void *lf2_) return 0; } +static char * +parse_partial_uuid(char *s) +{ + /* Accept a full or partial UUID. */ + if (uuid_is_partial_string(s) == strlen(s)) { + return s; + } + + /* Accept a full or partial UUID prefixed by 0x, since "ovs-ofctl + * dump-flows" prints cookies prefixed by 0x. */ + if (s[0] == '0' && (s[1] == 'x' || s[1] == 'X') + && uuid_is_partial_string(s + 2) == strlen(s + 2)) { + return s + 2; + } + + /* Not a (partial) UUID. */ + return NULL; +} + +static bool +is_partial_uuid_match(const struct uuid *uuid, const char *match) +{ + char uuid_s[UUID_LEN + 1]; + snprintf(uuid_s, sizeof uuid_s, UUID_FMT, UUID_ARGS(uuid)); + + return !strncmp(uuid_s, match, strlen(match)); +} + +static const struct sbrec_datapath_binding * +lookup_datapath(struct ovsdb_idl *idl, const char *s) +{ + const struct sbrec_datapath_binding *datapath; + + struct uuid uuid; + if (uuid_from_string(&uuid, s)) { + datapath = sbrec_datapath_binding_get_for_uuid(idl, &uuid); + if (datapath) { + return datapath; + } + } + + SBREC_DATAPATH_BINDING_FOR_EACH (datapath, idl) { + const char *name = smap_get(&datapath->external_ids, "name"); + if (name && !strcmp(name, s)) { + return datapath; + } + } + + return NULL; +} + static void cmd_lflow_list(struct ctl_context *ctx) { - const char *datapath = ctx->argc == 2 ? ctx->argv[1] : NULL; - struct uuid datapath_uuid = { .parts = { 0, }}; - const struct sbrec_logical_flow **lflows; - const struct sbrec_logical_flow *lflow; - size_t n_flows = 0, n_capacity = 64; + const struct sbrec_datapath_binding *datapath = NULL; + if (ctx->argc > 1) { + datapath = lookup_datapath(ctx->idl, ctx->argv[1]); + if (datapath) { + ctx->argc--; + ctx->argv++; + } + } - if (datapath && !uuid_from_string(&datapath_uuid, datapath)) { - VLOG_ERR("Invalid format of datapath UUID"); - return; + for (size_t i = 1; i < ctx->argc; i++) { + char *s = parse_partial_uuid(ctx->argv[i]); + if (!s) { + ctl_fatal("%s is not a UUID or the beginning of a UUID", + ctx->argv[i]); + } + ctx->argv[i] = s; } - lflows = xmalloc(sizeof *lflows * n_capacity); + const struct sbrec_logical_flow **lflows = NULL; + size_t n_flows = 0; + size_t n_capacity = 0; + const struct sbrec_logical_flow *lflow; SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->idl) { + if (datapath && lflow->logical_datapath != datapath) { + continue; + } + if (n_flows == n_capacity) { lflows = x2nrealloc(lflows, &n_capacity, sizeof *lflows); } lflows[n_flows] = lflow; n_flows++; } - qsort(lflows, n_flows, sizeof *lflows, lflow_cmp); - const char *cur_pipeline = ""; - size_t i; - for (i = 0; i < n_flows; i++) { + const struct sbrec_logical_flow *prev = NULL; + for (size_t i = 0; i < n_flows; i++) { lflow = lflows[i]; - if (datapath && !uuid_equals(&datapath_uuid, - &lflow->logical_datapath->header_.uuid)) { + + /* Figure out whether to print this particular flow. By default, we + * print all flows, but if any UUIDs were listed on the command line + * then we only print the matching ones. */ + bool include; + if (ctx->argc > 1) { + include = false; + for (size_t j = 1; j < ctx->argc; j++) { + if (is_partial_uuid_match(&lflow->header_.uuid, + ctx->argv[j])) { + include = true; + break; + } + } + } else { + include = true; + } + if (!include) { continue; } - if (strcmp(cur_pipeline, lflow->pipeline)) { + + /* Print a header line for this datapath or pipeline, if we haven't + * already done so. */ + if (!prev + || prev->logical_datapath != lflow->logical_datapath + || strcmp(prev->pipeline, lflow->pipeline)) { printf("Datapath: \"%s\" ("UUID_FMT") Pipeline: %s\n", smap_get_def(&lflow->logical_datapath->external_ids, "name", ""), UUID_ARGS(&lflow->logical_datapath->header_.uuid), lflow->pipeline); - cur_pipeline = lflow->pipeline; } + /* Print the flow. */ printf(" table=%-2" PRId64 "(%-19s), priority=%-5" PRId64 ", match=(%s), action=(%s)\n", lflow->table_id, smap_get_def(&lflow->external_ids, "stage-name", ""), lflow->priority, lflow->match, lflow->actions); + prev = lflow; } free(lflows); @@ -1039,9 +1124,11 @@ static const struct ctl_command_syntax sbctl_commands[] = { "--if-exists", RW}, /* Logical flow commands */ - {"lflow-list", 0, 1, "[DATAPATH]", pre_get_info, cmd_lflow_list, NULL, + {"lflow-list", 0, INT_MAX, "[DATAPATH] [LFLOW...]", + pre_get_info, cmd_lflow_list, NULL, "", RO}, - {"dump-flows", 0, 1, "[DATAPATH]", pre_get_info, cmd_lflow_list, NULL, + {"dump-flows", 0, INT_MAX, "[DATAPATH] [LFLOW...]", + pre_get_info, cmd_lflow_list, NULL, "", RO}, /* Friendly alias for lflow-list */ /* SSL commands (To Be Added). */
This makes it easy to find the logical flow that generated a particular OpenFlow flow, by running "ovn-sbctl dump-flows <cookie>". Later, this can be refined (and automated for "ofproto/trace"), but this is still a significant advance. Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/uuid.c | 22 +++++++- lib/uuid.h | 3 +- ovn/controller/lflow.c | 6 +- ovn/controller/ofctrl.c | 6 +- ovn/controller/ofctrl.h | 2 +- ovn/controller/physical.c | 34 ++++++------ ovn/utilities/ovn-sbctl.8.in | 13 ++++- ovn/utilities/ovn-sbctl.c | 129 ++++++++++++++++++++++++++++++++++++------- 8 files changed, 169 insertions(+), 46 deletions(-)