[Fuego] Test request rejection or execution based on board availability

Tim.Bird at sony.com Tim.Bird at sony.com
Fri May 14 22:59:54 UTC 2021


Please see comments inline below.

> -----Original Message-----
> From: Pavan Arun Deshpande <pavan.ad at pathpartnertech.com>
> 
> Hi Tim,
> I have modified the code according to what you have suggested.
> I have copied the respective patch files and fserver output snapshot below.
> 
> 
> I have also implemented a "get-board" ftc command which displays the board information(board fields) from <board>.json file.
> 
> here is the get-board command help:
> $  ftc get-board --help
> ftc get-board: retrieve the board information from <board>.json file
> 
> Usage: ftc get-board <board> [<field1>  <field2>...]
> 
> Get the one or more(or all) board fields from the server
> Allowed fields are: [status, description, information]
> 
> 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.

I like the ability to get multiple individual fields.

I'm thinking about how this compares with the ftc 'query-board' command, which
provides the ability to get information about the attributes from the local board
file.

Here is the online help from 'ftc query-board':

$ ftc query-board help
ftc query-board: Show information about a board.

Usage: ftc query-board -b <board> [-q] [--sh] [-n <attr>]
  Show information about an object.  Use the '-n' option to display
  the value of a single attribute, <attr>.  Use '-q' to show the
  attribute names only.

  If the '--sh' option is used, the output is formatted so that it is
  suitable for importing into a shell script using the '.' or
  'source' command.
-----

> 
> Here is some output examples
>  $ ftc get-board docker
> ----------------------------board fields--------------------------------------
> {
>     "status": "offline",
>     "information": "out of service",
>     "host": "pavan_wks",
>     "board": "docker"
> }
> $ ftc get-board docker status
> board status = offline
> 
> $ ftc get-board docker status information
> board status = offline
> board information = out of service


For comparison, here are some examples of using 'ftc query-board'
Here are some examples of using that command:
$ ftc query-board -b bbb
(shows the whole 'bbb.board' file)
$ ftc query-board -b bbb -n TRANSPORT
ssh
$ftc query-board -b bbb -n TRANSPORT --sh
TRANSPORT="ssh"

> 
> please let me know your thoughts on this.
> 
> 
> Here is the fserver patch:
> 
> 
> Signed-off-by: Pavan Arun Deshpande <pavan.ad at pathpartnertech.com <mailto:pavan.ad at pathpartnertech.com> 
I believe your e-mail client added this "mailto:" URI.  If that can be avoided in the future,
it would be good.

> ---
>  fserver.py | 27 ++++++---------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/fserver.py b/fserver.py
> index 3207e22..e23d122 100755
> --- a/fserver.py
> +++ b/fserver.py
> @@ -529,25 +529,14 @@ def do_update_board(req):
> 
>      send_response(result, msg)
> 
> -def do_get_board_file(req):
> +def do_get_board(req):
>      req_data_dir = req.config.data_dir + os.sep + "boards"
>      result = "OK"
>      msg = ""
> -    print(req)
> -    #convert form (cgi.fieldStorage) to dictionary
> -    new_dict = {}
> -    for k in req.form.keys():
> -        new_dict[k] = req.form[k].value
> -
> -    # remove action
> -    del(new_dict["action"])
> 
> -    # sanity check the submitted data
> -    # check for host and board
>      try:
> -        host = new_dict["host"]
> -        board = new_dict["board"]
> -        # FIXTHIS - check that host is registered
> +        host = req.form["host"].value
> +        board = req.form["board"].value
>      except:
>          msg += "Error: missing host or board in form data"
>          send_response("FAIL", msg)
> @@ -562,16 +551,12 @@ def do_get_board_file(req):
>          send_response("FAIL", msg)
>          return
> 
> -    #storing <board>.json fields into board dictionary
> +    #retrieving <board>.json fields into board dictionary
>      import json
>      board_fd = open(jfilepath, "r")
> -    board_dict = json.load(board_fd)
> +    msg = board_fd.read()
>      board_fd.close()
> 
> -    #fetching status field from <board>.json file
> -    board_status = board_dict["status"]
> -    msg +=board_status
> -
>      send_response(result,msg)
> 
> 
This is a diff on top of the previous diff.  In order to apply this, I need a diff against the
original fserver code.  Please merge these commits together and submit them
as a single patch.

> @@ -1493,7 +1478,7 @@ def main(req):
>          log_this("DEBUG: in main(), after call to cgi.FieldStorage")
> 
>      action_list = ["show", "put_test", "put_run", "put_request",
> -            "put_binary_package", "put_board", "update_board", "get_board_file",
> +            "put_binary_package", "put_board", "update_board", "get_board",
Same issue here.

>              "query_boards", "query_requests", "query_runs", "query_tests",
>              "get_request", "get_run_url", "get_test",
>              "remove_request", "remove_test", "remove_run",
> --
> 2.17.1

Since these patches are for two different projects, please send them individually.
I would prefer to receive the patch for fserver first, followed by the patch for
fuego-core.

Please format them using 'git format-patch', and send them both to the same
email recipients as this message.
See http://fuegotest.org/wiki/License_And_Contribution_Policy#Submitting_contributions
for some additional tips.

It doesn't have to be perfect, but hopefully this will train you in how to submit
code to other projects.  Alternatively, if you'd like to submit the patches as
pull requests to github (for the fserver code) or bitbucket (for the fuego-core code),
that we can walk through that process as well.  Either process would be good for you
to learn, if you are not already familiar with them.
 
> 
> Here is the ftc patch:
> 
> 
> Signed-off-by: Pavan Arun Deshpande <pavan.ad at pathpartnertech.com <mailto:pavan.ad at pathpartnertech.com> >
> ---
>  scripts/ftc | 85 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 79 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/ftc b/scripts/ftc
> index c951ed8..c3d7c98 100755
> --- a/scripts/ftc
> +++ b/scripts/ftc
> @@ -33,6 +33,7 @@
>  #    objects: board, host, test, request, run, plan (testplan), spec?
> 
>  # only import enough to get to container dispatcher in main()
> +import ast
I don't think this is needed.  The data returned from the server should
be in json format.  (If it's not, something has gone horribly wrong).
Please just use json to parse the data into a python dictionary.

>  import os
>  import sys
>  import subprocess
> @@ -322,6 +323,21 @@ 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>...]
> +
> +Get the one or more(or all) board fields from the server
> +Allowed fields are: [status, description, information]
> +
> +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.
> +"""),
> +
> +
> +
>  "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
> @@ -3402,15 +3418,18 @@ def do_run_request(conf, options):
>      board_name = req["board"]
>      board_field = {"host": conf.host, "board": board_name}
> 
> -    url = conf.SERVER_URL_BASE+"get_board_file"
> +    url = conf.SERVER_URL_BASE+"get_board"

Please merge the two sets of changes to ftc and send this as a single patch.

>      resp = requests.post <http://requests.post> (url, board_field)
>      result, content = resp.text.split('\n', 1)
> -    print(content)
> +
> +    board_file_dict = ast.literal_eval(content)
Please use json.loads here instead.

> +    board_status = board_file_dict["status"]
> +    board_information = board_file_dict["information"]
>      if result != "OK":
>          error_out("Can't read board data '%s' from server\nServer returned message: %s" % (run_id, content))
>          sys.exit(0)
>      #if board is ready run the request
> -    elif content == "ready":
> +    elif board_status == "ready":
>          # In do_run_request, notify server that request is in-progress
>          update_request(conf, req_id, "running")
> 
> @@ -3562,10 +3581,10 @@ def do_run_request(conf, options):
> 
>          sys.exit(rcode)
>      # if board is offline reject the request
> -    elif content == "offline" or content == "disable":
> +    elif board_status == "offline" or content == "disabled":
>          update_request(conf, req_id, "error",
> -                    {"reason": "board is offline"})
> -        error_out("%s board is offline" % (board_name))
> +                    {"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)
> 
>  def do_query_request(conf, options):
> @@ -4155,6 +4174,56 @@ def do_update_board(conf, options):
> 
>      print "OK"
> 
> +def do_get_board(conf,options):
> +    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)
> +    try:
> +        board = bmap[board_name]
> +    except:
> +        error_out("Unrecognized board %s" % (board_name))
> +
> +    url = conf.SERVER_URL_BASE+"get_board"
> +
> +    # Note: you can only change the status of a board in your own lab
Get rid of this  comment (which does not apply here)

> +    board_dict = {"host": conf.host, "board": board_name }
> +
> +    resp = requests.post <http://requests.post> (url, board_dict)
> +    result, content = resp.text.split('\n', 1)
> +    board_fields = ast.literal_eval(content)
Please use the python json module to parse the content here

> +    if result != "OK":
> +        error_out("Can't update board attribute.\nServer returned message: %s" % content)
> +    elif options:
> +        for option in options:
> +            attr = option
> +            #del(options[0])
> +            # should validate legal attributes and options here
> +            if attr not in ["status", "description", "information"]:
> +                error_out("Illegal attribute '%s' - please use 'status', 'description' or 'information'" % attr)
This is not needed, and will need to be maintained as attributes are added to
board json file.  For setting status, the fields need to be filtered to prevent
malicious data, but for just showing the field values, I don't have a use case
where I would limit the visibility of the fields.

So please remove the above check.

> +            try:
> +                if attr == "status":
> +                    print("board status = %s" %(board_fields["status"]))
> +                elif attr == "description":
> +                     print("board description = %s" %(board_fields["description"]))
> +                elif attr == "information":
> +                    print("board information = %s" %(board_fields["information"]))

This can be more free-form.  I think that can just be:
try:
   print("%s=%s" % (attr, board_fields[attr])

Now, about the default output value here.  I'm a bit torn about your
extra prefix of "board " on each attribute value printed, and to
print the attribute name with the value.

When printing multiple values, printing the attribute name is obviously helpful.

However, I envisioned that one way people would use  'ftc query-board' is as follows:
(from shell code):
export ARCH=$(ftc query-board -b bbb -n ARCHITECTURE)

By outputting only the value, this allow shell code to extract an individual
value from the board configuration file more easily.
'ftc query-board' never supported showing more than one option, so
the issue of needing to print the attribute name never came up.

I think I'd like to rework 'ftc query-board' and 'ftc get-board' to be similar,
and do the following:
if '-q' is specified, output only the attribute value
otherwise, output the attribute name and the attribute value:

So please add something like this:
(at top of routine)
global quiet

(where printing occurs)
if quiet:
	print board_fields[attr]
else:
	print "%s=%s" % (attr, board_fields[attr])


> +            except:
> +                board_fields[attr] = "None"
> +                print("board %s = %s" %(attr,board_fields[attr]))
I'm not sure why we set it to None, and then just look it up again.
I would change this to:
except:
   print "board attribute '%s' not found" % attr

> +
> +
> +    else:
> +        print("----------------------------board fields--------------------------------------")
> +        print(json.dumps(board_fields,indent=4))
Does this make the output alphabetical?  I'd prefer that, if possible.
(Maybe use sort_keys=True?)

> +
> +    if options:
> +        del options[:]
Why is this here?

> +
> 
>  def put_run_fuego(conf, run_filepath):
>      url = conf.SERVER_URL_BASE+"put_run"
> @@ -6171,6 +6240,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

You should add  usage help for the get-board command also.

See the command_help dictionary.

The structure is:
key = command name
value = tuple of (one-line description, multi-line usage help)

Thanks!  It's looking good.
 -- Tim

> 
> 
> 
> Thanks and regards
> Pavan Arun Deshpande
> 
> 
> On Wed, May 5, 2021 at 2:32 AM <Tim.Bird at sony.com <mailto:Tim.Bird at sony.com> > wrote:
> 
> 
> 	Pavan,
> 
> 	I am out of the office this week (taking some personal time for some family business).  Hence
> 	I cannot give a full review of this code at the moment.  Overall, this looks like the right approach
> 	(to have ftc grab the status of the board from the server and determine how to respond to
> 	'ftc run-request' based on the status.
> 
> 	I have a few comments below.  Can you please make the requested changes (or answer my
> 	questions), and we can discuss more when I am back in office next week?
> 
> 	> -----Original Message-----
> 	> From: Pavan Arun Deshpande <pavan.ad at pathpartnertech.com <mailto:pavan.ad at pathpartnertech.com> >
> 	>
> 	> Hi Tim,
> 	>
> 	> I have implemented "test request rejection or execution based on board availability" functionality.
> 	> I have tested and verified  this functionality in my local fserver  and attached fserver output snapshot and flowchart of logic
> implemented
> 	> below.
> 	>
> 	>
> 	> To retrieve board availability status field which is located in the board.json file on fserver requires one api to access the
> board.json file. For
> 	> that I have implemented "get_board_file" api on fserver
> 	> which helps to retrieve and send the  board status field back to the ftc request.
> 	>
> 	> I have implemented this  on my local fserver code
> 	>
> 	> Here is the patch of fserver
> 	> Signed-off-by: Pavan Arun Deshpande <pavan.ad at pathpartnertech.com <mailto:pavan.ad at pathpartnertech.com>
> <mailto:pavan.ad at pathpartnertech.com <mailto:pavan.ad at pathpartnertech.com> > >
> 	> ---
> 	>  fserver.py | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
> 	>  1 file changed, 51 insertions(+), 3 deletions(-)
> 	>
> 	> diff --git a/fserver.py b/fserver.py
> 	> index a4bca89..3207e22 100755
> 	> --- a/fserver.py
> 	> +++ b/fserver.py
> 	> @@ -64,11 +64,13 @@ VERSION=(0,6,0)
> 	>  # 2. local fserver in Fuego container
> 	>  # 3. test fserver on Tim's private server machine (birdcloud.org <http://birdcloud.org>  <http://birdcloud.org> )
> 	>  # 4. test fserver on Tim's home desktop machine (timdesk)
> 	> -base_dir = "/home/ubuntu/work/fserver/fserver-data"
> 	> +#base_dir = "/home/ubuntu/work/fserver/fserver-data"
> 	> +base_dir = "/home/pavan.ad/fserver/fserver/fserver-data <http://pavan.ad/fserver/fserver/fserver-data>
> <http://pavan.ad/fserver/fserver/fserver-data> "
> 	>  if not os.path.exists(base_dir):
> 	>      base_dir = "/usr/local/lib/fserver/fserver-data"
> 	>  if not os.path.exists(base_dir):
> 	> -    base_dir = "/home/tbird/work/fserver/fserver-data"
> 	> +    #base_dir = "/home/tbird/work/fserver/fserver-data"
> 	> +    base_dir = "/home/pavan.ad/fserver/fserver/fserver-data <http://pavan.ad/fserver/fserver/fserver-data>
> <http://pavan.ad/fserver/fserver/fserver-data> "
> 
> 	Obviously, these changes are specific to your server, and should be kept local.
> 	These would not be integrated into the upstream code.
> 
> 	>
> 	>  # this is used for debugging only
> 	>  def log_this(msg):
> 	> @@ -527,6 +529,52 @@ def do_update_board(req):
> 	>
> 	>      send_response(result, msg)
> 	>
> 	> +def do_get_board_file(req):
> 	I prefer the simpler name: 'do_get_board'
> 
> 	> +    req_data_dir = req.config.data_dir + os.sep + "boards"
> 	> +    result = "OK"
> 	> +    msg = ""
> 	> +    print(req)
> 	I think this debug print needs to be removed in the final code (if I'm not mistaken)
> 
> 	> +    #convert form (cgi.fieldStorage) to dictionary
> 	> +    new_dict = {}
> 	> +    for k in req.form.keys():
> 	> +        new_dict[k] = req.form[k].value
> 
> 	It's not clear to me why you need a new_dict here.
> 	We are not saving the data from the form into a file on the server,
> 	but merely extracting the host and board from the form data,
> 	in order to access the correct json file.
> 
> 	> +    # remove action
> 	> +    del(new_dict["action"])
> 	I believe this code was copied from the do_update_board?
> 	Since we are not saving the data, we can ignore the fact that the form
> 	data includes the 'action' field.  There's no need to remove it.
> 
> 	> +
> 	> +    # sanity check the submitted data
> 	> +    # check for host and board
> 	> +    try:
> 	> +        host = new_dict["host"]
> 	> +        board = new_dict["board"]
> 	> +        # FIXTHIS - check that host is registered
> 	We can set 'host' and 'board' directly from the req.form here, instead
> 	of from new_dict.
> 	e.g.
> 	host = req.form["host"].value
> 
> 	> +    except:
> 	> +        msg += "Error: missing host or board in form data"
> 	> +        send_response("FAIL", msg)
> 	> +        return
> 	> +
> 	> +    filename = "board-%s:%s" % (host, board)
> 	> +    jfilepath = req_data_dir + os.sep + filename + ".json"
> 	> +
> 	> +    # check that board is already registered
> 	> +    if not os.path.exists(jfilepath):
> 	> +        msg += "Error: board '%s:%s' is not registered" % (host, board)
> 	> +        send_response("FAIL", msg)
> 	> +        return
> 	> +
> 	> +    #storing <board>.json fields into board dictionary
> 
> 	I think I'd prefer the term 'retrieving' instead of 'storing' for this comment
> 
> 	> +    import json
> 	> +    board_fd = open(jfilepath, "r")
> 	> +    board_dict = json.load(board_fd)
> 	> +    board_fd.close()
> 	> +
> 	> +    #fetching status field from <board>.json file
> 	> +    board_status = board_dict["status"]
> 	> +    msg +=board_status
> 
> 	Rather than just pull out the single 'status' field to return to the user,
> 	I thought, based on the name of the action, that this would send the entire
> 	json data file to the user.  I think it would be better to structure this like so:
> 
> 	board_fd = open(jfilepath, "r")
> 	msg = board_fd.read()
> 
> 	This will send the entire json file to the client.  Of course, the client (ftc) will need to parse
> 	the JSON to extract the field it is interested in (in this case, the 'status' field).  However, this
> 	provides greater flexibility as the client can examine any of the board's fields.  This might be
> 	useful for future operations.
> 
> 	> +
> 	> +    send_response(result,msg)
> 	> +
> 	> +
> 	>  def do_put_request(req):
> 	>      req_data_dir = req.config.data_dir + os.sep + "requests"
> 	>      result = "OK"
> 	> @@ -1445,7 +1493,7 @@ def main(req):
> 	>          log_this("DEBUG: in main(), after call to cgi.FieldStorage")
> 	>
> 	>      action_list = ["show", "put_test", "put_run", "put_request",
> 	> -            "put_binary_package", "put_board", "update_board",
> 	> +            "put_binary_package", "put_board", "update_board", "get_board_file",
> 	Please change the action to "get_board" here.
> 
> 	>              "query_boards", "query_requests", "query_runs", "query_tests",
> 	>              "get_request", "get_run_url", "get_test",
> 	>              "remove_request", "remove_test", "remove_run",
> 	> --
> 	> 2.17.1
> 	>
> 	>
> 	>
> 	> The logic to reject or accept the request based on board availability is implemented in my local ftc script.
> 	> here is the patch
> 	>
> 	> Signed-off-by: Pavan Arun Deshpande <pavan.ad at pathpartnertech.com <mailto:pavan.ad at pathpartnertech.com>
> <mailto:pavan.ad at pathpartnertech.com <mailto:pavan.ad at pathpartnertech.com> > >
> 	> ---
> 	>  scripts/ftc | 33 +++++++++++++++++++++++++--------
> 	>  1 file changed, 25 insertions(+), 8 deletions(-)
> 	>
> 	> diff --git a/scripts/ftc b/scripts/ftc
> 	> index 6629642..c951ed8 100755
> 	> --- a/scripts/ftc
> 	> +++ b/scripts/ftc
> 	> def do_run_request(conf, options):
> 	>      put_run_flag = False
> 	>      allow_upgrade_flag = False
> 	> @@ -3399,12 +3399,23 @@ 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_file"
> 	Please use the action "get_board" here.
> 
> 	> +    resp = requests.post <http://requests.post>  <http://requests.post> (url, board_field)
> 	> +    result, content = resp.text.split('\n', 1)
> 	> +    print(content)
> 	Is this a debug statement?  Or did you intend to print the status here?
> 
> 	Now that the server is returning the full JSON for the board, you will need to
> 	parse out the status (using json.loads(content))
> 
> 	Get the status field from the dictionary you get back, and use that in the
> 	subsequent tests below, in place of 'content'.
> 
> 	> +    if result != "OK":
> 	> +        error_out("Can't read board data '%s' from server\nServer returned message: %s" % (run_id, content))
> 	> +        sys.exit(0)
> 	> +    #if board is ready run the request
> 	> +    elif content == "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"]
> 	> +        #board_name = req["board"]
> 	>          test_name = req["test_name"]
> 	>          req_version = req["version"]
> 	>          try:
> 	> @@ -3550,6 +3561,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 content == "offline" or content == "disable":
> 
> 	I'd like to handle these two cases separately.  However, this is a good start for now.
> 	Let's discuss different handling in a future conference call.
> 
> 
> 	> +        update_request(conf, req_id, "error",
> 	> +                    {"reason": "board is offline"})
> 	> +        error_out("%s board is offline" % (board_name))
> 	> +        sys.exit(0)
> 	>
> 	>  def do_query_request(conf, options):
> 	>      attr = None
> 	> 2.17.1
> 	>
> 	>
> 	> Please let me know your opinion on this and if you agree to this we can implement the same in the main fserver and ftc.
> 	>
> 	> Thanks and regards
> 	> Pavan Arun Deshpande
> 
> 	Thanks.  This is definitely headed in the direction I'd like this code to go.  Can you please make the suggested
> 	changes, test it, and send the patch again?
> 	 -- Tim
> 
> 
> 
> 
> 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.



More information about the Fuego mailing list