Created attachment 23252 [details]
patch for 21.08 (v1)

Created attachment 23280 [details]
patch for 21.08 (v2)

(In reply to Nate Rini from comment #2)
> Created attachment 23280 [details]
> patch for 21.08 (v2)

Reviewer,

This patch corrects the logic that notifies the client if there was a failure. If the serialization succeeded, then it would override a content failure. The second patch corrects an issue that was found while testing the first patch where there was an incorrect check of list_for_each() in _dump_stats_user_list() which falsely accused the 'diag' of failing.

Thanks,
--Nate

Hey Nate, I have a couple questions for ya:

> $ rest -vvvv -X PATCH 'http://localhost/slurmdb/v0.0.38/associations' 2>&1 |quote

* What is this "rest" command? I thought it might be an alias for slurmrestd, but slurmrestd doesn't have a -X option. So, I'm asking you.
* What is PATCH? I don't see it in the rest_api docs (https://slurm.schedmd.com/rest_api.html)

(In reply to Marshall Garey from comment #4)
> Hey Nate, I have a couple questions for ya:
> 
> > $ rest -vvvv -X PATCH 'http://localhost/slurmdb/v0.0.38/associations' 2>&1 |quote
> 
> * What is this "rest" command? I thought it might be an alias for
> slurmrestd, but slurmrestd doesn't have a -X option. So, I'm asking you.

It's my little cheat so I don't have to keep adding the auth stuff to every call:

> $ declare -f rest
> rest () 
> { 
>     curl --unix-socket "${SLURMRESTD}" -H "X-SLURM-USER-NAME:$(whoami)" -H "X-SLURM-USER-TOKEN:auth/none" $@
> }

I have it as a function since I might make it smart enough to handle tokens too (later).

> * What is PATCH? I don't see it in the rest_api docs
> (https://slurm.schedmd.com/rest_api.html)

> https://datatracker.ietf.org/doc/html/rfc5789

PATCH is a (relatively) new HTTP method to update an existing object. Matt was completely correct with it being a better method to use over POST when we are updating an existing record. For now, we just gotta make sure slurmrestd is giving the user the correct error.

Thanks.

Just making sure I understand the issue: Since we don't handle a PATCH request (therefore for us it is an invalid request), we want to send an error back, but we don't. Is that right?


Here's my script:

#!/bin/bash
SLURM_PATH="/home/marshall/slurm-local/21.08/install"
curl -o "$SLURM_PATH/c1/log/curl.log" -k -s -vvvv \
	--request PATCH \
	-H X-SLURM-USER-NAME:$(whoami) \
	-H X-SLURM-USER-TOKEN:$SLURM_JWT \
	-H "Content-Type: application/json" \
	--url localhost:8080/slurmdb/v0.0.37/associations

Running the script:

$ ./invalid_req_PATCH.sh 
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> PATCH /slurmdb/v0.0.37/associations HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.74.0
> Accept: */*
> X-SLURM-USER-NAME:marshall
> X-SLURM-USER-TOKEN:eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2NDQzNjQxMDAsImlhdCI6MTY0NDM2MjMwMCwic3VuIjoibWFyc2hhbGwifQ.v4arIiybSD4ZeHt0zwm7ir-Xd06LFDY67xUsfPcIlbU
> Content-Type: application/json
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Length: 274
< Content-Type: application/json
< 
{ [274 bytes data]
* Connection #0 to host localhost left intact


and the curl output just like you showed:

{
  "meta": {
    "plugin": {
      "type": "openapi\/dbv0.0.37",
      "name": "Slurm OpenAPI DB v0.0.37"
    },
    "Slurm": {
      "version": {
        "major": 21,
        "micro": 5,
        "minor": 8
      },
      "release": "21.08.5"
    }
  },
  "errors": [
  ]
}

Nate,

Here's my results from testing the first patch. Do you have a reproducer for the second patch?

With the patch, I noticed that slurmrestd now has this additional debug2 message (not an error):

slurmrestd: debug2: _on_message_complete_request: [[::ffff:127.0.0.1]:38656] on_http_request rejected: Unspecified error


However, there's still not an error in the JSON output:

{
  "meta": {
    "plugin": {
      "type": "openapi\/dbv0.0.37",
      "name": "Slurm OpenAPI DB v0.0.37"
    },
    "Slurm": {
      "version": {
        "major": 21,
        "micro": 5,
        "minor": 8
      },
      "release": "21.08.5"
    }
  },
  "errors": [
  ]
}

The output from running curl does tell you an error happened, but only if you use -v. Without -v, it is silent. Is that expected?

I personally would have expected some error in "errors" in the JSON output, but I could be expecting wrong.

What is supposed to happen?

(In reply to Marshall Garey from comment #6)
> Just making sure I understand the issue: Since we don't handle a PATCH
> request (therefore for us it is an invalid request), we want to send an
> error back, but we don't. Is that right?

Yes, we were blindly overwriting the rc with the result of the serialization.
 
> < HTTP/1.1 200 OK

This is what is wrong. We need to return an error here.

(In reply to Marshall Garey from comment #7)
> Here's my results from testing the first patch. Do you have a reproducer for
> the second patch?

The first patch, one merely needs to query diag URL. The second requires that you find something that doesn't work:

> $ rest -s -v 'http://localhost/slurm/v0.0.37/user/taco' 2>&1 |grep HTTP
> > GET /slurm/v0.0.37/user/taco HTTP/1.1
> < HTTP/1.1 404 NOT FOUND

in this case, it responds with 404 not found.

> With the patch, I noticed that slurmrestd now has this additional debug2
> message (not an error):
> 
> slurmrestd: debug2: _on_message_complete_request: [[::ffff:127.0.0.1]:38656]
> on_http_request rejected: Unspecified error

Yup, the correct error path is now being followed. There were no log changes in the patch itself.

> The output from running curl does tell you an error happened, but only if
> you use -v. Without -v, it is silent. Is that expected?

Yes, normally curl won't tell you the result of a query. It's convenient but it has a ton of idiosyncrasies you will find if you use it alot.

> I personally would have expected some error in "errors" in the JSON output,
> but I could be expecting wrong.
> 
> What is supposed to happen?

Not for this bug but yes that is another bug that needs to be fixed.

The code corrected is part of the generic HTTP handler code. If we want errors populated, we will need to change each of the methods to ensure they populate that. The errors field I added to give myself a nice easy way to know why something failed but HTTP only has that return code.

For instance, the newer db code does actually populate it:
> $ rest -s 'http://localhost/slurmdb/v0.0.37/user/taco'|jq .errors | quote
> [
>   {
>     "description": "Nothing found",
>     "error_number": 9003,
>     "error": "Nothing found with query",
>     "source": "slurmdb_users_get"
>   }
> ]

It looks like you and I have different definitions of "first" and "second" patch, but you answered my questions.

> The first patch, one merely needs to query diag URL.
(I was referring to this as the second patch.)

I can query diag URL and it works just fine (with or without any of these patches). No errors. Is there something more I need to reproduce this?

That said, the patch does make sense, but I'd like to reproduce the error.


> The code corrected is part of the generic HTTP handler code. If we want errors
> populated, we will need to change each of the methods to ensure they populate that.
> The errors field I added to give myself a nice easy way to know why something failed
> but HTTP only has that return code.
>
> For instance, the newer db code does actually populate it:

Thanks for the clarification on this. Is there a bug open to add errors to existing methods, or is this just something that we're going to do with just newer code?


Also, I assume that since .37 is the latest version (on 21.08) that we're just fixing .37 and not older versions. Is this correct?

When doing the merge to master, I'll make sure to add the fix to .38 as well.

(In reply to Marshall Garey from comment #9)
> It looks like you and I have different definitions of "first" and "second"
> I can query diag URL and it works just fine (with or without any of these
> patches). No errors. Is there something more I need to reproduce this?

Make sure there are pending jobs and RPCs running in the system.

> > The code corrected is part of the generic HTTP handler code. If we want errors
> > populated, we will need to change each of the methods to ensure they populate that.
> > The errors field I added to give myself a nice easy way to know why something failed
> > but HTTP only has that return code.
> >
> > For instance, the newer db code does actually populate it:
> 
> Thanks for the clarification on this. Is there a bug open to add errors to
> existing methods, or is this just something that we're going to do with just
> newer code?

You are more than welcome to open a ticket. I think it could be a fun project for the new support ppl.

> Also, I assume that since .37 is the latest version (on 21.08) that we're
> just fixing .37 and not older versions. Is this correct?

Yes, generally we only fix the latest stable version (and master) to give sites a nudge to upgrade. There are exceptions like security issues or data corruption bugs.

> When doing the merge to master, I'll make sure to add the fix to .38 as well.

I can provide the patch or you can do the sed if you want. Its usually a clean patch when you replace the version.

Well, I'm not sure whether I reproduced the diag issue (maybe? unless the error I saw was unrelated). But I think the commit is fine.

Thanks for your help on this one! I now know the basics in how to use Slurm's rest API, which is really nice.

This is in ahead of 21.08.6:

commit d8b42db8c51d024d9cd500d6b4345f89965ab33f (HEAD -> slurm-21.08, origin/slurm-21.08)
Author: Nathan Rini <nate@schedmd.com>
Date:   Fri Feb 4 08:40:27 2022 -0700

    openapi/dbv0.0.37 - correct check of list_for_each() output in _dump_stats_user_list
    
    Bug 13255

commit a8dd6ca67962d7804f8f44400d3493c8ebd64cee
Author: Nathan Rini <nate@schedmd.com>
Date:   Wed Feb 2 15:01:30 2022 -0700

    slurmrestd - honor content plugin callback error in _call_handler()
    
    Avoid overriding rc if the serialization is successful.
    
    Bug 13255


One style thing that unfortunately I failed to notice until after I pushed: the commit title for commit d8b42db8c51d024 (fixing the diag issue) is too long. Just make sure that the title is <= 72 characters. My bad for not noticing.


Finally, I opened bug 13391 to track adding more errors to rest API calls.

Closing as resolved/fixed