Re: [Fuego] [PATCH] Fix for api structuring for action buttons and board reservation display

<[email protected]> Fri, 2 Jul 2021 23:58:04 +0000
Newsgroups dev.linux.lists.fuego
Message-ID <BYAPR13MB25033CCB211B00DDD125ED7DFD1F9@BYAPR13MB2503.namprd13.prod.outlook.com>
See comments inline below.

> -----Original Message-----
> From: Pavan Arun Deshpande <[email protected]>
> 
> Use correct api format for action buttons  Fix a bug in
>  show_board_info with error in api structuring
> 
> Use correct key-word to extract the name of the user to whom the board is reserved .
> 
> Signed-off-by: Pavan Arun Deshpande <[email protected] <mailto:[email protected]> >
> ---
>  lcserver.py | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/lcserver.py b/lcserver.py
> index b929cd3..eea3b28 100755
> --- a/lcserver.py
> +++ b/lcserver.py
> @@ -56,7 +56,7 @@ import cgi
>  import re
>  import tempfile
>  import urllib
>  #simplejson loads faster than json, use that if available
>  try:
>      import simplejson as json

I'm not sure why this hunk is included, there are no changes in it.
This is strange.  How did you make this patch?

> @@ -92,7 +92,8 @@ VERSION=(0,6,0)
>  # 2. local lcserver in Fuego container
>  # 3. test lcserver on Tim's private server machine (birdcloud.org <http://birdcloud.org> )
>  # 4. test lcserver on Tim's home desktop machine (timdesk)
> -base_dir = "/home/ubuntu/work/labcontrol/lc-data"
> +#base_dir = "/home/ubuntu/work/labcontrol/lc-data"
> +base_dir = "/home/fuego/labcontrol/lc-data"

I'm omitting this hunk, as it is a change specific to your lab.
When you pull the code, you'll have to reset this string
for your situation.

>  if not os.path.exists(base_dir):
>      base_dir = "/usr/local/lib/labcontrol/lc-data"
>  if not os.path.exists(base_dir):


> @@ -767,7 +768,7 @@ def show_board_info(req, bmap):
> 
>      req.html.append("<h3>Status</h3>\n<ul>")
> 
> -    reservation = bmap.get("reservation", "None")
> +    reservation = bmap.get("AssignedTo", "None")
Looks good.

>      req.html.append("<li>Reservation: %s</li>" % reservation)
> 
>      # show power status
> @@ -785,12 +786,26 @@ def show_board_info(req, bmap):
> 
>      req.html.append("<h3>Actions</h3>\n<ul>\n")
>      if pc:
> -        reboot_link = req.config.url_base + "/api/devices/%s/power/reboot" % (bmap["name"])
> +        reboot_link = req.config.url_base + "/api/v0.2/devices/%s/power/reboot" % (bmap["name"])
Nice catch.  Thanks.

>          req.html.append("""
>  <form method="get" action=%s>
>  <input type="submit" name="button" value="Reboot">
>  </form>
>  """ % reboot_link)
> +        on_link = req.config.url_base + "/api/v0.2/devices/%s/power/on" % (bmap["name"])
> +        req.html.append("""
> +<form method="get" action=%s>
> +<input type="submit" name="button" value="ON">
> +</form>
> +""" % on_link)
> +        off_link = req.config.url_base + "/api/v0.2/devices/%s/power/off" % (bmap["name"])
> +        req.html.append("""
> +<form method="get" action=%s>
> +<input type="submit" name="button" value="OFF">
> +</form>
> +""" % off_link)

It's nice to add these buttons.  Thanks.
> +
> +
>      req.html.append("</ul>")
> 
>  # returns (RSLT_OK, status|RSLT_FAIL, message)
> @@ -1130,7 +1145,7 @@ def exec_command(req, board_map, resource_map, res_cmd):
>      # lookup command to execute in resource_map
>      res_cmd_str = res_cmd + "_cmd"
>      if res_cmd_str not in resource_map:
> -        msg = "Resource '%s' does not have %s attribute, cannot execute" % (resource["name"], res_cmd_str)
> +        msg = "Resource '%s' does not have %s attribute, cannot execute" % (resource_map["name"], res_cmd_str)
>          return (RSLT_FAIL, msg)
> 
>      cmd_str = resource_map[res_cmd_str]
> @@ -1144,8 +1159,10 @@ def exec_command(req, board_map, resource_map, res_cmd):
>      # or resource data
> 
>      rcode, result = getstatusoutput(cmd_str)
> +    print("rcode=")
> +    print(rcode)
I'm not putting this debug code in.  This is in the server, and depending on how
the cgi script is run (as a CGI script or as a WSGI app), this will have unexpected
effects.

The correct way to debug the server is to put in log_this() or dlog_this() statements,
and watch the log as the script is executed.  You can do something like this:

tail -f /home/fuego/labcontrol/lc-data/lcserver.log

Leave that running in its own window, and you can see stuff go by as requests
are handled and the web interface is used.

Note that log_this() will put something into the log always, and dlog_this() will
only put something in the log if the variable 'debug' is set.  This can be set to
debug=True by uncommenting the line near the top of the script.  You will see
a lot more information in the log when you use this.

>      if rcode:
> -        msg = "Result of %s operation on resource %s = %d" % (res_cmd, resource["name"], rcode)
> +        msg = "Result of %s operation on resource %s = %d" % (res_cmd, resource_map["name"], rcode)
I already had this.  That's what I checked in yesterday.

I assume you missed it because you didn't rebase your changes on top of mine.
Please do this in the future.

>          msg += "command output='%s'" % result
>          return (RSLT_FAIL, msg)
> 
> --
> 2.17.1
> 
Thanks for catching all these bugs.  This is now in the master branch.

Please pull it and try it out, and let me know if you see any problems.
 -- Tim