-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
The defaultIndex
advanced setting breaks after 8.0 upgrade
#133241
Comments
Pinging @elastic/kibana-core (Team:Core) |
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
This could be addressed by modifying the 8.0 migration in the kibana/src/core/server/ui_settings/saved_objects/migrations.ts Lines 78 to 106 in 3730dd0
We can calculate if the let newDefaultIndex: string | undefined;
if (doc.namespace !== undefined) {
newDefaultIndex = deterministicallyRegenerateObjectId(doc.namespace, 'index-pattern', doc.attributes.defaultIndex);
} Here is the algorithm for generating the new object ID: kibana/src/core/server/saved_objects/migrations/core/document_migrator.ts Lines 838 to 848 in a044026
It's not great practice to do this, but we could change the existing 8.0 migration in 8.3.1/8.4.0 so that it also fixes the default index ID. We can't add a new migration for 8.3.1 because we have no way of determining if a defaultIndex has already been changed at that point. WDYT @elastic/kibana-core ? |
Yeah, I don't love this idea, but I also don't see any other alternatives if this is something we want to fix. I guess I'd be comfortable with it as this technically represents a sort of data loss, and making a change to an existing migration like this is not without precedent. But I think we'd need to clearly document it in the code so there's an explanation for why the migration's implementation is different in the 8.3 branch. Will let others chime in though, and try to poke holes in this. |
The suggested setting migration looks fine to me, however I don't think we can realistically 'patch' the existing 8.0 migration to apply it. First obvious consequence would be that customers already in a 8.x version would not benefit from the fix. Also, as we can't backport lower than 8.3.x anyway, I don't really see any benefit on breaking the rule here. I think our only option would be to add it as a WDYT? |
The "need a way to detect if the current That's the only reason I suggested patching the 8.0 migration -- it won't help folks who've already upgraded to 8.x, sure, but the vast majority of our users haven't upgraded to 8.x yet, so this would help prevent the problem for them in the future (assuming they upgrade straight from 7.17 to 8.3.1+) |
I think what Joe was suggesting here was not backporting to 8.0, but rather changing the 8.0 migration on the |
Yes, exactly, sorry I tried to make that clear 😅 |
Yea, I get that. And as I said, I don't like it, for the reasons I enumerated in my previous comment. We won't fix the problem for users already in a
TIL. Any example coming to mind? Not for the past 2.5 years, at least that I'm aware of.
You mean we don't have any way to detect/guess if an ID is pre or post shareable-migration rewrite? If that's the case, that's plain terrible... But then I don't have a better idea than your suggestion. |
Well, we regenerate the ID using a hash function (uuidv5) so there's no way to reverse the hash. We could attempt to resolve the index pattern -- but migrations must be done in isolation, we can't query any other documents. Another option: |
Not sure which option I prefer to be honest. @lukeelmers, what do you think about this? |
My gut tells me adding a post-start custom migration is the way to go here to avoid the code discrepancies we'd have in different branches. Since that approach seemed to work out fine for the APM case, it looks like the best of the choices we have, certainly winning against hacking code for specific branches. |
After chatting with Pierre and Tina offline, I decided to take a different approach in #133339 --
|
Sorry not to respond here earlier.
FWIW this was the example I was thinking of, where a visualizations migration was retroactively added for an older version, and then later fixed in the same migration fn when a bug was found. I think there have been a few similar vis-related migrations that got fixed this way. I don't think it's necessarily something we should be doing, just saying that this wouldn't be a first :)
Took a look at the PR, and I think that approach is a good middle ground. It feels less hacky than the other 2 options discussed in this thread since we are mostly plugging into existing functionality 👍 |
Bad bot. Closing... |
Originally described in #46124 (comment)
Kibana version: 8.0, 8.1, 8.2, 8.3
Describe the bug:
Advanced settings (aka UI settings) are persisted in the
config
saved object, and there is one of these per space.In #100489 we migrated most isolated saved object types (including index patterns AKA data views) to be share-capable in the 8.0 release.
This means that, for any data view that was created in a custom space, its ID would be changed upon upgrading to 8.0.
The problem is that we never added a corresponding migration for the
defaultIndex
advanced setting, that is technically an object link and it does not use thereferences
field so it was never migrated the way it should have been.The
defaultIndex
is sort of self-healing (it will automatically be set if the user has appropriate privileges), but this is frustrating for clusters that have mostly read-only users with lots of spaces, and the if there are multiple data views then the correct one will be lost.Steps to reproduce:
defaultIndex
in advanced settings to your index pattern's IDdefaultIndex
advanced setting has the oldindex patterndata view IDExpected behavior:
The
defaultIndex
should be migrated properly.Screenshots (if relevant):
The text was updated successfully, but these errors were encountered: