Bridge handling improved (bsc#962824) #441

teclator · 2016-09-21T12:08:50Z

I have unified the configuration overview with bridges involved with the bonding one as you can see in the screenshots. There have been fixed also some small issue.

Bond overview

Bridge overview

Old bridge configuration

New bridge configuration

coveralls · 2016-09-21T12:11:51Z

Coverage increased (+0.1%) to 46.721% when pulling d04b8df on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

coveralls · 2016-09-21T12:13:30Z

Coverage increased (+0.1%) to 46.721% when pulling d04b8df on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

teclator · 2016-09-21T12:13:46Z

src/modules/Lan.rb

@@ -1039,7 +1042,6 @@ def Packages
        # for wlan require iw instead of wireless-tools (bnc#539669)
        "wlan" => "iw",
        "vlan" => "vlan",
-        "br"   => "bridge-utils",


As we are using iptools2 and wicked, this dependency should be removed as this way of configure bridges is currently deprecated.

coveralls · 2016-09-21T12:25:32Z

Coverage increased (+0.1%) to 46.721% when pulling 2acf172 on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

coveralls · 2016-09-22T06:11:48Z

Coverage increased (+0.1%) to 46.729% when pulling f8c614f on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

teclator · 2016-09-22T06:43:57Z

src/include/network/lan/address.rb

@@ -1553,8 +1555,6 @@ def AddressDialog
        LanItems.vlan_id = Builtins.tostring(
          Ops.get_integer(@settings, "VLAN_ID", 0)
        )
-      elsif LanItems.type == "br"
-        LanItems.bridge_ports = Ops.get_string(@settings, "BRIDGE_PORTS", "")


Moving this line to StoreBridge avoid current ports to be removed in case that the bridge ports tab is not visited.

Previously

After the fix

nice ;-) Very common pattern in network code :-/

coveralls · 2016-09-22T09:10:14Z

Coverage increased (+0.1%) to 46.694% when pulling 2437213 on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

coveralls · 2016-09-22T15:15:01Z

Coverage increased (+0.04%) to 46.63% when pulling 0084525 on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

teclator · 2016-09-22T15:16:15Z

src/include/network/lan/address.rb

+                            "Bridge ports: %s\n\n" \
+                            "Do you want to adapt them now?"), ports.join(", "))
+          if Popup.YesNoHeadline(Label.WarningMsg, message)
+            ports.each { |p| configure_as_bridge_port(p) }


We are just adapting it when the bridge is edited as we can see in this gif:

We could also ask for adapt the configuration when the enslaved interface is edited.

What is there for the user to decide? IMHO this pop-up is harmful, burdening the user with a decision they cannot understand ("old way, new way"?)
I propose that we simply adapt the config and just log the action.

@mvidner

It was a doubt I have at the beginning of the PBI as it was also commented in the card. After discussing it with @mchf the conclusion was that automatic conversion does not means not ask the user about it.

We could do it automatically, in that case when? when the interface is edited and saved or also do it although the user press cancel in the edition. If should be converted so why not just have a method that checks current interface configuration when the client is called and adapt old config style?

We just decided to prompt the user just to verify he really wanted to adapt the current configuration and also being a litle conservative taking in account that if something change in current workflow the network is restarted and should not restarted if not (so maybe the user does not expect changes when OK pressed but then he realized that the network was restarted and some files changed magically.

coveralls · 2016-09-23T07:21:40Z

Coverage increased (+0.1%) to 46.72% when pulling 0c4c35a on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

coveralls · 2016-09-23T08:39:40Z

Coverage increased (+0.1%) to 46.735% when pulling 38a91f2 on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

mchf · 2016-09-23T08:58:35Z

package/yast2-network.changes

+  - "NONE" is shown instead of 0.0.0.0 for old bridge configuration
+  - The bridge master is shown in the enslaved interface.
+  - The interfaces overview is updated after a bridge is modified
+- 3.1.170.1


if I remember correctly, SP3 didn't diverged so far.

This change #440 was the reason I choose that version.

ok, i didn't remember well ;-)

mchf · 2016-09-23T09:06:47Z

src/include/network/complex.rb

+      bootproto = devmap["BOOTPROTO"] || "static"
+
+      if bootproto.empty? || bootproto == "static"
+        devmap["IPADDR"]


I'd add || "" as old implementation doesn't count with nil. I know it is kind of edge case .... but

In case of nil bootproto is set to "static" as you can see in line 408. Now we have a unit test for DeviceProtocol method and we can see that for nil, "" and "static" we return the IP address.

I've addressed devmap["IPADDR"] here

mchf · 2016-09-23T13:09:42Z

src/include/network/lan/address.rb

@@ -1596,6 +1593,10 @@ def AddressDialog

  private

+    def get_selected_bridges


missing documentation for new method

Added documentation

mchf · 2016-09-23T13:10:18Z

src/include/network/lan/address.rb

-        key,
-        Ops.get_string(@settings, "BRIDGE_PORTS", "")
-      )
+      @settings["BRIDGE_PORTS"] = get_selected_bridges.join(" ")


misleading function name. Something like selected_bridge_ports would be better in my POV.

Renamed and also renamed get_selected_slaves

coveralls · 2016-09-26T16:39:09Z

Coverage increased (+0.2%) to 46.776% when pulling b48eba9 on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

mchf · 2016-09-27T07:22:47Z

src/include/network/lan/address.rb

+    # of a bridge. In case that some configuration is not correct then adapt it
+    # if the user accepts.
+    def check_bridge_ports_config
+      return false unless LanItems.type == "br"


As I try to drop dependency on these "global" status variables I prefer to have new functions stateless (e.g. checking device type on-the-fly as you never know who forgot to reset or set it somewhere to something reasonable). It should also ease "big bang" refactoring of LanItems one day.

This was exactly the reason I proposed the new method for adapt the config, because the other ones depends on the NetworkInterfaces instance, and it also changed the Current to the last port edited what could produce some unexpected issues.

Just thinking ...
... using NetworkInterfaces.GetType makes no sense when creating new bridge until new configuration is commited into NetworkInterfaces
... so you can just move the type check out of this method.

mchf · 2016-09-27T07:25:23Z

src/include/network/lan/address.rb

+      ports = LanItems.bridge_ports.split(" ").select { |p| old_bridge_port_config?(p) }
+
+      unless ports.empty?
+        ports.each { |p| configure_as_bridge_port(p) } if fix_bridge_port_config?(ports)


you really want to raise a popup for every old fashioned bridge port? Poor SLE-11 user with 10 ports bridge ;-)

It is only raised once with all the ports in the question

sure, my fault.

teclator · 2016-09-27T07:27:18Z

src/include/network/lan/bridge.rb

+
+    # Asks the user about adapt the current bridge port config in case that it
+    # is configured in the old way (BOOTPROTO == static) & (IPADDR == 0.0.0.0).
+    def fix_bridge_port_config?(ports)


Maybe adapt_bridge_port_config? could be a better name for this method, and then instead of configure_as_bridge_port define this method as it is an adaption and not a new configuration at all:

def adapt_bridge_port_config?(ports) ... end def adapt_bridge_port_config!(ifcfg_name) item_id = LanItems.get_configured(ifcfg_name) devmap = LanItems.GetDeviceMap(item_id) return false unless devmap devmap["IPADDR"] = "" devmap["NETMASK"] = "" devmap["BOOTPROTO"] = "none" # take out PREFIXLEN from old configuration (BNC#735109) devmap["PREFIXLEN"] = "" LanItems.SetDeviceMap(item_id, devmap) true end

@mchf what do you think?

well it better describes current use case, but i don't see any benefit in having such one purpose method. So do you have any other pros? ;-)

…ses.

coveralls · 2016-09-27T07:39:19Z

Coverage increased (+0.1%) to 46.715% when pulling b15ccbc on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

coveralls · 2016-09-27T09:19:16Z

Coverage increased (+0.2%) to 46.796% when pulling dd91db4 on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

coveralls · 2016-09-27T09:30:47Z

Coverage increased (+0.2%) to 46.796% when pulling 6f3fab4 on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

mchf · 2016-09-27T09:44:57Z

src/include/network/lan/bridge.rb

-      NetworkInterfaces.Edit(device)
+      unless NetworkInterfaces.Edit(device)
+        Lan.autoconf_slaves += [device] unless Lan.autoconf_slaves.include? device
+        return false


inconsistent return value

@teclator what is this method supposed to return in this case, and in the normal case? Add a @return tag?

At the end i decided to just let it as it was, and it always returns true and just add the changes if NetworkInterfaces.Edit(device) success and also remember the previous Interfaces selected by NetworkInterface to avoid let it in a different one what could produce not desired results.

https://github.com/teclator/yast-network/blob/bridge_overview_improvement/src/include/network/lan/bridge.rb#L92

