Re: [Fuego] [PATCH] Add support to run or reject tests based on the board availability

OK - there were a lot of issues.  I'll comment on them below, but I decided
to just fix them and add the feature based on the patch, rather than requiring
you to continue to fix and re-submit the patch.

Note that I put the fserver code live on fuegotest.  So you can use that server
for testing the feature, if you'd like.

> -----Original Message-----
> From: Pavan Arun Deshpande <[email protected]>
> 
> Hi Tim,
> I have copied the ftc patch below.
> if  any suggestions let me know.
> 
> 
> Subject: [PATCH] 1. Add support to run or reject tests  based on the board
>  availability.
The subject already covers the commit short-description, and it does not need
to be applied in the email message body.  Also, numbering a list with one
item in the subject and one in the commit message is a bit strange.

I reworded this.

> 
> 2. ftc: Add support ftc get-board <board> [<field1> <field2>...] [-q]
> 
>         get-board : retrieve the board information from <board>.json file
> 
> Signed-off-by: Pavan Arun Deshpande <[email protected] <mailto:[email protected]> >
> ---
>  scripts/ftc | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 81 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/ftc b/scripts/ftc
> index 6629642..5b2285f 100755
> --- a/scripts/ftc
> +++ b/scripts/ftc
> @@ -36,7 +36,7 @@
>  import os
>  import sys
>  import subprocess
> -
> +import json
This is not needed (or desirable).  'import json' is done in main at about line 6060.
ftc has rather complicated imports, because it tries to defer the majority of them
until after it has decided whether it will execute the command inside or outside
the docker container.  (When ftc is run outside the container, it does not need
the majority of imports, as it often just calls itself inside the container with
the same arguments.)

>  # delay other imports until we're sure we're running in the container
>  re = None
>  time = None
> @@ -322,6 +322,20 @@ Here are some examples:
>   $ ftc update-board status=busy information="processing local jobs"
>  """),
> 
> +"get-board": ("retrieve the board information from <board>.json file",
> +    """Usage: ftc get-board <board> [<field1> <field2>...] [-q]
> +       Use '-q' to show the attribute values only.
> +Get the one or more(or all) board fields from thre server
> +
> +Here are some examples:
> +    $ ftc get-board docker                                   ----- prints all the fields.
> +    $ ftc get-board docker status                            ----- prints only status field.
> +    $ ftc get-board docker status information                ----- prints both status and information fields.
> +    $ ftc get-board docker status information description    ----- prints the status,information and  description fields.
> +"""),

The board name needs to be the fully qualified board name - including host and board, in the
format timslab:bbb (ie: colon-separated).

Also, it would be good to note that sometimes information comes out in json format,
and sometimes in attr=value format.

> +
> +
> +
>  "put-binary-package": ("Put a test binary-package on the server.",
>     """Usage: ftc put-binary-package <test_binary_package_file>
>  Put a test binary package on the server.  The test must be an existing test
> @@ -2165,7 +2179,6 @@ def do_list_nodes(conf):
>  def do_list_jobs(conf):
>      job_list = [job['name'] for job in server.get_jobs()]
>      job_list.sort()
> -
It is not good to include changes that are outside the scope of
the feature being presented.  This white-space fix in an unrelated
routine should not have been included in this patch.  (see my next comment)

>      indent = show_list_title("Jenkins jobs in this system:")
>      for job in job_list:
>          print indent + job
> @@ -3367,6 +3380,7 @@ def find_best_test_match(test_name, req_version, test_tuples):
> 
>      return candidate
> 
> +
This whitespace fix is OK, given that this patch modifies do_run_request()

In general, code fixups like whitespace cleanups or comment changes are
only acceptable in the context of a patch that affects the area in question.
There is a certain hesitancy (when using 'git' to manage source code)
to apply whitespace cleanups other than in directly-affected code, as
these types of cleanups have the effect of making 'git annotate' harder to use.

>  def do_run_request(conf, options):
>      put_run_flag = False
>      allow_upgrade_flag = False
> @@ -3399,12 +3413,25 @@ def do_run_request(conf, options):
> 
>      print "Trying to get request '%s' from server" % req_id
>      req = get_request(conf, req_id)
> +    board_name = req["board"]
> +    board_field = {"host": conf.host, "board": board_name}
> +
> +    url = conf.SERVER_URL_BASE+"get_board"
> +    resp = requests.post <http://requests.post> (url, board_field)
> +    result, content = resp.text.split('\n', 1)
> 
> +    board_file_dict = json.loads(content)
> +    board_status = board_file_dict["status"]
> +    board_information = board_file_dict["information"]

These lines should be moved below the result check.

