cloud_controller_ng icon indicating copy to clipboard operation
cloud_controller_ng copied to clipboard

V3 process scaling contradicts V3 documentation

Open christo-3 opened this issue 3 years ago • 3 comments

Thanks for submitting an issue to cloud_controller_ng. We are always trying to improve! To help us, please fill out the following template.

Issue

(There may be other places this happens)

Either Documentation for V3 async opertations is incorrect or POST v3/processes/:guid/actions/scale is not functioning as intended

Documentation says "Instead, endpoints that require asynchronous processing will return 202 Accepted with a Location header pointing to the job resource to poll."

But scaling an app using v3/processes/:guid/actions/scale returns a 202 Accepted with a body and NO Location header.

Context

REQUEST: [2021-08-13T10:50:26-07:00]
POST /v3/apps/a5f34799-4326-48a5-927e-b048d1cb6f2e/processes/web/actions/scale HTTP/1.1
Host: api.domain.com
Accept: application/json
Authorization: [PRIVATE DATA HIDDEN]
Content-Type: application/json
User-Agent: cf7.exe/7.2.0+be4a5ce2b.2020-12-10 (go1.13.15; amd64 windows)
{​​​​​
  "instances": 4
}​​​​​

RESPONSE: [2021-08-13T10:50:26-07:00]
HTTP/1.1 202 Accepted
Content-Type: application/json; charset=utf-8
Date: Fri, 13 Aug 2021 17:50:27 GMT
Referrer-Policy: strict-origin-when-cross-origin
Server: nginx
X-B3-Spanid: f6b8454df3e2c2a5
X-B3-Traceid: f6b8454df3e2c2a5
X-Content-Type-Options: nosniff
X-Download-Options: noopen
X-Frame-Options: SAMEORIGIN
X-Permitted-Cross-Domain-Policies: none
X-Runtime: 0.189949
X-Vcap-Request-Id: 474b893c-cb0d-4039-415f-04b71b8f6876::c6c953ad-2c2e-44af-b9e7-cc06fab46559
X-Xss-Protection: 1; mode=block
{​​​​​
  "command": "$HOME/.bp/bin/start",
  "created_at": "2021-08-02T18:08:59Z",
  "disk_in_mb": 512,
  "guid": "a5f34799-4326-48a5-927e-b048d1cb6f2e",
  "health_check": {​​​​​
    "data": {​​​​​
      "invocation_timeout": null,
      "timeout": null
    }​​​​​,
    "type": "port"
  }​​​​​,
...
}​​​​​

christo-3 avatar Aug 13 '21 18:08 christo-3

@christo-3 is there a use case you have where you would like to see a Location header for the scale action? Or would it be sufficient to correct the docs? @ctlong since you opened a similar issue, did you have any thoughts about this?

I'm not seeing anything in the HTTP specs that require the location header for 202 Accepted responses, it just seems to be something we provide (in fact, it seems our use of the Location header for 202 response is slightly unconventional). My preference would be to note this endpoint being an exception in the docs, unless we have a good reason to do otherwise.

sethboyles avatar Dec 08 '21 22:12 sethboyles

I've no good reasons to do more than a doc update 👍 Although my preference would be to find some clever way to setup a job for this request, I've no good reasons to go that far. In a perfect world client procedures wouldn't have to be altered specifically for this endpoint.

Extra thoughts 🤔 IIRC the problem is that this endpoint is asynchronous, because it requires Diego to read the new desired state and change the actual state of the process, but it doesn't use a job because Diego will just handle it. Potentially terrible idea: return a job that intermittently polls Diego and succeeds when the actual state reflects the desired state.

ctlong avatar Dec 08 '21 23:12 ctlong

I believe the scale endpoint predates the Location header pattern we adopted, which may be why it uses 202 differently. I am hesitant to change it to return 200 since that is potentially a breaking change for clients.

I see the value in making all 202 responses consistent so that clients don't have to work with exceptions. I'm not sure about creating a job that doesn't actually do any work--to me that feels a little like faking it. There are also some edge cases to think about. For example, if there is an issue where the app cannot be scaled up (too much memory requested, for instance), I believe the app instance just ends up thrashing. In cases like those, we might end up with never-ending jobs.

That said, I am in favor of making a doc change for now and seeing if anyone has a compelling use case to why this should be changed.

sethboyles avatar Dec 09 '21 00:12 sethboyles