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

Convert mongo read_preference setting at a lower level. #16540

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

mtyaka
Copy link
Contributor

@mtyaka mtyaka commented Nov 13, 2017

Convert mongo read_preference setting at a lower level.

Doing it at the modulestore level does not cover all usages of mongo, for example when connecting to the contentstore.

This became a problem after https://github.com/edx/configuration/pull/4196 was merged. Our sandbox builds started failing with pymongo.errors.ConfigurationError: Not a valid read preference because the new default read_reference ("SECONDARY_PREFERRED") was not getting converted to ReadPreference.SECONDARY_PREFERRED constant for all mongo connections.

Note that the mongo client allows the read_preference parameter to be given either as a ReadPreference.SECONDARY_PREFERRED constant, or a string of the form "secondaryPreferred", but not "SECONDARY_PREFERRED", which is why the conversion is necessary.

This patch moves the conversion from "SECONDARY_PREFERRED" to ReadPreference.SECONDARY_PREFERRED, while also allowing other forms of read_preference parameters, such as "secondaryPreferred" for backwards compatibility.

JIRA: https://openedx.atlassian.net/browse/OSPR-2012

Sandbox URL:

Merge deadline: This is breaking edx_sandbox.yml builds, so would like to merge ASAP. https://github.com/edx/configuration/pull/4196 was reverted, so this is no longer critical.

Testing instructions:

  1. Without this patch, using the default value of EDXAPP_MONGO_READ_PREFERENCE ("SECONDARY_PREFERRED") from https://github.com/edx/configuration/pull/4196, try to spawn a new sandbox using the edx_sandbox.yml playbook. Observe that it fails with the pymongo.errors.ConfigurationError: Not a valid read preference error at the [demo : import demo course] step.
  2. Try to build a sandbox with this patch, observe that it succeeds.

Reviewers

@mtyaka mtyaka force-pushed the mtyaka/mongo-read-preference branch from 28fa273 to 4069d06 Compare November 13, 2017 09:11
Doing it at the modulestore level does not cover all usages of mongo,
for example when connecting to the contentstore.
Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

👍

2017-11-10 15:14:27+0500 INFO
TASK [demo : import demo course] ***********************************************
2017-11-10 15:14:28+0500 INFO
fatal: [137.74.29.219]: FAILED! => {"changed": true, "cmd": "/edx/app/edxapp/venvs/edxapp/bin/python ./manage.py cms --settings=aws import /edx/var/edxapp/data /edx/app/demo/edx-demo-course", "delta": "0:00:12.043405", "end": "2017-11-10 10:14:27.717954", "failed": true, "rc": 1, "start": "2017-11-10 10:14:15.674549", "stderr": "2017-11-10 10:14:22,067 WARNING 8036 [py.warnings] base.py:116 - /edx/app/edxapp/edx-platform/lms/djangoapps/courseware/models.py:396: RemovedInDjango19Warning: Model class courseware.models.CourseDynamicUpgradeDeadlineConfiguration doesn't declare an explicit app_label and either isn't in an application in INSTALLED_APPS or else was imported before its application was loaded. This will no longer be supported in Django 1.9.\n  class CourseDynamicUpgradeDeadlineConfiguration(OptOutDynamicUpgradeDeadlineMixin, ConfigurationModel):\n\n2017-11-10 10:14:22,068 WARNING 8036 [py.warnings] base.py:116 - /edx/app/edxapp/edx-platform/lms/djangoapps/courseware/models.py:418: RemovedInDjango19Warning: Model class courseware.models.OrgDynamicUpgradeDeadlineConfiguration doesn't declare an explicit app_label and either isn't in an application in INSTALLED_APPS or else was imported before its application was loaded. This will no longer be supported in Django 1.9.\n  class OrgDynamicUpgradeDeadlineConfiguration(OptOutDynamicUpgradeDeadlineMixin, ConfigurationModel):\n\n2017-11-10 10:14:26,440 INFO 8036 [dd.dogapi] dog_stats_api.py:66 - Initializing dog api to use statsd: localhost, 8125\nTraceback (most recent call last):\n  File \"./manage.py\", line 116, in <module>\n    execute_from_command_line([sys.argv[0]] + django_args)\n  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/__init__.py\", line 354, in execute_from_command_line\n    utility.execute()\n  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/__init__.py\", line 346, in execute\n    self.fetch_command(subcommand).run_from_argv(self.argv)\n  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/base.py\", line 394, in run_from_argv\n    self.execute(*args, **cmd_options)\n  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/base.py\", line 445, in execute\n    output = self.handle(*args, **options)\n  File \"/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/management/commands/import.py\", line 38, in handle\n    mstore = modulestore()\n  File \"/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/modulestore/django.py\", line 333, in modulestore\n    contentstore(),\n  File \"/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/contentstore/django.py\", line 28, in contentstore\n    _CONTENTSTORE[name] = class_(**options)\n  File \"/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/contentstore/mongo.py\", line 42, in __init__\n    port=port, tz_aware=tz_aware, user=user, password=password, proxy=proxy, **kwargs\n  File \"/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/mongo_utils.py\", line 42, in connect_to_mongodb\n    **kwargs\n  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/pymongo/mongo_client.py\", line 311, in __init__\n    option, value = common.validate(option, value)\n  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/pymongo/common.py\", line 446, in validate\n    value = validator(option, value)\n  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/pymongo/common.py\", line 243, in validate_read_preference\n    raise ConfigurationError(\"Not a valid read preference\")\npymongo.errors.ConfigurationError: Not a valid read preference", "stdout": "Importing.  Data_dir=/edx/var/edxapp/data, source_dirs=['/edx/app/demo/edx-demo-course']", "stdout_lines": ["Importing.  Data_dir=/edx/var/edxapp/data, source_dirs=['/edx/app/demo/edx-demo-course']"], "warnings": []}

