[core] Make ReleaseUnusedBundles Fault Tolerant #57786
Conversation
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
|
@Sparks0219 merge conflict, and could you please separate the worker interface refactoring into another PR? |
Sparks0219
force-pushed
the
joshlee/make-release-unused-bundles-fault-tolerant
branch
from
992e1bb to
240ef63
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Sparks0219
force-pushed
the
joshlee/make-release-unused-bundles-fault-tolerant
branch
2 times, most recently
from
f896cd6 to
240ef63
Compare
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
|
|
||
| TEST_F(NodeManagerTest, TestHandleRequestWorkerLeaseInfeasibleIdempotent) { | ||
| auto lease_spec = BuildLeaseSpec({{"CPU", 1}}); | ||
| auto lease_spec = BuildLeaseSpec({{"CPU", 11}}); |
There was a problem hiding this comment.
ah I needed to expand the cpu resources for this test from 0 to some positive number (10) for the bundle test, but then the infeasible lease test failed since 1 cpu is no longer infeasible. I'll change this to a constexpr and not some magic number so it's more clear
| bundle_spec_map_; | ||
|
|
||
| friend bool IsBundleRegistered(const PlacementGroupResourceManager &manager, | ||
| const BundleID &bundle_id); |
There was a problem hiding this comment.
👀
no way around? + is it necessary to assert on this
There was a problem hiding this comment.
mmm I think we need some way of looking into pgmanager state since ReleaseUnusedBundles is calling pgmanager methods and I don't think there's anything I can use for this in the class currently :(
| *local_lease_manager_); | ||
|
|
||
| placement_group_resource_manager_ = | ||
| std::make_unique<NewPlacementGroupResourceManager>(*cluster_resource_scheduler_); |
There was a problem hiding this comment.
can save for the refactor pr, but why is called new 😵💫
| monkeypatch.setenv( | ||
| "RAY_testing_rpc_failure", | ||
| "NodeManagerService.grpc_client.ReleaseUnusedBundles=1:100:0" | ||
| + ",NodeManagerService.grpc_client.CancelResourceReserve=100:100:0", |
There was a problem hiding this comment.
just do -1 if you want to kill it
but I don't love this
So like it's possible to happen without injecting failures on CancelResourceReserve because the gcs could be die before all the Cancel's happen. You should leave a comment describing that otherwise it's p hard to figure out
There was a problem hiding this comment.
Yea exactly, but its inherently flaky so we need to perma block CancelResourceReserve to make it deterministic. I'll leave a comment + put -1 instead to be mroe clear about my intentions
Signed-off-by: joshlee <[email protected]>
4 participants
Description
Making ReleaseUnusedBundles fault tolerant and enabling retries on network failures. Added cpp test to verify idempotency and created a python integration test. Also created a fake worker class since I needed to noop the underlying connection which is used in DestroyWorker and didn't want to modify the mock class.
Related issues
Types of change
Checklist
Does this PR introduce breaking changes?
Testing:
Code Quality:
git commit -s)Documentation:
doc/source/(if applicable)Additional context