Skip tracking clients OOM test when I/O threads are enabled

[uriyage]

Fix feedback loop in key eviction with tracking clients when using I/O threads.

This addresses the "client tracking don't cause eviction feedback loop" test failure.

Current issue:
Evicting keys while tracking clients or key space-notification exist creates a feedback loop when using I/O threads:

While evicting keys we send tracking async writes to I/O threads, preventing immediate release of tracking clients' COB memory consumption.

Before the I/O thread finishes its write, we recheck used_memory, which now includes the tracking clients' COB and thus continue to evict more keys.

Fix:
We will skip the test for now while IO threads are active. We may consider avoiding sending writes in processPendingWrites to I/O threads for tracking clients when we are out of memory.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.52%. Comparing base (002d052) to head (9e963ee).
Report is 1 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #764      +/-   ##
============================================
- Coverage     70.56%   70.52%   -0.04%     
============================================
  Files           112      112              
  Lines         61509    61509              
============================================
- Hits          43403    43382      -21     
- Misses        18106    18127      +21     

see 16 files with indirect coverage changes

[ranshid]

I would also change the top comment to indicate this is not only fixing the test. the issue is probably also related to keyspace notifications and pubsub.

[uriyage]

I would also change the top comment to indicate this is not only fixing the test. the issue is probably also related to keyspace notifications and pubsub.

I've updated the comment to include keyspace notifications. I'm unsure about pub/sub clients, as deleting keys won't create a feedback loop for them.

[ranshid]

@uriyage the fix as written now LGTM. However I still have concerns that we are mostly aiming to solve an issue which is related to the tests and not very realistic scenario. I understand that when we write to io-threads the eviction cycles MIGHT be longer but I also think that avoid doing so might also cause issues for example when large pipelines are used which will keep us above the maxmemory longer. I think that maybe we can keep an open issue to investigate this farther and for the time being just avoid async-io in this specific test?
@zuiderkwast WDYT?

[zuiderkwast]

I think it makes sense. Eviction feedback loop sounds like something that should be avoided. :)

@uriyage the fix as written now LGTM. However I still have concerns that we are mostly aiming to solve an issue which is related to the tests and not very realistic scenario. I understand that when we write to io-threads the eviction cycles MIGHT be longer but I also think that avoid doing so might also cause issues for example when large pipelines are used which will keep us above the maxmemory longer. I think that maybe we can keep an open issue to investigate this farther and for the time being just avoid async-io in this specific test?
@zuiderkwast WDYT?

I'm not completely aware of the impact and the affected scenarios. What are the worst cases and what are the most common scenario where this can happen?

Does it happen in a normal cache workload where LRU eviction is used?

Do we free just a few more keys than we should, or do the feedback loop make us free almost the whole database in the worst case?

[ranshid]

I think it makes sense. Eviction feedback loop sounds like something that should be avoided. :)

@uriyage the fix as written now LGTM. However I still have concerns that we are mostly aiming to solve an issue which is related to the tests and not very realistic scenario. I understand that when we write to io-threads the eviction cycles MIGHT be longer but I also think that avoid doing so might also cause issues for example when large pipelines are used which will keep us above the maxmemory longer. I think that maybe we can keep an open issue to investigate this farther and for the time being just avoid async-io in this specific test?
@zuiderkwast WDYT?

I'm not completely aware of the impact and the affected scenarios. What are the worst cases and what are the most common scenario where this can happen?

Does it happen in a normal cache workload where LRU eviction is used?

Do we free just a few more keys than we should, or do the feedback loop make us free almost the whole database in the worst case?

I was not saying that eviction feedback loop is not something which we would like to avoid. I was referring to the condition under check here. We are avoiding sending writes to io-threads when maxmemory is hit. Is this always the best decision? for example imagine case were clients are heavily pipelining. in case we take longer to get to reading from the sockets we might read much more data from clients each time which will also cause higher memory consumption on CIB (and potentially also on COB) right? I see pipelining much more common scenario than keyspace-notifications and client side caching (maybe I am wrong here) so I raised a concern. Maybe we can do this only in case we have many eviction events listeners?
Maybe we can also either fix or NOT fix this now and consider it later?

[zuiderkwast]

I was referring to the condition under check here. We are avoiding sending writes to io-threads when maxmemory is hit. Is this always the best decision? for example imagine case were clients are heavily pipelining. in case we take longer to get to reading from the sockets we might read much more data from clients each time which will also cause higher memory consumption on CIB (and potentially also on COB) right? I see pipelining much more common scenario than keyspace-notifications and client side caching (maybe I am wrong here) so I raised a concern. Maybe we can do this only in case we have many eviction events listeners?
Maybe we can also either fix or NOT fix this now and consider it later?

@ranshid Yeah, as I said, I'm not familiar enough with the impact of this. But yeah, pipelining is for sure more common than client-side caching and keyspace notifications.

Avoiding the I/O threads for all clients whenever we reach maxmemory sounds like we're pulling the breaks too hard. Can't we skip the I/O threads just for clients with tracking, c->flag.tracking? (and maybe some more condition, pubsub?)

@madolson thanks for stepping in! I believe you can make a better decision than I can. :)

[madolson]

For now I'm inclined to say let's wait and see how it get's used. There will likely be large swings in memory usage if you are using tracking, but they should stabilize once the IO threads offload the work. I'm guessing the specific example in the test likely has too few keys to actually let it stabilize. We haven't seen wide adoption of client side tracking and generally there aren't a lot of clients that are connected listening for keyspace notifications on "all" keys.

I would almost rather investigate ways to dampen the effects of the evictions, either by excluding some of the memory in the io threads output buffers or having some generic buffer that can be used, instead of stoping the usage of the IO threads. We don't want performance to suddenly drop either.

[ranshid]

For now I'm inclined to say let's wait and see how it get's used. There will likely be large swings in memory usage if you are using tracking, but they should stabilize once the IO threads offload the work. I'm guessing the specific example in the test likely has too few keys to actually let it stabilize. We haven't seen wide adoption of client side tracking and generally there aren't a lot of clients that are connected listening for keyspace notifications on "all" keys.

I would almost rather investigate ways to dampen the effects of the evictions, either by excluding some of the memory in the io threads output buffers or having some generic buffer that can be used, instead of stoping the usage of the IO threads. We don't want performance to suddenly drop either.

Couldn't agree more :)

[ranshid]

@uriyage I suggest for now let's disable/skip iothreads offload for this specific test

[uriyage]

Skipped the test with IO threads as suggested. Also corrected the incorrect condition for IO threads in two tests.

[ranshid]

Thanks @uriyage in general I would like it if we introduce a tag "io-theads::skip" and validate in tags_acceptable but that can be introduced later.
Can you please just print some indication that the test was skipped?

[uriyage]

Thanks @uriyage in general I would like it if we introduce a tag "io-theads::skip" and validate in tags_acceptable but that can be introduced later. Can you please just print some indication that the test was skipped?

Thank you, @ranshid. I've changed it to use the tag as you suggested. It looks better now, and we get the prints. For example:
[ignore]: stats: eventloop metrics: Not supported in io-threads mode

[ranshid]

LGTM