The sandbox that applies this patch, however, succeeds being provisioned, and is readily usable (i.e. https://pr16540.sandbox.opencraft.hosting/)

  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@openedx-webhooks
Copy link

Thanks for the pull request, @mtyaka! I've created OSPR-2012 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Nov 16, 2017
@openedx openedx deleted a comment from openedx-webhooks Nov 16, 2017
@nedbat
Copy link
Contributor

nedbat commented Nov 16, 2017

@edx/platform-team Opinions?

@macdiesel
Copy link
Contributor

@jdmulloy FYI. When we add the secondary preferred configuration back, lets use the secondaryPreferred string so we can remove this conversion at some point.

Copy link
Contributor

@macdiesel macdiesel left a comment

Choose a reason for hiding this comment

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

Changes look good, I would like a tweak to the test to ensure we are testing a wider range of options than just the Secondary strings.

self.assertEqual(connection.client.read_preference, ReadPreference.SECONDARY_PREFERRED)
# Support for read_preference given as mongos name.
connection = connect_to_mongodb(db, host, read_preference='secondaryPreferred')
self.assertEqual(connection.client.read_preference, ReadPreference.SECONDARY_PREFERRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this test be converted to a DDT so we could also test for ReadPreference.NEAREST and ReadPreference.PRIMARY please.

@macdiesel macdiesel added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization labels Nov 16, 2017
@mtyaka mtyaka force-pushed the mtyaka/mongo-read-preference branch from 77d1e33 to abd4859 Compare November 20, 2017 07:10
@mtyaka
Copy link
Contributor Author

mtyaka commented Nov 20, 2017

Thanks for the review @macdiesel! I converted the test to a DDT to test for more read preferences.

@macdiesel macdiesel merged commit 622ee7a into openedx:master Nov 28, 2017
@openedx-webhooks
Copy link

@mtyaka 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Wednesday, November 29, 2017.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been rolled back from the production environment.

@pomegranited pomegranited deleted the mtyaka/mongo-read-preference branch December 7, 2017 05:50
pomegranited pushed a commit to open-craft/edx-platform that referenced this pull request Dec 7, 2017
…eference

Convert mongo read_preference setting at a lower level.

(cherry picked from commit 622ee7a)
pomegranited pushed a commit to open-craft/edx-platform that referenced this pull request Dec 11, 2017
…eference

Convert mongo read_preference setting at a lower level.
pomegranited pushed a commit to rue89-tech/edx-platform that referenced this pull request Dec 21, 2017
…eference

Convert mongo read_preference setting at a lower level.

(cherry picked from commit 622ee7a)
pomegranited pushed a commit to open-craft/edx-platform that referenced this pull request Jul 25, 2018
…eference

Convert mongo read_preference setting at a lower level.

(cherry picked from commit 622ee7a)
pomegranited pushed a commit to open-craft/edx-platform that referenced this pull request Jul 25, 2018
…eference

Convert mongo read_preference setting at a lower level.

(cherry picked from commit 622ee7a)
pomegranited added a commit to open-craft/edx-platform that referenced this pull request Jul 25, 2018
…eference (#131)

Convert mongo read_preference setting at a lower level.

(cherry picked from commit 622ee7a)
pomegranited added a commit to edx-olive-oc/edx-platform that referenced this pull request Jul 25, 2018
…eference (#6)

Convert mongo read_preference setting at a lower level.

(cherry picked from commit 622ee7a)
pomegranited pushed a commit to edx-olive-oc/edx-platform that referenced this pull request Jul 27, 2018
…eference

Convert mongo read_preference setting at a lower level.

(cherry picked from commit 622ee7a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants