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

feat: shared elasticsearch #13

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

keithgg
Copy link
Contributor

@keithgg keithgg commented Jan 19, 2023

Description

This PR adds the helm chart to deploy a standalone Elasticsearch cluster. Ref overhangio/tutor-forum#4

When elasticsearch.enabled is true it will deploy a cluster with 3 replicas by default.

Operators will need to add/update the following setting in each of their instances.

K8S_HARMONY_ENABLE_SHARED_ELASTICSEARCH: true
RUN_ELASTICSEARCH: false

Caveats

  • In order for SSL to work without warnings the CA certificate needs to be mounted in the relevant pods. This is out of scope for this PR as it needs to be an instance level change, which requires more work than intended.
  • It's not possible to mount the CA certificate for the forum due to patchStrategicMerge doesn't work with jobs overhangio/tutor#791

Testing instructions

  • Follow the instructions to install the chart and rebuild the Open edX image (tutor image build openedx).
    • Or use my prebuilt docker image: DOCKER_IMAGE_OPENEDX: docker.io/keithgg/openedx:15.1.2
  • Add the configuration setting: tutor config save --set K8S_HARMONY_ENABLE_SHARED_ELASTICSEARCH=true --set RUN_ELASTICSEARCH=false
  • Run the command tutor harmony create-elasticsearch-user to set up your user on the ES cluster
  • tutor k8s start && tutor k8s init

Anything ES-related should work as normal, specifically importing a course and doing a search. You can then check that the indexes are created by running the following on one of the ES pods:

$ curl --insecure -u elastic:${ELASTIC_PASSWORD} https://localhost:9200/_cat/indices
green open .geoip_databases       22H5xGxATfexbL9sos_jQA 1 1 41 0 78.7mb 39.3mb
green open openedx-02-course_info m4cKRBuqReOhKMt2OLWzTA 1 1  0 0   452b   226

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 19, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jan 19, 2023

Thanks for the pull request, @keithgg! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

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

  • supporting documentation
  • Open edX discussion forum 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 be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@keithgg keithgg marked this pull request as draft January 19, 2023 16:11
@keithgg keithgg force-pushed the keith/shared-elasticsearch branch 2 times, most recently from d7e7323 to 9152acf Compare January 19, 2023 21:05
@keithgg keithgg marked this pull request as ready for review January 20, 2023 07:32
@e0d
Copy link

e0d commented Jan 20, 2023

@bradenmacdonald are you the appropriate reviewer?

@bradenmacdonald
Copy link
Contributor

@e0d yes I am planning to review this. But for this particular project we had said we'd like to have each PR reviewed by at least one other provider, so before it merges I'd like to have a review from someone outside of OpenCraft.

@MoisesGSalas would you be interested in reviewing this?

@MoisesGSalas
Copy link
Contributor

@bradenmacdonald sure, I will take a look as soon as possible.

@bradenmacdonald
Copy link
Contributor

Assuming you instance name is openedx-01, we'll need to generate a bcrypt hash for the user's ES password.

Add to your values.yml

...
 users:
   openedx-01: "{the bcrypt hash}"

@keithgg Maybe I'm missing something, but we need to avoid including any instance-specific setup in the chart and its values.yaml. So we can configure a "root" ElasticSearch user/password there, but if each Open edX instance needs to have its own credentials and to create its own ES user, that should happen in Tutor (probably in this repo's included Tutor plugin). Because we want the use of this chart to set up the cluster to be mostly a one-time thing, and then just run a Tutor or Grove command for each additional instance that gets installed.

@keithgg
Copy link
Contributor Author

keithgg commented Jan 30, 2023

@bradenmacdonald Ah good shout. In my head I was thinking in terms of Grove we you had an extra layer of software that will do the config for you.

Now that I think about it, it might be possible to add the users via the ES API as part of an init task. I'll check and revert back.

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jan 31, 2023
@itsjeyd
Copy link

itsjeyd commented Feb 15, 2023

Hey @keithgg, any updates on when you're planning to get back to this PR?

@keithgg
Copy link
Contributor Author

keithgg commented Feb 15, 2023

@itsjeyd I'll be working on it over the next two days.

@keithgg keithgg force-pushed the keith/shared-elasticsearch branch 2 times, most recently from 5ed20a1 to 51baa3a Compare February 20, 2023 08:56
@keithgg
Copy link
Contributor Author

keithgg commented Feb 20, 2023

@bradenmacdonald I've made the changes to simplify the setup. I created a command to set up the ElasticSearch auth that the operator will need to run once.

There wasn't a tutor hook that I could use to run it automatically, since all the current hooks are set up to run within a specific container or against a specific namespace.

@bradenmacdonald
Copy link
Contributor

Thanks @keithgg, I'll try to review in the next couple of days. Heads up that this will need to be rebased after #10 merges; I tried to merge it just now but don't have rights. Shouldn't affect most of the main focus of this PR though.

@bradenmacdonald
Copy link
Contributor

There wasn't a tutor hook that I could use to run it automatically, since all the current hooks are set up to run within a specific container or against a specific namespace.

How does this work for using shared MySQL, do you have to create the instance-specific credentials manually in that case?

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

@keithgg awesome work here 💯

Thanks for figuring this out and for working through the related issues in tutor and edx-search.

I'm happy with the code, just had some minor requests/comments. There are now quite a few merge conflicts so if you could please rebase it, I'll do a quick test on the rebased version then it's good to go from my perspective.

