Mock process memory readings in test_worker.py

[crusaderky]

In scope

• Change all tests in test_worker.py that rely on process memory readings to use a deterministic, robust and fast mock design instead
• Replace fragile timing-based test design with events.
• Supersedes Work around flakyness in spill hysteresis #5850
• Mitigates Memory may not shrink fast enough #5840
• Closes test_spill_hysteresis flaky on ubuntu #5848
• Partially reverts Allow memory monitor to evict data more aggressively #3424

Out of scope

• same treatment to test_scheduler.py::test_memory
• remediation to test_active_memory_manager.py, which should be carried out by adding a switch to use managed memory
• remediation to all tests around rebalance(): postponed indefinitely due to the whole function to be reimplemented on top of the Active Memory Manager

[github-actions]

Unit Test Results

       12 files  +       12         12 suites  +12   7h 43m 4s ⏱️ + 7h 43m 4s
  2 622 tests +  2 622    2 538 ✔️ +  2 538    80 💤 +  80  4 ❌ +4 
15 656 runs  +15 656  14 770 ✔️ +14 770  880 💤 +880  6 ❌ +6 

For more details on these failures, see this check.

Results for commit 3e56328. ± Comparison against base commit fb8484e.

[crusaderky]

All test failures are unrelated. Ready for review and merge.

[fjetter]

I would actually advocate for something close to a fake instead of a hard coded mock for this purpose. I don't think we should call any mock/fakes multiple times in a test to set numbers manually but the fake should be smart enough for it to work it out itself. In a sense, the behaviour we are relying on is

If our data buffer holds N elements of size X, we expect proc.memory_info().rss to be N * X larger than when it does not hold any of these elements.

This is the API we're relying on. We know it is not entirely true since the memory allocator causes fragmentation, buffering, etc. but the above statement is what our algorithm assumes and is what we want to test.

Therefore, I would suggest to introduce a fake like the one below

class Worker:

    # Encapsulate the call in a single function or method. This is the mock/fake target, not the psutil function
    # Technically, we can also just mock `Worker.proc.memory_info()` but by registering this hook we make it
    # explicit that this is part of our software design. Our software, our algorithm relies on this number, not
    # the process memory
    # After all, our algorithm doesn't care about the library or method we use for measuring this
    # it cares about one number. If we ever change the library measuring the proc memory,
    # our algorithm _should_ not change

    def _get_process_memory_metric(self):
        return self.proc.memory_info().rss  # real 

# All tests that are working with the fake will need to return a special type of data that is defining
# a new attribute that is otherwise not used anywhere. I'll call it `rss` but we are free to make
# this less likely to have naming collisions.
# We can also use a different mechanism, depending on what we want to test. For instance, 
# this attribute may just return the sizeof value if we don't care about the distinction. 
# It could return always 50% less, always 50% more, etc. depending on the test

class FakeData:
    rss = 100e6  # or harcode it...


def fake_rss_measure(self):
    # This is one possibility to write this fake. Other implementations might be better / more complete
    # (e.g. using a weakref C._instances which would decouple us from the Buffer)
    # IMPORTANT: Fakes must be tested as well!
    return sum(dat.rss for self.data.fast.values())

def test_foo(c, s, a, b):
    # Just patching the hook is sufficient to install the fake. We can also use mocking utilities
    # but there is not a huge benefit in doing so.
    a._get_process_memory_metric = fake_rss_measure

Test logic should otherwise not be impacted (i.e. not specific calls that "set_rss") other than using these pre-instrumented objects.

Thoughts? My intention was not to set RSS values manually at various places in the test but introduce a fake system. I wouldn't have pushed for this otherwise. I apologies for using improper terminology before. We can also hop on a call and discuss this in person briefly if that helps

[crusaderky]

Superseded by #5878