Timeout calculation in states/modules/... is error prone
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 15.7k
- Forks
- 5.6k
- Avg merge
- 2d 44m
- Merged PRs (30d)
- 80
Description
Description of Issue
The fact, that SaltStack uses time.time() in many places for calculating timeouts is a recipe for trouble which causes unpredictable behavior in cases of unreliable hardware/system clocks, NTP drift corrections, ... Currently, the timeout is calculated based on the system clock, not on the actual time that passed since the beginning of the timeout.
Setup
The problem became obvious on a Windows Server VM on top of VMware ESX, but this could basically affect any kind of system.
Steps to Reproduce Issue
- Set the time of system to be 10 minutes in the past
- Execute a state/module/... making use of
time.time()for calculating a timeout (e.g.states.loop.until()) - Correct the system time while waiting for the timeout
The timeout will be reached right at the moment, the system clock is updated, not after the actual timeout was reached.
Versions Report
Salt Version:
Salt: 2019.2.2
Dependency Versions:
cffi: 1.12.2
cherrypy: 17.4.1
dateutil: 2.8.0
docker-py: Not Installed
gitdb: 2.0.6
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.10.1
libgit2: Not Installed
libnacl: 1.6.1
M2Crypto: Not Installed
Mako: 1.0.7
msgpack-pure: Not Installed
msgpack-python: 0.5.6
mysql-python: Not Installed
pycparser: 2.19
pycrypto: Not Installed
pycryptodome: 3.8.1
pygit2: Not Installed
Python: 3.5.4 (v3.5.4:3f56838, Aug 8 2017, 02:17:05) [MSC v.1900 64 bit (AMD64)]
python-gnupg: 0.4.4
PyYAML: 3.13
PyZMQ: 18.0.1
RAET: Not Installed
smmap: 2.0.5
timelib: 0.2.4
Tornado: 4.5.3
ZMQ: 4.3.1
System Versions:
dist:
locale: cp1252
machine: AMD64
release: 2008ServerR2
system: Windows
version: 2008ServerR2 6.1.7601 SP1 Multiprocessor Free
Fix
In most cases, fixing this might be quite easy, e.g.:
diff --git a/_states/loop.py b/_states/loop.py
index edaf8c3..b11c877 100644
--- a/_states/loop.py
+++ b/_states/loop.py
@@ -96,11 +96,11 @@ def until(name,
return ret
def timed_out():
- if time.time() >= timeout:
+ if time.monotonic() >= timeout:
return True
return False
- timeout = time.time() + timeout
+ timeout = time.monotonic() + timeout
while not timed_out():
m_ret = __salt__[name](*m_args, **m_kwargs)
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.
Research direction
Start with _states/loop.py and the timeout calculation shown in the issue, then audit the other states/modules/... locations that use time.time() for timeouts. Confirm that affected operations measure elapsed time independently of system-clock adjustments; done means the relevant timeout paths no longer end early or late when the clock changes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- infrastructure
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100