SOR: downward-compatible bulkUpdate

[pgayvallet]

In #152807 / #161822 we implemented the BWC update algorithm on the single-object update API.

We now need to do the same for bulkUpdate.

Ideally, the logic should be as factorized and reused between the two APIs as possible (similar to what we do with internalBulkResolve). It may imply to refactor how we implemented update in #161822 We cannot implement an internalBulkUpdate for a single object update because the bulk API doesn't support upsert. However, we should share code as far as possible between the two API's where they perform similar/identical operations (calls to get docs etc).

Tasks

Preview Give feedback

1. factorize internal logic for reuse
2. refactor 161822 for reuse
3. extract internal helpers from 161822 where needed for an identical task in bulkUpdate
4. implement bulkUpdate BWC

[elasticmachine]

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

[TinaHeiligers]

Roger!
let's discuss after testing week.

[TinaHeiligers]

@pgayvallet bukkUpdate seems to only use _index. Does that mean that every saved object will be upserted if it doesn't exist when using bulkUpdate?
I'm working through the changes needed to invoke bulkUpdate for updating a single object.

[pgayvallet]

bukkUpdate seems to only use _index

We're using the bulk API with update operations:

kibana/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.ts

Lines 226 to 244 in 3b6b7ad

	bulkUpdateParams.push(
	{
	update: {
	_id: serializer.generateRawId(getNamespaceId(objectNamespace), type, id),
	_index: commonHelper.getIndexForType(type),
	...versionProperties,
	},
	},
	{
	doc: {
	...documentToSave,
	[type]: await encryptionHelper.optionallyEncryptAttributes(
	type,
	id,
	objectNamespace || namespace,
	documentToSave[type]
	),
	},
	}

(The bulk API is kinda particular, each operation is based on 2 parts in the payload, the first one being the metadata (operation, _id, _index...) and the second one being the actual doc the operation has to be performed on)

I'm working through the changes needed to invoke bulkUpdate for updating a single object.

Great! Please ping me if I can be of any help 🙂

[TinaHeiligers]

We're using the bulk API with update operations

Yea, I realized later I was looking at the mget request params.

[TinaHeiligers]

@pgayvallet I've worked on a few approaches and thought I'd double check with you before attempting yet another.

1. Why do you suggest using bulkUpdate for updating a single object? The underlying es API may be more performant but we're only dealing with a single object. We'd also need to interpret the result from the bulk request response before responding to the caller. I'm not against it, but I'd like to hear your rationale on that approach first.
2. I'm interpreting bulkUpdate to do the client-side object updates in memory. We'd need to restrict the API to only allow a certain payload size or number of docs to update, depending on the number of fields requested to change. Alternatively, should we consider restricting the payload size?
3. How would we handle situations where Kibana suddenly shuts down or loses connection with ES? We might be halfway through client-side updates and lose all of it. I guess this isn't necessarily a data loss but it's not a great UX.
4. How do we handle a situation where several updates to the same object come from different browsers? Last in first out or first in last out?

I'm sure more questions are gonna come to me but for now, this is what comes to mind.

[pgayvallet]

Why do you suggest using bulkUpdate for updating a single object? The underlying es API may be more performant but we're only dealing with a single object. We'd also need to interpret the result from the bulk request response before responding to the caller. I'm not against it, but I'd like to hear your rationale on that approach first.

Code factorization, mostly. I assumed the overall logic of single update and bulk update were close enough so that factorizing by having the two APIs executing the same underlying logic may be the best approach in term of maintainability.

Now, if there are things I overlooked that make this factorization somehow harder than I thought, I'm totally fine keeping the two implementations dissociated as it is the case today. If so, I'd just like to understand the reasons.

If it's "only" about the additional level of effort of refactoring update (once again), then I would also be totally fine keeping those two tasks (implementing BWC bulk update AND refactoring the BWC update to use the same code path) separate and doing the later one as a follow-up.

I'm interpreting bulkUpdate to do the client-side object updates in memory. We'd need to restrict the API to only allow a certain payload size or number of docs to update, depending on the number of fields requested to change. Alternatively, should we consider restricting the payload size?

This is a very good point, and we absolutely need to put a safeguard in place somewhere.

However, given our current usages of the bulkUpdate API, which AFAIK is only used to update a few, of dozens maximum, of documents, I don't think the risks are very high, so I think it would be acceptable to keep that as a follow-up enhancement (but please tell me if you think I'm wrong).

