fix: move ownership of Variables from Application to Profile

Currently, there is one global Variables object, as member of the global
Application. When loading profiles, variables are added to it, but never
removed. So when switching profiles, there are still variables present
from the previous profile.
With this commit, the Variables object is now owned by the Profile.
It is created by the profile loader, so that more complex laod scenarios
(multiple profiles, potentially with includes) still only use one Variables
object.

Signed-off-by: Adriaan Schmidt <adriaan.schmidt@siemens.com>

Author: adriaan42

tests/unit/plugins/test_base.py

@@ -30,12 +30,11 @@ class PluginBaseTestCase(unittest.TestCase):
 	def setUp(self):
 		self._plugin = DummyPlugin(monitors_repository,storage_factory,\
 			hardware_inventory,device_matcher,device_matcher_udev,\
-			plugin_instance_factory,None,None)
+			plugin_instance_factory,None)
 
 		self._commands_plugin = CommandsPlugin(monitors_repository,\
 			storage_factory,hardware_inventory,device_matcher,\
-			device_matcher_udev,plugin_instance_factory,None,\
-			profiles.variables.Variables())
+			device_matcher_udev,plugin_instance_factory,None)
 
 	def test_get_effective_options(self):
 		self.assertEqual(self._plugin._get_effective_options(\
@@ -51,13 +50,13 @@ def test_option_bool(self):
 	def test_create_instance(self):
 		instance = self._plugin.create_instance(\
 			'first_instance',0,'test','test','test','test',\
-			{'default_option1':'default_value2'})
+			{'default_option1':'default_value2'},None)
 		self.assertIsNotNone(instance)
 
 	def test_destroy_instance(self):
 		instance = self._plugin.create_instance(\
 			'first_instance',0,'test','test','test','test',\
-			{'default_option1':'default_value2'})
+			{'default_option1':'default_value2'},None)
 		instance.plugin.init_devices()
 
 		self._plugin.destroy_instance(instance)
@@ -67,7 +66,7 @@ def test_get_matching_devices(self):
 		""" without udev regex """
 		instance = self._plugin.create_instance(\
 			'first_instance',0,'right_device*',None,'test','test',\
-			{'default_option1':'default_value2'})
+			{'default_option1':'default_value2'},None)
 
 		self.assertEqual(self._plugin._get_matching_devices(\
 			instance,['bad_device','right_device1','right_device2']),\
@@ -76,7 +75,7 @@ def test_get_matching_devices(self):
 		""" with udev regex """
 		instance = self._plugin.create_instance(\
 			'second_instance',0,'right_device*','device[1-2]','test','test',\
-			{'default_option1':'default_value2'})
+			{'default_option1':'default_value2'},None)
 
 		device1 = DummyDevice('device1',{'name':'device1'})
 		device2 = DummyDevice('device2',{'name':'device2'})
@@ -105,15 +104,15 @@ def test_check_commands(self):
 
 	def test_execute_all_non_device_commands(self):
 		instance = self._commands_plugin.create_instance('test_instance',0,'',\
-			'','','',{'size':'XXL'})
+			'','','',{'size':'XXL'},profiles.variables.Variables())
 
 		self._commands_plugin._execute_all_non_device_commands(instance)
 
 		self.assertEqual(self._commands_plugin._size,'XXL')
 
 	def test_execute_all_device_commands(self):
 		instance = self._commands_plugin.create_instance('test_instance',0,'',\
-			'','','',{'device_setting':'010'})
+			'','','',{'device_setting':'010'},profiles.variables.Variables())
 
 		device1 = DummyDevice('device1',{})
 		device2 = DummyDevice('device2',{})
@@ -134,7 +133,7 @@ def test_process_assignment_modifiers(self):
 
 	def test_get_current_value(self):
 		instance = self._commands_plugin.create_instance('test_instance',0,'',\
-			'','','',{})
+			'','','',{},None)
 
 		command = [com for com in self._commands_plugin._commands.values()\
 			if com['name'] == 'size'][0]

tests/unit/profiles/test_loader.py

@@ -53,8 +53,7 @@ def setUp(self):
 		locator = profiles.Locator([self._profiles_dir])
 		factory = profiles.Factory()
 		merger = profiles.Merger()
-		self._loader = profiles.Loader(locator,factory,merger,None,\
-			profiles.variables.Variables())
+		self._loader = profiles.Loader(locator,factory,merger,None)
 
 	def test_safe_name(self):
 		self.assertFalse(self._loader.safe_name('*'))

tests/unit/profiles/test_merger.py

@@ -1,21 +1,23 @@
 import unittest
 from tuned.profiles.merger import Merger
 from tuned.profiles.profile import Profile
+from tuned.profiles.variables import Variables
 from collections import OrderedDict
 
 class MergerTestCase(unittest.TestCase):
 	def test_merge_without_replace(self):
 		merger = Merger()
