[Object spilling] Avoid worker crash when an object is spilled right after being restored

[kfstorm]

Why are these changes needed?

When the object store memory pressure is high, e.g. object store is almost full due to pinned objects, an object which is recently restored may be spilled again in a short time. The existing code in CoreWorker::PlasmaCallback involves Contains and Get calls. Object spilling may happen between the two calls. So here we can't use RAY_CHECK_OK on the Get call. Instead, we should fall back to the object-not-local code path.

Related issue number

Closes #15808

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[rkooo567]

Can you verify this works with some custom code manually btw? maybe we can add sleep that takes like 3 seconds -> 2 seconds -> 0 second and see if this works properly.

[rkooo567]

The code itself LGTM

[kfstorm]

@rkooo567 Manual test results:

w/o the fix:
sleep 3s: very likely to crash
sleep 2s: very likely to crash
sleep 1s: very likely to crash
sleep 0s: no crash

w/ the fix:
sleep 3s: no crash
sleep 2s: no crash
sleep 1s: no crash
sleep 0s: no crash

[rkooo567]

@kfstorm just to make sure - did the shuffle eventually finish right? (won't crash means the work was still in progress and it was done, am I correct)? We can test this by decreasing the sleep time for each call (3 -> 2 -> 1 -> 0 second)

[kfstorm]

@rkooo567 Oh, the manual test I mentioned above is to run the Python UT. We didn't test sleeping in our cluster. What I'm sure of is that our job finished successfully after the fix.

We logged CoreWorker::MemoryUsageString in our cluster and this is a typical log when the if condition in PlasmaCallback cannot pass:

num clients with quota: 0
quota map size: 0
pinned quota map size: 0
allocated bytes: 1200002042
allocation limit: 1579372544
pinned bytes: 3600004861
(global lru) capacity: 1579372544
(global lru) used: 0%
(global lru) num objects: 0
(global lru) num evictions: 1790
(global lru) bytes evicted: 158000458336

I guess (global lru) num objects: 0 means that all objects in plasma store are pinned, hence not in LRU, right?

[kfstorm]

PS: We've tested our Mars job in our cluster w/ and w/o the fix several times and verified that the fix is functional.

[rkooo567]

LGTM!

[rkooo567]

The flaky test failure seems to be unrelated.

[ericl]

I'm not certain but I think this PR is causing OSX test_object_spilling failures.

[ericl]

It's odd, since the PR build above looks ok for OSX (though it did flake). But I can't find any other proximate PR besides maybe a1375a9

[kfstorm]

I cannot reproduce the failure on my mac with 061e3fb.

[rkooo567]

Yeah it passed all tests before I merged it. (same for a1375a9). It is probably from some other factors (and the test failure is timeout, so I guess we should split some of them to other tests?)

[rkooo567]

cc @franklsf95

[ericl]

Hmm, it seems reverting this didn't help. It could be I reverted the wrong PR, or there was some other environment change.

[ericl]

(We can probably put it back)

[ericl]

(We can probably put it back)

[rkooo567]

@kfstorm can you create a PR and assign me there?

[kfstorm]

@rkooo567 #16012