Bug 1335394 - [CLI] Run bkr update-inventory again when a inventory job is submited
Summary: [CLI] Run bkr update-inventory again when a inventory job is submited
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Beaker
Classification: Community
Component: command line   
(Show other bugs)
Version: develop
Hardware: Unspecified
OS: Unspecified
low
low vote
Target Milestone: 24.3
Assignee: Jonathan Toppins
QA Contact: Dan Callaghan
URL:
Whiteboard:
Keywords: Patch, Triaged
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-05-12 07:29 UTC by wangdong
Modified: 2017-05-30 07:08 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-05-30 07:08:08 UTC
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

Description wangdong 2016-05-12 07:29:53 UTC
Description of problem:
From my test if you run bkr update-inventory twice it will show a error message.

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1. Run bkr update-inventory, check on web, the job is submited.
2. Run bkr update-inventory again.
3.

Actual results:
[root@dhcp-136-60 ~]# bkr update-inventory --prettyxml dev-kvm-guest-02.rhts.eng.bos.redhat.com
/usr/lib/python2.6/site-packages/bkr/client/commands/cmd_update_inventory.py:145: DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
  sys.stderr.write('Exception: %s\n' % e.message)
Exception: 409 Client Error: CONFLICT
Submitted: []

Expected results:
It should submit a job in queue or give user some errors message more clearly.

Additional info:
After step 1, if you cancel that job then run this command again, it can be submit.

Comment 1 Jonathan Toppins 2017-04-10 19:38:47 UTC
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.

Comment 2 Dan Callaghan 2017-04-11 00:37:08 UTC
--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.

Comment 3 Jonathan Toppins 2017-04-11 03:07:45 UTC
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

Comment 4 Jonathan Toppins 2017-04-11 03:37:23 UTC
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

Comment 5 Jonathan Toppins 2017-04-17 18:55:51 UTC
I will take a crack as solving this one and posting upstream.

Comment 6 Dan Callaghan 2017-04-19 07:26:34 UTC
(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.

Comment 7 Dan Callaghan 2017-04-19 07:30:43 UTC
(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.

Comment 11 Dan Callaghan 2017-05-30 07:08:08 UTC
Beaker 24.3 has been released.


Note You need to log in before you can comment on or make changes to this bug.