Skip to content
Snippets Groups Projects

WIP: EE8010 fall back to the default search functionality when elasticsearch is down

Closed Sean Smith requested to merge (removed):ee-8010-default-search-rollback into master
3 unresolved threads

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?

#8010

Does this MR meet the acceptance criteria?

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • @rpaik @amulvany mentioning you per gitter conversation, sorry for the duplicate, just renaming the branch to be the correct issue number.

    Edited by Sean Smith
  • Sean Smith added 1 commit

    added 1 commit

    • 4c018e8c - Update form and added a quick piece about troubleshooting

    Compare with previous version

  • Sean Smith added 500 commits

    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

    Compare with previous version

  • Sean Smith added 1 commit

    added 1 commit

    • 430d42ea - Initial implementation (not quite working) to fall back to the default search...

    Compare with previous version

  • Sean Smith added 208 commits

    added 208 commits

    • 430d42ea...ad4d5788 - 207 commits from branch gitlab-org:master
    • 6d89d554 - Initial implementation (not quite working) to fall back to the default search...

    Compare with previous version

  • 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.

    • 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?

  • 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 guidelines

    Since 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?
  • 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"}
  • 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)
  • 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.

  • Yeah I've definitely started going down the external application updating the application_settings to disable ES, my team vetoed that idea for us at least.

  • Sean Smith added 3748 commits

    added 3748 commits

    • 6d89d554...abe49170 - 3747 commits from branch gitlab-org:master
    • dfd1fc1e - Partial reworked implementation, running into some exceptions to fix still

    Compare with previous version

  • Sean Smith added 105 commits

    added 105 commits

    • dfd1fc1e...016c4c1b - 104 commits from branch gitlab-org:master
    • 5e9c00fb - Partial reworked implementation, running into some exceptions to fix still

    Compare with previous version

  • @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

  • Sean Smith added 119 commits

    added 119 commits

    • 5e9c00fb...2a9d8bed - 118 commits from branch gitlab-org:master
    • 306e3a1a - Partial reworked implementation, running into some exceptions to fix still

    Compare with previous version

  • mentioned in issue #11249 (closed)

  • Great @wwsean08!!! Do you need another round of review from @mdelaossa?

  • closed

  • Contributor

    @wwsean08 did you mean to close this MR?

  • Please register or sign in to reply
    Loading