WIP: EE8010 fall back to the default search functionality when elasticsearch is down
What does this MR do?
Adds a sidekiq worker that checks the health of Elasticsearch (via it's healthcheck endpoint), stores if it's healthy in the database and adjusts which search is used based on the state, either the elasticsearch search or the default internal one.
What are the relevant issue numbers?
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
EE specific content should be in the top level /ee
folder -
For a paid feature, have we considered GitLab.com plans, how it works for groups, and is there a design for promoting it to users who aren't on the correct plan? -
Security reports checked/validated by reviewer
Merge request reports
Activity
@rpaik @amulvany mentioning you per gitter conversation, sorry for the duplicate, just renaming the branch to be the correct issue number.
Edited by Sean Smithadded 1st contribution Community contribution labels
added advanced search label
added 1 commit
- 4c018e8c - Update form and added a quick piece about troubleshooting
added 500 commits
-
4c018e8c...d13bb8a2 - 498 commits from branch
gitlab-org:master
- 2baaf384 - Initial implementation (not quite working) to fall back to the default search...
- 817e1417 - Update form and added a quick piece about troubleshooting
-
4c018e8c...d13bb8a2 - 498 commits from branch
added 1 commit
- 430d42ea - Initial implementation (not quite working) to fall back to the default search...
So my current question is that I want to store the state of if elasticsearch is healthy, application settings seems convenient (even if the worker doesn't work yet) but also doesn't seem like the right place. One thing i had thought of was a new table to keep track of this (and more) information for an enhanced information like disk usage and stuff on the elasticsearch nodes which is available via the API.
/cc @vsizov
- I would prefer storing this sort of data purely in Redis instead of the database. Even if that record is cached in Redis like it's now.
- If ES server was down for some time, some records need to be reindexed. This MR will make it look like ES down is something expected and can be easily handled while it's not really true. I don't have a strong opinion on this one but I think all the efforts should be spent on making ES cluster reliable instead.
/cc @smcgivern @mdelaossa
@wwsean08 thanks for working on this!
💯 One thing i had thought of was a new table to keep track of this (and more) information for an enhanced information like disk usage and stuff on the elasticsearch nodes which is available via the API.
In &428 (closed) we have some other features related to this. In particular, this sounds very similar to https://gitlab.com/gitlab-org/gitlab-ee/issues/2973, although as @vsizov says, we don't necessarily need to store this in the database.
@mdelaossa would you mind reviewing this MR, please?
assigned to @mdelaossa
If ES server was down for some time, some records need to be reindexed. This MR will make it look like ES down is something expected and can be easily handled while it's not really true. I don't have a strong opinion on this one but I think all the efforts should be spent on making ES cluster reliable instead.
While I agree with this, my thought (which I haven't implemented yet) was to add some sort of notice to administrators to warn them about the situation and that some data may need to be reindexed.
The goal is that I don't want to have some sort of interrupt blocking users completely and requiring me to wake up in the middle of the night as the on call to click one button and disable the advanced search.
@smcgivern I hadn't seen the epic but I could see this being useful for #2973 (closed).
Also one question, I realize I'll have to rebase but I noticed that one of the tests failing is because i've added a couple of lines to a file, I assume that is an ok test to fail (adding lines that say ES is down so advanced search is temporarily disabled)
Thanks for the contribution @wwsean08!
Also one question, I realize I'll have to rebase but I noticed that one of the tests failing is because i've added a couple of lines to a file, I assume that is an ok test to fail (adding lines that say ES is down so advanced search is temporarily disabled)
If you're talking about the
ee-specific-lines-check
test, you don't have to worry about it - we'll fix that up for you later following these guidelinesSince this is still a WIP (and your commit says it's not quite working yet), I'll only take a cursory look now and save the in-depth review for when you fell this MR is ready :)
166 166 } 167 167 end 168 168 169 def elasticsearch_healthy? Gitlab::CurrentSettings
doesn't seem like the right place for this, how about creating a service instead (underee/app/services
)?The service could check redis for the value and if it's stale then update the value at that moment - this would also remove the need to have a separate sidekiq worker
changed this line in version 6 of the diff
2506 2507 t.datetime "created_at", null: false 2507 2508 t.datetime "updated_at", null: false 2508 2509 t.index ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree 2510 t.index ["path"], name: "index_redirect_routes_on_path_text_pattern_ops", using: :btree, opclasses: {"path"=>"varchar_pattern_ops"} changed this line in version 6 of the diff
1 # frozen_string_literal: true 2 3 class AddElasticsearchHealthyColumn < ActiveRecord::Migration[5.0] 4 include Gitlab::Database::MigrationHelpers 5 6 DOWNTIME = false 7 8 disable_ddl_transaction! 9 10 def up 11 add_column_with_default(:application_settings, :elasticsearch_healthy, :boolean, default: false, allow_null: false) changed this line in version 6 of the diff
The goal is that I don't want to have some sort of interrupt blocking users completely and requiring me to wake up in the middle of the night as the on call to click one button and disable the advanced search.
Ouch, that is definitely no fun. It occurs to me that you could also possibly build this service using the existing GitLab API, by updating
application_settings
to disable ES? That's not to say we shouldn't have this in the product, just another option that might help ease the pain.assigned to @wwsean08
added 3748 commits
-
6d89d554...abe49170 - 3747 commits from branch
gitlab-org:master
- dfd1fc1e - Partial reworked implementation, running into some exceptions to fix still
-
6d89d554...abe49170 - 3747 commits from branch
added 105 commits
-
dfd1fc1e...016c4c1b - 104 commits from branch
gitlab-org:master
- 5e9c00fb - Partial reworked implementation, running into some exceptions to fix still
-
dfd1fc1e...016c4c1b - 104 commits from branch
@wwsean08 please feel free to reach out if you have any trouble. We're here if you have any questions :)
Thanks, will do @mdelaossa, I'm finally getting time to pick this back up
added 119 commits
-
5e9c00fb...2a9d8bed - 118 commits from branch
gitlab-org:master
- 306e3a1a - Partial reworked implementation, running into some exceptions to fix still
-
5e9c00fb...2a9d8bed - 118 commits from branch
mentioned in issue #11249 (closed)
Great @wwsean08!!! Do you need another round of review from @mdelaossa?
@wwsean08 did you mean to close this MR?
added Enterprise Edition label