> +    if result != "OK":
> +        error_out("Can't read board data '%s' from server\nServer returned message: %s" % (run_id, content))
This routine has no run_id.  This should be req_id, and the message should be altered to
reflect that.

> +        sys.exit(0)
> +    #if board is ready run the request
> +    elif board_status == "ready":
>          # In do_run_request, notify server that request is in-progress
>          update_request(conf, req_id, "running")
> 
>          # now actually execute the request
> -    board_name = req["board"]
>          test_name = req["test_name"]
>          req_version = req["version"]
>          try:
> @@ -3550,6 +3577,12 @@ def do_run_request(conf, options):
>          #    print("Request %s was updated on the server" % req_id)
> 
>          sys.exit(rcode)
> +    # if board is offline reject the request
> +    elif board_status == "offline" or board_status == "disabled":
> +        update_request(conf, req_id, "error",
> +                    {"reason": "%s board is %s due to %s" %(board_name,board_status,board_information)})
> +        error_out("%s board is %s due to %s" % (board_name,board_status,board_information))
> +        sys.exit(0)

These lines need to be moved up before the bulk of the lines that do execute
the run request.  Otherwise, all the lines between the check for "ready" and
here need to be indented under the 'if board_status == "ready":' conditional.

> 
>  def do_query_request(conf, options):
>      attr = None
> @@ -4138,6 +4171,47 @@ def do_update_board(conf, options):
> 
>      print "OK"
> 
> +def do_get_board(conf,options):
> +    global quiet
> +    try:
> +        board_name = options[0]
> +    except:
> +        error_out("Must specify a board name get board fields")
> +    del(options[0])
> +
> +    # check whether argument is a legal board name
> +    bmap = get_fuego_boards(conf)
get_fuego_boards returns only the list of boards on this Fuego host.
However, the user may be asking for the information about a remote board.
So this check of the board_name against a local list of boards is incorrect.

This will work as long as you only try to do 'ftc get-board' for a board in
your own lab, but will fail for boards in remote labs.

> +    try:
> +        board = bmap[board_name]
> +    except:
> +        error_out("Unrecognized board %s" % (board_name))
> +
> +    url = conf.SERVER_URL_BASE+"get_board"
> +
> +
> +    board_dict = {"host": conf.host, "board": board_name }

Same issue here.  Using conf.host for the host will only allow a user to
get information about boards in their own lab.

I changed the parsing so that the lab must be specified as part of
the board name.  ie 'ftc get-board timslab:bbb'

> +
> +    resp = requests.post <http://requests.post> (url, board_dict)
> +    result, content = resp.text.split('\n', 1)
> +    board_fields = json.loads(content)
> +    if result != "OK":
> +        error_out("Can't update board attribute.\nServer returned message: %s" % content)
not 'update', should be changed to 'get'  This is a copy/paste bug in the error message.

> +    elif options:
> +        for option in options:
> +            attr = option
> +            try:
> +                if quiet:
> +                    print(board_fields[attr])
> +                else:
> +                    print("%s=%s" % (attr, board_fields[attr]))
> +
> +            except:
> +                print("board attribute %s not found" % attr)
> +        del(options[:])
> +    else:
> +        print("----------------------------board fields--------------------------------------")
> +        print(json.dumps(board_fields,indent=4,sort_keys=True))
In hindsight, it seems like extra work to convert from json text (received from
the server) into a python dictionary, and then back to json text to print it
out.  So I changed this to  just 'print(content)'

> +
> 
>  def put_run_fuego(conf, run_filepath):
>      url = conf.SERVER_URL_BASE+"put_run"
> @@ -6154,6 +6228,10 @@ def main():
>          do_update_board(conf, options)
>          sys.exit(0)
> 
> +    if command == "get-board":
> +        do_get_board(conf, options)
> +        sys.exit(0)
> +
>      if command == "put-run":
>          do_put_run(conf, options)
>          sys.exit(0)
> --
> 2.17.1
> 

The patch was accepted, with some changes as indicated above.

It has been pushed to the fuego-core master branch.

Please try it out and let me know if it behaves as you expect and works
in your lab.

 -- Tim

> Thanks and Regards
> Pavan Arun Deshpande
> 
> 
> 
> This message contains confidential information and is intended only for the individual(s) named. If you are not the intended recipient, you
> are notified that disclosing, copying, distributing or taking any action in reliance on the contents of this mail and attached file/s is strictly
> prohibited. Please notify the sender immediately and delete this e-mail from your system. E-mail transmission cannot be guaranteed to be
> secured or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The
> sender therefore does not accept liability for any errors or omissions in the contents of this message, which arise as a result of e-mail
> transmission.