[BD-14] Updates Blockstore Python API [TNL-8746] [FAL-2702]

[pomegranited]

Description

Continues the work started by #140, and makes the remaining changes necessary for the Blockstore as app ADR: #71.

Replaces #97.

• Update the Python API to use the models directly.
• Enhance unit tests to improve coverage (from 85% to 98%)

Author Comments, Concerns, and Open Questions

See #97 for discussions.

Test Instructions

See openedx/openedx-platform#29779.

TODOs

• Update README to reflect these changes, specifically the blockstore overview and Running integration tests.
• Squash before merging
• Tag the new release version.

Next steps

Once these PRs merge, we will need to:

Clean up the edx-platform codebase:

• Remove the waffle flag added by [BD-14] Install the blockstore app into edx-platform, behind a waffle switch openedx/openedx-platform#29779.
• Replace the openedx.core.lib.blockstore_api with blockstore.app.api.
• Copy over the data from the blockstore database to the edxapp database.
• Remove the database router added above.

Clean up the blockstore codebase:

• Remove blockstore.apps.core.
• Remove the blockstore.app.rest_api
• Remove remaining tagstore code.

[openedx-webhooks]

Thanks for the pull request, @pomegranited! I've created BLENDED-1068 to keep track of it in Jira. More details are on the BD-14 project page.

When this pull request is ready, tag your edX technical lead.

[natabene]

@pomegranited Thank you for your contribution. Please let me know once this is ready.

[pomegranited]

@symbolist @gabor-boros There's a database compatibility problem with the db_collation setting that was chosen for some of the fields in these models.

Blockstore tests and production use MySQL, so using db_collation='utf8mb4_general_ci' was fine.
However, my attempts to run the unit tests with blockstore installed have failed because edx-platform uses sqlite.

• Sqlite, doesn't support utf8mb4_general_ci; the best it can do is binary.
• Postgres doesn't support utf8mb4_general_ci either, and its use contradicts the Postgres compatibility as documented for these models. Postgres's closest built-in equivalent collation sequence is the standard ucs_basic.

While it's relatively easy to configure the charset and collation sequence options at the settings.DATABASES level, we don't want to do this for all fields, and there isn't a nice way to do this per-field, except by modifying the model code directly.

So a full solution would require:

• detecting which database is in use for the given class, and
• returning an appropriate collation setting for that database engine.

All of which makes the model.py and the generated migrations pretty complicated, and difficult to reliably test and maintain.

@ormsbee has raised the very timely issue of whether we can formally drop support for Postgres, but that's a longer discussion.

Any suggestions for how to handle this better right now?

[ormsbee]

Does it make sense to create a custom field that subclasses CharField and has the logic to apply database-specific collations during migrations? I think it's primarily two use cases:

• Human-readable text that should be collated to be case-insensitive and as aware of accents, etc. as possible.
• Identifiers, which should be case sensitive.

?

[bradenmacdonald]

I think the tests don't depend on unicode-aware cast insensitive sorting, so binary should work fine for test purposes? The main thing they test is that all unicode characters including emojis can be used without issue, and binary should be fine for that.

And yeah +1 to what Dave is suggesting.

[pomegranited]

@bradenmacdonald

I think the tests don't depend on unicode-aware cast insensitive sorting,

Yep, this hack works for testing, but it's not pretty, and doesn't handle the Postgres use case.

Does it make sense to create a custom field that subclasses CharField and has the logic to apply database-specific collations during migrations?

Interesting suggestion, @ormsbee.. a custom field class seems doable, but it will still need to be passed information about the database selected for the migration so that it can exhibit the best field options for that engine. I'd have to dig into Django's field and migration logic to see if that can be done with the existing auto-generated migrations or not, but it's worth a try!

Since this is likely something that would be broadly useful outside of Blockstore, is there an openedx repo that would be an appropriate place for this change to live?

I'm going to have to leave this hack in place right now though, and create a follow-up task for this, since it's out of scope for this change.

[ormsbee]

Since this is likely something that would be broadly useful outside of Blockstore, is there an openedx repo that would be an appropriate place for this change to live?

edx-django-utils is probably what you want. It's installed on pretty much every service.

[symbolist]

Reviewed this as part of openedx/openedx-platform#29779. Looks good to launch! 🚀

Once this has been merged, I am assuming we will deploy this to the blockstore service as well? Before merging openedx/openedx-platform#29779 i.e..

[pomegranited]