As an alternative I added the adapt_old_port_config method which avoid to work over NetworkInterfaces and where we could also add the logic to write the config to the files without restarting the network

https://github.com/teclator/yast-network/blob/bridge_overview_improvement/src/include/network/lan/bridge.rb#L113

mchf · 2016-09-27T09:45:39Z

src/include/network/lan/bridge.rb

@@ -60,6 +87,21 @@ def configure_as_bridge_port(device)

      NetworkInterfaces.Commit
      NetworkInterfaces.Add
+
+      NetworkInterfaces.Current = selected_interface


why to return original Current?

is not returned anymore, i let it as it was returning true again

mchf · 2016-09-27T10:02:44Z

src/include/network/lan/bridge.rb

      log.info("Adapt device #{device} as bridge port")

      # when using wicked every device which can be bridged
      # can be set to BOOTPROTO=none. No workaround with
      # BOOTPROTO=static required anymore
-      NetworkInterfaces.Edit(device)
+      unless NetworkInterfaces.Edit(device)
+        Lan.autoconf_slaves += [device] unless Lan.autoconf_slaves.include? device


well this autoconf stuff is part of a fate request for bonding devices where it was requested to do an udev update for slaves. As it was not requested for bridges, it shouldn't be implemented for bridges in my POV.

Ok, as we was talking some time ago about unifying bonding and bridge management as much as possible was what I tried but this is going too further and some changes should not be necessary and also invalid, so taking this in account I'm going to modify the PR in concordance.

coveralls · 2016-09-29T05:50:22Z

Coverage increased (+0.2%) to 46.789% when pulling 95a7ced on teclator:bridge_overview_improvement into 825b7af on yast:SLE-12-SP2.

mvidner · 2016-10-04T13:05:05Z

test/bridge_test.rb

-        "eth12",
-        "tap0"
-      ]
+  describe "#old_bridge_port_config?" do


please add a test for the "eth1" case , which has { "BOOTPROTO" => "none" } and so should return false

mvidner · 2016-10-04T13:50:07Z

src/include/network/lan/address.rb

-        key,
-        Ops.get_string(@settings, "BRIDGE_PORTS", "")
-      )
+      @settings["BRIDGE_PORTS"] = selected_bridge_ports.join(" ")


NP: selected_bridge_ports is only used once, from here, and it is a one line method hiding 1000 lines away.
Readability suffers.

mvidner · 2016-10-04T14:24:36Z

src/include/network/lan/address.rb

-      )
+      @settings["BRIDGE_PORTS"] = selected_bridge_ports.join(" ")
+
+      LanItems.bridge_ports = @settings["BRIDGE_PORTS"]


NP: So @settings["BRIDGE_PORTS"] is a String but @settings["SLAVES"] is an Array. WTF. I know this is a bit out of focus, but could we at least document it and add a FIXME to unify it?

Sure, and is how differs also in configuration per interface. As bonding master is configured as:

BONDING_SLAVE0='eth0' BONDING_SLAVE1='eth1'

while bridge_ports are configured as

BRIDGE_PORTS='eth0 eth1'

and more or less it are documented in LanItems also them are published variables so it means changed them we would be changing public interface, isn't it?

https://github.com/yast/yast-network/blob/master/src/modules/LanItems.rb#L2617

mvidner

For me the biggest problem is the new pop-up which I think should not be introduced. The rest is implementation details.
@mchf what is your overall comment on this PR?

mvidner · 2016-10-04T14:56:18Z

src/modules/LanItems.rb

@@ -732,7 +732,7 @@ def IsBondable(bondMaster, itemId)
    # @param [Fixnum] itemId        index into LanItems::Items
    # TODO: bridgeMaster is not used yet bcs detection of bridge master


This TODO is no longer valid

teclator · 2016-10-04T15:52:32Z

@mchf @mvidner
I have doubts about what to do with the Popup and also about refreshing all the interfaces that were added to the bridge, as currently just the interfaces configuration was written but the interface was not updated, and you could see them as not configured and things like that.

So i was in doubt if just duplicate current behavior or try to unify it with bonding.

mchf · 2016-10-06T07:29:23Z

src/include/network/lan/bridge.rb

+    # @param [String] ip
+    # @return [String] empty string if given ip is 0.0.0.0; given ip
+    # otherwise
+    def bridge_ip(ip)


it should be bridge_port_ip

…anOverview.

Otherwise SetDeviceVars overwrites default empty string in LanItems#bridge_ports (hardcoded in the code) by nil when creating new bridge.

teclator · 2016-10-14T08:08:31Z

Has been replaced by #448

teclator added 3 commits September 21, 2016 13:01

Improved handling of bridges (bsc#962824).

429b66e

Bump version.

b231a8c

Fix comment.

d04b8df

teclator commented Sep 21, 2016

View reviewed changes

teclator changed the title ~~Bridge handling improved (bsc#962824)~~ [WIP] Bridge handling improved (bsc#962824) Sep 21, 2016

Make rubocop happy.

2acf172

Fixed bridge_ports was removed if bridge_ports tab was not selected.

f8c614f

teclator commented Sep 22, 2016

View reviewed changes

Modify old configuration file when bridge is edited.

2437213

Ask for adapt interfaces with old bridge configuration.

0084525

teclator commented Sep 22, 2016

View reviewed changes

teclator added 2 commits September 23, 2016 08:17

Added tests for new methods

9861552

Better way to check devmap for the given interface name.

0c4c35a

Added test for #DeviceProtocol

38a91f2

teclator changed the title ~~[WIP] Bridge handling improved (bsc#962824)~~ Bridge handling improved (bsc#962824) Sep 23, 2016

teclator changed the title ~~Bridge handling improved (bsc#962824)~~ [WIP] Bridge handling improved (bsc#962824) Sep 23, 2016

teclator added blog and removed blog labels Sep 23, 2016

mchf reviewed Sep 23, 2016

View reviewed changes

Some changes based on CR.

b48eba9

teclator force-pushed the bridge_overview_improvement branch from 5d50cc4 to b48eba9 Compare September 26, 2016 16:35

mchf reviewed Sep 27, 2016

View reviewed changes

teclator commented Sep 27, 2016

View reviewed changes

DeviceProtocol: if an IP is returned, return it as a string in all ca…

b15ccbc

…ses.

Adapt the configuration instead of configure as new.

dd91db4

Added comments to adapt_bridge_port_config method

6f3fab4

mchf reviewed Sep 27, 2016

View reviewed changes

teclator added 2 commits September 28, 2016 13:14

Skip udev rules in bridge configuration.

4194c88

Some changes based on code review.

95a7ced

mvidner reviewed Oct 4, 2016

View reviewed changes

mvidner requested changes Oct 4, 2016

View reviewed changes

Some changes based on CR.

e403943

mchf reviewed Oct 6, 2016

View reviewed changes

Just delegates in DeviceProtocol what to show as it is used only in L…

4087111

…anOverview.

This was referenced Oct 6, 2016

Network: adapt old network configuration when needed (bsc#962824) yast/yast-yast2#500

Merged

Alternative improve bridge handling (bsc#962824) #448

Merged

Use empty string as a default for bridge ports (yml update)

b4db386

Otherwise SetDeviceVars overwrites default empty string in LanItems#bridge_ports (hardcoded in the code) by nil when creating new bridge.

teclator closed this Oct 14, 2016

		@@ -1596,6 +1593,10 @@ def AddressDialog

		private

		def get_selected_bridges

		@@ -732,7 +732,7 @@ def IsBondable(bondMaster, itemId)
		# @param [Fixnum] itemId index into LanItems::Items
		# TODO: bridgeMaster is not used yet bcs detection of bridge master

Bridge handling improved (bsc#962824) #441

Bridge handling improved (bsc#962824) #441

Conversation

teclator commented Sep 21, 2016

Bond overview

Bridge overview

Old bridge configuration

New bridge configuration

coveralls commented Sep 21, 2016

coveralls commented Sep 21, 2016

Choose a reason for hiding this comment

coveralls commented Sep 21, 2016

coveralls commented Sep 22, 2016

teclator Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Previously

After the fix

Choose a reason for hiding this comment

coveralls commented Sep 22, 2016

coveralls commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 23, 2016

coveralls commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teclator Sep 27, 2016 • edited by mvidner Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 27, 2016

coveralls commented Sep 27, 2016

coveralls commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 29, 2016

Choose a reason for hiding this comment

mvidner Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

mvidner Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

teclator Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

mvidner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teclator commented Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

teclator commented Oct 14, 2016 • edited Loading

teclator Sep 22, 2016 •

edited

Loading

teclator Sep 27, 2016 •

edited by mvidner

Loading

mvidner Oct 4, 2016 •

edited

Loading

mvidner Oct 4, 2016 •

edited

Loading

teclator Oct 4, 2016 •

edited

Loading

teclator commented Oct 4, 2016 •

edited

Loading

teclator commented Oct 14, 2016 •

edited

Loading