From da7698de9da05b43b79aa1ac6a68eaedb1b949cb Mon Sep 17 00:00:00 2001 From: Rogerio Motitsuki Date: Wed, 20 Mar 2019 15:57:34 -0300 Subject: [PATCH 1/2] Fix issue #883. Fix the method get_next_available_tag to get and use the tag in single step. Deprecated the method use_tag, but maintained the behavior of the workflow using get tag / use tag. Fix linter messages. --- kytos/core/interface.py | 22 ++++++---- kytos/core/link.py | 28 +++++++++++-- tests/helper.py | 48 +++++++++++++++++++++ tests/test_core/test_interface.py | 47 ++++++++++++++++++++- tests/test_core/test_link.py | 69 +++++++++++++++++++++++++++++++ 5 files changed, 202 insertions(+), 12 deletions(-) diff --git a/kytos/core/interface.py b/kytos/core/interface.py index 63fd0c1ba..411260405 100644 --- a/kytos/core/interface.py +++ b/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,11 +137,13 @@ 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): @@ -150,7 +151,12 @@ def is_tag_available(self, tag): 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: diff --git a/kytos/core/link.py b/kytos/core/link.py index 331f7a88d..3c2206ab2 100644 --- a/kytos/core/link.py +++ b/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: + # 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: + if tag in available_tags_b: + is_success = self.endpoint_a.use_tag(tag) + if not is_success: + # Tag already in use. Try another tag. + continue + + is_success = self.endpoint_b.use_tag(tag) + if not is_success: + # Tag already in use. + # Return endpoint_a tag and try another 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): diff --git a/tests/helper.py b/tests/helper.py index 984e1e00f..c9f52c1a8 100644 --- a/tests/helper.py +++ b/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,50 @@ 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: + 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 diff --git a/tests/test_core/test_interface.py b/tests/test_core/test_interface.py index 287ed6d54..4047a3658 100644 --- a/tests/test_core/test_interface.py +++ b/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) diff --git a/tests/test_core/test_link.py b/tests/test_core/test_link.py index cccff061a..a68928bb9 100644 --- a/tests/test_core/test_link.py +++ b/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() From d6588801fd007f97bb912747f234f4a0496551a5 Mon Sep 17 00:00:00 2001 From: Rogerio Motitsuki Date: Thu, 11 Apr 2019 16:08:42 -0300 Subject: [PATCH 2/2] Fix comments made to PR #891 --- kytos/core/link.py | 30 +++++++++++++++--------------- tests/helper.py | 3 ++- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/kytos/core/link.py b/kytos/core/link.py index 3c2206ab2..119f056e9 100644 --- a/kytos/core/link.py +++ b/kytos/core/link.py @@ -115,21 +115,21 @@ def get_next_available_tag(self): available_tags_b = self.endpoint_b.available_tags.copy() for tag in available_tags_a: - if tag in available_tags_b: - is_success = self.endpoint_a.use_tag(tag) - if not is_success: - # Tag already in use. Try another tag. - continue - - is_success = self.endpoint_b.use_tag(tag) - if not is_success: - # Tag already in use. - # Return endpoint_a tag and try another tag. - self.endpoint_a.make_tag_available(tag) - continue - - # Tag used successfully by both endpoints. Returning. - return tag + # 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 diff --git a/tests/helper.py b/tests/helper.py index c9f52c1a8..88ad7f604 100644 --- a/tests/helper.py +++ b/tests/helper.py @@ -155,7 +155,8 @@ def call_test_func(): """Call the test method.""" try: test_func(*args, **kwargs) - except Exception: + except Exception as _e: + exceptions.append(_e) raise threads = [] for _ in range(times):