-
Notifications
You must be signed in to change notification settings - Fork 237
Persistence of dynamic tuning changes #758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -164,23 +164,35 @@ def _get_matching_devices(self, instance, devices): | |||||||||||||
| udev_devices = self._device_matcher_udev.match_list(instance.devices_udev_regex, udev_devices) | ||||||||||||||
| return set([x.sys_name for x in udev_devices]) | ||||||||||||||
|
|
||||||||||||||
| def restore_devices(self, instance, devices): | ||||||||||||||
| if not self._devices_supported: | ||||||||||||||
| return | ||||||||||||||
|
|
||||||||||||||
| log.debug("Restoring devices of instance %s: %s" % (instance.name, " ".join(devices))) | ||||||||||||||
| for device in devices: | ||||||||||||||
| if device not in self._free_devices: | ||||||||||||||
| continue | ||||||||||||||
| self._free_devices.remove(device) | ||||||||||||||
| instance.assigned_devices.add(device) | ||||||||||||||
| self._assigned_devices.add(device) | ||||||||||||||
|
|
||||||||||||||
| def assign_free_devices(self, instance): | ||||||||||||||
| if not self._devices_supported: | ||||||||||||||
| return | ||||||||||||||
|
|
||||||||||||||
| log.debug("assigning devices to instance %s" % instance.name) | ||||||||||||||
| to_assign = self._get_matching_devices(instance, self._free_devices) | ||||||||||||||
| instance.active = len(to_assign) > 0 | ||||||||||||||
| if not instance.active: | ||||||||||||||
| log.warning("instance %s: no matching devices available" % instance.name) | ||||||||||||||
| else: | ||||||||||||||
| if len(to_assign) > 0: | ||||||||||||||
| name = instance.name | ||||||||||||||
| if instance.name != self.name: | ||||||||||||||
| name += " (%s)" % self.name | ||||||||||||||
| log.info("instance %s: assigning devices %s" % (name, ", ".join(to_assign))) | ||||||||||||||
| instance.assigned_devices.update(to_assign) # cannot use |= | ||||||||||||||
| self._assigned_devices |= to_assign | ||||||||||||||
| self._free_devices -= to_assign | ||||||||||||||
| instance.active = len(instance.assigned_devices) > 0 | ||||||||||||||
| if not instance.active: | ||||||||||||||
| log.warning("instance %s: no matching devices available" % instance.name) | ||||||||||||||
|
Comment on lines
+193
to
+195
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Include processed devices when determining instance activity. The active state at line 193 is based only on 🐛 Proposed fix- instance.active = len(instance.assigned_devices) > 0
+ instance.active = len(instance.assigned_devices) + len(instance.processed_devices) > 0
if not instance.active:📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| def release_devices(self, instance): | ||||||||||||||
| if not self._devices_supported: | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,9 +24,6 @@ def __init__(self, profile_locator, profile_factory, profile_merger, global_conf | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._global_config = global_config | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._variables = variables | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _create_profile(self, profile_name, config): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return tuned.profiles.profile.Profile(profile_name, config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def safe_name(cls, profile_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return re.match(r'^[a-zA-Z0-9_.-]+$', profile_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -57,6 +54,7 @@ def load(self, profile_names): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # FIXME hack, do all variable expansions in one place | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._expand_vars_in_devices(final_profile) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._expand_vars_in_regexes(final_profile) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| final_profile.calculate_hash() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return final_profile | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _expand_vars_in_devices(self, profile): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -68,6 +66,47 @@ def _expand_vars_in_regexes(self, profile): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| profile.units[unit].cpuinfo_regex = self._variables.expand(profile.units[unit].cpuinfo_regex) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| profile.units[unit].uname_regex = self._variables.expand(profile.units[unit].uname_regex) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def create_snapshot(self, profile, instances): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot = profile.snapshot(instances) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.debug("Storing profile snapshot in %s:\n%s" % (consts.PROFILE_SNAPSHOT_FILE, snapshot)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open(consts.PROFILE_SNAPSHOT_FILE, "w") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f.write(snapshot) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+69
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add error handling for file write operation. The 🛡️ Proposed fix to add error handling def create_snapshot(self, profile, instances):
snapshot = profile.snapshot(instances)
log.debug("Storing profile snapshot in %s:\n%s" % (consts.PROFILE_SNAPSHOT_FILE, snapshot))
- with open(consts.PROFILE_SNAPSHOT_FILE, "w") as f:
- f.write(snapshot)
+ try:
+ with open(consts.PROFILE_SNAPSHOT_FILE, "w") as f:
+ f.write(snapshot)
+ except (OSError, IOError) as e:
+ log.error("Failed to write profile snapshot: %s" % e)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def restore_snapshot(self, profile): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if profile is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # When tuning is stopped, we are called with profile==None -> skip | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if os.path.isfile(consts.PROFILE_SNAPSHOT_FILE): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.debug("Found profile snapshot '%s'" % consts.PROFILE_SNAPSHOT_FILE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config = self._load_config_data(consts.PROFILE_SNAPSHOT_FILE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot_hash = config.get("main", {}).get("profile_base_hash", None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if snapshot_hash == profile._base_hash: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot = self._profile_factory.create("restore", config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot.name = profile.name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # the snapshot is created directly (not via the merger), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # so extract its [variables] section manually | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if consts.PLUGIN_VARIABLES_UNIT_NAME in snapshot.units: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot.variables.update(snapshot.units[consts.PLUGIN_VARIABLES_UNIT_NAME].options) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| del snapshot.units[consts.PLUGIN_VARIABLES_UNIT_NAME] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._variables.add_from_cfg(snapshot.variables) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._expand_vars_in_devices(snapshot) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._expand_vars_in_regexes(snapshot) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.info("Restored profile snapshot: %s" % snapshot.name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.debug("Snapshot hash '%s' does not match current base hash '%s'. Not restoring." % (snapshot_hash, profile._base_hash)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| os.remove(consts.PROFILE_SNAPSHOT_FILE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+85
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate hash values before comparison. At line 85, From the code flow, 🛡️ Proposed fix to add validation try:
config = self._load_config_data(consts.PROFILE_SNAPSHOT_FILE)
snapshot_hash = config.get("main", {}).get("profile_base_hash", None)
- if snapshot_hash == profile._base_hash:
+ if snapshot_hash is not None and profile._base_hash is not None and snapshot_hash == profile._base_hash:
snapshot = self._profile_factory.create("restore", config)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except InvalidProfileException as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.error("Could not process profile snapshot: %s" % e) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return snapshot | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def remove_snapshot(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| os.remove(consts.PROFILE_SNAPSHOT_FILE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except FileNotFoundError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _load_profile(self, profile_names, profiles, processed_files): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for name in profile_names: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| filename = self._profile_locator.get_config(name, processed_files) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,24 +1,27 @@ | ||||||||||||
| import tuned.profiles.unit | ||||||||||||
| import tuned.consts as consts | ||||||||||||
| import collections | ||||||||||||
| import hashlib | ||||||||||||
| import json | ||||||||||||
|
|
||||||||||||
| class Profile(object): | ||||||||||||
| """ | ||||||||||||
| Representation of a tuning profile. | ||||||||||||
| """ | ||||||||||||
|
|
||||||||||||
| __slots__ = ["_name", "_options", "_variables", "_units"] | ||||||||||||
| __slots__ = ["_name", "_options", "_variables", "_units", "_base_hash"] | ||||||||||||
|
|
||||||||||||
| def __init__(self, name=None, config={}): | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace mutable default argument with The default mutable argument As per coding guidelines, mutable default arguments should be replaced with 🐛 Proposed fix- def __init__(self, name=None, config={}):
+ def __init__(self, name=None, config=None):
+ if config is None:
+ config = {}
self._name = name📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.15.15)[warning] 14-14: Do not use mutable data structures for argument defaults Replace with (B006) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||||||||||||
| self._name = name | ||||||||||||
| self._variables = collections.OrderedDict() | ||||||||||||
| self._init_options(config) | ||||||||||||
| self._init_units(config) | ||||||||||||
| self._base_hash = config.get("main", {}).get("profile_base_hash", None) | ||||||||||||
|
|
||||||||||||
| def _init_options(self, config): | ||||||||||||
| self._options = {} | ||||||||||||
| if consts.PLUGIN_MAIN_UNIT_NAME in config: | ||||||||||||
| self._options = dict(config[consts.PLUGIN_MAIN_UNIT_NAME]) | ||||||||||||
| self._options = collections.OrderedDict(config[consts.PLUGIN_MAIN_UNIT_NAME]) | ||||||||||||
|
|
||||||||||||
| def _init_units(self, config): | ||||||||||||
| self._units = collections.OrderedDict() | ||||||||||||
|
|
@@ -30,6 +33,35 @@ def _init_units(self, config): | |||||||||||
| def _create_unit(self, name, config): | ||||||||||||
| return tuned.profiles.unit.Unit(name, config) | ||||||||||||
|
|
||||||||||||
| def as_ordered_dict(self): | ||||||||||||
| """generate serializable (with json.dumps()) representation for hashing""" | ||||||||||||
| profile_dict = collections.OrderedDict() | ||||||||||||
| profile_dict["main"] = self.options | ||||||||||||
| profile_dict["variables"] = self._variables | ||||||||||||
| for name, unit in self._units.items(): | ||||||||||||
| profile_dict[name] = unit.as_ordered_dict() | ||||||||||||
| return profile_dict | ||||||||||||
|
|
||||||||||||
| def calculate_hash(self): | ||||||||||||
| serialized = json.dumps(self.as_ordered_dict()) | ||||||||||||
| self._base_hash = hashlib.md5(serialized.encode(), usedforsecurity=False).hexdigest() | ||||||||||||
|
|
||||||||||||
| def snapshot(self, instances): | ||||||||||||
| """generate config representation that will re-create the data when read as a profile""" | ||||||||||||
| snapshot = "[main]\n" | ||||||||||||
| snapshot += "active_profile=%s\n" % self.name | ||||||||||||
| snapshot += "profile_base_hash=%s\n" % self._base_hash | ||||||||||||
| snapshot += "\n[variables]\n" | ||||||||||||
| for key, value in self._variables.items(): | ||||||||||||
| snapshot += "%s=%s\n" % (key, value) | ||||||||||||
| for unit in self.units.values(): | ||||||||||||
| snapshot += "\n" + unit.snapshot() | ||||||||||||
| for instance in instances: | ||||||||||||
| if instance.name == unit.name: | ||||||||||||
| snapshot += "__devices__=%s\n" % " ".join(instance.assigned_devices | instance.processed_devices) | ||||||||||||
| break | ||||||||||||
| return snapshot | ||||||||||||
|
|
||||||||||||
| @property | ||||||||||||
| def name(self): | ||||||||||||
| """ | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log or handle devices that cannot be restored.
The method silently skips devices not in
_free_devices(line 173-174). If a device is missing from the free pool (e.g., already assigned to another instance or removed from the system), the restore operation partially fails without notification. This could leave instances in an inconsistent state compared to the snapshot.📋 Proposed fix to log missing devices
def restore_devices(self, instance, devices): if not self._devices_supported: return log.debug("Restoring devices of instance %s: %s" % (instance.name, " ".join(devices))) for device in devices: if device not in self._free_devices: + log.warning("Cannot restore device '%s' to instance '%s': device not in free pool" % (device, instance.name)) continue self._free_devices.remove(device)📝 Committable suggestion
🤖 Prompt for AI Agents