On 20.03.2019 21:10, Ben Pfaff wrote: > On Wed, Mar 20, 2019 at 06:46:22PM +0300, Ilya Maximets wrote: >> On 20.03.2019 4:20, Ben Pfaff wrote: >>> On Tue, Mar 19, 2019 at 07:59:55PM +0300, Ilya Maximets wrote: >>>> On 19.03.2019 18:51, Ilya Maximets wrote: >>>>> On 19.03.2019 12:23, Eelco Chaudron wrote: >>>>>> When debugging an issue we noticed that by accident someone has changes >>>>>> the >>>>>> bridge datapath_type to netdev, where it's clearly a kernel (system >>>>>> bridge) >>>>>> with physical and tap devices. Unfortunately, this is not something you >>>>>> will easily spot, as the bridge datapath type value is not shown by >>>>>> default. >>>>>> >>>>>> In addition, OVS is not warning you about this potential mismatch in >>>>>> interface and bridge datapath. >>>>>> >>>>>> I'm sending out this patch as an RFC as I could not find a clear >>>>>> demarcation between bridge datapath types and interface datapath types. >>>>>> The patch below will at least warn for netdev bridges with system >>>>>> interfaces. But no warning will be given for some unsupported virtual >>>>>> interfaces. For system bridges, the dpdk types will no be recognized as >>>>>> system/virtual interfaces (unless the name exists) which will result in >>>>>> an error. >>>>>> >>>>>> Signed-off-by: Eelco Chaudron <echau...@redhat.com> >>>>>> --- >>>>> >>>>> Hi. >>>>> >>>>> Hmm. What do you mean under misconfiguration? >>>>> In practice, userspace datapath is able to open "system" type netdevs. >>>>> It's supported by netdev-linux to open system network devices via socket. >>>>> And these devices has "system" type. >>>>> On 'datapath_type' changes, bridge/ofproto will be destroyed and >>>>> re-created. >>>>> All the ports from kernel datapath will be destroyed and new ones created >>>>> in userspace. All the ports should still work as usual. >>>>> >>>>> The only possible issue will be that this bridge will no longer >>>>> communicate >>>>> with other bridges, because they're in different datapaths now. >>>> >>>> For this issue, probably, following warning will give a clue to a user: >>>> >>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>>> index 21dd54bab..df51aaa3a 100644 >>>> --- a/ofproto/ofproto-dpif.c >>>> +++ b/ofproto/ofproto-dpif.c >>>> @@ -3557,6 +3557,10 @@ ofport_update_peer(struct ofport_dpif *ofport) >>>> >>>> break; >>>> } >>>> + if (!ofport->peer) { >>>> + VLOG_WARN_RL(&rl, "No usable peer '%s' exist for patch port >>>> '%s'.", >>>> + peer_name, netdev_get_name(ofport->up.netdev)); >>>> + } >>>> free(peer_name); >>>> } >>>> >>>> --- >>>> >>>> I could send this as a proper patch. >>> >>> In the log message, s/exist/exists/ for English grammar reasons. >> >> Sure. Thanks for spotting. >> >>> >>> Even with the log message, the error might not be obvious to the reader. >>> It would be nice, if a port with the given name existed on a different >>> backer, to somehow point that out. It might be hard for this code to >>> figure it out though. Maybe it would be helpful to simply note the >>> datapath type; that might give the reader the hint that the datapath >>> type could be relevant. >> >> I thought about this a little bit more and now I think that message >> like that will be more confusing than helpful. It's because it will >> be printed each time while one side of the patch already created but >> the peer is not added yet. Logs could be confusing unless we reporting >> successful pairing. > > OK. > >> Maybe it's more appropriate to propagate issue like this to the 'error' >> column of the interface. But I didn't figure out yet how to do that >> in a good way. > > You're right, that would be better. (I don't remember how that > propagation works.) > >> BTW, maybe the following small change will help for debugging issues >> like that: >> >> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c >> >> index a36905186..4948137ef 100644 >> >> --- a/utilities/ovs-vsctl.c >> >> +++ b/utilities/ovs-vsctl.c >> >> @@ -1001,6 +1001,7 @@ static struct cmd_show_table cmd_show_tables[] = { >> >> &ovsrec_bridge_col_name, >> >> {&ovsrec_bridge_col_controller, >> >> &ovsrec_bridge_col_fail_mode, >> >> + &ovsrec_bridge_col_datapath_type, >> >> &ovsrec_bridge_col_ports}, >> >> {NULL, NULL, NULL} >> >> }, >> --- >> >> With this change 'ovs-vsctl show' will print non-default datapath types and >> it'll be easy to spot. > > I agree. Would you mind sending an official patch?
Sure. Sent here: https://patchwork.ozlabs.org/patch/1059987/ Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev