[BUG] Substantial performance degradation on TCP transport after transport refactor
@dwoz is already working on this.
Since Feb 24, 2022.
- Dominant language
- Python
- Stars
- 15.7k
- Forks
- 5.6k
- Avg merge
- 2d 44m
- Merged PRs (30d)
- 80
Description
Description
The big transport refactor (https://github.com/saltstack/salt/pull/61450) that was meant to decouple some general logic from transport and easy the implementation of new transport methods brought an unwanted performance degradation to TCP transport.
In particular the performance degradation can be seen when new job is published to specific minion (target_lst) instead of broadcast. The previous logic for publishing job was pretty straightforward (https://github.com/saltstack/salt/blob/v3004/salt/transport/tcp.py#L1517 ):
- Check if
topic_lstwas passed (this contains list of all minions the job is meant for) - Loop through all topics (all minions)
- Find appropriate connected clients for the minion (there might be multiple connected for single minion because of reasons)
- Write the job to the socket
While after the refactor the logic is mostly the opposite (https://github.com/saltstack/salt/blob/master/salt/transport/tcp.py#L910):
- Check if
topic_lstwas passed - Loop through all topics
- Loop through all clients
- Check if client matches the topic and if so, write the job to the socket
In practice this means that if there is 10k connected minions and new job should be published to just one single minion, it's immediate (one iteration through topic_lst loop and lookup in dict), while after the refactor the worst case scenario is 10k iterations of the clients loop. If we publish job to multiple minions, this just gets worse. For 5k minions in a job in 10k minion setup, the previous complexity would be mostly equal to number of minions in a job, while now it's number of minions in a job times number of connected clients.
The source of the issue stems from how the clients are looked up and how authentication mechanism works in Salt:
- When minion connects, it's added into
self.clientsset (https://github.com/saltstack/salt/blob/master/salt/transport/tcp.py#L905, https://github.com/saltstack/salt/blob/v3004/salt/transport/tcp.py#L1512) - The stream is read and the data is processed (https://github.com/saltstack/salt/blob/master/salt/transport/tcp.py#L885, https://github.com/saltstack/salt/blob/v3004/salt/transport/tcp.py#L1482)
- The minion is authenticated and added into
presentdict => while in the previous version this dict was present directly in the transport internals, now it's decoupled and part of server channelpresence_callback(https://github.com/saltstack/salt/blob/master/salt/channel/server.py#L701) - When new job is published, the previous logic used to look into
presencedictionary, performing dict lookup, while now it's not available to the TCP transport classes
This also brings additional DDOS-type of issue when one can write simple TCP client that just connects to the master without any extra steps (just hanging the connection) -> the TCP client would be accounted for in the publish loop, instead of relying on the presence dictionary.
Setup
(Please provide relevant configs and/or SLS files (be sure to remove sensitive info. There is no general set-up of Salt.)
Any new Salt Master using TCP transport.
Please be as specific as possible and give set-up details.
- on-prem machine
- VM (Virtualbox, KVM, etc. please specify)
- VM running on a cloud service, please be explicit and add details
- container (Kubernetes, Docker, containerd, etc. please specify)
- or a combination, please be explicit
- jails if it is FreeBSD
Steps to Reproduce the behavior
See above
Expected behavior
Publish of single job on master with 10k minions doesn't take dozens of seconds.
Screenshots
n/a
Versions Report
n/a, latest master branch
Additional context
n/a
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.