Then I'll approve it and hand it over to @MoisesGSalas for a final review.

README.md Outdated Show resolved Hide resolved
tutor-multi-chart/Chart.yaml Show resolved Hide resolved
tutor-multi-chart/values.yaml Show resolved Hide resolved
README.md Show resolved Hide resolved
@keithgg
Copy link
Contributor Author

keithgg commented Feb 23, 2023

Thanks for the review @bradenmacdonald. I'll go through your comments and make the necessary changes tomorrow.

@keithgg keithgg force-pushed the keith/shared-elasticsearch branch 2 times, most recently from 4440ccf to f50d89d Compare February 26, 2023 10:08
@keithgg
Copy link
Contributor Author

keithgg commented Feb 26, 2023

@bradenmacdonald I've made the requested changes. Please take a look when time allows.


There wasn't a tutor hook that I could use to run it automatically, since all the current hooks are set up to run within a specific container or against a specific namespace.

How does this work for using shared MySQL, do you have to create the instance-specific credentials manually in that case?

I'm not sure what you mean here? There isn't a shared MySQL implementation (unless I'm mistaken).

@bradenmacdonald
Copy link
Contributor

I'm not sure what you mean here? There isn't a shared MySQL implementation (unless I'm mistaken).

What I meant was that you can use Tutor with an existing shared MySQL cluster as described in the docs, and it requires a root user/password (MYSQL_ROOT_USERNAME /MYSQL_ROOT_PASSWORD) which can be per-instance or shared by all instances, but in any case init code like this will then create service-specific users.

Can we not do the same thing? It doesn't really matter which container or namespace the init script runs in, as long as it can connect to the shared ElasticSearch DB and provision a new user account over the network. But if that approach is not secure enough, we can do something else.

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Thanks @keithgg , I think this is a great start.

README.md Outdated Show resolved Hide resolved
README.md Outdated

#### Caveats

- In order for SSL to work without warnings the CA certificate needs to be mounted in the relevant pods. This is out of scope for this PR as it needs to be an instance level change, which requires more work than intended.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be confusing to say "out of scope of this PR" in the README since it won't be part of a PR once merged... maybe say "this isn't yet implemented" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @bradenmacdonald, make sense. I've clarified the wording.

@bradenmacdonald
Copy link
Contributor

@MoisesGSalas Once you are back and have time, could you please do the final review on this? We can merge it once you've approved it too.

Copy link
Contributor

@MoisesGSalas MoisesGSalas left a comment

Choose a reason for hiding this comment

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

Sorry for the late response @keithgg, I left a few comments so you can take a look when you have a chance.

tutor-multi-chart/values.yaml Outdated Show resolved Hide resolved
@keithgg keithgg force-pushed the keith/shared-elasticsearch branch 2 times, most recently from 5607268 to 104568c Compare March 2, 2023 16:05
Copy link
Contributor

@MoisesGSalas MoisesGSalas left a comment

Choose a reason for hiding this comment

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

Seems like it's working fine now. Thanks a lot @keithgg.

@keithgg keithgg force-pushed the keith/shared-elasticsearch branch from 104568c to 1b9920f Compare March 5, 2023 16:20
@keithgg
Copy link
Contributor Author

keithgg commented Mar 5, 2023

Thank you for the review @MoisesGSalas!

I'm not sure what you mean here? There isn't a shared MySQL implementation (unless I'm mistaken).

What I meant was that you can use Tutor with an existing shared MySQL cluster as described in the docs, and it requires a root user/password (MYSQL_ROOT_USERNAME /MYSQL_ROOT_PASSWORD) which can be per-instance or shared by all instances, but in any case init code like this will then create service-specific users.

Can we not do the same thing? It doesn't really matter which container or namespace the init script runs in, as long as it can connect to the shared ElasticSearch DB and provision a new user account over the network. But if that approach is not secure enough, we can do something else.

@bradenmacdonald we can, but that means we'll need to add explicit steps for the user to retrieve the admin password and add it to their config.yml. My earlier understanding was that we wanted to limit the number of setup steps users need to run so I automated as much as I could.

If you're happy, I'll add this as part #23, since we'll need to retrieve the CA cert from the tutor-multi as well.

@bradenmacdonald
Copy link
Contributor

@keithgg

My earlier understanding was that we wanted to limit the number of setup steps users need to run so I automated as much as I could.

Yeah, alright. Let's track this is #23 as you said, and see what approaches we can come up with.

@itsjeyd
Copy link

itsjeyd commented Mar 8, 2023

@keithgg Is this PR blocked on #23 now, or could we merge it once tests are green (since @bradenmacdonald and @MoisesGSalas already approved the changes)?

@itsjeyd itsjeyd added needs test run Author's first PR to this repository, awaiting test authorization from Axim and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 8, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 8, 2023
@keithgg
Copy link
Contributor Author

keithgg commented Mar 13, 2023

@itsjeyd this can be merged. The remaining work will take place in #23. I don't have permissions to merge though. @bradenmacdonald are you able to?

@bradenmacdonald bradenmacdonald merged commit 0a15bdc into openedx:main Mar 13, 2023
@bradenmacdonald bradenmacdonald deleted the keith/shared-elasticsearch branch March 13, 2023 16:30
@bradenmacdonald
Copy link
Contributor

Done!

@openedx-webhooks
Copy link

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

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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants