GH-69 _has_link() in lib/corosync.py returns wrong status in case of …

…link state is Unknown (#82)

* GH-69 _has_link() in lib/corosync.py returns wrong status in case of link state is Unknown

Fixes #69.

The _has_link function does not account for the "unknown" state. This function should use ioctl to retrieve the current
attributes of the network interface in question and should check to see if it is both up and running.

Signed-off-by: Will Johnson <[email protected]>

* - The failing test has an interface with 17 characters. Update the ifreq structure to allow 17 characters.

Signed-off-by: Will Johnson <[email protected]>

* - Fix spelling

Signed-off-by: Will Johnson <[email protected]>

* A quick test running in the python repel shows that if the ifr_name is a c_char * 17 the ifr_flags will always be 0,
  which is not what we want. Setting the type to 16 characters, however, does work, and matches the length specified in
  /usr/src/kernels/3.10.0-957.1.3.el7_lustre.x86_64/include/uapi/linux/if.h

Signed-off-by: Will Johnson <[email protected]>

* - Unfortunately, the socket does not return the operstate, which indicates the operational state of the ethernet device.
NetLink is an alternative but would require writing a small and complex socket library to get the state. The path of
least resistance is to check the value in "/sys/class/net/<devicename>/operstate". This will be one of the valid values
listed in https://www.kernel.org/doc/Documentation/networking/operstates.txt. Ultimately, the has_link method should
return True if the state is "up", otherwise False.

Signed-off-by: Will Johnson <[email protected]>

* - Fix tests

Signed-off-by: Will Johnson <[email protected]>

chroma_agent/lib/corosync.py

@@ -28,6 +28,8 @@
 # Used to make noise on ring1 to enable corosync detection.
 talker_thread = None
 
+operstate = "/sys/class/net/{}/operstate"
+
 
 class RingDetectionError(Exception):
     pass
@@ -449,10 +451,6 @@ def bindnetaddr(self):
 
     @property
     def has_link(self):
-        import array
-        import struct
-        import fcntl
-
         old_link_state_up = self.is_up
 
         # HYD-2003: Some NICs require the interface to be in an UP state
@@ -463,15 +461,20 @@ def has_link(self):
             AgentShell.try_run(["/sbin/ip", "link", "set", "dev", self.name, "up"])
             time_left = 10
 
+        def _get_device_state(name):
+            try:
+                filepath = operstate.format(name)
+                if os.path.exists(filepath):
+                    with open(filepath, "r") as f:
+                        return f.read().strip()
+                else:
+                    return "unknown"
+            except IOError:
+                print("Could not read state of ethernet device {}".format(name))
+                return "unknown"
+
         def _has_link():
-            SIOCETHTOOL = 0x8946
-            ETHTOOL_GLINK = 0x0000000A
-            sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
-            ecmd = array.array("B", struct.pack("2I", ETHTOOL_GLINK, 0))
-            ifreq = struct.pack("16sP", self.name, ecmd.buffer_info()[0])
-            fcntl.ioctl(sock.fileno(), SIOCETHTOOL, ifreq)
-            sock.close()
-            return bool(struct.unpack("4xI", ecmd.tostring())[0])
+            return _get_device_state(self.name) == "up"
 
         try:
             while time_left:

tests/test_corosync_config.py

@@ -475,34 +475,29 @@ def test_find_subnet(self):
         for args, output in test_map.items():
             self.assertEqual(output, find_subnet(*args))
 
-    def test_failed_has_link(self):
-        self.link_patcher.stop()
-
-        mock.patch(
-            "chroma_agent.lib.corosync.CorosyncRingInterface.__getattr__",
-            return_value=False,
-        ).start()
-
-        import errno
-
-        def boom(*args):
-            # EMULTIHOP is what gets raised with IB interfaces
-            raise IOError(errno.EMULTIHOP)
-
-        mock.patch("fcntl.ioctl", side_effect=boom).start()
-
-        from chroma_agent.lib.corosync import get_ring0
-
-        iface = get_ring0()
-
-        # add shell commands to be expected
-        self.add_commands(
-            CommandCaptureCommand(("/sbin/ip", "link", "set", "dev", iface.name, "up")),
-            CommandCaptureCommand(
-                ("/sbin/ip", "link", "set", "dev", iface.name, "down")
-            ),
-        )
-
-        self.assertFalse(iface.has_link)
-
-        self.assertRanAllCommandsInOrder()
+    def test_link_state_unknown(self):
+        with mock.patch("__builtin__.open", mock.mock_open(read_data="unknown")):
+            with mock.patch(
+                "chroma_agent.lib.corosync.CorosyncRingInterface.__getattr__",
+                return_value=False,
+            ):
+                with mock.patch("os.path.exists", return_value=True):
+                    self.link_patcher.stop()
+
+                    from chroma_agent.lib.corosync import get_ring0
+
+                    iface = get_ring0()
+
+                    # add shell commands to be expected
+                    self.add_commands(
+                        CommandCaptureCommand(
+                            ("/sbin/ip", "link", "set", "dev", iface.name, "up")
+                        ),
+                        CommandCaptureCommand(
+                            ("/sbin/ip", "link", "set", "dev", iface.name, "down")
+                        ),
+                    )
+
+                    self.assertFalse(iface.has_link)
+
+                    self.assertRanAllCommandsInOrder()