Bug 1335394

I am also getting this failure when "--dry-run" is provided on the command line, which seems odd as I would think dry-run would not communicate at all with the server.

$ bkr update-inventory --xml --pretty-xml --dry-run rdma-qe-16.lab.bos.redhat.com
Exception: 409 Client Error: CONFLICT
$ bkr update-inventory --pretty-xml --dry-run rdma-qe-16.lab.bos.redhat.com
Exception: 409 Client Error: CONFLICT
$ bkr --version
24.1

The host is currently running a hardware scan so this is the exact same issue.

--dry-run (for this and all the other commands) *does* talk to the server to fetch host info and distro info in order to compile the job, it's just "dry" in the sense that it doesn't enqueue the job that it produces.

Agreed that it is clearly doing the wrong thing in this case though.

I think this will fix the odd handling of --dryrun. It doesn't seem necessary to run find_current_hardware_scan_recipe() if we are not going to submit the job anyway. At least one will get their xml generated for them if they are using --xml.

diff --git a/Server/bkr/server/jobs.py b/Server/bkr/server/jobs.py
index b6a1ec2d771d..82f378df564b 100644
--- a/Server/bkr/server/jobs.py
+++ b/Server/bkr/server/jobs.py
@@ -1238,7 +1238,7 @@ def submit_inventory_job():
         system = System.by_fqdn(fqdn, identity.current.user)
     except DatabaseLookupError:
         raise BadRequest400('System not found: %s' % fqdn)
-    if system.find_current_hardware_scan_recipe():
+    if not dryrun and system.find_current_hardware_scan_recipe():
         raise Conflict409('Hardware scanning already in progress')
     distro = system.distro_tree_for_inventory()
     if not distro:


I have not been able to figure out how to get the description from the base class PlainTextHTTPException in Server/bkr/server/flask_util.py to propagate over.

The server does setup a nice description:
$cat -n Server/bkr/server/jobs.py
[...]
  1240	        raise BadRequest400('System not found: %s' % fqdn)
  1241	    if system.find_current_hardware_scan_recipe():
  1242	        raise Conflict409('Hardware scanning already in progress')
  1243	    distro = system.distro_tree_for_inventory()
[...]

It doesn't seem to be coming through in the http response those. A locally modified version of the client to print out the HTTPError class shows this:

$ cat -n /usr/lib/python2.7/site-packages/bkr/client/commands/cmd_update_inventory.py
   144	                res.raise_for_status()
   145	            except HTTPError, e:
   146			sys.stderr.write(repr(e) + "\n")
   147	                sys.stderr.write('Exception: %s\n' % e.message)
   148	                failed = True
   149	            else:

$ bkr update-inventory --xml --pretty-xml --dry-run rdma-qe-16.lab.bos.redhat.com
HTTPError('409 Client Error: CONFLICT',)
Exception: 409 Client Error: CONFLICT

It seems like the server is dropping the description, aka the body, of the response.

Oddly the server is using a wrapped version of werkzeug[1] for generating the error responses and the brk client is using the requests library for processing the responses. I just thought that odd.

[1] https://github.com/pallets/werkzeug/blob/master/werkzeug/exceptions.py

It helps to just read the documentation[1] :D

I think this will solve the poor error message:

$ git diff
diff --git i/Client/src/bkr/client/commands/cmd_update_inventory.py w/Client/src/bkr/client/commands/cmd_update_inventory.py
index c80ca673ab89..53abd54cba75 100644
--- i/Client/src/bkr/client/commands/cmd_update_inventory.py
+++ w/Client/src/bkr/client/commands/cmd_update_inventory.py
@@ -151,7 +151,7 @@ def run(self, *args, **kwargs):
             try:
                 res.raise_for_status()
             except HTTPError, e:
-                sys.stderr.write('Exception: %s\n' % e.message)
+                sys.stderr.write('Error: %s\n' % e.response.text)
                 failed = True
             else:
                 res_data = res.json()

I get the following when run:

$ bkr update-inventory --xml --pretty-xml --dry-run rdma-qe-16.lab.bos.redhat.com
Error: Hardware scanning already in progress

If you want me to submit a pull-request / post email to devel list for these I will.

[1] http://docs.python-requests.org/en/master/user/quickstart/#response-content

I will take a crack as solving this one and posting upstream.

(In reply to Jonathan Toppins from comment #3)
> I think this will fix the odd handling of --dryrun. It doesn't seem
> necessary to run find_current_hardware_scan_recipe() if we are not going to
> submit the job anyway. At least one will get their xml generated for them if
> they are using --xml.
> 
> diff --git a/Server/bkr/server/jobs.py b/Server/bkr/server/jobs.py
> index b6a1ec2d771d..82f378df564b 100644
> --- a/Server/bkr/server/jobs.py
> +++ b/Server/bkr/server/jobs.py
> @@ -1238,7 +1238,7 @@ def submit_inventory_job():
>          system = System.by_fqdn(fqdn, identity.current.user)
>      except DatabaseLookupError:
>          raise BadRequest400('System not found: %s' % fqdn)
> -    if system.find_current_hardware_scan_recipe():
> +    if not dryrun and system.find_current_hardware_scan_recipe():
>          raise Conflict409('Hardware scanning already in progress')
>      distro = system.distro_tree_for_inventory()
>      if not distro:

This looks good to me.

(In reply to Jonathan Toppins from comment #4)
> It helps to just read the documentation[1] :D
> 
> I think this will solve the poor error message:
> 
> $ git diff
> diff --git i/Client/src/bkr/client/commands/cmd_update_inventory.py
> w/Client/src/bkr/client/commands/cmd_update_inventory.py
> index c80ca673ab89..53abd54cba75 100644
> --- i/Client/src/bkr/client/commands/cmd_update_inventory.py
> +++ w/Client/src/bkr/client/commands/cmd_update_inventory.py
> @@ -151,7 +151,7 @@ def run(self, *args, **kwargs):
>              try:
>                  res.raise_for_status()
>              except HTTPError, e:
> -                sys.stderr.write('Exception: %s\n' % e.message)
> +                sys.stderr.write('Error: %s\n' % e.response.text)
>                  failed = True
>              else:
>                  res_data = res.json()

Ah yes... We actually have this error handling done properly in Client/src/bkr/client/main.py line 114. The problem is this command (and I think the other job submission related commands, like job-submit, job-clone, etc) is implementing its own error handling inside the loop. The idea there is if the command is trying to submit multiple jobs, each one will be submitted even if one of them fails.

Your patch is not quite enough though, it also needs to check the content type of the response before printing it. If the response came back as a 500 Internal Server Error, the body will be a pile of HTML. We don't want to dump that on the user as it will be an unreadable mess that obscures what actually happened.

Posted:
* https://gerrit.beaker-project.org/#/c/5684/
* https://gerrit.beaker-project.org/#/c/5683/

Beaker 24.3 has been released.