Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change status query into a information message when finishing a rolling restage - main #2666

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

ccjaimes
Copy link
Contributor

This change will prevent the issue reported in Jira's ticket CLI-50 which creates a race condition when trying to query the app's rolling restage status at the same moment the old restage group gets deleted.

Link for v8: #2664
Link for v7: #2665

Copy link
Contributor Author

@ccjaimes ccjaimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self review, no critical change in logic, LGTM

Copy link
Member

@a-b a-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks ok, please add tests. Here is an example Expect(testUI.Out).To(Say("Staging app and tracing logs..."))

@pivotalgeorge
Copy link
Contributor

as this change was made to resolve a tricky-to-reproduce issue, going to document the setup here:

  • used a TAS 4.0 testing environment, set up such that the compute bosh job (which hosts garden and therefore app instances) was on a GCP VM with 32GB memory, 131GB root disk, and 2vCPUs
  • used a version of the cf-cli built off the v8 branch - specifically c7d46b6
  • 10 sample apps, each using the dora code but named dora-1, dora-2, etc.
  • 3-at-a-time restaging of those apps, i.e. cf restage "dora-$i" --strategy rolling --no-wait & for i := {1,2,3}, followed by the next 3, etc.
  • observed Process not found FAILED, etc.
  • rotating too many (all 10) apps at a time also resulted in Process not found, but this was a red herring as it was caused by InsufficientResources (out of memory)
  • observed that the same testing procedure did not result in an error using a version of the binary built from this PR (d444bc1)

@ccjaimes ccjaimes merged commit de13c02 into main Nov 20, 2023
12 checks passed
@moleske moleske deleted the main-CLI50 branch December 15, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants