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

Update morango and stop locking sync when db backend is postgres #9556

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions Makefile
@@ -1,3 +1,5 @@
SHELL := /bin/bash

# List most target names as 'PHONY' to prevent Make from thinking it will be creating a file of the same name
.PHONY: help clean clean-assets clean-build clean-pyc clean-docs lint test test-all assets coverage docs release test-namespaced-packages staticdeps staticdeps-cext writeversion setrequirements buildconfig pex i18n-extract-frontend i18n-extract-backend i18n-transfer-context i18n-extract i18n-django-compilemessages i18n-upload i18n-pretranslate i18n-pretranslate-approve-all i18n-download i18n-regenerate-fonts i18n-stats i18n-install-font i18n-download-translations i18n-download-glossary i18n-upload-glossary docker-whl docker-demoserver docker-devserver docker-envlist

Expand Down Expand Up @@ -29,9 +31,11 @@ help:
@echo "lint: check Python style with flake8"
@echo "test: run tests quickly with the default Python"
@echo "test-all: run tests on every Python version with Tox"
@echo "test-with-postgres: run tests quickly with a temporary postgresql backend"
@echo "test-namespaced-packages: verify that we haven't fetched anything namespaced into kolibri/dist"
@echo "coverage: run tests, recording and printing out Python code coverage"
@echo "docs: generate developer documentation"
@echo "start-foreground-with-postgres: run Kolibri in foreground mode with a temporary postgresql backend"
@echo ""
@echo "Internationalization"
@echo "--------------------"
Expand Down Expand Up @@ -90,6 +94,29 @@ test:
test-all:
tox

%-with-postgres:
@echo -e "\e[33mWARNING: for testing purposes only; postgresql database backend is ephemeral\e[0m"
@echo -e "\e[36mINFO: run 'docker-compose -v' to remove the database volume\e[0m"
export KOLIBRI_DATABASE_ENGINE=postgres; \
export KOLIBRI_DATABASE_NAME=default; \
export KOLIBRI_DATABASE_USER=postgres; \
export KOLIBRI_DATABASE_PASSWORD=postgres; \
export KOLIBRI_DATABASE_HOST=127.0.0.1; \
export KOLIBRI_DATABASE_PORT=5432; \
set -ex; \
function _on_interrupt() { docker-compose down; }; \
trap _on_interrupt SIGINT SIGTERM SIGKILL ERR; \
docker-compose up --detach; \
until docker-compose logs --tail=1 postgres | grep -q "database system is ready to accept connections"; do \
echo "$(date) - waiting for postgres..."; \
sleep 1; \
done; \
$(MAKE) -e $(subst -with-postgres,,$@); \
docker-compose down -v

start-foreground:
kolibri start --foreground

assets:
yarn install
yarn run build
Expand Down
16 changes: 16 additions & 0 deletions docker-compose.yml
@@ -0,0 +1,16 @@
version: '3.4'

services:
postgres:
image: postgres:12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking into account that GCP uses v13 as default, and thinking in the future, wouldn it be best to use postgres:13?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our CI tests currently use 12, so I think good for it to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our github actions (in newer release branches) use version 12, so this is consistent with that. Hopefully this shouldn't make much of a difference

ports:
- "5432:5432"
environment:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_DB: default
volumes:
- kolibri-pg:/var/lib/postgresql/data
# command: "postgres -c log_statement=all -c log_line_prefix='%t %d '"
volumes:
kolibri-pg:
9 changes: 7 additions & 2 deletions kolibri/core/auth/management/commands/sync.py
Expand Up @@ -23,7 +23,7 @@
from kolibri.core.logger.utils.data import bytes_for_humans
from kolibri.core.tasks.exceptions import UserCancelledError
from kolibri.core.tasks.management.commands.base import AsyncCommand
from kolibri.core.utils.lock import db_lock
from kolibri.core.utils.lock import db_lock_sqlite_only
from kolibri.utils import conf

DATA_PORTAL_SYNCING_BASE_URL = conf.OPTIONS["Urls"]["DATA_PORTAL_SYNCING_BASE_URL"]
Expand Down Expand Up @@ -216,7 +216,12 @@ def _lock(self):
cancellable = self.job.cancellable
self.job.save_as_cancellable(cancellable=False)

with db_lock():
# Morango v0.6.13+ uses read repeatable isolation for postgres, and handles database
# transactions in a special manner that would be problematic if we began a transaction here.
# If data has changed during morango's transactions, postgres may throw a serialization
# rollback error, which will cause morango to retry it. So it's safe to avoid locking here
# when using postgres
with db_lock_sqlite_only():
yield

if self.job:
Expand Down
3 changes: 2 additions & 1 deletion kolibri/core/auth/test/test_morango_integration.py
Expand Up @@ -7,6 +7,7 @@
import uuid

from django.test import TestCase
from django.test import TransactionTestCase
from morango.models import InstanceIDModel
from morango.models import Store
from morango.models import syncable_models
Expand Down Expand Up @@ -44,7 +45,7 @@ def test_partition_and_id_values(self):
self.assertTrue(partition.startswith(dataset_id))


class DateTimeTZFieldTestCase(TestCase):
class DateTimeTZFieldTestCase(TransactionTestCase):
def setUp(self):
self.controller = MorangoProfileController(PROFILE_FACILITY_DATA)
InstanceIDModel.get_or_create_current_instance()
Expand Down
24 changes: 21 additions & 3 deletions kolibri/core/utils/lock.py
Expand Up @@ -30,8 +30,7 @@ def execute(self):


@contextmanager
def db_lock():
lock_id = 1
def db_lock_sqlite_only():
if connection.vendor == "sqlite":
while True:
try:
Expand All @@ -44,11 +43,30 @@ def db_lock():
except OperationalError as e:
if "database is locked" not in str(e):
raise e
elif connection.vendor == "postgresql":
else:
yield


@contextmanager
def db_lock_postgresql_only():
lock_id = 1
if connection.vendor == "postgresql":
with transaction.atomic():
operation = PostgresLock(key=lock_id)
operation.execute()
yield
else:
yield


@contextmanager
def db_lock():
if connection.vendor == "sqlite":
with db_lock_sqlite_only():
yield
elif connection.vendor == "postgresql":
with db_lock_postgresql_only():
yield
else:
raise NotImplementedError(
"kolibri.core.utils.cache.DatabaseLock not implemented for vendor {vendor}".format(
Expand Down
2 changes: 1 addition & 1 deletion requirements/base.txt
Expand Up @@ -20,7 +20,7 @@ kolibri_exercise_perseus_plugin==1.3.5
jsonfield==2.0.2
requests-toolbelt==0.8.0
clint==0.5.1
morango==0.6.13
morango==0.6.14
tzlocal==2.1
pytz==2020.5
python-dateutil==2.7.5
Expand Down