Conversation
karolz-ms
requested review from
eerhardt,
mitchdenny and
radical
as code owners
| return suffix; | ||
| } | ||
|
|
||
| public async Task DeleteResourcesAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
This is dead code, remnant from early Aspire lifetime. Never worked properly.
| { | ||
| await app.StartAsync(); | ||
| }).DefaultTimeout(); | ||
| }).DefaultTimeout(TestConstants.DefaultOrchestratorTestTimeout); |
There was a problem hiding this comment.
Are extra constants needed? LongTimeoutDuration is 20 seconds locally and 120 seconds in CI. That seams like it is enough. What takes over 20 seconds locally? We could make the long duration longer if needed.
I don't know what is the right thing for Default, Long (and a new bigger value if required?) timeout consts vs having values explicitly for one product area. I worry that someone will go though and place orchestrator timeouts everywhere just because they're writing code in that area vs using the defaults which suit them.
I want a situation where DefaultTimeout() works for 99% of code, and the other 1% can be handled by specifying a long timeout. Keep things simple.
There was a problem hiding this comment.
the right timeout when debugging is none 😄
There was a problem hiding this comment.
There was a problem hiding this comment.
Lets jut increase the default to something high
There was a problem hiding this comment.
The default timeout being high on the server is fine, and the long timeout being high is fine locally and on the server, but when running tests you don't want to wait 60 seconds because something will never happen. It's a productivity sink.
Default timeout should fail fast in 5 seconds to not waste the developers time.
There was a problem hiding this comment.
No it should waste developers time. It should “never” happened locally and when it does there are lots tools you can use (like a debugger)
if you don’t make it high then we won’t be using it in certain classes of tests and it’ll defeat the entire purpose
There was a problem hiding this comment.
The primary goal of this change is to eliminate false negatives when I am verifying a change by running multiple tests from the IDE, via TestExplorer. With the current timeout values, on my daily go-to dev machine (64 GB RAM, SSD drive, not too old) running DistributedApplicationTests results in a handful of tests timing out pretty much on every run. I am wasting a lot of time re-running these failed tests.
Actual test debugging is not an issue with longer timeouts in my experience. When I need a tight inner loop for debugging a particular test, I can just lower/extend individual timeouts as necessary; that scenario happens much less often than "run all test to verify nothing is broken", so I want to optimize for the latter.
There are many reasons why 5 seconds for "short" and 20 seconds for "long" may not be enough for tests that involve DCP. For example, during startup DCP is invoked not 1, but 3 times and does probing for container runtime. If container runtime is "sleeping" that probe may take a while. Anything involving a container is subject to image download/image build delays, and involves retries that DCP does to work-around container orchestrator flakiness. Tests running in parallel affecting each other. Antivirus having to scan new build of Aspire/DCP. Etc. etc.
There was a problem hiding this comment.
No it should waste developers time. It should “never” happened locally and when it does there are lots tools you can use (like a debugger)
if you don’t make it high then we won’t be using it in certain classes of tests and it’ll defeat the entire purpose
Yeah I know that timeouts should never happen when tests are in a good state, but deving code can break things.
When you run tests locally to double check things are good, you want broken tests to fail in 5 seconds with a timeout rather than a minute.
|
@karolz-ms can you remove these in this PR: @joperezr can you review those changes please? I want to make sure we can run without cleaning up to see if things are working well with the latest DCP changes plus these new app model changes. |
| <HelixPreCommand Include="$(_DeleteDockerVolumesCommand)" /> | ||
| <HelixPreCommand Include="docker network prune -f" /> | ||
|
|
||
| <HelixPostCommand Include="docker container ls --all" /> |
There was a problem hiding this comment.
This is useful in situations where the container crashes, to see what is happening on the machine. It is also nice to guarantee somewhat stable machines.
Do we have to delete this?
cc @radical
There was a problem hiding this comment.
While that is true, it's nicer if we can actually find bugs instead of having the CI cover up dcp problems 😄
There was a problem hiding this comment.
Can we at least add back the diagnostics lines? If you don't want the prune/clean calls for now, that's fine.
There was a problem hiding this comment.
@karolz-ms can you add the diagnostic ones back?
|
Since this change went in I'm seeing containers stuck in the starting state and a large set of new tests failing as a result. Lets add the diagnostics back so we can see if things are not being cleaned up properly in some cases. I wonder if some tests are not calling StopAsync properly and things still aren't being cleaned up on a per test basis. |
Sorry @davidfowl just saw this. Changes look good other than @eerhardt comments about it being nice to have the diagnostic lines. |
|
We added them back, well @karolz-ms did 😄 |
|
@karolz-ms Can confirm now that with the diagnostic messages added back, we didn't fully fix the issue with dcp. Containers now don't start on the CI. I'll submit a change that does cleanup on start, that way we can track if there are leaking networks still but tests won't fail. We might need more logs here to understand why this is still happening. cc @dbreshears |
github-actions
Bot
added
the
area-app-model
5 participants
Description
This change takes advantage of the just-introduced DCP capability for API-triggered shutdown. This should significantly reduce resource consumption when running tests. Also removes some dead code and increases timeouts for tests using the app orchestrator which were unrealistically short (the latter applies to development test run only).
Note: if the programmatic shutdown fails for some reason (unlikely), the existing mechanism of DCP watching the AppHost process will still kick in. Exactly the same resource cleanup code is executed in either case.
Fixes # (issue)
#6678
#6561
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):