Merge branch 'fix-negative-running-tasks' into 'master'

Fix negative running tasks

See merge request allianceauth/allianceauth!1524
This commit is contained in:
Ariel Rin 2023-08-05 09:17:42 +00:00
commit c58ed53369
5 changed files with 232 additions and 149 deletions

View File

@ -5,59 +5,27 @@ import logging
from typing import List, Optional
from pytz import utc
from redis import Redis, RedisError
from redis import Redis
from allianceauth.utils.cache import get_redis_client
from .helpers import get_redis_client_or_stub
logger = logging.getLogger(__name__)
class _RedisStub:
"""Stub of a Redis client.
It's purpose is to prevent EventSeries objects from trying to access Redis
when it is not available. e.g. when the Sphinx docs are rendered by readthedocs.org.
"""
def delete(self, *args, **kwargs):
pass
def incr(self, *args, **kwargs):
return 0
def zadd(self, *args, **kwargs):
pass
def zcount(self, *args, **kwargs):
pass
def zrangebyscore(self, *args, **kwargs):
pass
class EventSeries:
"""API for recording and analyzing a series of events."""
_ROOT_KEY = "ALLIANCEAUTH_EVENT_SERIES"
def __init__(self, key_id: str, redis: Redis = None) -> None:
self._redis = get_redis_client() if not redis else redis
try:
if not self._redis.ping():
raise RuntimeError()
except (AttributeError, RedisError, RuntimeError):
logger.exception(
"Failed to establish a connection with Redis. "
"This EventSeries object is disabled.",
)
self._redis = _RedisStub()
def __init__(self, key_id: str, redis: Optional[Redis] = None) -> None:
self._redis = get_redis_client_or_stub() if not redis else redis
self._key_id = str(key_id)
self.clear()
@property
def is_disabled(self):
"""True when this object is disabled, e.g. Redis was not available at startup."""
return isinstance(self._redis, _RedisStub)
return hasattr(self._redis, "IS_STUB")
@property
def _key_counter(self):
@ -97,7 +65,7 @@ class EventSeries:
self._redis.delete(self._key_counter)
def count(self, earliest: dt.datetime = None, latest: dt.datetime = None) -> int:
"""Count of events, can be restricted to given timeframe.
"""Count of events, can be restricted to given time frame.
Args:
- earliest: Date of first events to count(inclusive), or -infinite if not specified

View File

@ -1,21 +1,63 @@
"""Helpers for Task Statistics."""
import logging
from typing import Optional
from redis import Redis, RedisError
from django.core.cache import cache
from allianceauth.utils.cache import get_redis_client
logger = logging.getLogger(__name__)
class _RedisStub:
"""Stub of a Redis client.
It's purpose is to prevent EventSeries objects from trying to access Redis
when it is not available. e.g. when the Sphinx docs are rendered by readthedocs.org.
"""
IS_STUB = True
def delete(self, *args, **kwargs):
pass
def incr(self, *args, **kwargs):
return 0
def zadd(self, *args, **kwargs):
pass
def zcount(self, *args, **kwargs):
pass
def zrangebyscore(self, *args, **kwargs):
pass
class ItemCounter:
"""A process safe item counter."""
"""A process safe item counter.
Args:
- name: Unique name for the counter
- minimum: Counter can not go below the minimum, when set
- redis: A Redis client. Will use AA's cache client by default
"""
CACHE_KEY_BASE = "allianceauth-item-counter"
DEFAULT_CACHE_TIMEOUT = 24 * 3600
def __init__(self, name: str) -> None:
def __init__(
self, name: str, minimum: Optional[int] = None, redis: Optional[Redis] = None
) -> None:
if not name:
raise ValueError("Must define a name")
self._name = str(name)
self._minimum = minimum
self._redis = get_redis_client_or_stub() if not redis else redis
@property
def _cache_key(self) -> str:
@ -23,7 +65,11 @@ class ItemCounter:
def reset(self, init_value: int = 0):
"""Reset counter to initial value."""
cache.set(self._cache_key, init_value, self.DEFAULT_CACHE_TIMEOUT)
with self._redis.lock(f"{self.CACHE_KEY_BASE}-reset"):
if self._minimum is not None and init_value < self._minimum:
raise ValueError("Can not reset below minimum")
cache.set(self._cache_key, init_value, self.DEFAULT_CACHE_TIMEOUT)
def incr(self, delta: int = 1):
"""Increment counter by delta."""
@ -34,11 +80,29 @@ class ItemCounter:
def decr(self, delta: int = 1):
"""Decrement counter by delta."""
try:
cache.decr(self._cache_key, delta)
except ValueError:
pass
with self._redis.lock(f"{self.CACHE_KEY_BASE}-decr"):
if self._minimum is not None and self.value() == self._minimum:
return
try:
cache.decr(self._cache_key, delta)
except ValueError:
pass
def value(self) -> Optional[int]:
"""Return current value or None if not yet initialized."""
return cache.get(self._cache_key)
def get_redis_client_or_stub() -> Redis:
"""Return AA's default cache client or a stub if Redis is not available."""
redis = get_redis_client()
try:
if not redis.ping():
raise RuntimeError()
except (AttributeError, RedisError, RuntimeError):
logger.exception(
"Failed to establish a connection with Redis. "
"This EventSeries object is disabled.",
)
return _RedisStub()
return redis

View File

@ -1,48 +1,19 @@
import datetime as dt
from unittest.mock import patch
from pytz import utc
from redis import RedisError
from django.test import TestCase
from django.utils.timezone import now
from allianceauth.authentication.task_statistics.event_series import (
EventSeries,
_RedisStub,
)
from allianceauth.authentication.task_statistics.helpers import _RedisStub
MODULE_PATH = "allianceauth.authentication.task_statistics.event_series"
class TestEventSeries(TestCase):
def test_should_abort_without_redis_client(self):
# when
with patch(MODULE_PATH + ".get_redis_client") as mock:
mock.return_value = None
events = EventSeries("dummy")
# then
self.assertTrue(events._redis, _RedisStub)
self.assertTrue(events.is_disabled)
def test_should_disable_itself_if_redis_not_available_1(self):
# when
with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
mock_get_master_client.return_value.ping.side_effect = RedisError
events = EventSeries("dummy")
# then
self.assertIsInstance(events._redis, _RedisStub)
self.assertTrue(events.is_disabled)
def test_should_disable_itself_if_redis_not_available_2(self):
# when
with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
mock_get_master_client.return_value.ping.return_value = False
events = EventSeries("dummy")
# then
self.assertIsInstance(events._redis, _RedisStub)
self.assertTrue(events.is_disabled)
def test_should_add_event(self):
# given
events = EventSeries("dummy")
@ -166,3 +137,15 @@ class TestEventSeries(TestCase):
results = events.all()
# then
self.assertEqual(len(results), 2)
def test_should_not_report_as_disabled_when_initialized_normally(self):
# given
events = EventSeries("dummy")
# when/then
self.assertFalse(events.is_disabled)
def test_should_report_as_disabled_when_initialized_with_redis_stub(self):
# given
events = EventSeries("dummy", redis=_RedisStub())
# when/then
self.assertTrue(events.is_disabled)

View File

