review changes

Author: riya-17

lib/pbench/agent/results.py

@@ -2,7 +2,6 @@
 import errno
 import os
 import requests
-import sys
 import tarfile
 
 from configparser import ConfigParser
@@ -26,32 +25,38 @@ def __init__(self, result_dir, target_dir, user, prefix, config, logger):
         self.logger = logger
 
     def check_result_target_dir(self, result_dir, target_dir):
-        sts = 0
         if not result_dir:
-            self.logger.error("Result directory not provided")
-            sts = 1
-        if not target_dir:
-            self.logger.error("Target directory not provided")
-            sts = 1
-        try:
-            self.result_dir = Path(result_dir).resolve(strict=True)
-        except FileNotFoundError:
-            self.logger.error("Invalid result directory provided: {}", result_dir)
-            sts = 1
+            raise FileNotFoundError("Result directory not provided")
         else:
-            if not self.result_dir.is_dir():
-                self.logger.error("Invalid result directory provided: {}", result_dir)
-                sts = 1
-        try:
-            self.target_dir = Path(target_dir).resolve(strict=True)
-        except FileNotFoundError:
-            self.logger.error("Invalid target directory provided: {}", target_dir)
-            sts = 1
+            try:
+                self.result_dir = Path(result_dir).resolve(strict=True)
+            except Exception:
+                raise FileNotFoundError(
+                    "Invalid result directory provided: {}", result_dir
+                )
+            else:
+                if not self.result_dir.is_dir():
+                    self.logger.error(
+                        "Invalid result directory provided: {}", result_dir
+                    )
+                    return 1
+
+        if not target_dir:
+            raise FileNotFoundError("Target directory not provided")
         else:
-            if not self.target_dir.is_dir():
-                self.logger.error("Invalid target directory provided: {}", target_dir)
-                sts = 1
-        return sts
+            try:
+                self.target_dir = Path(target_dir).resolve(strict=True)
+            except Exception:
+                raise FileNotFoundError(
+                    "Invalid target directory provided: {}", target_dir
+                )
+            else:
+                if not self.target_dir.is_dir():
+                    self.logger.error(
+                        "Invalid target directory provided: {}", target_dir
+                    )
+                    return 1
+        return 0
 
     def make_result_tb(self):
         if os.path.exists(f"{self.result_dir}.copied"):
@@ -60,7 +65,7 @@ def make_result_tb(self):
 
         pbench_run_name = self.result_dir.name
         if os.path.exists(f"{self.result_dir}/.running"):
