| Message ID | 155298740972.37904.5761805205559576350.stgit@dbuild |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,RFC] vswitchd: warn about misconfigured interface type on netdev bridge | expand |
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 <echaudro@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. So, below warning will be printed on any attempt to add legitimate system network interface to the userspace bridge. "system" devices supported by both datapaths, but "dpdk" devices supported only by userspace, that is why you see the error in case of changing to 'datapath_type=system'. Maybe I'm missing something? What is the issue you're trying to address? Best regards, Ilya Maximets. > vswitchd/bridge.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index a427b0122..42c33d1d9 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, > goto error; > } > > + if (!iface_is_internal(iface_cfg, br->cfg) > + && !strcmp(br->type, "netdev") > + && !strcmp(netdev_get_type(netdev), "system")) { > + VLOG_WARN("bridge %s: interface %s is a system type where the bridge " > + "is a netdev one", br->name, iface_cfg->name); > + } > VLOG_INFO("bridge %s: added interface %s on port %d", > br->name, iface_cfg->name, *ofp_portp); > >
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 <echaudro@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. Best regards, Ilya Maximets. > > So, below warning will be printed on any attempt to add legitimate > system network interface to the userspace bridge. > "system" devices supported by both datapaths, but "dpdk" devices supported > only by userspace, that is why you see the error in case of changing to > 'datapath_type=system'. > > Maybe I'm missing something? What is the issue you're trying to address? > > Best regards, Ilya Maximets. > >> vswitchd/bridge.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> index a427b0122..42c33d1d9 100644 >> --- a/vswitchd/bridge.c >> +++ b/vswitchd/bridge.c >> @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, >> goto error; >> } >> >> + if (!iface_is_internal(iface_cfg, br->cfg) >> + && !strcmp(br->type, "netdev") >> + && !strcmp(netdev_get_type(netdev), "system")) { >> + VLOG_WARN("bridge %s: interface %s is a system type where the bridge " >> + "is a netdev one", br->name, iface_cfg->name); >> + } >> VLOG_INFO("bridge %s: added interface %s on port %d", >> br->name, iface_cfg->name, *ofp_portp); >> >> > >
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 <echaudro@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. 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.
On 19 Mar 2019, at 16: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 <echaudro@redhat.com> >> --- > > Hi. > > Hmm. What do you mean under misconfiguration? Good question, as the documentation and code are not clear about this part… My interpretation is that for the netdev datapath bridges DPDK/vhost ports, internal ports, patch-ports, and user space tunnel ports are the once supported. But any other kernel ports are not, however, your text below suggests otherwise. > 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. With the below statement in mind, I configure the following: ovs-vsctl add-br test ip link add name right1 type veth peer name left1 ip link add name right2 type veth peer name left2 ovs-vsctl add-port test left1 ovs-vsctl add-port test left2 ovs-vsctl add-port test eth0 ip link set dev left1 up ip link set dev left2 up ip netns add netns1 ip netns add netns2 ip link set dev right1 netns netns1 ip link set dev right2 netns netns2 ip netns exec netns1 ip link set dev lo up ip netns exec netns1 ip link set dev right1 up ip netns exec netns2 ip link set dev right2 up ip netns exec netns2 ip link set dev lo up ip netns exec netns1 ip a a dev right1 192.168.0.1/24 ip netns exec netns2 ip a a dev right2 192.168.0.2/24 This is all working fine now, and now some accidentally does this: ovs-vsctl set bridge test datapath_type=netdev And you suggest all continues to work as is? > The only possible issue will be that this bridge will no longer > communicate > with other bridges, because they're in different datapaths now. > > So, below warning will be printed on any attempt to add legitimate > system network interface to the userspace bridge. > "system" devices supported by both datapaths, but "dpdk" devices > supported > only by userspace, that is why you see the error in case of changing > to > 'datapath_type=system'. Are you saying ALL system devices are supported by the netdev datapath? Or only a specific set? If the later we should check for those (and maybe add some infrastructure to identify easily which devices are supported by which datapath. If it exists please point me to it, as I might have overlooked it). > Maybe I'm missing something? What is the issue you're trying to > address? The above example stops working due to checksum offloading on veth. But if you are right this is a valid configuration I’ll further investigate this. > Best regards, Ilya Maximets. > >> vswitchd/bridge.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> index a427b0122..42c33d1d9 100644 >> --- a/vswitchd/bridge.c >> +++ b/vswitchd/bridge.c >> @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, >> goto error; >> } >> >> + if (!iface_is_internal(iface_cfg, br->cfg) >> + && !strcmp(br->type, "netdev") >> + && !strcmp(netdev_get_type(netdev), "system")) { >> + VLOG_WARN("bridge %s: interface %s is a system type where >> the bridge " >> + "is a netdev one", br->name, iface_cfg->name); >> + } >> VLOG_INFO("bridge %s: added interface %s on port %d", >> br->name, iface_cfg->name, *ofp_portp); >> >>
On 20.03.2019 11:01, Eelco Chaudron wrote: > > > On 19 Mar 2019, at 16: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 <echaudro@redhat.com> >>> --- >> >> Hi. >> >> Hmm. What do you mean under misconfiguration? > > Good question, as the documentation and code are not clear about this part… > > My interpretation is that for the netdev datapath bridges DPDK/vhost ports, internal ports, patch-ports, and user space tunnel ports are the once supported. But any other kernel ports are not, however, your text below suggests otherwise. See below. > >> 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. > > With the below statement in mind, I configure the following: > > ovs-vsctl add-br test > ip link add name right1 type veth peer name left1 > ip link add name right2 type veth peer name left2 > ovs-vsctl add-port test left1 > ovs-vsctl add-port test left2 > ovs-vsctl add-port test eth0 > ip link set dev left1 up > ip link set dev left2 up > ip netns add netns1 > ip netns add netns2 > ip link set dev right1 netns netns1 > ip link set dev right2 netns netns2 > ip netns exec netns1 ip link set dev lo up > ip netns exec netns1 ip link set dev right1 up > ip netns exec netns2 ip link set dev right2 up > ip netns exec netns2 ip link set dev lo up > ip netns exec netns1 ip a a dev right1 192.168.0.1/24 > ip netns exec netns2 ip a a dev right2 192.168.0.2/24 > > This is all working fine now, and now some accidentally does this: > > ovs-vsctl set bridge test datapath_type=netdev > > And you suggest all continues to work as is? In general, yes. > >> The only possible issue will be that this bridge will no longer communicate >> with other bridges, because they're in different datapaths now. >> >> So, below warning will be printed on any attempt to add legitimate >> system network interface to the userspace bridge. > >> "system" devices supported by both datapaths, but "dpdk" devices supported >> only by userspace, that is why you see the error in case of changing to >> 'datapath_type=system'. > > Are you saying ALL system devices are supported by the netdev datapath? Or only a specific set? If the later we should check for those (and maybe add some infrastructure to identify easily which devices are supported by which datapath. If it exists please point me to it, as I might have overlooked it). OVS netdev datapath could open any linux network interfacce that could be opened with the raw socket. There is nothing device specific here. BTW, tests/system-userspace-testsuite.at completely relies on ability to open and forward traffic through the veth pairs by netdev datapath. > >> Maybe I'm missing something? What is the issue you're trying to address? > > The above example stops working due to checksum offloading on veth. > But if you are right this is a valid configuration I’ll further investigate this. Configuration is valid. The issue here is that OVS netdev datapath doesn't support TX checksum offloading (this is not easy task with arguable profit). i.e. if packet arrives with bad/no checksum it will be sent to the output port with same bad/no checksum. Everything works in case of kernel datapth because the packet doesn't leave the kernel space. In case of netdev datapath some information (like CHECKSUM_VALID skb flags) is lost while receiving via socket in userspace and subsequently kernel expects valid checksum while receiving the packet from userspace because TX offloading is not enabled. This kind of issues usually mitigated by disabling TX offloading on the "right*" interfaces, or by setting iptables to fill the checksums like this: iptables -A POSTROUTING -t mangle -p udp -m udp -j CHECKSUM --checksum-fill Some related OpenStack bug: https://bugs.launchpad.net/neutron/+bug/1244589 > >> Best regards, Ilya Maximets. >> >>> vswitchd/bridge.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >>> index a427b0122..42c33d1d9 100644 >>> --- a/vswitchd/bridge.c >>> +++ b/vswitchd/bridge.c >>> @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, >>> goto error; >>> } >>> >>> + if (!iface_is_internal(iface_cfg, br->cfg) >>> + && !strcmp(br->type, "netdev") >>> + && !strcmp(netdev_get_type(netdev), "system")) { >>> + VLOG_WARN("bridge %s: interface %s is a system type where the bridge " >>> + "is a netdev one", br->name, iface_cfg->name); >>> + } >>> VLOG_INFO("bridge %s: added interface %s on port %d", >>> br->name, iface_cfg->name, *ofp_portp); >>> >>> > >
On 20.03.2019 11:34, Ilya Maximets wrote: > On 20.03.2019 11:01, Eelco Chaudron wrote: >> >> >> On 19 Mar 2019, at 16: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 <echaudro@redhat.com> >>>> --- >>> >>> Hi. >>> >>> Hmm. What do you mean under misconfiguration? >> >> Good question, as the documentation and code are not clear about this part… >> >> My interpretation is that for the netdev datapath bridges DPDK/vhost ports, internal ports, patch-ports, and user space tunnel ports are the once supported. But any other kernel ports are not, however, your text below suggests otherwise. > > See below. > >> >>> 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. >> >> With the below statement in mind, I configure the following: >> >> ovs-vsctl add-br test >> ip link add name right1 type veth peer name left1 >> ip link add name right2 type veth peer name left2 >> ovs-vsctl add-port test left1 >> ovs-vsctl add-port test left2 >> ovs-vsctl add-port test eth0 >> ip link set dev left1 up >> ip link set dev left2 up >> ip netns add netns1 >> ip netns add netns2 >> ip link set dev right1 netns netns1 >> ip link set dev right2 netns netns2 >> ip netns exec netns1 ip link set dev lo up >> ip netns exec netns1 ip link set dev right1 up >> ip netns exec netns2 ip link set dev right2 up >> ip netns exec netns2 ip link set dev lo up >> ip netns exec netns1 ip a a dev right1 192.168.0.1/24 >> ip netns exec netns2 ip a a dev right2 192.168.0.2/24 >> >> This is all working fine now, and now some accidentally does this: >> >> ovs-vsctl set bridge test datapath_type=netdev >> >> And you suggest all continues to work as is? > > In general, yes. > >> >>> The only possible issue will be that this bridge will no longer communicate >>> with other bridges, because they're in different datapaths now. >>> >>> So, below warning will be printed on any attempt to add legitimate >>> system network interface to the userspace bridge. >> >>> "system" devices supported by both datapaths, but "dpdk" devices supported >>> only by userspace, that is why you see the error in case of changing to >>> 'datapath_type=system'. >> >> Are you saying ALL system devices are supported by the netdev datapath? Or only a specific set? If the later we should check for those (and maybe add some infrastructure to identify easily which devices are supported by which datapath. If it exists please point me to it, as I might have overlooked it). > > OVS netdev datapath could open any linux network interfacce that could be > opened with the raw socket. There is nothing device specific here. > > BTW, tests/system-userspace-testsuite.at completely relies on ability to > open and forward traffic through the veth pairs by netdev datapath. > >> >>> Maybe I'm missing something? What is the issue you're trying to address? >> >> The above example stops working due to checksum offloading on veth. >> But if you are right this is a valid configuration I’ll further investigate this. > > Configuration is valid. The issue here is that OVS netdev datapath doesn't > support TX checksum offloading (this is not easy task with arguable profit). > i.e. if packet arrives with bad/no checksum it will be sent to the output port > with same bad/no checksum. Everything works in case of kernel datapth because > the packet doesn't leave the kernel space. In case of netdev datapath some > information (like CHECKSUM_VALID skb flags) is lost while receiving via > socket in userspace and subsequently kernel expects valid checksum while > receiving the packet from userspace because TX offloading is not enabled. > > This kind of issues usually mitigated by disabling TX offloading on the > "right*" interfaces, or by setting iptables to fill the checksums like this: > > iptables -A POSTROUTING -t mangle -p udp -m udp -j CHECKSUM --checksum-fill > > Some related OpenStack bug: https://bugs.launchpad.net/neutron/+bug/1244589 Also, note that this happens only for virtual interfaces like veth/tap because kernel always tries to delay checksum calculation/validation as much as possible. Correct packets received from the wire will always have correct checksums. > >> >>> Best regards, Ilya Maximets. >>> >>>> vswitchd/bridge.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >>>> index a427b0122..42c33d1d9 100644 >>>> --- a/vswitchd/bridge.c >>>> +++ b/vswitchd/bridge.c >>>> @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, >>>> goto error; >>>> } >>>> >>>> + if (!iface_is_internal(iface_cfg, br->cfg) >>>> + && !strcmp(br->type, "netdev") >>>> + && !strcmp(netdev_get_type(netdev), "system")) { >>>> + VLOG_WARN("bridge %s: interface %s is a system type where the bridge " >>>> + "is a netdev one", br->name, iface_cfg->name); >>>> + } >>>> VLOG_INFO("bridge %s: added interface %s on port %d", >>>> br->name, iface_cfg->name, *ofp_portp); >>>> >>>> >> >>
On 20.03.2019 12:18, Ilya Maximets wrote: > On 20.03.2019 11:34, Ilya Maximets wrote: >> On 20.03.2019 11:01, Eelco Chaudron wrote: >>> >>> >>> On 19 Mar 2019, at 16: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 <echaudro@redhat.com> >>>>> --- >>>> >>>> Hi. >>>> >>>> Hmm. What do you mean under misconfiguration? >>> >>> Good question, as the documentation and code are not clear about this part… >>> >>> My interpretation is that for the netdev datapath bridges DPDK/vhost ports, internal ports, patch-ports, and user space tunnel ports are the once supported. But any other kernel ports are not, however, your text below suggests otherwise. >> >> See below. >> >>> >>>> 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. >>> >>> With the below statement in mind, I configure the following: >>> >>> ovs-vsctl add-br test >>> ip link add name right1 type veth peer name left1 >>> ip link add name right2 type veth peer name left2 >>> ovs-vsctl add-port test left1 >>> ovs-vsctl add-port test left2 >>> ovs-vsctl add-port test eth0 >>> ip link set dev left1 up >>> ip link set dev left2 up >>> ip netns add netns1 >>> ip netns add netns2 >>> ip link set dev right1 netns netns1 >>> ip link set dev right2 netns netns2 >>> ip netns exec netns1 ip link set dev lo up >>> ip netns exec netns1 ip link set dev right1 up >>> ip netns exec netns2 ip link set dev right2 up >>> ip netns exec netns2 ip link set dev lo up >>> ip netns exec netns1 ip a a dev right1 192.168.0.1/24 >>> ip netns exec netns2 ip a a dev right2 192.168.0.2/24 >>> >>> This is all working fine now, and now some accidentally does this: >>> >>> ovs-vsctl set bridge test datapath_type=netdev >>> >>> And you suggest all continues to work as is? >> >> In general, yes. >> >>> >>>> The only possible issue will be that this bridge will no longer communicate >>>> with other bridges, because they're in different datapaths now. >>>> >>>> So, below warning will be printed on any attempt to add legitimate >>>> system network interface to the userspace bridge. >>> >>>> "system" devices supported by both datapaths, but "dpdk" devices supported >>>> only by userspace, that is why you see the error in case of changing to >>>> 'datapath_type=system'. >>> >>> Are you saying ALL system devices are supported by the netdev datapath? Or only a specific set? If the later we should check for those (and maybe add some infrastructure to identify easily which devices are supported by which datapath. If it exists please point me to it, as I might have overlooked it). >> >> OVS netdev datapath could open any linux network interfacce that could be >> opened with the raw socket. There is nothing device specific here. >> >> BTW, tests/system-userspace-testsuite.at completely relies on ability to >> open and forward traffic through the veth pairs by netdev datapath. >> >>> >>>> Maybe I'm missing something? What is the issue you're trying to address? >>> >>> The above example stops working due to checksum offloading on veth. >>> But if you are right this is a valid configuration I’ll further investigate this. >> >> Configuration is valid. The issue here is that OVS netdev datapath doesn't >> support TX checksum offloading (this is not easy task with arguable profit). >> i.e. if packet arrives with bad/no checksum it will be sent to the output port >> with same bad/no checksum. Everything works in case of kernel datapth because >> the packet doesn't leave the kernel space. In case of netdev datapath some >> information (like CHECKSUM_VALID skb flags) is lost while receiving via I meant CHECKSUM_UNNECESSARY. >> socket in userspace and subsequently kernel expects valid checksum while >> receiving the packet from userspace because TX offloading is not enabled. >> >> This kind of issues usually mitigated by disabling TX offloading on the >> "right*" interfaces, or by setting iptables to fill the checksums like this: >> >> iptables -A POSTROUTING -t mangle -p udp -m udp -j CHECKSUM --checksum-fill >> >> Some related OpenStack bug: https://bugs.launchpad.net/neutron/+bug/1244589 > > Also, note that this happens only for virtual interfaces like veth/tap because > kernel always tries to delay checksum calculation/validation as much as possible. > Correct packets received from the wire will always have correct checksums. > >> >>> >>>> Best regards, Ilya Maximets. >>>> >>>>> vswitchd/bridge.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >>>>> index a427b0122..42c33d1d9 100644 >>>>> --- a/vswitchd/bridge.c >>>>> +++ b/vswitchd/bridge.c >>>>> @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, >>>>> goto error; >>>>> } >>>>> >>>>> + if (!iface_is_internal(iface_cfg, br->cfg) >>>>> + && !strcmp(br->type, "netdev") >>>>> + && !strcmp(netdev_get_type(netdev), "system")) { >>>>> + VLOG_WARN("bridge %s: interface %s is a system type where the bridge " >>>>> + "is a netdev one", br->name, iface_cfg->name); >>>>> + } >>>>> VLOG_INFO("bridge %s: added interface %s on port %d", >>>>> br->name, iface_cfg->name, *ofp_portp); >>>>> >>>>> >>> >>> > >
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 <echaudro@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. 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. 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. $ ovs-vsctl show c2513e49-0e08-4cd7-b633-461c88d810ff Bridge "br0" datapath_type: netdev Port "br0" Interface "br0" type: internal Best regards, Ilya Maximets.
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 <echaudro@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?
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 <echaudro@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.
On 20 Mar 2019, at 10:28, Ilya Maximets wrote: > On 20.03.2019 12:18, Ilya Maximets wrote: >> On 20.03.2019 11:34, Ilya Maximets wrote: >>> On 20.03.2019 11:01, Eelco Chaudron wrote: >>>> >>>> >>>> On 19 Mar 2019, at 16: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 <echaudro@redhat.com> >>>>>> --- >>>>> >>>>> Hi. >>>>> >>>>> Hmm. What do you mean under misconfiguration? >>>> >>>> Good question, as the documentation and code are not clear about >>>> this part… >>>> >>>> My interpretation is that for the netdev datapath bridges >>>> DPDK/vhost ports, internal ports, patch-ports, and user space >>>> tunnel ports are the once supported. But any other kernel ports are >>>> not, however, your text below suggests otherwise. >>> >>> See below. >>> >>>> >>>>> 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. >>>> >>>> With the below statement in mind, I configure the following: >>>> >>>> ovs-vsctl add-br test >>>> ip link add name right1 type veth peer name left1 >>>> ip link add name right2 type veth peer name left2 >>>> ovs-vsctl add-port test left1 >>>> ovs-vsctl add-port test left2 >>>> ovs-vsctl add-port test eth0 >>>> ip link set dev left1 up >>>> ip link set dev left2 up >>>> ip netns add netns1 >>>> ip netns add netns2 >>>> ip link set dev right1 netns netns1 >>>> ip link set dev right2 netns netns2 >>>> ip netns exec netns1 ip link set dev lo up >>>> ip netns exec netns1 ip link set dev right1 up >>>> ip netns exec netns2 ip link set dev right2 up >>>> ip netns exec netns2 ip link set dev lo up >>>> ip netns exec netns1 ip a a dev right1 192.168.0.1/24 >>>> ip netns exec netns2 ip a a dev right2 192.168.0.2/24 >>>> >>>> This is all working fine now, and now some accidentally does this: >>>> >>>> ovs-vsctl set bridge test datapath_type=netdev >>>> >>>> And you suggest all continues to work as is? >>> >>> In general, yes. >>> >>>> >>>>> The only possible issue will be that this bridge will no longer >>>>> communicate >>>>> with other bridges, because they're in different datapaths now. >>>>> >>>>> So, below warning will be printed on any attempt to add legitimate >>>>> system network interface to the userspace bridge. >>>> >>>>> "system" devices supported by both datapaths, but "dpdk" devices >>>>> supported >>>>> only by userspace, that is why you see the error in case of >>>>> changing to >>>>> 'datapath_type=system'. >>>> >>>> Are you saying ALL system devices are supported by the netdev >>>> datapath? Or only a specific set? If the later we should check for >>>> those (and maybe add some infrastructure to identify easily which >>>> devices are supported by which datapath. If it exists please point >>>> me to it, as I might have overlooked it). >>> >>> OVS netdev datapath could open any linux network interfacce that >>> could be >>> opened with the raw socket. There is nothing device specific here. >>> >>> BTW, tests/system-userspace-testsuite.at completely relies on >>> ability to >>> open and forward traffic through the veth pairs by netdev datapath. >>> >>>> >>>>> Maybe I'm missing something? What is the issue you're trying to >>>>> address? >>>> >>>> The above example stops working due to checksum offloading on veth. >>>> But if you are right this is a valid configuration I’ll further >>>> investigate this. >>> >>> Configuration is valid. The issue here is that OVS netdev datapath >>> doesn't >>> support TX checksum offloading (this is not easy task with arguable >>> profit). >>> i.e. if packet arrives with bad/no checksum it will be sent to the >>> output port >>> with same bad/no checksum. Everything works in case of kernel >>> datapth because >>> the packet doesn't leave the kernel space. In case of netdev >>> datapath some >>> information (like CHECKSUM_VALID skb flags) is lost while receiving >>> via > > I meant CHECKSUM_UNNECESSARY. Thanks Ilya for all the details above, this helped me to quickly get trough the bottom of this. >>> socket in userspace and subsequently kernel expects valid checksum >>> while >>> receiving the packet from userspace because TX offloading is not >>> enabled. >>> >>> This kind of issues usually mitigated by disabling TX offloading on >>> the >>> "right*" interfaces, or by setting iptables to fill the checksums >>> like this: >>> >>> iptables -A POSTROUTING -t mangle -p udp -m udp -j CHECKSUM >>> --checksum-fill >>> >>> Some related OpenStack bug: >>> https://bugs.launchpad.net/neutron/+bug/1244589 >> >> Also, note that this happens only for virtual interfaces like >> veth/tap because >> kernel always tries to delay checksum calculation/validation as much >> as possible. >> Correct packets received from the wire will always have correct >> checksums. >> >>> >>>> >>>>> Best regards, Ilya Maximets. >>>>> >>>>>> vswitchd/bridge.c | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >>>>>> index a427b0122..42c33d1d9 100644 >>>>>> --- a/vswitchd/bridge.c >>>>>> +++ b/vswitchd/bridge.c >>>>>> @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, >>>>>> goto error; >>>>>> } >>>>>> >>>>>> + if (!iface_is_internal(iface_cfg, br->cfg) >>>>>> + && !strcmp(br->type, "netdev") >>>>>> + && !strcmp(netdev_get_type(netdev), "system")) { >>>>>> + VLOG_WARN("bridge %s: interface %s is a system >>>>>> type where the bridge " >>>>>> + "is a netdev one", br->name, >>>>>> iface_cfg->name); >>>>>> + } >>>>>> VLOG_INFO("bridge %s: added interface %s on port %d", >>>>>> br->name, iface_cfg->name, >>>>>> *ofp_portp); >>>>>> >>>>>> >>>> >>>> >> >>
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index a427b0122..42c33d1d9 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -1808,6 +1808,12 @@ iface_do_create(const struct bridge *br, goto error; } + if (!iface_is_internal(iface_cfg, br->cfg) + && !strcmp(br->type, "netdev") + && !strcmp(netdev_get_type(netdev), "system")) { + VLOG_WARN("bridge %s: interface %s is a system type where the bridge " + "is a netdev one", br->name, iface_cfg->name); + } VLOG_INFO("bridge %s: added interface %s on port %d", br->name, iface_cfg->name, *ofp_portp);
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 <echaudro@redhat.com> --- vswitchd/bridge.c | 6 ++++++ 1 file changed, 6 insertions(+)