@ -0,0 +1,142 @@
from unittest import TestCase
from unittest.mock import patch
from redis import RedisError
from allianceauth.authentication.task_statistics.helpers import (
ItemCounter, _RedisStub, get_redis_client_or_stub,
)
MODULE_PATH = "allianceauth.authentication.task_statistics.helpers"
COUNTER_NAME = "test-counter"
class TestItemCounter(TestCase):
def test_can_create_counter(self):
# when
counter = ItemCounter(COUNTER_NAME)
# then
self.assertIsInstance(counter, ItemCounter)
def test_can_reset_counter_to_default(self):
# given
counter = ItemCounter(COUNTER_NAME)
# when
counter.reset()
# then
self.assertEqual(counter.value(), 0)
def test_can_reset_counter_to_custom_value(self):
# given
counter = ItemCounter(COUNTER_NAME)
# when
counter.reset(42)
# then
self.assertEqual(counter.value(), 42)
def test_can_increment_counter_by_default(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(0)
# when
counter.incr()
# then
self.assertEqual(counter.value(), 1)
def test_can_increment_counter_by_custom_value(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(0)
# when
counter.incr(8)
# then
self.assertEqual(counter.value(), 8)
def test_can_decrement_counter_by_default(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(9)
# when
counter.decr()
# then
self.assertEqual(counter.value(), 8)
def test_can_decrement_counter_by_custom_value(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(9)
# when
counter.decr(8)
# then
self.assertEqual(counter.value(), 1)
def test_can_decrement_counter_below_zero(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(0)
# when
counter.decr(1)
# then
self.assertEqual(counter.value(), -1)
def test_can_not_decrement_counter_below_minimum(self):
# given
counter = ItemCounter(COUNTER_NAME, minimum=0)
counter.reset(0)
# when
counter.decr(1)
# then
self.assertEqual(counter.value(), 0)
def test_can_not_reset_counter_below_minimum(self):
# given
counter = ItemCounter(COUNTER_NAME, minimum=0)
# when/then
with self.assertRaises(ValueError):
counter.reset(-1)
def test_can_not_init_without_name(self):
# when/then
with self.assertRaises(ValueError):
ItemCounter(name="")
def test_can_ignore_invalid_values_when_incrementing(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(0)
# when
with patch(MODULE_PATH + ".cache.incr") as m:
m.side_effect = ValueError
counter.incr()
# then
self.assertEqual(counter.value(), 0)
def test_can_ignore_invalid_values_when_decrementing(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(1)
# when
with patch(MODULE_PATH + ".cache.decr") as m:
m.side_effect = ValueError
counter.decr()
# then
self.assertEqual(counter.value(), 1)
class TestGetRedisClient(TestCase):
def test_should_return_mock_if_redis_not_available_1(self):
# when
with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
mock_get_master_client.return_value.ping.side_effect = RedisError
result = get_redis_client_or_stub()
# then
self.assertIsInstance(result, _RedisStub)
def test_should_return_mock_if_redis_not_available_2(self):
# when
with patch(MODULE_PATH + ".get_redis_client") as mock_get_master_client:
mock_get_master_client.return_value.ping.return_value = False
result = get_redis_client_or_stub()
# then
self.assertIsInstance(result, _RedisStub)

View File

@ -1,74 +0,0 @@
from unittest import TestCase
from allianceauth.authentication.task_statistics.helpers import ItemCounter
COUNTER_NAME = "test-counter"
class TestItemCounter(TestCase):
def test_can_create_counter(self):
# when
counter = ItemCounter(COUNTER_NAME)
# then
self.assertIsInstance(counter, ItemCounter)
def test_can_reset_counter_to_default(self):
# given
counter = ItemCounter(COUNTER_NAME)
# when
counter.reset()
# then
self.assertEqual(counter.value(), 0)
def test_can_reset_counter_to_custom_value(self):
# given
counter = ItemCounter(COUNTER_NAME)
# when
counter.reset(42)
# then
self.assertEqual(counter.value(), 42)
def test_can_increment_counter_by_default(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(0)
# when
counter.incr()
# then
self.assertEqual(counter.value(), 1)
def test_can_increment_counter_by_custom_value(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(0)
# when
counter.incr(8)
# then
self.assertEqual(counter.value(), 8)
def test_can_decrement_counter_by_default(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(9)
# when
counter.decr()
# then
self.assertEqual(counter.value(), 8)
def test_can_decrement_counter_by_custom_value(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(9)
# when
counter.decr(8)
# then
self.assertEqual(counter.value(), 1)
def test_can_decrement_counter_below_zero(self):
# given
counter = ItemCounter(COUNTER_NAME)
counter.reset(0)
# when
counter.decr(1)
# then
self.assertEqual(counter.value(), -1)