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

Prevent refresh fields error from breaking index patterns management page #11885

Merged

Conversation

@chrisronline
Copy link
Contributor

commented May 18, 2017

Closes #9224

Summary

This PR removes a single line of code that is preventing the Discovery and Index Patterns Management page (and potentially others) from loading correctly. This is not affecting 100% of users as the execution path only occurs in a specific scenario.

Background

The referenced issue was opened due to some users experiencing a problem where they were unable to delete a kibana index pattern if the respective ES index had been removed. I had a hard time reproducing the issue as the common code path worked fine, but thanks to a stack trace from @danielmotaleite, I realized it was in a certain code path that my setup wasn't calling.

The code path starts in route resolution if the page is requesting index patterns (through courier.indexPatterns., like on the Discovery page and the Index Patterns Management page, and eventually leads to an attempt to refresh an index pattern's fields from ES if the index pattern has no fields or does not have a valid field (note: I'm not exactly sure what would cause this scenario). If this request fails (which it will if the matching ES index no longer exists), the error is rethrown in the stack which is breaking the mentioned pages.

Here is what it looks like:
discover
index_patterns

Steps to Reproduce

  1. Create an ES index and populate some data (makelogs works)
  2. Create an index pattern in Kibana referencing that index
  3. Delete the ES index (using curl/console)
  4. Delete the fields part of the index pattern (or just remove the checked fields)
POST /.kibana/index-pattern/<insert index pattern>/_update
{
  "script": "ctx._source.remove(\"fields\")"
}
  1. Refresh Kibana
  2. Attempt to load the Discovery page or Index Patterns Management page (with the index as the selected one)
  3. Verify behavior exhibited in above screenshots

Solution

The solution is a simple one line change - stop throwing the error back into the stack if the error indicates a missing ES index (which is represented by the IndexPatternMissingIndices error). The error itself is still surfaced in a notify.error call so the user will know about it, but the broken pages will load properly now.

Here is what the fix looks like:
discover_fixed
index_patterns_fixed

From here, the user can successfully remove the orphaned index pattern (which they were unable to do before).

chrisronline added some commits May 18, 2017

No longer throw an error to the rest of the stack if there is an erro…
…r refreshing fields for an index pattern
@ycombinator

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

Functionality and code LGTM.

I think this fix is an improvement over the current UX. However, I'm a bit concerned that we don't understand still why an index pattern might get into a bad state (either no fields at all or no searchable or aggregatable fields) in the first place. While this fix would improve the UX it could also potentially mask an underlying issue.

Now that we have figured out a fix and have it ready to go, I'd like to try and spend some more time trying to investigate why we might be getting into the bad index pattern state. I've asked a question on the issue; lets see if that gives us any clues. If we don't get anywhere for a few more days (arbitrarily saying 5), lets proceed with this PR.

@chrisronline chrisronline removed the request for review from bevacqua May 18, 2017

@chrisronline

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

@Bargs - Do you mind looking over this? I noticed you had a hand in parts of this code path

@chrisronline chrisronline self-assigned this May 18, 2017

@ycombinator

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

That @Bargs, has a hand in everything ;)

@Bargs

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

That @Bargs, has a hand in everything ;)

Everything that breaks :)

@Bargs Bargs self-requested a review May 18, 2017

@Bargs

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

I'm a bit concerned that we don't understand still why an index pattern might get into a bad state (either no fields at all or no searchable or aggregatable fields) in the first place. While this fix would improve the UX it could also potentially mask an underlying issue.

There are two ways the index pattern can end up with no searchable/aggregatable info that I know of.

  1. Index pattern without searchable/aggregatable is created directly in ES via an external process. This usually happens when migrating from 2.x to 5.x. If the user doesn't migrate the data indices the pattern points to, field stats will fail and Kibana will never be able to fill in the searchable/aggregatable info.

  2. This is a newer one. If you have a geo field and you upgrade from 2.x to 5.3 your field stats request will fail due to a bug in ES. Kibana will never be able to retrieve searchable/aggregatable info.

Basically anything that causes field_stats to fail after upgrading from 2.x to 5.x will put the index pattern in a bad state. Things should hopefully be better once we switch to the field_caps API.

@Bargs

Bargs approved these changes May 18, 2017

Copy link
Contributor

left a comment

LGTM

@w33ble

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

If we don't get anywhere for a few more days (arbitrarily saying 5), lets proceed with this PR.

Index pattern without searchable/aggregatable is created directly in ES via an external process. This usually happens when migrating from 2.x to 5.x. If the user doesn't migrate the data indices the pattern points to, field stats will fail and Kibana will never be able to fill in the searchable/aggregatable info.

I don't think we need to block this PR, or even keep the other issue. It sounds like there are two problems here, so if @chrisronline just opens a new issue about the 2.x to 5.x migration issues, pings the users who are active on 9224, we can move forward with this fix. At least then users won't get stuck, and we get this fix into the next release, pending a more inclusive fix. Thoughts?

@w33ble

w33ble approved these changes May 18, 2017

Copy link
Contributor

left a comment

LGTM

@chrisronline

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

@Bargs Is there already a ticket for the 2.x -> 5.x migration issue, ala the first issue you mentioned above?

@Bargs

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

#9571 is the closest I think. It's a symptom of not having any indexed data at least. I wouldn't say there's any migration issue to fix really. Field stats doesn't work without indexed data and there's nothing we can do about that beyond allowing the user to delete the index pattern if they don't plan on ever adding data. If they do add data after migrating the index pattern, they simply need to refresh their field list.

@w33ble w33ble added the v5.4.1 label May 18, 2017

@w33ble

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

Oh, right, of course. I didn't grok that from the original explanation, I read it as a migration issue. So yeah, I'm not sure there's anything else for us to fix, the field_stats thing just fails like we see, and it's going to be replaced anyway.

@chrisronline

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

Thanks @Bargs

We don't seem to have any evidence indicating that there is an ongoing issue with migrated indices in 5.x. The reports only indicate a broken experience with deleted ES indices so I don't think there is really anything to do more with this but ship this particular fix.

@ycombinator
Copy link
Contributor

left a comment

Now that we understand root cause (see recent comments on issue), this fix LGTM.

@chrisronline chrisronline merged commit bd66f16 into elastic:master May 18, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

chrisronline added a commit that referenced this pull request May 18, 2017

Prevent refresh fields error from breaking index patterns management …
…page (#11885)

* No longer throw an error to the rest of the stack if there is an error refreshing fields for an index pattern

* Only prevent rethrowing for the certain error scenario

chrisronline added a commit that referenced this pull request May 18, 2017

Prevent refresh fields error from breaking index patterns management …
…page (#11885)

* No longer throw an error to the rest of the stack if there is an error refreshing fields for an index pattern

* Only prevent rethrowing for the certain error scenario
@chrisronline

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

Backport

5.x: 096db27
5.4: 25d27b8

@chrisronline chrisronline deleted the chrisronline:fix/blocking-field-refresh-error branch May 18, 2017

snide added a commit to snide/kibana that referenced this pull request May 30, 2017

Prevent refresh fields error from breaking index patterns management …
…page (elastic#11885)

* No longer throw an error to the rest of the stack if there is an error refreshing fields for an index pattern

* Only prevent rethrowing for the certain error scenario

Dreadnoth added a commit to Dreadnoth/kibana that referenced this pull request Aug 8, 2017

Prevent refresh fields error from breaking index patterns management …
…page (elastic#11885)

* No longer throw an error to the rest of the stack if there is an error refreshing fields for an index pattern

* Only prevent rethrowing for the certain error scenario
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.