Merge lp:~tribaal/landscape-client/fix-1130130-backwards-incompatible-messgae into lp:~landscape/landscape-client/trunk

[Chris Glass]

This branch removes the previously introduced "speed" member of the "network-device" message, and adds another top-level key instead, called "device-speeds" that contains the interface, the speed, and a newly introduced "duplex" flag (to know if a link is in duplex mode or not).

Since we have logic in place to filter unknown top-level keys, landscape servers should not wedge computers even if they are older that the client (i.e. if they don't have the schema introduced bu this branch).

The new key is marked optional for the opposing case - a new server will not wedge computers with an older version of the client.

[Alberto Donato (ack)]

Look good! +1

#1:
+ for interface in get_active_interfaces(sock):
+ if interface in skipped_interfaces:
+ continue
+ if skip_vlan and "." in interface:
+ continue
+ if skip_alias and ":" in interface:
+ continue

since the same logic is present in get_active_device_info, it'd be nice to extract it in a method, such as

+ for interface in get_active_interfaces(sock):
+ if skip_interface(interface, skipped_interfaces, skip_vlan, skip_alias):
+ continue

review: Approve

[lp:~tribaal/landscape-client/fix-1130130-backwards-incompatible-messgae]

629. By Chris Glass on 2013-02-21

Refactored common skipping logic in a separate function, as suggested.

[Thomas Herve (therve)]

[1]
+ # avaialble. We'll just assume it's False in that case.

Typo: available.

[2] I think I would keep a single get_active_device_info function, and change the message in the plugin instead. This would mean only calling get_active_interfaces once, only creating one socket, etc.

Thanks!

review: Needs Fixing

[lp:~tribaal/landscape-client/fix-1130130-backwards-incompatible-messgae]

630. By Chris Glass on 2013-02-21

Made suggested changes. There is only one method again in the network library,
and the message splitting is done in the plugin instead.

631. By Chris Glass on 2013-02-21

Typo run

[Thomas Herve (therve)]

Looks great, +1! Maybe put a comment in message_schemas to explain the weird thing we're doing here. You may want to wait a bit for landing this until the matching server branch is ready, too.

review: Approve

[lp:~tribaal/landscape-client/fix-1130130-backwards-incompatible-messgae]

632. By Chris Glass on 2013-02-21

Added a little comment to the message schema

=== modified file 'landscape/lib/network.py'
--- landscape/lib/network.py	2013-02-15 17:20:21 +0000
+++ landscape/lib/network.py	2013-02-21 13:39:30 +0000
@@ -40,7 +40,7 @@
     return struct.calcsize("l") == 8
 
 
-# initialize the struct size as per the machine's archictecture
+# initialize the struct size as per the machine's architecture
 IF_STRUCT_SIZE = is_64() and IF_STRUCT_SIZE_64 or IF_STRUCT_SIZE_32
 
 
@@ -51,7 +51,7 @@
     """
     max_interfaces = 128
 
-    # Setup an an array to hold our response, and initialized to null strings.
+    # Setup an array to hold our response, and initialized to null strings.
     interfaces = array.array("B", "\0" * max_interfaces * IF_STRUCT_SIZE)
     buffer_size = interfaces.buffer_info()[0]
     packed_bytes = struct.pack(
@@ -157,8 +157,9 @@
                 sock, interface)
             interface_info["netmask"] = get_netmask(sock, interface)
             interface_info["flags"] = get_flags(sock, interface)
-            interface_info["speed"] = get_network_interface_speed(sock,
-                                                                  interface)
+            speed, duplex = get_network_interface_speed(sock, interface)
+            interface_info["speed"] = speed
+            interface_info["duplex"] = duplex
             results.append(interface_info)
     finally:
         del sock
@@ -220,7 +221,7 @@
         * 10, 100, 1000, 2500, 10000: The interface speed in Mbps
         * -1: The interface does not support querying for max speed, such as
           virtio devices for instance.
-        * 0: The cable is not connected to the interface. We cannot mesure
+        * 0: The cable is not connected to the interface. We cannot measure
           interface speed, but could if it was plugged in.
     """
     cmd_struct = struct.pack('I39s', ETHTOOL_GSET, '\x00' * 39)
@@ -231,16 +232,23 @@
     try:
         fcntl.ioctl(sock, SIOCETHTOOL, packed)  # Status ioctl() call
         res = status_cmd.tostring()
-        speed = struct.unpack('12xH29x', res)[0]
+        speed, duplex = struct.unpack('12xHB28x', res)
     except IOError as e:
         if e.errno != errno.EOPNOTSUPP:
             raise e
         # e is "Operation not supported".
         speed = -1
+        duplex = False
 
     # Drivers apparently report speed as 65535 when the link is not available
     # (cable unplugged for example).
     if speed == 65535:
         speed = 0
 
-    return speed
+    # The drivers report "duplex" to be 255 when the information is not
+    # available. We'll just assume it's False in that case.
+    if duplex == 255:
+        duplex = False
+    duplex = bool(duplex)
+
+    return speed, duplex

=== modified file 'landscape/lib/tests/test_network.py'
--- landscape/lib/tests/test_network.py	2013-02-18 09:17:48 +0000
+++ landscape/lib/tests/test_network.py	2013-02-21 13:39:30 +0000
@@ -18,10 +18,10 @@
         network devices, compare and verify the output against
         that returned by ifconfig.
         """
-        mock_get_interface_speed = self.mocker.replace(
+        mock_get_network_interface_speed = self.mocker.replace(
             get_network_interface_speed)
-        mock_get_interface_speed(ANY, ANY)
-        self.mocker.result(100)
+        mock_get_network_interface_speed(ANY, ANY)
+        self.mocker.result((100, True))
         self.mocker.count(min=1, max=None)
         self.mocker.replay()
 
@@ -50,6 +50,7 @@
             if flags & 4096:
                 self.assertIn("MULTICAST", block)
             self.assertEqual(100, device["speed"])
+            self.assertEqual(True, device["duplex"])
 
     def test_skip_loopback(self):
         """The C{lo} interface is not reported by L{get_active_device_info}."""
@@ -225,12 +226,13 @@
         self.mocker.result(0)
 
         mock_unpack = self.mocker.replace("struct")
-        mock_unpack.unpack("12xH29x", ANY)
-        self.mocker.result((100,))
+        mock_unpack.unpack("12xHB28x", ANY)
+        self.mocker.result((100, False))
 
         self.mocker.replay()
 
-        self.assertEqual(100, get_network_interface_speed(sock, "eth0"))
+        self.assertEqual((100, False),
+                         get_network_interface_speed(sock, "eth0"))
 
     def test_get_network_interface_speed_unplugged(self):
         """
@@ -241,15 +243,15 @@
         # ioctl always succeeds
         mock_ioctl = self.mocker.replace("fcntl")
         mock_ioctl.ioctl(ANY, ANY, ANY)
-        self.mocker.result(0)
+        self.mocker.result((0, False))
 
         mock_unpack = self.mocker.replace("struct")
-        mock_unpack.unpack("12xH29x", ANY)
-        self.mocker.result((65535,))
+        mock_unpack.unpack("12xHB28x", ANY)
+        self.mocker.result((65535, False))
 
         self.mocker.replay()
 
-        self.assertEqual(0, get_network_interface_speed(sock, "eth0"))
+        self.assertEqual((0, False), get_network_interface_speed(sock, "eth0"))
 
     def test_get_network_interface_speed_not_supported(self):
         """
@@ -270,7 +272,8 @@
 
         self.mocker.replay()
 
-        self.assertEqual(-1, get_network_interface_speed(sock, "eth0"))
+        self.assertEqual((-1, False),
+                         get_network_interface_speed(sock, "eth0"))
 
     def test_get_network_interface_speed_other_io_error(self):
         """

=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2013-02-18 14:28:44 +0000
+++ landscape/message_schemas.py	2013-02-21 13:39:30 +0000
@@ -403,6 +403,10 @@
     "eucalyptus-info-error",
     {"error": String()})
 