Thank you for your review @symbolist ! @bradenmacdonald is going to have a look as core committer, and then I can squash the changes in preparation for merge.

Once this has been merged, I am assuming we will deploy this to the blockstore service as well? Before merging openedx/openedx-platform#29779 i.e..

Hi @jristau1984 I see you're the Reporter for TNL-8746. Can you help us find someone to shepherd this through to release on to edX's blockstore service?

[jristau1984]

@pomegranited our team planned our sprint yesterday, so this was not included. I will ensure this is discussed in our next sprint planning (March 14). Please let me know if that does not work for your team. Thanks!

[pomegranited]

Brilliant, thank you @jristau1984 ! I think MIT are awaiting this change too, so I'll forward your timeline details to them.

There's a few steps required to merge and deploy all of this:

• CC and/or upstream review
• Merge and tag [BD-14] Updates Blockstore Python API [TNL-8746] [FAL-2702] #149
• Deploy [BD-14] Updates Blockstore Python API [TNL-8746] [FAL-2702] #149 to edX/2U's stage and prod blockstore services.
• Regression testing for edX and LabXchange.
• Update the blockstore dependency in [BD-14] Install the blockstore app into edx-platform, behind a waffle switch openedx/openedx-platform#29779 (comment)
• Update edx-private configuration to support the blockstore app DB and file storage, which is shared with edX/2U's existing blockstore service. (see [BD-14] Install the blockstore app into edx-platform, behind a waffle switch openedx/openedx-platform#29779 for details)
• Deploy edx-platform#29779 to stage.edx.
• Regression testing for edX and LabXchange, with waffle flag switching between the service and the app.
• Deploy to prod and test.

[bradenmacdonald]

I read through the code and it looks good to me, thanks! I tested the sandbox on openedx/openedx-platform#29779 but will generally rely on @symbolist 's testing here.

[pdpinch]

We (at MIT) are eager to see this merged, but our deployment is on maple so we need some additional work before we can actually test it. However, we don't have an existing deployment of blockstore, so most of the steps after "merge and tag" are irrelevant to us.

We'll keep an eye on this, and openedx/openedx-platform#29779 to see when they merge.

[jristau1984]

[Update] T&L has pulled this into our sprint starting today. @doctoryes will be driving the work from our side. Thanks!

[symbolist]

@jristau1984 Thanks!

@doctoryes Since these changes, particularly openedx/openedx-platform#29779 are risky, will you be able to share the rollout plan and deployment windows with us whenever you have done the initial planning so we can have labxchange firefighters available to monitor the roll-out? Thanks!

[doctoryes]

@symbolist Yes, I will do so. I'm currently working on the infrastructure changes needed for LMS to access the blockstore DB. Once I have an idea of when I'll finish that work, I'll let you know.

[symbolist]

@doctoryes thanks!

[doctoryes]

@pomegranited : Some additional work has presented itself for this repo's transition to Django application:

The ATOMIC_REQUESTS: True config key was removed from the blockstore DB connection when moving the connection info to edxapp's DATABASES Django setting. As edxapp will service many views which do not connect to the blockstore DB, it adds too much unnecessary overhead to create a DB transaction on the blockstore DB upon each Django view serviced by edxapp. Instead, @transaction.atomic(using=BLOCKSTORE_DB_ALIAS) decorators need to be added in the blockstore to views which need an atomic DB transaction.

@symbolist is aware of the need for this addition - but just wanted to mention it here as well.

[pdpinch]

Is there a new etm (estimate time to merge) for this PR and it's companion openedx/openedx-platform#29779?

[doctoryes]

@pomegranited @symbolist : The work was merged to allow access from the edx.org stage and production environments to the blockstore DB. The blockstore DB should now be accessible from Django in both the edxapp web and worker instances using a DB alias of blockstore.

[openedx-webhooks]

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[symbolist]

@doctoryes Great, thanks!

Before we install this in edxapp, we should deploy the changes to the blockstore service. Can we do this on Monday?

The steps after that should be:

• Add @transaction.atomic(using=BLOCKSTORE_DB_ALIAS) decorators.
• Update blockstore package version in [BD-14] Install the blockstore app into edx-platform, behind a waffle switch openedx/openedx-platform#29779 and merge it.
• Turn on the flag on stage and do a round of testing.
• Schedule a window to turn on the flag on production. We will need FFs available to monitor and turn off the flag if needed.

Does TNL have capacity over the next two weeks to support this deployment? CC @pdpinch

