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: operators can prefix elasticsearch indexes for multi-tenancy #404

Merged

Conversation

keithgg
Copy link
Contributor

@keithgg keithgg commented Dec 12, 2022

Description

Adds a new configuration setting, ELASTICSEARCH_INDEX_PREFIX. If set, all Elasticsearch indexes and aliases that are generated by the application begin with the prefix.

This is to enable multi-tenancy, so that more than one version of this application can run on an ElasticSearch deployment.

Testing instructions

When testing this, be sure to clear out your current tutor install beforehand (tutor local stop and rm -r ~/.local/share/tutor)

  • Update your tutor version to at least 14.2.0: pip install --upgrade tutor[full]
  • Install the tutor-forum plugin: pip install git+https://github.com/open-craft/tutor-forum@keith/prefix-elasticsearch-indexes
  • Enable the plugin tutor plugins enable forum
  • tutor config save --set ELASTICSEARCH_INDEX_PREFIX=starting_ \
       --set FORUM_REPOSITORY=https://github.com/open-craft/cs_comments_service.git \
       --set FORUM_RELEASE="keith/prefix-elasticsearch-indexes"
    
  • tutor images build forum to build the forum image
  • tutor local quickstart Create a tutor local install
  • Verify that all indices contain the prefix starting_
    docker exec -it tutor_local-elasticsearch-1 curl http://localhost:9200/_cat/indices
    yellow open starting_comment_threads_20221212091447199 f7pYo7oDRGWWXgwxGDQoAQ 1 1 0 0 208b 208b
    yellow open starting_comments_20221212091450166        EGE-6_FpROGpDHUGXhy0ag 1 1 0 0 208b 208b
    yellow open starting_comments_20221212091447199        hXu_pX-cQ4eawUrNXYu3vg 1 1 0 0 208b 208b
    yellow open starting_comment_threads_20221212091450166 3pTwlphYQKKMCQuQMmTO9w 1 1 0 0 208b 208b
    
  • Verify that all aliases begin with the prefix starting_
    docker exec -it tutor_local-elasticsearch-1 curl http://localhost:9200/_cat/aliases
    starting_comments        starting_comments_20221212103600407        - - - -
    starting_comment_threads starting_comment_threads_20221212103600407 - - - -
    

Other information

I couldn't run the tests locally as the test documentation is outdated due to #392. If upstream confirms this is a worthwhile addition, I'll add tests.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 12, 2022
@openedx-webhooks
Copy link

openedx-webhooks commented Dec 12, 2022

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 December 12, 2022 08:37
@keithgg keithgg force-pushed the keith/prefix-elasticsearch-indexes branch from d0bab28 to 6f70cfa Compare December 12, 2022 10:49
@keithgg keithgg marked this pull request as ready for review December 12, 2022 11:27
Copy link

@gabor-boros gabor-boros left a comment

Choose a reason for hiding this comment

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

@keithgg This is good to go from my side. 👍 🎉

  • I tested what's in the test instructions
  • I read through the code

@itsjeyd itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 21, 2022
@codecov
Copy link

codecov bot commented Dec 21, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (8951368) 96.10% compared to head (e6a6009) 96.12%.

❗ Current head e6a6009 differs from pull request most recent head d997705. Consider uploading reports for the commit d997705 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
+ Coverage   96.10%   96.12%   +0.01%     
==========================================
  Files          58       58              
  Lines        4550     4568      +18     
==========================================
+ Hits         4373     4391      +18     
  Misses        177      177              
Impacted Files Coverage Δ
app.rb 89.28% <ø> (ø)
api/search.rb 98.18% <100.00%> (ø)
lib/task_helpers.rb 80.57% <100.00%> (+0.14%) ⬆️
models/comment.rb 88.09% <100.00%> (+0.19%) ⬆️
models/comment_thread.rb 92.00% <100.00%> (+0.13%) ⬆️
spec/lib/task_helpers_spec.rb 100.00% <100.00%> (ø)
spec/lib/tasks/search_rake_spec.rb 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 5, 2023
@itsjeyd
Copy link

itsjeyd commented Jan 6, 2023

Thanks for enabling tests here, @e0d!