+# The network-device message is split in two top level keys because we don't
+# support adding sub-keys in a backwards-compatible way (only top-level keys).
+# New servers will see an optional device-speeds key, and old servers will
+# simply ignore the extra info..
 NETWORK_DEVICE = Message(
     "network-device",
     {"devices": List(KeyDict({"interface": String(),
@@ -410,9 +414,13 @@
                               "mac_address": String(),
                               "broadcast_address": String(),
                               "netmask": String(),
-                              "flags": Int(),
-                              "speed": Int()},
-                             optional=["speed"]))})
+                              "flags": Int()})),
+
+     "device-speeds": List(KeyDict({"interface": String(),
+                                    "speed": Int(),
+                                    "duplex": Bool()}))},
+    optional=["device-speeds"])
+
 
 NETWORK_ACTIVITY = Message(
     "network-activity",

=== modified file 'landscape/monitor/networkdevice.py'
--- landscape/monitor/networkdevice.py	2010-12-10 13:57:11 +0000
+++ landscape/monitor/networkdevice.py	2013-02-21 13:39:30 +0000
@@ -20,5 +20,19 @@
         super(NetworkDevice, self).register(registry)
         self.call_on_accepted(self.message_type, self.exchange, True)
 
-    def get_data(self):
-        return self._device_info()
+    def get_message(self):
+        device_data = self._device_info()
+        # Persist if the info is new.
+        if self._persist.get("network-device-data") != device_data:
+            self._persist.set("network-device-data", device_data)
+            # We need to split the message in two top-level keys (see bug)
+            device_speeds = []
+            for device in device_data:
+                speed_entry = {"interface": device["interface"]}
+                speed_entry["speed"] = device.pop("speed")
+                speed_entry["duplex"] = device.pop("duplex")
+                device_speeds.append(speed_entry)
+
+            return {"type": self.message_type,
+                    "devices": device_data,
+                    "device-speeds": device_speeds}

=== modified file 'landscape/monitor/tests/test_networkdevice.py'
--- landscape/monitor/tests/test_networkdevice.py	2013-02-18 14:28:44 +0000
+++ landscape/monitor/tests/test_networkdevice.py	2013-02-21 13:39:30 +0000
@@ -1,5 +1,6 @@
 from landscape.tests.helpers import LandscapeTest, MonitorHelper
-from landscape.lib.network import get_active_device_info
+from landscape.lib.network import (
+    get_active_device_info)
 from landscape.monitor.networkdevice import NetworkDevice
 
 
@@ -35,21 +36,6 @@
         self.assertEqual(8, flags & 8)  # LOOPBACK
         self.assertEqual(64, flags & 64)  # RUNNING
 
-    def test_no_speed(self):
-        """
-        A message is sent with the device info, and no error is raised if the
-        speed parameter is omitted (to be backwards-compatible).
-        """
-        def test_get_active_device_info_no_speed():
-            devices = get_active_device_info(skipped_interfaces=())
-            for device in devices:
-                device.pop("speed")
-            return devices
-
-        plugin = NetworkDevice(test_get_active_device_info_no_speed)
-        self.monitor.add(plugin)
-        plugin.exchange()
-
     def test_no_message_with_no_changes(self):
         """If no device changes from the last message, no message is sent."""
         self.plugin.exchange()

=== modified file 'landscape/schema.py'
--- landscape/schema.py	2011-07-28 12:54:34 +0000
+++ landscape/schema.py	2013-02-21 13:39:30 +0000
@@ -38,6 +38,7 @@
         raise InvalidError("%r did not match any schema in %s"
                            % (value, self.schemas))
 
+
 class Bool(object):
     """Something that must be a C{bool}."""
     def coerce(self, value):
@@ -128,7 +129,7 @@
     @param schema: A sequence of schemas, which will be applied to
         each value in the tuple respectively.
     """
-    
+
     def __init__(self, *schema):
         self.schema = schema