diff --git a/cms/djangoapps/contentstore/api/views/course_import.py b/cms/djangoapps/contentstore/api/views/course_import.py index f2f8f168df50..17152941a922 100644 --- a/cms/djangoapps/contentstore/api/views/course_import.py +++ b/cms/djangoapps/contentstore/api/views/course_import.py @@ -3,14 +3,12 @@ """ -import base64 import logging import os +from uuid import uuid4 -from django.conf import settings from django.core.files import File from edx_django_utils.monitoring import set_custom_attribute, set_custom_attributes_for_course_key -from path import Path as path from rest_framework import status from rest_framework.exceptions import AuthenticationFailed from rest_framework.generics import GenericAPIView @@ -18,7 +16,12 @@ from user_tasks.models import UserTaskStatus from cms.djangoapps.contentstore.storage import course_import_export_storage -from cms.djangoapps.contentstore.tasks import CourseImportTask, import_olx +from cms.djangoapps.contentstore.tasks import ( + CourseImportTask, + course_import_working_dir, + import_olx, + remove_course_import_working_dir, +) from cms.djangoapps.contentstore.utils import IMPORTABLE_FILE_TYPES from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes @@ -131,22 +134,26 @@ def post(self, request, course_key): developer_message='Parameter in the wrong format', error_code='internal_error', ) - course_dir = path(settings.GITHUB_REPO_ROOT) / base64.urlsafe_b64encode( - repr(course_key).encode('utf-8') - ).decode('utf-8') + # Staging directory private to this upload, so that a concurrent import of + # the same course cannot overwrite this archive or delete it on its way out. + course_dir = course_import_working_dir(course_key, f'upload-{uuid4().hex}') temp_filepath = course_dir / filename - if not course_dir.isdir(): - os.mkdir(course_dir) - - log.debug(f'importing course to {temp_filepath}') - with open(temp_filepath, "wb+") as temp_file: - for chunk in request.FILES['course_data'].chunks(): - temp_file.write(chunk) - - log.info("Course import %s: Upload complete", course_key) - with open(temp_filepath, 'rb') as local_file: - django_file = File(local_file) - storage_path = course_import_export_storage.save('olx_import/' + filename, django_file) + os.makedirs(course_dir, exist_ok=True) + + try: + log.debug(f'importing course to {temp_filepath}') + with open(temp_filepath, "wb+") as temp_file: + for chunk in request.FILES['course_data'].chunks(): + temp_file.write(chunk) + + log.info("Course import %s: Upload complete", course_key) + with open(temp_filepath, 'rb') as local_file: + django_file = File(local_file) + storage_path = course_import_export_storage.save('olx_import/' + filename, django_file) + finally: + # The archive now lives in storage; the import task downloads it into its + # own working directory, so this staging copy is no longer needed. + remove_course_import_working_dir(course_dir) async_result = import_olx.delay( request.user.id, str(course_key), storage_path, filename, request.LANGUAGE_CODE) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 73f111ff8462..ab6f78d21782 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -522,6 +522,36 @@ def sync_discussion_settings(course_key, user): LOGGER.info(f'Course import {course.id}: DiscussionsConfiguration sync failed: {exc}') +def course_import_working_dir(courselike_key, unique_id): + """ + Return a private scratch directory for one course import. + + ``unique_id`` must be unique per import (a task id, an upload id, ...). Every + import needs its own directory: they used to share a single directory derived + from the course key alone, so two imports of the same course would overwrite + each other's archive, and whichever import finished first deleted the other's + extracted OLX mid-run. + """ + subdir = base64.urlsafe_b64encode(repr(courselike_key).encode('utf-8')).decode('utf-8') + return path(settings.GITHUB_REPO_ROOT) / subdir / str(unique_id) + + +def remove_course_import_working_dir(course_dir): + """ + Delete a working directory created by :func:`course_import_working_dir`. + + Also drops the per-course parent directory once the last import using it is + gone. Both steps tolerate the directory already being missing, so that + cleanup never masks the error that triggered it. + """ + shutil.rmtree(course_dir, ignore_errors=True) + try: + os.rmdir(os.path.dirname(course_dir)) + except OSError: + # Still holds another import's working directory, or is already gone. + pass + + @shared_task(base=CourseImportTask, bind=True) # Note: The decorator @set_code_owner_attribute cannot be used here because the UserTaskMixin # does stack inspection and can't handle additional decorators. @@ -538,8 +568,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan self.status.set_state(current_step) data_root = path(settings.GITHUB_REPO_ROOT) - subdir = base64.urlsafe_b64encode(repr(courselike_key).encode('utf-8')).decode('utf-8') - course_dir = data_root / subdir + course_dir = course_import_working_dir(courselike_key, self.request.id) def validate_user(): """Validate if the user exists otherwise log error. """ @@ -647,8 +676,7 @@ def get_dir_for_filename(directory, filename): LOGGER.info(f'{log_prefix}: unpacking step started') temp_filepath = course_dir / get_valid_filename(archive_name) - if not course_dir.isdir(): - os.mkdir(course_dir) + os.makedirs(course_dir, exist_ok=True) LOGGER.info(f'{log_prefix}: importing course to {temp_filepath}') @@ -684,9 +712,8 @@ def read_chunk(): LOGGER.info(f'{log_prefix}: entrance exam milestone content reference has been removed') # Send errors to client with stage at which error occurred. except Exception as exception: # pylint: disable=broad-except - if course_dir.isdir(): - shutil.rmtree(course_dir) - LOGGER.info(f'{log_prefix}: Temp data cleared') + remove_course_import_working_dir(course_dir) + LOGGER.info(f'{log_prefix}: Temp data cleared') self.status.fail(UserErrors.UNKNOWN_ERROR_IN_UNPACKING) LOGGER.exception(f'{log_prefix}: Unknown error while unpacking', exc_info=True) @@ -742,9 +769,8 @@ def read_chunk(): except Exception as exception: # pylint: disable=broad-except handle_course_import_exception(courselike_key, exception, self.status, known=False) finally: - if course_dir.isdir(): - shutil.rmtree(course_dir) - LOGGER.info(f'{log_prefix}: Temp data cleared') + remove_course_import_working_dir(course_dir) + LOGGER.info(f'{log_prefix}: Temp data cleared') if self.status.state == 'Updating' and is_course: # Reload the course so we have the latest state diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index e4cc5a04cd17..ea5451c6b86a 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/cms/djangoapps/contentstore/views/import_export.py @@ -4,12 +4,11 @@ """ -import base64 +import hashlib import json import logging import os import re -import shutil from wsgiref.util import FileWrapper from django.conf import settings @@ -28,7 +27,6 @@ from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocator from openedx_authz.constants.permissions import COURSES_EXPORT_COURSE, COURSES_IMPORT_COURSE -from path import Path as path from storages.backends.s3boto3 import S3Boto3Storage from user_tasks.conf import settings as user_tasks_settings from user_tasks.models import UserTaskArtifact, UserTaskStatus @@ -42,7 +40,15 @@ from xmodule.modulestore.django import modulestore # pylint: disable=wrong-import-order from ..storage import course_import_export_storage -from ..tasks import CourseExportTask, CourseImportTask, create_export_tarball, export_olx, import_olx +from ..tasks import ( + CourseExportTask, + CourseImportTask, + course_import_working_dir, + create_export_tarball, + export_olx, + import_olx, + remove_course_import_working_dir, +) from ..utils import IMPORTABLE_FILE_TYPES, get_export_url, get_import_url, reverse_course_url __all__ = [ @@ -110,15 +116,28 @@ def _save_request_status(request, key, status): request.session.save() +def _upload_id(request, filename): + """ + Identify one chunked upload, so that its staging directory is not shared. + + Chunks of a single upload all have to land in the same directory, so the id + is derived from the session and the file name rather than being random. + """ + session_key = request.session.session_key or '' + digest = hashlib.sha256(f'{session_key}:{filename}'.encode()).hexdigest() + return f'upload-{digest[:32]}' + + def _write_chunk(request, courselike_key): # pylint: disable=too-many-statements """ Write the OLX file data chunk from the given request to the local filesystem. """ - # Upload .tar.gz or .zip to local filesystem for one-server installations not using S3 or Swift - data_root = path(settings.GITHUB_REPO_ROOT) - subdir = base64.urlsafe_b64encode(repr(courselike_key).encode('utf-8')).decode('utf-8') - course_dir = data_root / subdir filename = request.FILES['course-data'].name + # Upload .tar.gz or .zip to local filesystem for one-server installations not using S3 or Swift. + # The staging directory is private to this upload: it has to survive between chunks of the same + # upload, but must not be shared with another author's upload or with a running import task, + # which would delete it out from under us on its way out. + course_dir = course_import_working_dir(courselike_key, _upload_id(request, filename)) set_custom_attributes_for_course_key(courselike_key) current_step = 'Uploading' @@ -139,8 +158,7 @@ def error_response(message, status, stage): return error_response(error_message, 415, 0) temp_filepath = course_dir / filename - if not course_dir.isdir(): - os.mkdir(course_dir) + os.makedirs(course_dir, exist_ok=True) logging.info(f'Course import {courselike_key}: importing course to {temp_filepath}') @@ -207,15 +225,17 @@ def error_response(message, status, stage): with open(temp_filepath, 'rb') as local_file: django_file = File(local_file) storage_path = course_import_export_storage.save('olx_import/' + filename, django_file) + # The archive now lives in storage; the import task downloads it into its own + # working directory, so this staging copy is no longer needed. + remove_course_import_working_dir(course_dir) import_olx.delay( request.user.id, str(courselike_key), storage_path, filename, request.LANGUAGE_CODE) # Send errors to client with stage at which error occurred. except Exception as exception: # pylint: disable=broad-except _save_request_status(request, courselike_string, -1) - if course_dir.isdir(): - shutil.rmtree(course_dir) - log.info("Course import %s: Temp data cleared", courselike_key) + remove_course_import_working_dir(course_dir) + log.info("Course import %s: Temp data cleared", courselike_key) monitor_import_failure(courselike_key, current_step, exception=exception) log.exception(f'Course import {courselike_key}: error importing course.') diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 9f0e7e0a9c35..9d072e4fe457 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -1,6 +1,7 @@ """ Unit tests for course import and export """ +import base64 import copy import itertools import json @@ -21,6 +22,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.core.exceptions import SuspiciousOperation +from django.core.files import File from django.core.files.storage import FileSystemStorage from django.test.utils import override_settings from milestones.tests.utils import MilestonesTestCaseMixin @@ -30,11 +32,13 @@ from rest_framework import status from rest_framework.test import APIClient from storages.backends.s3boto3 import S3Boto3Storage -from user_tasks.models import UserTaskStatus +from user_tasks.models import UserTaskArtifact, UserTaskStatus from cms.djangoapps.contentstore import errors as import_error +from cms.djangoapps.contentstore import tasks from cms.djangoapps.contentstore.api.tests.base import BaseCourseViewTest from cms.djangoapps.contentstore.storage import course_import_export_storage +from cms.djangoapps.contentstore.tasks import import_olx from cms.djangoapps.contentstore.tests.test_libraries import LibraryTestCase from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.utils import reverse_course_url @@ -748,6 +752,123 @@ def test_import_status_response_is_not_cached(self, fmt): self.assertEqual(resp.headers['Cache-Control'], 'no-cache, no-store, must-revalidate') # noqa: PT009 +@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) +class ConcurrentImportTestCase(CourseTestCase): + """ + Tests for two imports of the same course overlapping in time. + + Regression tests for a bug where every import of a given course shared one + working directory, so whichever import finished first deleted the other's + archive and extracted OLX mid-run. The surviving task then failed with a + bare filesystem error (`[Errno 116] Stale file handle` on NFS-backed + ``GITHUB_REPO_ROOT``, `[Errno 2]` on a local disk). + """ + ARCHIVE_NAME = 'course.tar.gz' + + def setUp(self): + super().setUp() + self.content_dir = path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.content_dir, True) + + source_dir = tempfile.mkdtemp(dir=self.content_dir) + os.makedirs(os.path.join(source_dir, 'course')) + with open(os.path.join(source_dir, 'course.xml'), 'w') as course_xml: + course_xml.write('') + with open(os.path.join(source_dir, 'course', '2013_Spring.xml'), 'w') as run_xml: + run_xml.write('') + + self.archive = os.path.join(self.content_dir, self.ARCHIVE_NAME) + with tarfile.open(self.archive, 'w:gz') as archive: + archive.add(source_dir, arcname='exported_course') + + data_root = path(settings.GITHUB_REPO_ROOT) + if not data_root.isdir(): + os.makedirs(data_root) + self.per_course_dir = data_root / base64.urlsafe_b64encode( + repr(self.course.id).encode('utf-8') + ).decode('utf-8') + self.addCleanup(shutil.rmtree, self.per_course_dir, True) + + def stage_upload(self): + """Park a copy of the archive in storage, the way the upload view does.""" + with open(self.archive, 'rb') as archive: + return course_import_export_storage.save( + 'olx_import/' + self.ARCHIVE_NAME, File(archive) + ) + + def run_import(self, storage_path): + """Run one import_olx task to completion (celery is eager under test).""" + return import_olx.delay( + self.user.id, str(self.course.id), storage_path, self.ARCHIVE_NAME, 'en' + ) + + @staticmethod + def status_of(result): + """The UserTaskStatus recorded for a finished task.""" + return UserTaskStatus.objects.get(task_id=result.id) + + @staticmethod + def error_of(task_status): + """The error message a failed task showed the user, if any.""" + artifact = UserTaskArtifact.objects.filter(status=task_status, name='Error').first() + return artifact.text if artifact else None + + def test_concurrent_imports_of_same_course_both_succeed(self): + """ + An import that finishes while a second one is still unpacking must not + disturb it. + + Only the timing is simulated: the first import is run at the exact + moment the second one reaches its extraction step. Every filesystem + effect is real. + """ + first_upload = self.stage_upload() + second_upload = self.stage_upload() + self.assertNotEqual(first_upload, second_upload) # noqa: PT009 + + real_extractall = tasks.safe_extractall + observed = {} + + def extract_with_a_concurrent_import(file_name, output_path): + """Runs as the second import extracts; the first import lands here.""" + if not observed.get('fired'): + observed['fired'] = True # the first import must not re-enter + observed['archive_before'] = os.path.exists(file_name) + observed['first_result'] = self.run_import(first_upload) + observed['archive_after'] = os.path.exists(file_name) + observed['dir_after'] = os.path.isdir(output_path) + return real_extractall(file_name, output_path) + + with patch.object(tasks, 'safe_extractall', extract_with_a_concurrent_import): + second_result = self.run_import(second_upload) + + first_status = self.status_of(observed['first_result']) + second_status = self.status_of(second_result) + + self.assertEqual(first_status.state, UserTaskStatus.SUCCEEDED) # noqa: PT009 + # The first import's cleanup left the second import's files alone. + self.assertTrue(observed['archive_before']) # noqa: PT009 + self.assertTrue(observed['archive_after']) # noqa: PT009 + self.assertTrue(observed['dir_after']) # noqa: PT009 + self.assertEqual( # noqa: PT009 + second_status.state, UserTaskStatus.SUCCEEDED, self.error_of(second_status) + ) + # ...and between them they left no scratch data behind. + self.assertFalse(os.path.exists(self.per_course_dir)) # noqa: PT009 + + def test_import_working_dirs_are_not_shared(self): + """Two imports of one course never get the same working directory.""" + self.assertNotEqual( # noqa: PT009 + tasks.course_import_working_dir(self.course.id, 'task-1'), + tasks.course_import_working_dir(self.course.id, 'task-2'), + ) + self.assertTrue( # noqa: PT009 + tasks.course_import_working_dir(self.course.id, 'task-1').startswith( + self.per_course_dir + ) + ) + + @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) @ddt.ddt class ExportTestCase(CourseTestCase):