caching or versioning problem with multiple Kibana instances

[LeeDr]

If you have 2 or more instances of Kibana sharing the same .kibana index, they can clobber each other's changes with values they have in cache.

To Reproduce;

1. Start 2 instances of Kibana either on different machines or on different ports. I'm running #1 on port 5601 and #2 on port 5602 on the same machine.
2. Go to Settings > Advanced on both instances
3. on #1 change discover:sampleSize (Default: 500) to 499
4. on #2 change discover:sampleSize (Default: 500) to 501
5. on #1 change courier:maxSegmentCount (Default: 30) to 29
6. refresh both instances, and/or check the config object http://localhost:9200/.kibana/config/_search

Expected discover:sampleSize (Default: 500) to be 501 since it was the last change made to that setting.

Actual discover:sampleSize (Default: 500) to 499 (probably because #1 had 499 cached when it wrote the courier:maxSegmentCount so it wrote the whole config object)

If possible, it should know the version of the config object it has cached.

When it tries to write the object it should check the version first and, if later, show the user an error if it is a later version, and refresh the page to show the current version. The user can try to save again, or has to make their change again and save again.

OR

When it tries to write the object it should check the version first and, if later, get the latest version and re-apply the change and try to save it again (again checking the version).

OR

When it tries to write the object the write should include the next version and fail if that version already exist in Elasticsearch? Not sure which of these functionalities is possible. Then handle the error by getting the latest version, and re-applying the change. This could be the best performing solution because it only takes one round-trip in most cases.

[LeeDr]

#21947 fixes the issue for index patterns. If a user tries to save a change and their version is not the latest (someone else changed the index pattern while they still had a copy in their cache) then their save is rejected with a message telling them to refresh the page and try again.

We still have the issue on other saved object types.

[elasticmachine]

Pinging @elastic/kibana-operations (Team:Operations)

[tylersmalley]

@LeeDr is this still valid, or can we close? If it's still valid we should move to the Core team.

[LeeDr]

I think this is still valid. Here's another example;

1. Two users open the same Lens visualization
2. user 1 makes a change and saves it
3. user 2 makes a different change and saves it. This overwrites user 1's change

Concrete example with eCommerce sample data

1. Two users open Avg. items sold lens visualization
2. user 1 changes it from a metric to a "Bar vertical stacked" and clicks Save
3. user 2 changes it from a metric that they still see, to a "Bar vertical" and saves it

In these cases, user 2 doesn't have any idea that they are working from a stale version of the visualization and overwriting other changes. It could even be multiple different changes that get overwritten.

Compared to the case of index-patterns (data views), when you try to save a change to a stale version the UI tells you that you have to refresh (and re-do your changes) before saving.

[tylersmalley]

Reading through the issue, I don't even think this is a core thing. I think that each application will need to figure out how to handle this. I do know that the saved objects have the ability to track the version so they can prevent a write if the underlying value originally read changed... Which is what I think is happening here.

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[tylersmalley]

Reassigning to core, didn't realize they owned Advanced Settings. This feels like an advanced settings specific bug.

[Bamieh]

I wonder if we can use the documents _version field in elasticsearch for optimistic locking (https://www.elastic.co/blog/elasticsearch-versioning-support)

[pgayvallet]

I wonder if we can use the documents _version field in elasticsearch for optimistic locking

We're doing that already. We're using the if_* fields for SOs already (if_seq_no and if_primary_term) for optimistic concurrency control, based on the document's version. If the option is provided by API consumers, that is.

Problem with the uiSettings is more that it's an higher level API and the SO APIs are just the underlying implementation, so there isn't an easy way to keep track of the version of the config object the settings were coming from, especially on the UI.

Looking at the issue's description, it may be more that we're updating more settings than necessary when saving in the advanced settings page?

on #1 change discover:sampleSize (Default: 500) to 499
on #2 change discover:sampleSize (Default: 500) to 501
on #1 change courier:maxSegmentCount (Default: 30) to 29

I would expect the second save operation on instance 1 to only update the setting(s) that were not previously saved (e.g here, only courier:maxSegmentCount)

Actually I thought we were already doing that. Given this is an issue from 2016, it's probably worth checking that this isn't fixed already.

regarding the second use case from @LeeDr in #7055 (comment)

Two users open the same Lens visualization
user 1 makes a change and saves it
user 2 makes a different change and saves it. This overwrites user 1's change

The SO apis already allow consumers to pass a version option to avoid this very scenario

e.g for update:

kibana/src/core/server/saved_objects/service/saved_objects_client.ts

Lines 223 to 225 in 1ca8771

	export interface SavedObjectsUpdateOptions<Attributes = unknown> extends SavedObjectsBaseOptions {
	/** An opaque version number which changes on each successful write operation. Can be used for implementing optimistic concurrency control. */
	version?: string;

And this version field is exposed on every savedObject returned from our APIs:

kibana/src/core/types/saved_objects.ts

Lines 69 to 75 in 1965315

	export interface SavedObject<T = unknown> {
	/** The ID of this Saved Object, guaranteed to be unique for all objects of the same `type` */
	id: string;
	/** The type of Saved Object. Each plugin can define it's own custom Saved Object types. */
	type: string;
	/** An opaque version number which changes on each successful write operation. Can be used for implementing optimistic concurrency control. */
	version?: string;

it's just that our API consumers are almost never using it.

[afharo]

Based on Pierre's comment, I've changed the labels to UI Settings as it seems that SO APIs allow the concurrency checks as long as version is provided, so we may probably need to change it on UI Settings.

[elasticmachine]

Pinging @elastic/appex-sharedux (Team:SharedUX)

[vadimkibana]

Not sure why it was assigned to Shared UX, but reading through the issue it looks to me that the behaviour is actually expected. If an app wants to prevent this behaviour, there is an option to do optimistic concurrency control (conditional updates).