Merge pull request #891 from rmotitsuki/master

Fix method get_next_available_tag (fix #883)

kytos/core/interface.py

@@ -103,11 +103,10 @@ def __eq__(self, other):
 
     @property
     def id(self):  # pylint: disable=invalid-name
-        """Return id from Interface intance.
+        """Return id from Interface instance.
 
         Returns:
             string: Interface id.
-
         """
         return "{}:{}".format(self.switch.dpid, self.port_number)
 
@@ -138,19 +137,26 @@ def enable(self):
         self._enabled = True
 
     def use_tag(self, tag):
-        """Remove a specific tag from available_tags if it is there."""
-        for available_tag in self.available_tags:
-            if tag == available_tag:
-                self.available_tags.remove(available_tag)
-                return True
+        """Remove a specific tag from available_tags if it is there.
+
+        Return False in case the tag is already removed.
+        """
+        if tag in self.available_tags:
+            self.available_tags.remove(tag)
+            return True
         return False
 
     def is_tag_available(self, tag):
         """Check if a tag is available."""
         return tag in self.available_tags
 
     def get_next_available_tag(self):
-        """Return the next available tag if exists."""
+        """Get the next available tag from the interface.
+
+        Return the next available tag if exists and remove from the
+        available tags.
+        If no tag is available return False.
+        """
         try:
             return self.available_tags.pop()
         except IndexError:

kytos/core/link.py

@@ -92,7 +92,10 @@ def available_tags(self):
                 self.endpoint_b.available_tags]
 
     def use_tag(self, tag):
-        """Remove a specific tag from available_tags if it is there."""
+        """Remove a specific tag from available_tags if it is there.
+
+        Deprecated: use only the get_next_available_tag method.
+        """
         if self.is_tag_available(tag):
             self.endpoint_a.use_tag(tag)
             self.endpoint_b.use_tag(tag)
@@ -106,9 +109,28 @@ def is_tag_available(self, tag):
 
     def get_next_available_tag(self):
         """Return the next available tag if exists."""
-        for tag in self.endpoint_a.available_tags:
-            if tag in self.endpoint_b.available_tags:
-                return tag
+        # Copy the available tags because in case of error
+        # we will remove and add elements to the available_tags
+        available_tags_a = self.endpoint_a.available_tags.copy()
+        available_tags_b = self.endpoint_b.available_tags.copy()
+
+        for tag in available_tags_a:
+            # Tag does not exist in endpoint B. Try another tag.
+            if tag not in available_tags_b:
+                continue
+
+            # Tag already in use. Try another tag.
+            if not self.endpoint_a.use_tag(tag):
+                continue
+
+            # Tag already in use in B. Mark the tag as available again.
+            if not self.endpoint_b.use_tag(tag):
+                self.endpoint_a.make_tag_available(tag)
+                continue
+
+            # Tag used successfully by both endpoints. Returning.
+            return tag
+
         return False
 
     def make_tag_available(self, tag):

tests/helper.py

@@ -1,6 +1,7 @@
 """Module with some helpers for tests."""
 import os
 import sys
+import threading
 import time
 from socket import socket
 
@@ -122,3 +123,51 @@ def new_handshaked_client(options=None):
         options = get_config().options['daemon']
     client = new_client(options)
     return do_handshake(client)
+
+
+def test_concurrently(times):
+    """
+    Decorator to test concurrently many times.
+
+    Add this decorator to small pieces of code that you want to test
+    concurrently to make sure they don't raise exceptions when run at the
+    same time.
+
+    E.g., some views that do a SELECT and then a subsequent
+    INSERT might fail when the INSERT assumes that the data has not changed
+    since the SELECT.
+    E.g.:
+        def test_method(self):
+        from tests.helpers import test_concurrently
+        @test_concurrently(10)
+        def toggle_call_your_method_here():
+            print('This is a test')
+
+        toggle_call_your_method_here()
+    """
+    def test_concurrently_decorator(test_func):
+        """Decorator thread execution."""
+        def wrapper(*args, **kwargs):
+            """Thread wrapper."""
+            exceptions = []
+
+            def call_test_func():
+                """Call the test method."""
+                try:
+                    test_func(*args, **kwargs)
+                except Exception as _e:
+                    exceptions.append(_e)
+                    raise
+            threads = []
+            for _ in range(times):
+                threads.append(threading.Thread(target=call_test_func))
+            for thread in threads:
+                thread.start()
+            for thread in threads:
+                thread.join()
+            if exceptions:
+                raise Exception('test_concurrently intercepted '
+                                '%s exceptions: %s' %
+                                (len(exceptions), exceptions))
+        return wrapper
+    return test_concurrently_decorator

tests/test_core/test_interface.py

@@ -5,7 +5,7 @@
 
 from pyof.v0x04.common.port import PortFeatures
 
-from kytos.core.interface import Interface
+from kytos.core.interface import Interface, TAG, TAGType
 from kytos.core.switch import Switch
 
 logging.basicConfig(level=logging.CRITICAL)
@@ -116,3 +116,48 @@ def test_interface_available_tags(self):
         self.iface.set_available_tags(custom_range)
         intf_values = [tag.value for tag in self.iface.available_tags]
         self.assertListEqual(intf_values, custom_range)
+
+    def test_all_available_tags(self):
+        """Test all available_tags on Interface class."""
+        max_range = 4096
+
+        for i in range(1, max_range):
+            next_tag = self.iface.get_next_available_tag()
+            self.assertIs(type(next_tag), TAG)
+            self.assertEqual(next_tag.value, max_range - i)
+
+        next_tag = self.iface.get_next_available_tag()
+        self.assertEqual(next_tag, False)
+
+    def test_interface_is_tag_available(self):
+        """Test is_tag_available on Interface class."""
+        max_range = 4096
+        for i in range(1, max_range):
+            tag = TAG(TAGType.VLAN, i)
+
+            next_tag = self.iface.is_tag_available(tag)
+            self.assertTrue(next_tag)
+
+        # test lower limit
+        tag = TAG(TAGType.VLAN, 0)
+        self.assertFalse(self.iface.is_tag_available(tag))
+        # test upper limit
+        tag = TAG(TAGType.VLAN, max_range)
+        self.assertFalse(self.iface.is_tag_available(tag))
+
+    def test_interface_use_tags(self):
+        """Test all use_tag on Interface class."""
+
+        tag = TAG(TAGType.VLAN, 100)
+        # check use tag for the first time
+        is_success = self.iface.use_tag(tag)
+        self.assertTrue(is_success)
+
+        # check use tag for the second time
+        is_success = self.iface.use_tag(tag)
+        self.assertFalse(is_success)
+
+        # check use tag after returning the tag to the pool
+        self.iface.make_tag_available(tag)
+        is_success = self.iface.use_tag(tag)
+        self.assertTrue(is_success)

tests/test_core/test_link.py

@@ -1,5 +1,6 @@
 """Link tests."""
 import logging
+import time
 import unittest
 from unittest.mock import Mock
 
@@ -49,3 +50,71 @@ def test_link_id(self):
         link1 = Link(self.iface1, self.iface2)
         link2 = Link(self.iface2, self.iface1)
         self.assertEqual(link1.id, link2.id)
+
+    def test_get_next_available_tag(self):
+        """Test get next available tags returns different tags"""
+        link = Link(self.iface1, self.iface2)
+        tag = link.get_next_available_tag()
+        next_tag = link.get_next_available_tag()
+
+        self.assertNotEqual(tag, next_tag)
+
+    def test_get_tag_multiple_calls(self):
+        """Test get next available tags returns different tags"""
+        link = Link(self.iface1, self.iface2)
+        tag = link.get_next_available_tag()
+        self.assertEqual(tag.value, 1)
+
+        next_tag = link.get_next_available_tag()
+        self.assertEqual(next_tag.value, 2)
+
+    def test_next_tag__with_use_tags(self):
+        """Test get next availabe tags returns different tags"""
+        link = Link(self.iface1, self.iface2)
+        tag = link.get_next_available_tag()
+        self.assertEqual(tag.value, 1)
+
+        is_available = link.is_tag_available(tag)
+        self.assertFalse(is_available)
+        link.use_tag(tag)
+
+    def test_tag_life_cicle(self):
+        """Test get next available tags returns different tags"""
+        link = Link(self.iface1, self.iface2)
+        tag = link.get_next_available_tag()
+        self.assertEqual(tag.value, 1)
+
+        is_available = link.is_tag_available(tag)
+        self.assertFalse(is_available)
+
+        link.make_tag_available(tag)
+        is_available = link.is_tag_available(tag)
+        self.assertTrue(is_available)
+
+    def test_concurrent_get_next_tag(self):
+        """Test get next available tags in concurrent execution"""
+        from tests.helper import test_concurrently
+        _link = Link(self.iface1, self.iface2)
+
+        _i = []
+
+        @test_concurrently(20)
+        def test_get_next_available_tag():
+            """Test get next availabe tags returns different tags"""
+            _i.append(1)
+            _i_len = len(_i)
+            tag = _link.get_next_available_tag()
+            time.sleep(0.0001)
+            _link.use_tag(tag)
+
+            next_tag = _link.get_next_available_tag()
+            _link.use_tag(next_tag)
+
+            self.assertNotEqual(tag, next_tag)
+
+            # Check if in the 20 iteration the tag value is 40
+            # It happens because we get 2 tags for every iteration
+            if _i_len == 20:
+                self.assertEqual(next_tag.value, 40)
+
+        test_get_next_available_tag()