getHostStatus needs thread local storage

[c-taylor]

As parents of a topology are marked down under load, 'HostStatus::getHostStatus' can cause excessive lock behaviour resulting in high system time, reduced output and stats holes.

When performing failure testing: Overloading configured parents causes lock contention on the stats storage.
It was possible to consume almost all ET_NET thread time with a few failing parents and fewer than 5,000 RPS.

Fault replication

Increase load through an edge -> parent configuration until the parents start to fail.
I used connection limits as the failure trigger as it was predictable to fail.

Observations

As parents fail there is an increase in 'HostStatus::getHostStatus' contention, especially when the last parent fails.
This causes a reduction in all 'good' work, errors to clients, content already in cache.

1. perf traces and flame graphs show near 100% system consumption on lock activity.

2. traffic_server metrics stop updating
3. Response and data rates drop

[jrushford]

@c-taylor Was this on master or which release are you testing?

[c-taylor]

8.1.1 in this case.

[jrushford]

@c-taylor ok. I've got some ideas about this, I'll dig into it. I'll try to replicate this, you lowered the connection limits on your parents to trigger it? Also, you're using consistent hashing in your parent.config?

[vmamidi]

HostStatus structures aren't currently optimized. It was a planned task in the project
https://github.com/apache/trafficserver/projects/7

[c-taylor]

@jrushford , consistent hash yes

I’m using a load generator with 3 request types.

ControlA: Cache hits at about 3200 RPS
ControlB: Cache miss at about 800 RPS
TestA: Constant arrival rate requests to a URL that sleeps on the backend.

This builds a head of connections until they time out...

I suspect there are other methods that would also work...

[jrushford]

@c-taylor I submitted PR #7377 that I think will help. It removes a couple of redundant calls to getHostStatus() in ParentConsistentHash::selectParent(). Also, when getHostStatus() is called, it checks to see if the internal unordered_map that it uses is empty or not. If empty, ie no parents marked down with traffic_ctl` there is no need to lock and search the map so, it returns right away with the default HOST_STATUS_UP.

I'm sure you'll see improvement when there are no hosts marked down with 'traffic_ctl host down' but, when you mark at least one down with 'traffic_ctl' the map will then be locked and searched. I think in this case the removal of the two redundant calls will be helpful. I'd appreciate it if you could retest with this patch and let me know wether it's helped.

[c-taylor]

@jrushford Thank you.

It will take a few days to re-factor to 9.x and re-test, so please bear with me.

My suspicion is that general running will improve but there may still be some contention under failure modes like that which i trigger in my reproduction case. It looks as though there might be reduced overall lock activity, which may bring this to an acceptable level and reduce the secondary impact: Test will show this (just thinking out loud).

With regards to reading metrics back as part of monitoring (or at all): Is there some room for not requiring a read lock at all? I would have thought that the table update for a single host entry would be atomic, so the need for read locks would be low/none? Additionally dirty reads in this case would appear to have low/acceptable risk.

[jrushford]

@c-taylor I'm continuing today to replicate your observations and will keep you posted. I was wondering since the PR patch is minor, perhaps you could cherry-pick it on top of the 8.1 build your were originally testing with to eliminate any other changes in 9 that might affect the behavior. Also, I think that I do need the read locks on the unordered map as an insert might invalidate a read iterator and cause a crash.

[c-taylor]

Also, I think that I do need the read locks on the unordered map as an insert might invalidate a read iterator and cause a crash.

Thank you for this. I've been being educated by @vmamidi off issue.

[c-taylor]

@jrushford I tested this today:
I think general running improves, but I am still able to trigger the condition that makes ATS near unresponsive for what should be unrelated traffic.

I do get further into the test pattern, more time under stress, more connections, but the outcome is the same.