Blockstore as App [SE-3321]

[symbolist]

Description

This PR makes the changes necessary for the Blockstore as app ADR: #71.

Changes:

• Move the HTTP API from blockstore/apps/api to blockstore/apps/rest_api.
• Copy over the Python API interface from https://github.com/edx/edx-platform/tree/7804511/openedx/core/lib/blockstore_api to blockstore/apps/api.
• Update the Python API to use the models directly.
• Enhance tests.
• Package the apps.

Once the above changes have been made we will need to update edx-platform with the following changes:

• Install the blockstore package.
• Copy over the data from the blockstore database to the edxapp database and shift to using the Python API included in this repo instead of the one in https://github.com/edx/edx-platform/tree/7804511/openedx/core/lib/blockstore_api. The logistics of this still need to be decided.

Author Comments, Concerns, and Open Questions

TODO

Test Instructions

TODO

TODOs

If anything isn't yet done, list it here

• Squash before merging

[openedx-webhooks]

Thanks for the pull request, @symbolist! I've created OSPR-5143 to keep track of it in JIRA.

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

[kdmccormick]

This is exciting! Thanks for sharing your work so far.

A few general comments and questions:

1. When we migrate from Blockstore-as-an-App to Blockstore-as-a-Service, there will need to be a period of dual support, so we can easily deploy and if necessary roll back with zero downtime. Given that, do you think it'll be feasible to maintain this repo as both a runnable service and an install-able app for a bit? Or would it be easier to clone the repo? Personally, I have no preference.
2. My assumption is that, once this transition is complete, we'd remove that dual support, such that Blockstore would only be installable as an app. Do you agree, or are you hoping that Blockstore-as-a-Service would remain an option to Open edX operators? My thinking it that we shouldn't support a use case that we don't use ourselves; let me know what you think.
3. Could you mark this PR as a draft?

[symbolist]

@kdmccormick

When we migrate from Blockstore-as-an-App to Blockstore-as-a-Service, there will need to be a period of dual support, so we can easily deploy and if necessary roll back with zero downtime. Given that, do you think it'll be feasible to maintain this repo as both a runnable service and an install-able app for a bit? Or would it be easier to clone the repo? Personally, I have no preference.

Yup, initially this will have dual support. The way I am planning it is:

1. Move as much code (and all that mutates data) from the views in to the Python API.
2. Deploy it to production for a few days so we can be sure this implementation of the Python API has the exact same behavior as the one in edx-platform that talks to the HTTP API.
3. Shift edx-platform to use the Python API directly instead of going through the HTTP wrapper.

My assumption is that, once this transition is complete, we'd remove that dual support, such that Blockstore would only be installable as an app. Do you agree, or are you hoping that Blockstore-as-a-Service would remain an option to Open edX operators? My thinking it that we shouldn't support a use case that we don't use ourselves; let me know what you think.

Yes, I am in agreement with you that we should not continue maintaining dual support. Python APIs can be much richer than the RESTish APIs and we will likely be able to do more performance optimizations if not restricted by trying to maintain support for an HTTP API.

After have worked on this and a user facing platform on top of it, I think it would be interesting to explore other service boundaries. For example there are some very interesting ideas here: https://openedx.atlassian.net/wiki/spaces/~dormsbee/pages/135103039/XBlock+Runtime+Isolation

[kdmccormick]

@symbolist All of that sounds perfect. Looking forward to helping with the migration.

The database copy will be interesting; I've never done that before. A temporary write-freeze for bundles will probably be needed. But we'll get there when we get there.

And XBlock runtime is always an interesting and relevant topic--sounds like a great next thing to work on.

[natabene]

@symbolist Just checking if you need anything from us to keep moving this forward.

[symbolist]

@natabene Thanks for checking in! I am focusing on wrapping up BD-04 in the next few weeks. Will be picking this back up after that. 🙂

[natabene]

@symbolist Just checking if there are any updates with this.

[symbolist]

@natabene We wrapped up BD-04 last week so this is next on my list to focus on. 🙂

[symbolist]

I have completed the first draft of the Python API implementation. The tests in https://github.com/edx/edx-platform/blob/master/openedx/core/lib/blockstore_api/tests/test_blockstore_api.py are working against it. Next step is to get this hooked up inside edx-platform and test that content libraries and LabXchange features work with this version of the API.

[kdmccormick]

Looking good @symbolist . I'm excited to do this migration! Let us know when you're ready to talk logistics.

In terms of the code, have you considered exposing the Python API from blockstore.api instead of blockstore.apps.api?

[symbolist]

@kdmccormick

Let us know when you're ready to talk logistics.

Yup, will do!

In terms of the code, have you considered exposing the Python API from blockstore.api instead of blockstore.apps.api?

Yup, that makes sense.

[natabene]

@symbolist Please let us know once it is ready again for our review.

[natabene]

@symbolist Just checking if you are still interested in working on this.

[symbolist]

Hi @natabene Yes, definitely. The code is actually mostly done. The next part is figuring out the deployment strategy - since we will need to merge the data from the blockstore database into the edxapp database with minimal downtime. I will be able to start looking at it start of August. Thanks for following up!

[symbolist]

@ormsbee @kdmccormick I am going to resume working on this. The next thing we need to figure out is how to deploy this change with minimal downtime (so that the remaining changes can be done accordingly and I am assuming the ops team will need some heads-up to plan this out).

Option A.

1. Add a second database config to edx-platform which will point to the blockstore db.
2. Install blockstore as an app inside edx-platform and release it.
3. Rollback in case there is any issue; fix the issue and try again.
4. After it has been confirmed that everything is working as expected with the Python API, copy over the tables from the blockstore db to the edxapp db and turn off the Blockstore service.

Option B.

1. Copy over the tables from the blockstore db to the edxapp db and update the Blockstore service to use the edxapp db.
2. Install blockstore as an app inside edx-platform and release it.
3. Rollback in case there is any issue; fix the issue and try again.
4. After it has been confirmed that everything is working as expected with the Python API, we can turn off the Blockstore service.

Option A will require some more work but ops may be more comfortable with it in case 3 takes long.
Option B will be somewhat simpler to execute.

Thoughts? 🤔

[kdmccormick]

@symbolist I like Option A, with a couple modifications:

1. Point edxapp at the blockstore db.
2. Install the blockstore app into edx-platform, but put its use behind a Django setting feature flag, so that we can flip between using the app and calling out the service.
3. Enable the setting, disable + fix as necessary.
4. Remove the setting and the extra code once we're stable.
5. Shut off the blockstore service.
6. If SRE wants to, they could merge the blockstore db into the edxapp db. Or, we could keep them separate.

How does that sound?

It's more work on our part, but only step 6 would require SRE intervention, so it'd allow us to move independently of their schedule. I'll reach out to them soon and see if they agree.

[symbolist]

@kdmccormick Sounds like a plan to me. 👍🏽

[ormsbee]

If SRE wants to, they could merge the blockstore db into the edxapp db. Or, we could keep them separate.

I think this sequence is fine, but I do think that the default should be in the same database, and that we should move it there when we can. We've been bitten by various weird things when having the CSMH table in a separate DB (e.g. connection limits were different because of the relative size of the instances, transaction errors and occasional state mismatch, etc.)

[natabene]

@symbolist Is this Blended work?

[symbolist]

@natabene Nope, this is not Blended work.

[symbolist]

@kdmccormick Did you get a chance to discuss Option A with SRE? I am syncing up the Python dependencies between this and edx-platform. After that the next step is to make this a package and install it in edx-platform. If we are going to go ahead with Option A I'll set up the needed configuration hooks when doing that.

[natabene]

@kdmccormick Will you be the reviewer for this?

[kdmccormick]

So sorry @symbolist and @natabene , I missed the notification from your comments somehow.

Usman: Yup, we'll be going with Option A. You can spy on https://openedx.atlassian.net/browse/TNL-8705 and its subtasks if you want to see exactly how we plan on carrying out the migration. Once you let us know that this PR is ready for review, we should be able to pull its review and then the migration ticket into our next sprint.

Natalia: Someone from T&L will review, not sure exactly whom.

[symbolist]

@kdmccormick That is great, thank you!

[symbolist]

@kdmccormick @arbrandes I do not recall why I made this change but it caused a change in the table names that were being queried (hence the 404s). And which is why it wasn't caught when testing with a fresh devstack+CI run.

Fix coming up.

[jvdm]

@kdmccormick:

Django uses the app label to define the actual table name it uses.. In particular, it prefixes the application tables with it. The devstack didn't capture this because it runs migrations by default during provisioning, although it's easy to see it in the devstack if you (a.) create a library, (b.) deploy the new code, (c.) read the library. It fails with a bundle not found. This was not captured in our acceptance criteria for the patch.

In any case, next steps from here:

1. Keep bundles as the default name. Might impact the maintainability of the platform since it "taints" the app namespace with a very common and unidentifiable name called "bundles", where operators and maintainers will potentially get confused.
2. Explicitly see the table name in the bundles' application model to "blockstore_bundles". We keep using the label, which helps with the application's namespace, but still affects the database names.
3. Ship migrations to rename the database tables. This is the preferred option in my opinion. I need to investigate further what's the best approach here. But, the good thing is, we don't have to do it right now. We can ship this patch with (1.) and do the migration later, with Blockstore-as-a-service, then deploy Blockstore-as-an-app.

But, while I take a look at (3.), do you have any feedback to share on these? Wanna make sure we are on the same page this time and avoid the bad deployment on the next patch.

[jvdm]

(Oh, almost forgot, I think we should label the application "blockstore_bundles" rather than "blockstore_app_bundles" since the "app" is redundant.)

[symbolist]

@jvdm Renaming to blockstore_bundles sounds good (I do not recall if this was the reason why I updated the label or if there was something else too) but how about we keep the table names the same for now if possible and then update them when moving them inside edxapp?

[jvdm]

@symbolist:

how about we keep the table names the same for now if possible and then update them when moving them inside edxapp?

Is that Option 3, or do you mean something different?

[jvdm]

I am following up on this on this PR: #140

The idea is to merge that one, integrate the changes here, at finally tackle the remaining items in the description with a cleaner patch.

CC @symbolist @kdmccormick

[pomegranited]

@symbolist I'm working on rebasing this PR with latest changes in master, and can't work out from the discussion here what is meant to happen with the application and table names.

#140 kept bundles as the migration app name, but your branch started here changed the application label to blockstore_app_bundles. The discussion above also mentions blockstore_bundles.

I understand that at some point, we're going to have to copy the original data from the blockstore database into the edxapp database. But for where this PR is in the migration, what should the app label be?

[symbolist]

@pomegranited The table names need to remain the same for now since there is data in the system. I had changed the app labels to add namespacing but if it is not needed for other reasons we can leave it as is for now and consider it when moving the tables into the edxapp db.

[jvdm]

@kdmccormick:

1. Point edxapp at the blockstore db.

Curious to understand the operational steps here: are talking about a Django database routing for Blockstore models? Or some other mechanism?

[arbrandes]

@symbolist and @jvdm, what is the status of this PR? João tells me it might have been superseded by #140, but I'm not sure.

[jvdm]

@symbolist and @jvdm, what is the status of this PR? João tells me it might have been superseded by #140, but I'm not sure.

From what I know this PR's obsolete after #140 was merged. The additional changes required to support Blockstore as an app are to be done in the platform. @symbolist did the original planning though, so, let's get his confirmation before closing the PR and moving on.

[symbolist]

@arbrandes @jvdm Once this PR is rebased on master, it will have Python API implementation that will be used from edx-platform contained in the commit bf2aba8. #140 (and other earlier PRs) contained the other commits in this repo and some other work to allow installing this package in edx-platform.

[natabene]

Looks like this is still in the works.

[symbolist]

Everyone, @natabene, apologies for the delay here! @pomegranited will be taking on the remaining work on this (thanks so much!) which you can follow on #149. 🙂 I am going to close this PR.

[openedx-webhooks]

@symbolist Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.