@keithgg Before we put this up for review could you please fix the failing check?

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

keithgg commented Jan 9, 2023

Thank for checking @itsjeyd. It's a coverage issue so I need to add more tests.

@keithgg
Copy link
Contributor Author

keithgg commented Jan 12, 2023

@e0d I've added some tests for the methods I added. Do you mind enabling the tests here again?

@e0d
Copy link
Contributor

e0d commented Jan 12, 2023

@keithgg I've approved the tests again.

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

itsjeyd commented Jan 17, 2023

Hey @asadazam93, would it be possible for you to help us get this PR lined up for engineering review by the Infinity team?

@asadazam93
Copy link
Contributor

@keithgg is this also supported by AWS open search?

@keithgg
Copy link
Contributor Author

keithgg commented Jan 19, 2023

@asadazam93 It should be, because I'm making use of features that have been part of ElasticSearch since before the split. That being said, I haven't tested it with OpenSearch. Would you like me to?

Note: I've put this in Draft mode, because I have a much simpler implementation that I'm testing, which should be done later today.

Update: New changes are live. The implementation is much simpler now, negating the need to modify the functions within task_helper.rb

@keithgg keithgg marked this pull request as draft January 19, 2023 09:59
@keithgg keithgg force-pushed the keith/prefix-elasticsearch-indexes branch from e2396e6 to b0d5bcd Compare January 19, 2023 16:01
@keithgg keithgg marked this pull request as ready for review January 19, 2023 16:04
app.rb Outdated Show resolved Hide resolved
app.rb Outdated Show resolved Hide resolved
@itsjeyd itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 24, 2023
@e0d
Copy link
Contributor

e0d commented Jan 26, 2023

@mtyaka making sure that you see that there were some coverage issues.

@keithgg
Copy link
Contributor Author

keithgg commented Jan 27, 2023

Thank you @e0d. I'll sort them out on Monday.

Update: The coverage issue is in app.rb which isn't reloaded by the tests so it's a little bit tricky to fix without mangling code in other places.

I'll spend more time on it in my current sprint.

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

itsjeyd commented Feb 8, 2023

Hey @keithgg, when do you expect to be getting back to this PR?

@keithgg keithgg force-pushed the keith/prefix-elasticsearch-indexes branch 5 times, most recently from 720d3b0 to 36329fa Compare February 15, 2023 13:31
@keithgg
Copy link
Contributor Author

keithgg commented Feb 15, 2023

Hi @e0d, do you mind kicking off the tests here? They should pass now.

@e0d
Copy link
Contributor

e0d commented Feb 15, 2023

@keithgg tests running.

@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 15, 2023
@itsjeyd
Copy link

itsjeyd commented Feb 15, 2023

@asadazam93 We've got a green build here so this is ready for engineering review by your team.

@itsjeyd
Copy link

itsjeyd commented Mar 1, 2023

@keithgg Looks like this will need a rebase.

@asadazam93 If you could provide a rough estimate on when this PR might get a review, that would be great :)

Adds a new configuration setting, ELASTICSEARCH_INDEX_PREFIX.
If set, all Elasticsearch indexes and aliases that are generated
by the application begin with the prefix.

This is to enable multi-tenancy, so that more than one version
of this application can run on an ElasticSearch deployment.
@asadazam93
Copy link
Contributor

@keithgg I just have one concern. It would be great if you can test this out with open search. Rest looks good to me

@keithgg
Copy link
Contributor Author

keithgg commented Mar 7, 2023

Thanks @asadazam93. I'll test on OpenSearch and confirm if it's working as intended.

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

e0d commented Mar 8, 2023

@keithgg looks like there are some test failures.

@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 10, 2023

@asadazam93 I tested the changes on the latest OpenSearch 2.6.0 and it works as expected.

@e0d I think that's a flaky test. I've experienced some intermittent test failures on master as well. Haven't been able to figure it out as it usually passes on the next run. I've pushed an empty commit. Do you mind kicking them off again?

@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 14, 2023
@itsjeyd itsjeyd requested a review from asadazam93 March 14, 2023 14:30
@asadazam93 asadazam93 merged commit 249e461 into openedx:master Mar 16, 2023
@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.

7 participants