-            self.logger.debug(
+            self.logger.warning(
                 "The benchmark is still running in {} - skipping; if that is"
                 " not true, rmdir {}/.running, and try again",
                 pbench_run_name,
@@ -124,7 +129,8 @@ def make_result_tb(self):
                     self.logger.error("error removing failed tar ball, {}", tarball)
             return (1, "")
 
-        self.make_md5sum(tarball)
+        ret = self.make_md5sum(tarball)
+        return (ret, "")
 
         # The contract with the caller is to just return the full path to the
         # created tar ball.
@@ -149,7 +155,8 @@ def make_md5sum(self, tarball):
                     self.logger.error(
                         "error removing failed tar ball MD5, {}", tarball_md5
                     )
-            sys.exit(1)
+            return 1
+        return 0
 
 
 class CopyResultTb:
@@ -192,17 +199,25 @@ def copy_result_tb(self, token):
         with open(self.tarball_md5, "r") as md5fp:
             md5sum = md5fp.read().split()[0]
         filename = secure_filename(str(self.tarball))
-        headers = {"filename": filename, "Content-MD5": md5sum, "token": token}
+        headers = {
+            "filename": filename,
+            "Content-MD5": md5sum,
+            "Authorization": f"Bearer {token}",
+        }
         with self.tarball.open("rb") as f:
             try:
                 response = requests.put(
                     self.upload_url, data=self.read_in_chunks(f), headers=headers
                 )
                 self.logger.info("File uploaded successfully")
             except Exception:
-                self.logger.exception("There was something wrong with your request")
+                self.logger.exception(
+                    "There was something wrong with file upload request: file: '{}', URL: '{}'",
+                    self.tarball,
+                    self.upload_url,
+                )
                 return 1
-        if not response.status_code == 200:
+        if not response.ok:
             self.logger.error(
                 "There was something wrong with your request, error code: '{}'",
                 response.status_code,

lib/pbench/cli/agent/commands/results/__init__.py

@@ -23,22 +23,15 @@ def execute(self):
 
         logger = get_pbench_logger("pbench-agent", config)
 
-        if not self.context.controller:
-            controller = os.environ.get("_pbench_full_hostname")
-            if not controller:
-                logger.error("Missing controller name (should be 'hostname -f' value)")
-                sys.exit(1)
-
         results_webserver = self.config.get("results", "webserver")
         if not results_webserver:
             logger.error(
                 "No web server host configured from which we can fetch the FQDN of the host to which we copy/move results"
             )
-            logger.debug("'webserver' key in 'results' section not set")
 
         server_rest_url = self.config.get("results", "server_rest_url")
         response = requests.get(f"{server_rest_url}/host_info")
-        if response.status_code not in [200, 201]:
+        if response.ok:
             logger.error(
                 "Unable to determine results host info from %s/host_info",
                 server_rest_url,
@@ -58,44 +51,55 @@ def execute(self):
             )
             sys.exit(1)
 
-        try:
-            temp_dir = tempfile.mkdtemp(
-                dir=self.config.pbench_tmp, prefix="pbench-move-results."
-            )
-        except Exception:
-            logger.error("Failed to create temporary directory")
-            sys.exit(1)
-
-        user = self.context.user if self.context.user else "pbench"
-        prefix = self.context.prefix if self.context.prefix else ""
+        temp_dir = tempfile.mkdtemp(
+            dir=self.config.pbench_tmp, prefix="pbench-move-results."
+        )
 
         runs_copied = 0
         failures = 0
 
         for dirent in config.pbench_run.iterdir():
-            if not self.dirent.is_dir():
+            if not dirent.is_dir():
                 continue
             if dirent.name.startswith("tools-") or dirent.name == "tmp":
                 continue
             result_dir = dirent
-            mrt = MakeResultTb(result_dir, temp_dir, user, prefix, self.config, logger)
-            make_return_val = mrt.check_result_target_dir(result_dir, temp_dir)
+            mrt = MakeResultTb(
+                result_dir,
+                temp_dir,
+                self.context.user,
+                self.context.prefix,
+                self.config,
+                logger,
+            )
+
+            try:
+                make_return_val = mrt.check_result_target_dir(result_dir, temp_dir)
+            except FileNotFoundError as e:
+                logger.error(e)
+
             if make_return_val:
-                sys.exit(1)
+                failures += 1
+                continue
             status, result_tb_name = mrt.make_result_tb()
 
             if status == 0:
                 if os.path.exists(result_tb_name):
                     assert (
                         result_tb_name
                     ), "Logic bomb!  make_result_tb() always returns a tar ball name"
-                    crt = CopyResultTb(controller, result_tb_name, self.config, logger)
+
+                    crt = CopyResultTb(
+                        self.context.controller, result_tb_name, self.config, logger
+                    )
                     copy_return_val = crt.check_tb_tbmd5()
                     if copy_return_val:
-                        sys.exit(1)
+                        failures += 1
+                        continue
                     ret = crt.copy_result_tb(self.context.token)
                     if ret:
-                        sys.exit(1)
+                        failures += 1
+                        continue
             else:
                 logger.error("Error occured while creating Result Tarball")
                 failures += 1
@@ -136,14 +140,20 @@ def execute(self):
 @click.command()
 @common_options
 @click.option(
-    "--controller", help="Controller name (should be hostname -f value)",
+    "--controller",
+    required=True,
+    envvar="_pbench_full_hostname",
+    prompt=False,
+    help="Controller name",
 )
 @click.option(
-    "--user", help="Pbench user",
+    "--user", default="pbench", prompt=False, help="Pbench user",
 )
 @click.option(
     "--token",
-    prompt=False,
+    required=True,
+    prompt=True,
+    envvar="PBENCH_ACCESS_TOKEN",
     help="pbench server authentication token (will prompt if unspecified)",
 )
 @click.option(