Repository navigation
fix(services): report the real outcome of slow service writes - #6018
Open
nabramovitz wants to merge 16 commits into
Open
nabramovitz wants to merge 16 commits into
nabramovitz wants to merge 16 commits into
Conversation
Remove LongRunningCfOperationsService, its base LongRunningOperationsService, ServiceInstanceLastOpComponent and the stray shared.module.ts.backup file. Nothing injects either service and no handleLongRunning* or isLongRunning call exists; they were only listed as providers in the package, test and shared modules and three specs. The last-op component's selector is used in no template, route, module or barrel. writeWithJob now covers long-running service operations.
pollJob gave up on the first poll it could not read: a network blip, a 5xx from a restarting backend or a 429 resolved the write as UNKNOWN while the job was still running, and every caller then reported it as done. A poll that could not be answered says nothing about the job, so it is retried with the same backoff, logged once when polls start failing and once when they recover. Only a 404 (the backend does not know the job) or another 4xx but 429 ends polling, as UNKNOWN: the operation has ended and its result cannot be determined. A 202 without a job id stays UNKNOWN. An optional AbortSignal lets a caller stop waiting; the promise then rejects with an AbortError, which is not an outcome. Refs cloudfoundry#6002
A write that resolved UNKNOWN (the backend no longer knows the job, or a 202 came back without one) was treated as success: deletes dropped the row and ran the favourites and recents cleanup, the summary pages and the app delete navigated away, detach showed a green tick, the create wizard said "created" and skipped the bind, a bind said "created and bound", and a rollback closed as done. The operation has ended, but nobody can say how, so none of that is true. EntityDeleteController now ends such a delete as `unknown`: it marks the entity's own slice and its relation-graph slices stale instead of removing the row, and runs no cleanup hooks. runCfDelete resolves 'deleted' or 'unknown', and every caller keeps the row, stays on the page and says so. The other writes do the same, re-reading what they show. One message helper, unknownOutcomeMessage, words it: the caller's lead-in, "the operation ended, but its result could not be determined", and the place that shows the current state. Bulk deletes count these as results that could not be determined. Refs cloudfoundry#6002
A CF job can complete while the broker records the operation as failed on the resource's last_operation. Only service instance create and update re-read it, and only when the job resolved inside the fast-path window: a handed-off job, a binding, a key or a route binding reported a broker failure as success. The CF job translator now does the check when a job is COMPLETE. It follows the job's resource link (service_instances, service_instance, service_credential_binding, service_route_binding), GETs the resource, and reports the job FAILED with the broker's description when last_operation records the job's own operation as failed. Fast path and polling both resolve through the translator, so one check covers both, and the two per-handler instance checks go. A missing link, a GET error (404 after a delete) or a succeeded or in-progress operation leaves the job COMPLETE, as before. Bindings and keys report cf.service_credential_binding.last_operation .failed, route bindings cf.service_route_binding.last_operation.failed; instances keep cf.service_instance.last_operation.failed.
A job poll that landed on a Jetstream replica that did not create the job (another pod without session affinity, a restart, a redeploy) got 404, so the UI lost the outcome of a CF job that was still running. CF job ids now carry what is needed to poll the job again: "<kind>.<endpoint guid>.<CF job guid>". A replica that does not hold the id decodes it, rebuilds the job for the session user (never a user taken from the id), and polls CF as usual. The user must hold a token for the endpoint; CF answering 404 for the job, like an id that does not decode, is reported as an unknown job. Id minting is an optional translator capability, so the tracker stays generic. Restage and rollback keep random ids: Jetstream drives their state machines itself and that state exists only in the process that started them.
The job poll answered any session that knew a job id, refreshing the job with its creator's token. Random ids made that hard to exploit; ids that now name the endpoint and CF job do not. Translators name the user a ref polls as, the tracker records that user as the job's owner, and a poll from anyone else answers 404, the same as an unknown id. A job whose translator cannot name its owner is served to nobody. A job rebuilt on another replica is owned by the session user it was rebuilt for.
The service instance wizard named the instance from wizard state, which create never fills and edit seeds with the name before any rename. A create reported `Service instance "Service instance" created.`, an unknown outcome `Create service instance ""`, and a renamed instance its old name. Messages now use the name the form sent. Binding an existing instance names that instance and says it was bound, not created. The specs used the same name for wizard state and form, which hid this; they now set state as the wizard does.
answerServiceWrite passed `unknown` bodies to HttpTestingController's flush, which tsc on the library config rejects. The recordings are parsed JSON, so type them as objects or null.
The service instance wizard waited on a write for as long as the broker took, with no ceiling and nothing on screen but a busy button. A broker that never finishes kept the user there indefinitely. After 30 seconds the wizard now closes, says the operation carries on and names where to watch it (the Services list, or the application's Services tab). Tracking continues: the outcome is reported wherever the user is, and a failure or unknown result stays on screen until dismissed. Leaving the wizard early hands the write off the same way; before, a failure that ended after Cancel was never shown.
The tracker evicted a job only after it reached a terminal state, so a job whose polling never got there stayed PROCESSING in memory for good: a job rebuilt on another replica whose only poll hit a transport error, or any job whose browser tab was closed. The tracker could grow without bound. The sweeper that already expires terminal jobs now also drops a non-terminal job once it has gone an hour without a poll. Age counts from the last poll, not from the start, and a poll that hits a transport error still counts, so a long backend operation (brokers can take 25 minutes or more; CF keeps polling a job for up to 7 days) stays tracked for as long as a client polls it. An hour is generous: the frontend polls at least every 3 seconds, and a session expires after 20 idle minutes by default, so a client silent for an hour is gone. The limit is a config field like the terminal TTL, defaulting to a package constant. A later poll of a dropped job answers 404, as for any unknown id. A CF job id is rebuilt from CF on that poll and reports the job's real state. Restage and rollback state machines advance only when polled, so dropping an idle one stops nothing that was running; a client that returns after an hour reads the outcome as unknown.
The x-cap-long-running request header switched a passthrough onto a separate HTTP client timeout (HTTP_CLIENT_TIMEOUT_LONGRUNNING_IN_SECS) and returned a synthetic 202 "still active" response after 30 seconds. It came over with the v2 merge, when the frontend sent it for CF v2 service operations that could block on a broker. The frontend actions that set it were removed with the dead ngrx action classes, and no code sends the header any more. CF v3 writes return 202 with a job, so no proxied write waits on a broker. Remove the header handling from ProxyRequest, ProxySingleRequest and doRequest, the long-running client in GetHttpClientForRequest, the CNSIRequest.LongRunning field, the config field, and the setting from the example, dev and deployment configs. The config loader only looks up the keys its struct declares, so an operator who still sets the environment variable sees it ignored.
Leaving the service instance wizard while a write was still running armed the late-failure report but said nothing. The "carries on in the background" message waited for the 30-second timer and then appeared on whatever page the user had moved to, 15 seconds or more after they left. Leaving the wizard now hands the write off there and then: the message shows as the wizard closes, once, and the timer no longer repeats it.
After the wizard hands a write off, its outcome arrives while the user
is elsewhere, and with several writes running a failure has to say
which one failed. Success already named the instance; failures said
"Failed to create service instance: ..." and "Failed to create service
instance binding: ...".
A failed create or update now names the instance. A failed bind names
the instance too, and when the instance was created first it says so
("was created, but binding it to the application failed"), as the
unknown-result message already did.
When a broker accepts a key or app binding and then fails it, Cloud Foundry fails the job with its own exception name and keeps the broker's description on the binding. Stratos passed the job's error on, so the user read "Failed to create key: create could not be completed: VCAP::CloudController::V3::LastOperationFailedState" (CF API 3.226, broker failing asynchronously). A failed credential or route binding job now re-reads the binding and reports its failed last_operation description, as a completed job already did. When the binding cannot be read, CF's error stays. A failed service instance job is unchanged: its error already carries the broker's description.
Extends the async service outcomes fragment with the wizard naming the instance in failures and bindings reporting the broker's own reason.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A slow or failed service write could be reported as something it was not. The wizard waited on a slow broker with no ceiling. The first poll that failed (a network blip, a backend restart) ended tracking, and the UI then said "created" or "deleted" regardless. A job polled on a replica that had not started it answered 404. A binding or key the broker failed after accepting it was reported as a success. This is the second group of #6002: steps 2, 3, 6 and 7 of its plan, plus what the live runs found.
Polling carries on through transient errors.
pollJobretries a poll it could not read (5xx, 429, network) with the same backoff, and logs once when polls start failing and once when they recover. Only a 404 or another 4xx ends polling, as UNKNOWN.An unknown result is reported as unknown. A write that resolved UNKNOWN used to be treated as success. Deletes dropped the row and ran the favourites and recents cleanup, pages navigated away, and the wizard said "created" and skipped the bind. Now every caller keeps the row, stays on the page and says
<lead-in>: the operation ended, but its result could not be determined. <where to look> shows the current state.Bulk deletes count these separately.The wizard hands off a slow write. After 30 seconds, or at once if the user leaves, the wizard closes, says the operation carries on and names where to watch it (the Services list, or the application's Services tab). Tracking continues, the outcome is reported wherever the user is, and a failure or unknown result stays on screen until dismissed. Messages name the instance by the name the form sent, failures included. A bind that fails after a create says the instance was created.
The backend checks the resource, not just the job. A CF job can complete while the broker records the operation as failed on the resource. The CF job translator now re-reads
last_operationfor service instances, credential bindings (keys and app bindings) and route bindings. Fast path and polling both resolve through it, so the two per-handler instance checks are gone. A failed binding job reports the broker's description instead ofVCAP::CloudController::V3::LastOperationFailedState.Any replica can resolve a job, and only its owner can read it. CF job ids now carry
<kind>.<endpoint guid>.<CF job guid>. A replica that does not hold the id rebuilds the job for the session user (never a user taken from the id) and polls CF. A poll from anyone but the job's owner answers 404, the same as an unknown id. Restage and rollback keep random ids, because their state lives in the process that started them.Idle jobs are dropped. The sweeper now drops a non-terminal job after an hour without a poll. Age counts from the last poll, so a long broker operation stays tracked while someone is watching it.
Dead code is removed. This removes
LongRunningCfOperationsService, its base class andServiceInstanceLastOpComponent. It also removes the proxy'sx-cap-long-runningpath and theHTTP_CLIENT_TIMEOUT_LONGRUNNING_IN_SECSsetting from the shipped configs and Helm chart; nothing has sent the header since the v2 actions went. A deployment that still sets the variable is unaffected, because the value is ignored.Tried:
make check gatepasses (lint, 3869 unit tests, Go tests, production build warnings). The baseline scenarios L1–L10 were run on develop and again on this branch, against a CF (API 3.226) with overview-broker:Failed to create key: Operation failedx-cap-long-runningheaderNot in this PR: a page reload during a write still loses tracking.
Refs #6002.
Changelog fragment:
changelog.d/0009-async-service-outcomes.md.AI disclosure (RFC-0047): prepared with AI coding assistance; reviewed by the author.