[symbolist]

@pdpinch Do you need this on the head of the master branch or on a named release?

[pdpinch]

@symbolist In this repo, it can be in the master branch. I don't think BTR is ready to support blockstore as part of nutmeg.

It would be best if the openedx/openedx-platform#29779 could be in nutmeg (RC to be cut April 9)

[pomegranited]

Thank you for merging this @symbolist ! Could you (or @doctoryes ) tag the latest master so I can reference it in openedx/openedx-platform#29779? The current version is 1.2.0, though the existing tags are just for the named releases, so I'm not sure what the format should be.

[doctoryes]

@pomegranited I have created the 1.2.0 tag in the blockstore repo, so the edx-platform requirement line can now be:
-e git+https://github.com/open-craft/blockstore.git@1.2.0#egg=blockstore==1.2.0
However, ideally this repo would now be built as an installable package and uploaded to PyPI. Is that work something that's planned?

[pomegranited]

Brilliant, thank you @doctoryes ! I've updated the blockstore requirement on openedx/openedx-platform#29779.

However, ideally this repo would now be built as an installable package and uploaded to PyPI. Is that work something that's planned?

I agree, but I haven't seen that mentioned anywhere. @farhaanbukhsh or @kaustavb12 is that something that would fit into the BD-14 epic?

[doctoryes]

@pomegranited This PR caused the latest Blockstore deploy as an IDA (not as a Django app in edx-platform yet) to fail with this error:

  File "/edx/app/blockstore/blockstore/blockstore/apps/bundles/models.py", line 60, in <module>
    from .store import DraftRepo, SnapshotRepo, bytes_from_hex_str
  File "/edx/app/blockstore/blockstore/blockstore/apps/bundles/store.py", line 28, in <module>
    from .storage import default_asset_storage
  File "/edx/app/blockstore/blockstore/blockstore/apps/bundles/storage.py", line 188, in <module>
    default_asset_storage = AssetStorage()
  File "/edx/app/blockstore/blockstore/blockstore/apps/bundles/storage.py", line 141, in __init__
    self.url_backend = LongLivedSignedUrlStorage()
  File "/edx/app/blockstore/blockstore/blockstore/apps/bundles/storage.py", line 99, in __init__
    **settings.BUNDLE_ASSET_STORAGE_SETTINGS['STORAGE_KWARGS']
KeyError: 'STORAGE_KWARGS'

I see that BUNDLE_ASSET_URL_STORAGE_KEY / BUNDLE_ASSET_URL_STORAGE_SECRET are defined in Django settings - but BUNDLE_ASSET_STORAGE_SETTINGS hasn't been added yet.
Questions:

• The code currently expects a new Django setting as shown in this test code. Was that the intent?
• If so, what is each of those values expected to be? On stage/prod? Is the access_key/secret_key the same as the values in the existing key/secret? What about the other values?

This missing config will need to be fixed first before proceeding with deploying Blockstore-as-a-Django-app in edx-platform. FYI @symbolist

[pomegranited]

@doctoryes Oh bummer.. I missed that issue with my testing :(

I've submitted #166 to resolve this issue, but an interim option would be to add the following to edx-internal:

BUNDLE_ASSET_STORAGE_SETTINGS:
   STORAGE_CLASS: 'storages.backends.s3boto3.S3Boto3Storage'
   STORAGE_KWARGS: {}

As noted in #166, the storage kwargs will be pulled from the top-level settings; the only issue here is that I didn't allow that dict to be omitted.

[pomegranited]

Some additional work has presented itself for this repo's transition to Django application:

The ATOMIC_REQUESTS: True config key was removed from the blockstore DB connection when moving the connection info to edxapp's DATABASES Django setting. As edxapp will service many views which do not connect to the blockstore DB, it adds too much unnecessary overhead to create a DB transaction on the blockstore DB upon each Django view serviced by edxapp. Instead, @transaction.atomic(using=BLOCKSTORE_DB_ALIAS) decorators need to be added in the blockstore to views which need an atomic DB transaction.

@symbolist there's a problem -- if we add @transaction.atomic(using=BLOCKSTORE_DB_ALIAS) , then this works for the edxapp deployment which uses a separate blockstore database. But anyone running the default deployment will be using the edxapp default database, not blockstore.

So we're going to have to detect which database is the actual one being used to applying with this wrapper. Ugh!

[pomegranited]

NVM @symbolist , you addressed this issue on Slack. Thank you!