Conversation
Migrate delete, delete-org, and delete-space from aggregated PollJob to a streaming job poll (PollJobToEventStream -> WaitForResult), so warnings print live instead of dumped at the end. Dedupe distinct warnings to at most once per operation.
|
Backport for v8: #3860 |
|
@johha I noticed the integration tests are currently failing. Could you take a look at fixing those while I review the rest of the PR? |
|
|
||
| if waitForCompletion { | ||
| fmt.Fprint(ui.Writer(), "Waiting for the operation to complete") | ||
| fmt.Fprintln(ui.Writer(), "Waiting for the operation to complete") |
There was a problem hiding this comment.
This newline moves the progress dots onto their own line, and WaitForResult is shared by 14 commands — the 3 delete commands added here plus 11 pre-existing service commands (bind-service, unbind-service, bind-route-service, unbind-route-service, create-service, update-service, upgrade-service, delete-service, create-service-key, delete-service-key, cleanup-outdated-service-bindings).
The unit tests were correctly updated for all of them:
-Say(`Waiting for the operation to complete\.\.\.\n`),
+Say(`Waiting for the operation to complete\n\.\.\.`),But the identical assertions in integration/v7/isolated/ were not and this PR touches no files under integration/. These 11 assertions expect dots immediately after complete, which no longer holds:
Worth calling out in the PR description as well: the change alters output for 11 service commands, not only the three delete commands the title describes. Anyone parsing cf bind-service --wait output is affected.
There was a problem hiding this comment.
Fixed. Updated all affected files under integration/v7/isolated/ with the same regex adjustment already applied to the unit tests.
| } | ||
| stream <- PollJobEvent{Warnings: Warnings(deleteWarnings)} | ||
|
|
||
| for event := range actor.PollJobToEventStream(jobURL) { |
There was a problem hiding this comment.
Polling each route's job to completion inside the per-route loop serializes what used to be concurrent. Currently, every DeleteRoute was issued first and the job URLs collected into jobQueue, so Cloud Controller worked on all of them at once; only then were they polled:
for _, route := range routes {
jobURL, ... := DeleteRoute(route.GUID) // all issued up front
jobQueue = append(jobQueue, jobURL)
}
for _, job := range jobQueue {
PollJob(job) // then polled
}Apps with several mapped routes will notice.
Issuing all the DeleteRoute calls first and then streaming each job's events in turn preserves the old concurrency while keeping the streaming behaviour — the events would still arrive in route order, just without blocking the next DELETE on the previous poll.
Minor related note: the route deletions are now side effects of a goroutine that only runs as the consumer drains the channel, so a caller that abandons the stream early leaves later routes undeleted. WaitForResult drains correctly today, so this isn't a live bug, but it's a sharper contract than the old synchronous version and worth a comment on the exported method.
There was a problem hiding this comment.
Good catch. Fixed by restoring the two-phase approach: all DeleteRoute calls are now issued first before any polling starts, so CC can process them in parallel as before.
Update integration/v7/isolated/ banner regex to match new newline-before-dots output. Restore two-phase DeleteRoute: issue all calls first, then poll, so CC can process them concurrently as before.
Thanks for the review @prkalle. The integration tests are skipped now. The CVE job seems to fail for almost all jobs. Also addressed all findings. Also adjust the v8 PR #3860 |
This branch has not been deployed
2 participants
Description of the Change
cf delete,cf delete-org, andcf delete-spacekick off an asynchronous Cloud Controller job and then poll it to completion. Previously the CLI aggregated every warning returned across all poll ticks and dumped them at the very end, after the job reached a terminal state. Because Cloud Controller re-sends the same warnings on every poll, a single warning could be printed dozens of times.This PR migrates those three commands from the aggregated
CloudControllerClient.PollJobpath to a streaming poll: the actor returns achan PollJobEvent(viaPollJobToEventStream) thatcommand/v7/shared.WaitForResultdrains, printing warnings as they arrive instead of at the end.Key changes:
DeleteApplicationByNameAndSpace,DeleteOrganization, andDeleteSpacenow return(chan PollJobEvent, Warnings, error). Forcf delete -r, the app-delete job and each route-delete job are merged into a single stream in the actor, so the whole operation streams through one channel.WaitForResultprints each distinct warning at most once per operation, collapsing the tick-by-tick repeats (including warnings that interleave across ticks).Example:
cf delete-orgagainst a job that returns warnings on every pollBefore (warnings aggregated and repeated per poll tick):
After (each distinct warning shown once, streamed live with progress dots):
cf delete APP -r(delete app + mapped routes) andcf delete-spacebehave the same way - warnings from the app job and each route/space job stream through as the jobs progress.Why Is This PR Valuable?
Users deleting orgs, spaces, or apps get feedback while the operation runs rather than a wall of duplicated text at the end. Warnings that Cloud Controller emits mid-operation (progress hints, content warnings) now surface promptly and legibly - each distinct message once - instead of being buffered until the job finishes or the poll window times out.
This is part of the ongoing effort to make recursive/async delete operations more intuitive (cloudfoundry/cloud_controller_ng#3589). The corresponding Cloud Controller work that emits these async-delete warnings is landing on the server side; this PR is the CLI half that surfaces them well.
Applicable Issues
How Urgent Is The Change?
Not urgent
Other Relevant Parties
None.
This PR was drafted with the help of Claude (Opus).