+		variables = Variables()
 		config1 = OrderedDict([
 			("main", {"test_option" : "test_value1"}),
 			("net", { "devices": "em0", "custom": "custom_value"}),
 		])
-		profile1 = Profile('test_profile1',config1)
+		profile1 = Profile('test_profile1',config1,variables)
 		config2 = OrderedDict([
 			('main', {'test_option' : 'test_value2'}),
 			('net', { 'devices': 'em1' }),
 		])
-		profile2 = Profile("test_profile2",config2)
+		profile2 = Profile("test_profile2",config2,variables)
 
 		merged_profile = merger.merge([profile1, profile2])
 
@@ -27,16 +29,17 @@ def test_merge_without_replace(self):
 
 	def test_merge_with_replace(self):
 		merger = Merger()
+		variables = Variables()
 		config1 = OrderedDict([
 			("main", {"test_option" : "test_value1"}),
 			("net", { "devices": "em0", "custom": "option"}),
 		])
-		profile1 = Profile('test_profile1',config1)
+		profile1 = Profile('test_profile1',config1,variables)
 		config2 = OrderedDict([
 			("main", {"test_option" : "test_value2"}),
 			("net", { "devices": "em1", "replace": True }),
 		])
-		profile2 = Profile('test_profile2',config2)
+		profile2 = Profile('test_profile2',config2,variables)
 		merged_profile = merger.merge([profile1, profile2])
 
 		self.assertEqual(merged_profile.options["test_option"],"test_value2")
@@ -46,15 +49,16 @@ def test_merge_with_replace(self):
 
 	def test_merge_multiple_order(self):
 		merger = Merger()
+		variables = Variables()
 		config1 = OrderedDict([ ("main", {"test_option" : "test_value1"}),\
 			("net", { "devices": "em0" }) ])
-		profile1 = Profile('test_profile1',config1)
+		profile1 = Profile('test_profile1',config1,variables)
 		config2 = OrderedDict([	("main", {"test_option" : "test_value2"}),\
 			("net", { "devices": "em1" }) ])
-		profile2 = Profile('test_profile2',config2)
+		profile2 = Profile('test_profile2',config2,variables)
 		config3 = OrderedDict([	("main", {"test_option" : "test_value3"}),\
 			("net", { "devices": "em2" }) ])
-		profile3 = Profile('test_profile3',config3)
+		profile3 = Profile('test_profile3',config3,variables)
 		merged_profile = merger.merge([profile1, profile2, profile3])
 
 		self.assertEqual(merged_profile.options["test_option"],"test_value3")

tests/unit/profiles/test_profile.py

@@ -9,33 +9,33 @@ def _create_unit(self, name, config):
 class ProfileTestCase(unittest.TestCase):
 
 	def test_init(self):
-		MockProfile("test", {})
+		MockProfile("test", {}, None)
 
 	def test_create_units(self):
 		profile = MockProfile("test", {
 			"main": { "anything": 10 },
 			"network" : { "type": "net", "devices": "*" },
 			"storage" : { "type": "disk" },
-		})
+		}, None)
 
 		self.assertIs(type(profile.units), collections.OrderedDict)
 		self.assertEqual(len(profile.units), 2)
 		self.assertListEqual(sorted([name_config for name_config in profile.units]), sorted(["network", "storage"]))
 
 	def test_create_units_empty(self):
-		profile = MockProfile("test", {"main":{}})
+		profile = MockProfile("test", {"main":{}}, None)
 
 		self.assertIs(type(profile.units), collections.OrderedDict)
 		self.assertEqual(len(profile.units), 0)
 
 	def test_sets_name(self):
-		profile1 = MockProfile("test_one", {})
-		profile2 = MockProfile("test_two", {})
+		profile1 = MockProfile("test_one", {}, None)
+		profile2 = MockProfile("test_two", {}, None)
 		self.assertEqual(profile1.name, "test_one")
 		self.assertEqual(profile2.name, "test_two")
 
 	def test_change_name(self):
-		profile = MockProfile("oldname", {})
+		profile = MockProfile("oldname", {}, None)
 		self.assertEqual(profile.name, "oldname")
 		profile.name = "newname"
 		self.assertEqual(profile.name, "newname")
@@ -44,15 +44,15 @@ def test_sets_options(self):
 		profile = MockProfile("test", {
 			"main": { "anything": 10 },
 			"network" : { "type": "net", "devices": "*" },
-		})
+		}, None)
 
 		self.assertIs(type(profile.options), dict)
 		self.assertEqual(profile.options["anything"], 10)
 
 	def test_sets_options_empty(self):
 		profile = MockProfile("test", {
 			"storage" : { "type": "disk" },
-		})
+		}, None)
 
 		self.assertIs(type(profile.options), dict)
 		self.assertEqual(len(profile.options), 0)

