Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API des membres + Cache = KBOOOM ! #2727

Closed
SpaceFox opened this issue May 19, 2015 · 8 comments
Closed

API des membres + Cache = KBOOOM ! #2727

SpaceFox opened this issue May 19, 2015 · 8 comments
Labels
Bloquant Ticket qui doit être traité avant la prochaine mise à jour C-API Concerne une API du site S-BUG Corrige un problème
Milestone

Comments

@SpaceFox
Copy link
Contributor

Bug découvert en tentant d'activer Memcached dans Travis (cf ce job) et parfaitement reproductible en local.

Quand on lance les tests avec Memcached d'activé, certains tests plantent. Tous sont des tests liés à l'API des membres.

Le fait de ne lancer que cette batterie de tests ou l'intégralité de la suite ne change rien.

Le fait d'utiliser les moteurs de cache suivants ne change rien et provoque les erreurs (settings de cache CACHES directement repiqués dans la doc) (memcached est évidemment arrêté pour éviter les effets de bord lors des test avec les autres systèmes de cache)

  • Memcached
  • Filesystem caching
  • Local-memory caching

Par contre le moteur "Database caching" fonctionne (!) (et j'ai vérifié, il crée bien la table de cache en mode test).

Et évidemment "Dummy caching" fonctionne, puisque par définition il ne fait rien.

C'est bloquant parce que le cache est actif en prod -- il l'est déjà aujourd'hui, et il faut qu'on sache si c'est juste les tests qui sont incompatibles avec le cache (ce que je pense probable) ou si on a un truc foireux / qui va devenir foireux à la prochaine MEP.

Idées de tests :

  1. Vérifier en local avec la version actuellement en prod, voir si on a ce problème
  2. Vérifier "manuellement" en prod / bêta si les résultat qui sont HS dans les tests le sont aussi en vrai

Ceci devrait nous permettre de savoir si l'API est cassée ou si c'est "juste" les tests qui sont incompatibles avec le cache.

Logs sur les tests de l'API des membres, Debian 8, MySQL :

(zdsenv)spacefox@azathoth:~/Dev/Django/zds-site$ sudo systemctl stop memcached.service 
(zdsenv)spacefox@azathoth:~/Dev/Django/zds-site$ python manage.py test zds.member.api
Creating test database for alias 'default'...
......................................................................
----------------------------------------------------------------------
Ran 70 tests in 16.790s

OK
Destroying test database for alias 'default'...
(zdsenv)spacefox@azathoth:~/Dev/Django/zds-site$ sudo systemctl start memcached.service 
(zdsenv)spacefox@azathoth:~/Dev/Django/zds-site$ python manage.py test zds.member.api
Creating test database for alias 'default'...
..............................................FFFFFF..................
======================================================================
FAIL: test_list_of_users_empty (zds.member.api.tests.MemberListAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/spacefox/Dev/Django/zds-site/zds/member/api/tests.py", line 26, in test_list_of_users_empty
    self.assertEqual(response.data.get('count'), 0)
AssertionError: 10 != 0

======================================================================
FAIL: test_list_of_users_for_a_page_given (zds.member.api.tests.MemberListAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/spacefox/Dev/Django/zds-site/zds/member/api/tests.py", line 64, in test_list_of_users_for_a_page_given
    self.assertEqual(response.data.get('count'), 11)
AssertionError: 10 != 11

======================================================================
FAIL: test_list_of_users_for_a_wrong_page_given (zds.member.api.tests.MemberListAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/spacefox/Dev/Django/zds-site/zds/member/api/tests.py", line 74, in test_list_of_users_for_a_wrong_page_given
    self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
AssertionError: 200 != 404

======================================================================
FAIL: test_list_of_users_with_a_custom_page_size (zds.member.api.tests.MemberListAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/spacefox/Dev/Django/zds-site/zds/member/api/tests.py", line 86, in test_list_of_users_with_a_custom_page_size
    self.assertEqual(response.data.get('count'), 20)
AssertionError: 10 != 20

======================================================================
FAIL: test_list_of_users_with_a_wrong_custom_page_size (zds.member.api.tests.MemberListAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/spacefox/Dev/Django/zds-site/zds/member/api/tests.py", line 102, in test_list_of_users_with_a_wrong_custom_page_size
    self.assertEqual(response.data.get('count'), page_size_value)
AssertionError: 10 != 101

======================================================================
FAIL: test_list_of_users_with_several_pages (zds.member.api.tests.MemberListAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/spacefox/Dev/Django/zds-site/zds/member/api/tests.py", line 52, in test_list_of_users_with_several_pages
    self.assertEqual(response.data.get('count'), settings.REST_FRAMEWORK['PAGINATE_BY'] + 1)
AssertionError: 10 != 11

----------------------------------------------------------------------
Ran 70 tests in 18.514s

FAILED (failures=6)
Destroying test database for alias 'default'...
@SpaceFox SpaceFox added S-BUG Corrige un problème C-API Concerne une API du site labels May 19, 2015
@SpaceFox SpaceFox changed the title API des MP + Cache = 💥 API des MP + Cache = KBOOOM ! May 19, 2015
@SpaceFox SpaceFox added the Bloquant Ticket qui doit être traité avant la prochaine mise à jour label May 19, 2015
@GerardPaligot GerardPaligot changed the title API des MP + Cache = KBOOOM ! API des membres + Cache = KBOOOM ! May 19, 2015
@GerardPaligot
Copy link
Member

Les échecs des tests mentionnés par Travis sont cohérents avec le comportement actuel de l'API des membres en production. J'étais persuadé que le problème se situait sur les problèmes serveurs que nous avons eu avec OVH mais cette issue me fait douter.

Donc nous semblons avoir un problème de cache avec la pagination (ici et en prod) mais, dans le code source, nous renseignons bien la pagination comme clé pour le cache. Je ne comprends donc pas pourquoi les tests échouent pour l'API des membres (et passent pour les tests de l'API des MPs au passage).

@DevHugo
Copy link
Contributor

DevHugo commented May 20, 2015

Les tests chez moi passe pas non plus pour les mp, si on active le cache.

On voit clairement d'après, les logs qu'il essaye toujours de comparer un résultat en cache (qui est de 10).

On peut supposer que lors du test, il est passé par une méthode qui a insérer 10 membres et qu'il a fait une requête GET vers api-member-list. Le cache est créé avec les 10 réponses pour une période par défaut. Lors des appels suivant, il réutilise le cache d'ou les erreurs.

@DevHugo
Copy link
Contributor

DevHugo commented May 29, 2015

Je permet de te ping @GerardPaligot pour avoir ton avis, il ont mis à jours la documentation de l'api et propose une solution pour invalider le cache.

Si je résume la solution de la documentation, dans le ListKeyConstructor, on ajoute un champ comme ceci updated_at = UpdatedAtKeyBit().

Après la méthode associé qui nous permet de créé une clé de cache api_updated_at_timestamp qui représentera quand l'objet sera mis à jours.

class UpdatedAtKeyBit(KeyBitBase):
    def get_data(self, **kwargs):
        key = 'api_updated_at_timestamp'
        value = cache.get(key, None)
        if not value:
            value = datetime.datetime.utcnow()
            cache.set(key, value=value)
        return force_text(value)

Pour invalider le cache quand on modifie un user par-exemple:

    post_save.connect(receiver=change_api_updated_at, sender=model)

def change_api_updated_at(sender=None, instance=None, *args, **kwargs):
    cache.set('api_updated_at_timestamp', datetime.datetime.utcnow())

La doc officielle est ici, c'est juste avant le paragraphe: Key constructor params.

T'en pense quoi ? c'est un peu de boulot et ça manque un peu de finesse (on invalide tous alors qu'on a modifié un user seulement) mais je pense qu'on peut faire un truc plus fin. L'idée générale me plait bien ^^.

@GerardPaligot
Copy link
Member

Je ne comprends pas le rapport avec le problème de cette issue qui est lié à la pagination et dont la clé est renseignée en fait.

@DevHugo
Copy link
Contributor

DevHugo commented May 29, 2015

Pour moi, c'est pas liés à la pagination.

Je me permet de me citer:

On peut supposer que lors du test, il est passé par une méthode qui a insérer 10 membres et
qu'il a fait une requête GET vers api-member-list. Le cache est créé avec les 10 réponses pour
une période par défaut. Lors des appels suivant, il réutilise le cache d'ou les erreurs.

@GerardPaligot
Copy link
Member

C'est sans doute ce qu'il fait mais pour moi, il ne faut pas modifier le code source parce que nous savons pas comment le tester. Si les tests Django sont unitaires, le cache ne devrait pas subsister à travers les méthodes et toutes les données devraient être supprimées. Après, s'ils ne sont pas unitaires, il faut faire évoluer les tests en conséquence puisqu'ils ont été développé en partant du principe qu'ils étaient unitaires.

@DevHugo
Copy link
Contributor

DevHugo commented May 29, 2015

Ok, je comprend tes arguments, ça me parait pertinent.

@pierre-24
Copy link
Member

Sur ce, on est finalement OK :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bloquant Ticket qui doit être traité avant la prochaine mise à jour C-API Concerne une API du site S-BUG Corrige un problème
Projects
None yet
Development

No branches or pull requests

4 participants