saltstack / saltstack/salt

[BUG] Substantial performance degradation on TCP transport after transport refactor

Open
#61,656 2 comments 0 reactions 2 assignees View on GitHub

@dwoz is already working on this.

Since Feb 24, 2022.

bug Phosphorus v3005.0
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 ):

  1. Check if topic_lst was passed (this contains list of all minions the job is meant for)
  2. Loop through all topics (all minions)
  3. Find appropriate connected clients for the minion (there might be multiple connected for single minion because of reasons)
  4. 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):

  1. Check if topic_lst was passed
  2. Loop through all topics
  3. Loop through all clients
  4. 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:

  1. When minion connects, it's added into self.clients set (https://github.com/saltstack/salt/blob/master/salt/transport/tcp.py#L905, https://github.com/saltstack/salt/blob/v3004/salt/transport/tcp.py#L1512)
  2. 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)
  3. The minion is authenticated and added into present dict => while in the previous version this dict was present directly in the transport internals, now it's decoupled and part of server channel presence_callback (https://github.com/saltstack/salt/blob/master/salt/channel/server.py#L701)
  4. When new job is published, the previous logic used to look into presence dictionary, 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.