How would we handle situations where Kibana suddenly shuts down or loses connection with ES? We might be halfway through client-side updates and lose all of it. I guess this isn't necessarily a data loss but it's not a great UX.

As long as we're performing a single _bulk call to perform the update of all documents "atomically", then I don't think it would be any different of any other scenario where that kind of outages could occur?

Unless you're thinking of something I missed?

How do we handle a situation where several updates to the same object come from different browsers? Last in first out or first in last out?

Not sure to follow you. We don't have a "queue" of operations to perform from the repository. If multiple update calls are performed concurrently, they will be executed concurrently too.

We just need to make sure we're properly using the version properties in the update payload to protect in case of conflicts, as it is done today in bulkUpdate:

kibana/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.ts

Lines 227 to 233 in 3b6b7ad

	{
	update: {
	_id: serializer.generateRawId(getNamespaceId(objectNamespace), type, id),
	_index: commonHelper.getIndexForType(type),
	...versionProperties,
	},
	},

and as we did in update:

kibana/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.ts

Lines 283 to 284 in ddaed42

	// using version from the source doc if not provided as option to avoid erasing changes in case of concurrent calls
	...decodeRequestVersion(version || migrated!.version),

Note that you made me doubt, so I checked if the index bulk operation was supporting version fields as it does for update bulk operations, and if I trust the client typings, it's the case.

[TinaHeiligers]

Now, if there are things I overlooked that make this factorization somehow harder than I thought, I'm totally fine keeping the two implementations dissociated as it is the case today. If so, I'd just like to understand the reasons.

1. bulkUpdate doesn't appear to support the upsert action, which we need for update. Now I may be overlooking some hidden option somewhere but it's not obvious to me. We would need to extend the options to handle upsert for each object to support a single doc update, which should be doable.
2. bulkUpdate also doesn't appear to check if an object is shared, at least not obviously either.

These were the initial questions I had. I'll follow up again shortly to your other responses

[pgayvallet]

bulkUpdate doesn't appear to support the upsert action, which we need for update

I absolutely overlooked that, and that seems like a very good reason to not want to factorize the implementation of the two APIs

[TinaHeiligers]

I absolutely overlooked that, and that seems like a very good reason to not want to factorize the implementation of the two APIs

It's decided then: factorize update internals as far as needing to reuse the internal helpers in bulkUpdate, rather than try to work around bulkUpdate not supporting upsert.

[TinaHeiligers]

Progress update:

Based on the discussions in this issue, #171245 only required a few minor changes to ensure BWC for the SO bulkUpdate API.

The main changes are:

• Fetch all documents, regardless of the namespace type
• migrate to the client version
• Update each document client-side
• migrate updated documents to the version the server is on
• index the updated docs

Other minor changes include type updates and adapting unit test helpers to take the additional changes into account.

Test status for these changes to bulkUpdate are as follows:

• bulkUpdate unit tests refactored & passing. The PR includes additional tests for migrations

• src/core/server/integration_tests/saved_objects/routes/bulk_update.test.ts passing locally

• src/core/server/integration_tests/saved_objects/service/lib/bulk_update.test.ts added (version changes)

• test/api_integration/apis/saved_objects/bulk_update.ts,

• x-pack/test/saved_object_api_integration/security_and_spaces (basic & trial license configs),

• x-pack/test/saved_object_api_integration/spaces_only

• (TODO): Ensure Kibana runs and updates documents in all supported environments

Open questions:

• How can we monitor how much memory is consumed by client-side updates?
• How can we prevent (or minimize) the chance of data loss if Kibana can't index updated documents (e.g. during temporary connectivity issues or ES being unavailable)?
• ...other unknowns