From a23b259bb0d2963998f0183e15dc7c754e95a978 Mon Sep 17 00:00:00 2001 From: Fred Sharp Date: Thu, 9 Jul 2020 14:14:18 -0500 Subject: [PATCH 1/3] Ready for testing --- adjust | 24 +++++++-- adjust.py | 152 ------------------------------------------------------ util.py | 2 + 3 files changed, 23 insertions(+), 155 deletions(-) delete mode 100644 adjust.py diff --git a/adjust b/adjust index 77b3bf5..09f7ce2 100755 --- a/adjust +++ b/adjust @@ -6,6 +6,8 @@ import signal import subprocess +from adjust import Adjust, AdjustError + import json # 'compact' format json encode (no spaces) json_enc = json.JSONEncoder(separators=(",",":")).encode @@ -66,6 +68,17 @@ def merge_settings(dest, src): return +def merge_monitoring(dest, src): + """merge src['settings'] into dest['settings'], raise exception if the + same monitoring key is defined in both""" + + for mn,mv in src.get("monitoring",{}).items(): + if mn in dest: + raise ValueError(mn) + dest[mn] = mv + + return + def _query(app_id='app'): comps = {} @@ -76,8 +89,13 @@ def _query(app_id='app'): g_state[d] = q # merge after all data is collected + descriptor = {"application" : { "components" : {}}, "monitoring": {}} + comps = descriptor['application']['components'] + monitoring = descriptor['monitoring'] + for d,info in drivers: # TODO: s[application] (global app settings not supported for now) + merge_monitoring(monitoring, g_state[d]) for cn, cd in g_state[d]["application"]["components"].items(): c = comps.setdefault(cn, {}) try: @@ -87,7 +105,7 @@ def _query(app_id='app'): # NOTE we don't keep track where the first one came from, just report the current driver as the culprit raise ValueError("{}: duplicate setting for component {}: {}".format(os.path.basename(d), cn, str(x))) - return {"application" : { "components" : comps }} + return descriptor def _adjust(app_id, new_settings): @@ -150,9 +168,9 @@ def _adjust(app_id, new_settings): for d,s in p_status.items(): if s["status"] == "ok": continue msg.append("{}: {}: {}".format(os.path.basename(d), s["status"], s.get("message",""))) - raise Exception( "\n".join(msg) ) + raise AdjustError( "\n".join(msg), status=s["status"], reason=s["reason"] ) -class A(adjust.Adjust): +class A(Adjust): def query(self): return _query() diff --git a/adjust.py b/adjust.py deleted file mode 100644 index 9303d66..0000000 --- a/adjust.py +++ /dev/null @@ -1,152 +0,0 @@ -from __future__ import print_function # py2 compatibility - -import json -import argparse -import sys - - -class Adjust(object): - ''' - Base class for Optune adjust driver command. This implements common functionality - and is meant to be sub-classed, not run as is. - - Example usage: - from adjust import Adjust - class MyClass(Adjust): - def info(self): - ... - def query(self): - ... - def handle_cancel(self, signal, frame): - ... - def adjust(self): - ... - if __name__ == '__main__': - foo = MyClass(VERSION, DESC, HAS_CANCEL) - foo.run() - - ''' - ################################################## - # METHODS THAT SHOULD NOT BE OVERWRITTEN - # (unless you know what you are doing) - ################################################## - - def __init__(self, version, cli_desc, supports_cancel): - - # Parse Args - self.parser = argparse.ArgumentParser(description=cli_desc) - - self.parser.add_argument( - '--version', help='print version and exit', default=False, action='store_true') - self.parser.add_argument( - '--info', help='output driver info and exit', default=False, action='store_true') - qry_help = 'output current state of settings for this application' - self.parser.add_argument( - '--query', help=qry_help, default=False, action='store_true') - # alias for query - self.parser.add_argument( - '--describe', dest='query', help=qry_help, default=False, action='store_true') - self.parser.add_argument( - 'app_id', help='Name/ID of the application to adjust', nargs='?') - self.args = self.parser.parse_args() - - self.version = version - self.app_id = self.args.app_id - self.supports_cancel = supports_cancel - - def run(self): - if self.args.version: - print(self.version) - sys.exit(0) - - if self.args.info: - print(json.dumps( - {"version": self.version, "has_cancel": self.supports_cancel})) - sys.exit(0) - - # Valcheck - if self.args.app_id is None: - self.parser.error( - 'Missing required param app_id') - - # Handle --query - if self.args.query: - try: - query = self.query() - if "application" not in query: - query = { "application" : query } # legacy compat. - print(json.dumps(query)) - sys.exit(0) - except Exception as e: - self.print_json_error( - e.__class__.__name__, - "failure", - str(e) - ) - raise - - # Parse input - try: - # self.debug("Reading stdin") - input_data = json.loads(sys.stdin.read()) - self.input_data = input_data # LEGACY mode, remove when drivers are updated to use arg - except Exception as e: - self.print_json_error( - e.__class__.__name__, - "failed to parse input", - str(e) - ) - raise - - # Adjust // TODO: print output?? - try: - c = self.adjust.__code__.co_argcount - if c == 2: - self.adjust(input_data) - else: - self.adjust() # LEGACY mode - # if the above didn't raise an exception, all done (empty completion data, status 'ok') - print(json.dumps(dict(status='ok'))) - except Exception as e: - self.print_json_error( - e.__class__.__name__, - "failure", - str(e) - ) - raise - - def debug(self, *message): - print(*message, flush=True, file=sys.stderr) - - def print_json_error(self, error, cl, message): - ''' - Prints JSON formatted error - ''' - print(json.dumps( - { - "error": error, - "class": cl, - "message": message - }), flush=True) - - ################################################## - # METHODS THAT MUST BE OVERWRITTEN - ################################################## - def query(self): - ''' - ''' - raise Exception("Not implemented") - - def adjust(self): - ''' - ''' - raise Exception("Not implemented") - - ################################################## - # METHODS THAT CAN BE OVERWRITTEN - ################################################## - def handle_cancel(self, signal, frame): - ''' - Handles SIGUSR1 signal - ''' - self.debug("Received cancel signal", signal) diff --git a/util.py b/util.py index c0a4f2b..9c90306 100644 --- a/util.py +++ b/util.py @@ -151,6 +151,8 @@ def run_and_track(path, *args, data = None, progress_cb: typing.Callable[..., No if rc != 0: # error, add verbose info to returned data if not rsp.get("status"): # NOTE if driver didn't return any json, status will be "nodata". Preferably, it should print structured data even on failures, so errors can be reported neatly. rsp["status"] = "failed" + if not rsp.get("reason"): + rsp["reason"] = "unknown" m = rsp.get("message", "") # if config[report_stderr]: rs = os.environ.get("OPTUNE_VERBOSE_STDERR", "all") # FIXME: default setting? From 264174da2ed7adc8dc2a610afec34a9750c743ce Mon Sep 17 00:00:00 2001 From: Fred Sharp Date: Thu, 9 Jul 2020 14:27:27 -0500 Subject: [PATCH 2/3] Allow later driver to overwrite monitoring of prev --- adjust | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/adjust b/adjust index 09f7ce2..5d5e511 100755 --- a/adjust +++ b/adjust @@ -68,17 +68,6 @@ def merge_settings(dest, src): return -def merge_monitoring(dest, src): - """merge src['settings'] into dest['settings'], raise exception if the - same monitoring key is defined in both""" - - for mn,mv in src.get("monitoring",{}).items(): - if mn in dest: - raise ValueError(mn) - dest[mn] = mv - - return - def _query(app_id='app'): comps = {} @@ -95,7 +84,7 @@ def _query(app_id='app'): for d,info in drivers: # TODO: s[application] (global app settings not supported for now) - merge_monitoring(monitoring, g_state[d]) + monitoring.update(g_state[d].get("monitoring",{})) for cn, cd in g_state[d]["application"]["components"].items(): c = comps.setdefault(cn, {}) try: From bb4f5aef773644f1607d56db6e3d64f6c6e8a446 Mon Sep 17 00:00:00 2001 From: Blake Watters Date: Tue, 14 Jul 2020 03:41:05 -0700 Subject: [PATCH 3/3] Provide an actionable error on subprocess decoding failure --- adjust | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/adjust b/adjust index 5d5e511..7f5df30 100755 --- a/adjust +++ b/adjust @@ -25,7 +25,11 @@ def run(path, *args, data = None): data = json_enc(data).encode("UTF-8") r = subprocess.check_output([path]+list(args), input = data) - return json.loads(r.decode("UTF-8")) + try: + output = r.decode("UTF-8") + return json.loads(output) + except json.decoder.JSONDecodeError as error: + raise AdjustError(f"JSON decoding of subprocess output failed:\n{output}") from error def locate_drivers():