[Fuego] Test request rejection or execution based on board availability
Tim.Bird at sony.com
Tim.Bird at sony.com
Tue May 4 21:01:55 UTC 2021
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>
>
> 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> >
> ---
> 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> )
> # 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> "
> 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> "
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> >
> ---
> 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> (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
More information about the Fuego
mailing list