tuned/daemon/application.py

@@ -39,10 +39,9 @@ def __init__(self, profile_name = None, config = None):
 		device_matcher = hardware.DeviceMatcher()
 		device_matcher_udev = hardware.DeviceMatcherUdev()
 		plugin_instance_factory = plugins.instance.Factory()
-		self.variables = profiles.variables.Variables()
 
 		plugins_repository = plugins.Repository(monitors_repository, storage_factory, hardware_inventory,\
-			device_matcher, device_matcher_udev, plugin_instance_factory, self.config, self.variables)
+			device_matcher, device_matcher_udev, plugin_instance_factory, self.config)
 		def_instance_priority = int(self.config.get(consts.CFG_DEFAULT_INSTANCE_PRIORITY, consts.CFG_DEF_DEFAULT_INSTANCE_PRIORITY))
 		unit_manager = units.Manager(
 				plugins_repository, monitors_repository,
@@ -51,7 +50,7 @@ def __init__(self, profile_name = None, config = None):
 		profile_factory = profiles.Factory()
 		profile_merger = profiles.Merger()
 		profile_locator = profiles.Locator(self.config.get_list(consts.CFG_PROFILE_DIRS, consts.CFG_DEF_PROFILE_DIRS))
-		profile_loader = profiles.Loader(profile_locator, profile_factory, profile_merger, self.config, self.variables)
+		profile_loader = profiles.Loader(profile_locator, profile_factory, profile_merger, self.config)
 
 		self._daemon = daemon.Daemon(unit_manager, profile_loader, profile_name, self.config, self)
 		self._controller = controller.Controller(self._daemon, self.config)

tuned/daemon/daemon.py

@@ -198,7 +198,7 @@ def _thread_code(self):
 		if self._profile is None:
 			raise TunedException("Cannot start the daemon without setting a profile.")
 
-		self._unit_manager.create(self._profile.units)
+		self._unit_manager.create(self._profile.units, self._profile.variables)
 		self._save_active_profile(" ".join(self._active_profiles),
 					  self._manual)
 		self._save_post_loaded_profile(self._post_loaded_profile)

tuned/plugins/base.py

@@ -19,7 +19,7 @@ class Plugin(object):
 	Intentionally a lot of logic is included in the plugin to increase plugin flexibility.
 	"""
 
-	def __init__(self, monitors_repository, storage_factory, hardware_inventory, device_matcher, device_matcher_udev, instance_factory, global_cfg, variables):
+	def __init__(self, monitors_repository, storage_factory, hardware_inventory, device_matcher, device_matcher_udev, instance_factory, global_cfg):
 		"""Plugin constructor."""
 
 		self._storage = storage_factory.create(self.__class__.__name__)
@@ -33,7 +33,6 @@ def __init__(self, monitors_repository, storage_factory, hardware_inventory, dev
 		self._init_commands()
 
 		self._global_cfg = global_cfg
-		self._variables = variables
 		self._has_dynamic_options = False
 		self._devices_inited = False
 
@@ -93,14 +92,14 @@ def _option_bool(self, value):
 	# Interface for manipulation with instances of the plugin.
 	#
 
-	def create_instance(self, name, priority, devices_expression, devices_udev_regex, script_pre, script_post, options):
+	def create_instance(self, name, priority, devices_expression, devices_udev_regex, script_pre, script_post, options, variables):
 		"""Create new instance of the plugin and seize the devices."""
 		if name in self._instances:
 			raise Exception("Plugin instance with name '%s' already exists." % name)
 
 		effective_options = self._get_effective_options(options)
 		instance = self._instance_factory.create(self, name, priority, devices_expression, devices_udev_regex, \
-			script_pre, script_post, effective_options)
+			script_pre, script_post, effective_options, variables)
 		self._instances[name] = instance
 		self._instances = collections.OrderedDict(sorted(self._instances.items(), key=lambda x: x[1].priority))
 
@@ -249,8 +248,8 @@ def _call_device_script(self, instance, script, op, devices, rollback = consts.R
 		dir_name = os.path.dirname(script)
 		ret = True
 		for dev in devices:
-			environ = os.environ
-			environ.update(self._variables.get_env())
+			environ = os.environ.copy()
+			environ.update(instance._variables.get_env())
 			arguments = [op]
 			if rollback == consts.ROLLBACK_FULL:
 				arguments.append("full_rollback")
@@ -459,13 +458,13 @@ def _storage_unset(self, instance, command, device_name=None):
 
 	def _execute_all_non_device_commands(self, instance):
 		for command in [command for command in list(self._commands.values()) if not command["per_device"]]:
-			new_value = self._variables.expand(instance.options.get(command["name"], None))
+			new_value = instance._variables.expand(instance.options.get(command["name"], None))
 			if new_value is not None:
 				self._execute_non_device_command(instance, command, new_value)
 
 	def _execute_all_device_commands(self, instance, devices):
 		for command in [command for command in list(self._commands.values()) if command["per_device"]]:
-			new_value = self._variables.expand(instance.options.get(command["name"], None))
+			new_value = instance._variables.expand(instance.options.get(command["name"], None))
 			if new_value is None:
 				continue
 			for device in devices:
@@ -474,7 +473,7 @@ def _execute_all_device_commands(self, instance, devices):
 	def _verify_all_non_device_commands(self, instance, ignore_missing):
 		ret = True
 		for command in [command for command in list(self._commands.values()) if not command["per_device"]]:
-			new_value = self._variables.expand(instance.options.get(command["name"], None))
+			new_value = instance._variables.expand(instance.options.get(command["name"], None))
 			if new_value is not None:
 				if self._verify_non_device_command(instance, command, new_value, ignore_missing) == False:
 					ret = False
@@ -483,7 +482,7 @@ def _verify_all_non_device_commands(self, instance, ignore_missing):
 	def _verify_all_device_commands(self, instance, devices, ignore_missing):
 		ret = True
 		for command in [command for command in list(self._commands.values()) if command["per_device"]]:
-			new_value = self._variables.expand(instance.options.get(command["name"], None))
+			new_value = instance._variables.expand(instance.options.get(command["name"], None))
 			if new_value is None:
 				continue
 			for device in devices:

tuned/plugins/instance/instance.py

@@ -4,14 +4,15 @@ class Instance(object):
 	"""
 	"""
 
-	def __init__(self, plugin, name, priority, devices_expression, devices_udev_regex, script_pre, script_post, options):
+	def __init__(self, plugin, name, priority, devices_expression, devices_udev_regex, script_pre, script_post, options, variables):
 		self._plugin = plugin
 		self._name = name
 		self._devices_expression = devices_expression
 		self._devices_udev_regex = devices_udev_regex
 		self._script_pre = script_pre
 		self._script_post = script_post
 		self._options = options
+		self._variables = variables
 
 		self._active = True
 		self._priority = priority
@@ -71,6 +72,10 @@ def script_post(self):
 	def options(self):
 		return self._options
 
+	@property
+	def variables(self):
+		return self._variables
+
 	@property
 	def has_static_tuning(self):
 		return self._has_static_tuning

tuned/plugins/plugin_sysfs.py

@@ -51,7 +51,7 @@ def _instance_cleanup(self, instance):
 
 	def _instance_apply_static(self, instance):
 		for key, value in list(instance._sysfs.items()):
-			v = self._variables.expand(value)
+			v = instance.variables.expand(value)
 			for f in glob.iglob(key):
 				if self._check_sysfs(f):
 					instance._sysfs_original[f] = self._read_sysfs(f)
@@ -62,7 +62,7 @@ def _instance_apply_static(self, instance):
 	def _instance_verify_static(self, instance, ignore_missing, devices):
 		ret = True
 		for key, value in list(instance._sysfs.items()):
-			v = self._variables.expand(value)
+			v = instance.variables.expand(value)
 			for f in glob.iglob(key):
 				if self._check_sysfs(f):
 					curr_val = self._read_sysfs(f)

tuned/plugins/repository.py

@@ -8,7 +8,7 @@
 
 class Repository(ClassLoader):
 
-	def __init__(self, monitor_repository, storage_factory, hardware_inventory, device_matcher, device_matcher_udev, plugin_instance_factory, global_cfg, variables):
+	def __init__(self, monitor_repository, storage_factory, hardware_inventory, device_matcher, device_matcher_udev, plugin_instance_factory, global_cfg):
 		super(Repository, self).__init__()
 		self._plugins = set()
 		self._monitor_repository = monitor_repository
@@ -18,7 +18,6 @@ def __init__(self, monitor_repository, storage_factory, hardware_inventory, devi
 		self._device_matcher_udev = device_matcher_udev
 		self._plugin_instance_factory = plugin_instance_factory
 		self._global_cfg = global_cfg
-		self._variables = variables
 
 	@property
 	def plugins(self):
@@ -33,7 +32,7 @@ def create(self, plugin_name):
 		log.debug("creating plugin %s" % plugin_name)
 		plugin_cls = self.load_class(plugin_name)
 		plugin_instance = plugin_cls(self._monitor_repository, self._storage_factory, self._hardware_inventory, self._device_matcher,\
-			self._device_matcher_udev, self._plugin_instance_factory, self._global_cfg, self._variables)
+			self._device_matcher_udev, self._plugin_instance_factory, self._global_cfg)
 		self._plugins.add(plugin_instance)
 		return plugin_instance