SchedMD - Slurm Support – Full Text Ticket Listing
- Home
- | New
- | Browse
- | Search
- | [?]
- | Reports
- | Help
- | New Account
- | Log In
- | Forgot Password
(In reply to Andrew Bruno from comment #0) > we've successfully generated clients in both Python and > Go and were able to successfully consume the payloads from slurmrestd. Which SDK did you use to generate the clients? (In reply to Nate Rini from comment #1) > (In reply to Andrew Bruno from comment #0) > > we've successfully generated clients in both Python and > > Go and were able to successfully consume the payloads from slurmrestd. > > Which SDK did you use to generate the clients? We're using openapi-generator as it appears to be the most feature complete. This is the fork of Swagger Codegen. https://github.com/OpenAPITools/openapi-generator We are discussing internally how we want to proceed. Thanks for the patch and trying slurmrestd. (In reply to Nate Rini from comment #3) > We are discussing internally how we want to proceed. Thanks for the patch > and trying slurmrestd. Sounds good, thanks! Let us know if you need anything from us. I'm happy to break this patch up into smaller bits if needed. Bruno, Looking over the commit: > - Fix wrong types in the job_properties schema component. The following > properties had the wrong type: > > deadline = string -> integer > nodes = string -> integer > priority = string -> integer These are correct as strings since there are multiple possible string values but integers are also accepted. --Nate (In reply to Nate Rini from comment #14) > Looking over the commit: > > > - Fix wrong types in the job_properties schema component. The following > > properties had the wrong type: > > > > deadline = string -> integer > > nodes = string -> integer > > priority = string -> integer > > These are correct as strings since there are multiple possible string values > but integers are also accepted. > Ah ok, sounds good. I had only seen integers for these properties so assumed they could safely be integers. Would you like me to submit a new patch with these changed back to strings? (In reply to Andrew Bruno from comment #15) > Ah ok, sounds good. I had only seen integers for these properties so assumed > they could safely be integers. Most of the code is silently compatible with the formats provided in the man pages for the equiv cli calls. I rather point users towards the cleaner (more JSON structured) requests. > Would you like me to submit a new patch with > these changed back to strings? No need as we are making major changes and incorporating these changes as we go. (In reply to Andrew Bruno from comment #0) > 1. openapi.json fixes JSON should be valid now in all cases. > - Add start to better versioning support. After a good bit of thought, we decided to increment the version to v0.0.36 instead of modifying v0.0.35 following our strict backwards compatibility requirements. The existing v0.0.35 code has been pluginified and duplicated to v0.0.36 to allow the changes suggested. This will ensure that we do not break any existing implementations until we decide to deprecate v0.0.35. As an add bonus, sites can choose which standard they support or to add their own plugins if they so wish. > In order to support this, our patch makes some minor modifications to > registering path tags and binding operations. This removed the need to hard > code "/slurm/v0.0.35/" in each ops/*.c operation file. It should be noted, > these changes do not break backwards compatibility with the Slurm REST API > in it's current form. All paths in v0.0.36 are now using a server base path. > - We propose to remove the use of "oneOf" and just make them string types. > The use of "oneOf" makes it difficult to implement client SDKs in strongly > typed languages. "oneOf" is currently being used in the following > job_properties: exclusive and nodes. It does not seem like oneOf is that > useful here and using a string type will be much easier for clients to > consume. It's also used in the "signal" schema type, and again using a > string will allow for broader support among client SDKs. oneOf has been removed. > - Add "operationId" to all Operation objects. In it's current form, > generating a client SDK results in method names like GetSlurmV0035Jobs. The > operationId suggests more readable method names. operationId has been added to everything but the OAS retrieval paths per the spec. > - Add content response objects describing the various components returned > from GET requests. These are completely missing and describe the response > payload returned from the operation. Our patch adds schema components for: > error, diag statistics, pings, nodes, and partitions. The descriptions for > many component properties are not yet fleshed out. These have been added and we plan to also fully spec out the schemas before 20.11 release. > - The data payload returned from GET /ping is currently a dictionary keyed > by hostname. Our patch changes this to return an array of pings making it > easier for clients to consume. For example, currently the endpoint returns: Change applied. > - Same for the data payload returned from GET /nodes and /partitions. > Changing this to return an array instead of a dictionary. This makes it much > easier to describe schema components in the OpenAPI spec as well as making > it easier for clients to consume instead of a dictionary keyed by > hostname/partition. For example, the result from GET /nodes now looks like > this: Change applied. > - Changed time properties to return -1 instead of INFINITE. Various > properties return by the API currently return the string "INFINITE" or an > integer. To be more consistent and help support client SDKs in static > languages, these properties could been changed to "integer" types in the > OpenAPI spec and will return -1 for INFINITE instead of a string. Change applied. > All of the above proposed issues/changes were found when attempting to > generate a client SDK from the current openapi.json spec file. After > applying our patch, we've successfully generated clients in both Python and > Go and were able to successfully consume the payloads from slurmrestd. These > issues would be great to get fixed as it should promote easier adoption of > the Slurm REST API. Please give the v0.0.36 a try and see if we missed/broke anything. I'm going to close this ticket but your response will automatically reopen it and we can continue from there. Thanks, --Nate (In reply to Nate Rini from comment #19) > > Please give the v0.0.36 a try and see if we missed/broke anything. I'm going > to close this ticket but your response will automatically reopen it and we > can continue from there. > Hi Nate, Apologies for the late reply. Finally had some time to test out slurmrestd some more. All looks great. I've been doing some further testing against the latest v0.0.37 and found a few minor issues. 1. preemption_mode in openapi.json spec should be an array not a string. I was getting an error in the generated client and changing this to an array of strings fixed the issue. 2. Most dates in openapi.json spec appear to be strings. This makes it quite painful for clients to do any type of date math on these properties. AFAICT, these are all unix time stamp integers so would be helpful if the spec as adjusted accordingly. 3. Finally, the nodes endpoint was missing some critical values that we (University at Buffalo) care about, namely: partitions, tres_used, alloc_memory, alloc_cpus, and idle_cpus. Attached is a patch that fixes the above 3 issues. Let me know if you'd like any changes made. Would be great to get this merged so we can start leveraging slurmrestd more in production. Thanks again for all the great work. --Andrew Created attachment 20031 [details]
patch to fix minor slurmrestd issues in v0.0.37
Andrew These patches are now upstream and will be included with 21.08 release. Please give them a try with the master branch. Also, for future (non-bugfix) patches please open a new ticket. Thanks, --Nate |
Created attachment 14454 [details] slurmrestd OpenAPI spec bug fix and api patch We (University at Buffalo) recently upgraded to v20.02.3 and are very interested in making use of the new Slurm REST API. After playing around a bit we found it's not very usable in it's current form and found several areas where it can be improved. This bug report provides some feedback and describes issues we found in the openapi.json spec along with suggestions for minor tweaks to the REST API. These changes are geared towards making the REST API more usable for client applications. We also provide a patch (attached) which implements most of these proposed changes. Would love any feedback and happy to make any changes/modifications to help get this accepted upstream. The following summarizes the proposed changes and provides some background and motivation (mostly a copy of what's described in the patch). For clarity, it's broken into two sections: 1. openapi.json fixes and 2. REST API changes: 1. openapi.json fixes The openapi.json file contains the OpenAPI Specification (OAS) for the Slurm REST API. In it's current form, it's not valid JSON and missing many OpenAPI objects required for useful documentation and client SDK generation. A few of the issues we found were: - It's not valid. The openapi.json document includes erroneous tabs/commas making it an invalid JSON document: $ openapi-spec-validator openapi.json while scanning for the next token found character '\t' that cannot start any token in "<urllib response>", line 303, column 1 - We propose to remove the use of "oneOf" and just make them string types. The use of "oneOf" makes it difficult to implement client SDKs in strongly typed languages. "oneOf" is currently being used in the following job_properties: exclusive and nodes. It does not seem like oneOf is that useful here and using a string type will be much easier for clients to consume. It's also used in the "signal" schema type, and again using a string will allow for broader support among client SDKs. - Add "operationId" to all Operation objects. In it's current form, generating a client SDK results in method names like GetSlurmV0035Jobs. The operationId suggests more readable method names. - Add content response objects describing the various components returned from GET requests. These are completely missing and describe the response payload returned from the operation. Our patch adds schema components for: error, diag statistics, pings, nodes, and partitions. The descriptions for many component properties are not yet fleshed out. - Add Servers Object URL. According to the OpenAPI spec, the paths should be "A relative path to an individual endpoint" and "The path is appended to the expanded URL from the Server Object's url field in order to construct the full URL". Currently, each path includes the version like "/slurm/v0.0.35/jobs" which seems less than ideal. Our patch adds a server object URL of "/slurm/v0.0.35" and each path is changes to it's relative form "/jobs", "/nodes", etc. which feels much cleaner. 2. REST API changes - The data payload returned from GET /ping is currently a dictionary keyed by hostname. Our patch changes this to return an array of pings making it easier for clients to consume. For example, currently the endpoint returns: { "errors": [], "ping": { "mode": "primary", "ping": "UP", "srv-m14-27": {}, "status": 0 } } This was changed to: { "errors": [ ], "pings": [ { "hostname": "srv-m14-27", "ping": "UP", "status": 0, "mode": "primary" } ] } - Same for the data payload returned from GET /nodes and /partitions. Changing this to return an array instead of a dictionary. This makes it much easier to describe schema components in the OpenAPI spec as well as making it easier for clients to consume instead of a dictionary keyed by hostname/partition. For example, the result from GET /nodes now looks like this: { "errors": [ ], "nodes": [ { "architecture": "x86_64", "name": "cpn-d07-04-01", "hostname": "cpn-d07-04-01", "state": "allocated", "slurmd_version": "20.02.3" }, ] } - Changed time properties to return -1 instead of INFINITE. Various properties return by the API currently return the string "INFINITE" or an integer. To be more consistent and help support client SDKs in static languages, these properties could been changed to "integer" types in the OpenAPI spec and will return -1 for INFINITE instead of a string. - Add start to better versioning support. Not sure what the overall plan is for versioning the Slurm REST API. But currently, it seems hard coding "/slurm/v0.0.35/" into each endpoint will be difficult to maintain. In the previous section, we describe the change made to the openapi.json file which adds the Server Object URL (which used to be called "basePath" in previous versions of the OpenAPI spec). This feels like a better approach to handle versioning. In order to support this, our patch makes some minor modifications to registering path tags and binding operations. This removed the need to hard code "/slurm/v0.0.35/" in each ops/*.c operation file. It should be noted, these changes do not break backwards compatibility with the Slurm REST API in it's current form. All of the above proposed issues/changes were found when attempting to generate a client SDK from the current openapi.json spec file. After applying our patch, we've successfully generated clients in both Python and Go and were able to successfully consume the payloads from slurmrestd. These issues would be great to get fixed as it should promote easier adoption of the Slurm REST API.