SOR: implement downward-compatible update

[pgayvallet]

Part of #150312

Because our update and bulkUpdate operations are effectively partial updates, we can’t run the attributes against our migration function (migration functions take the full SO document as input/out).

So, in order to support the new modelVersion option for update operations, we would need to perform a ‘client-side’ update, by having Kibana fetch the document and perform the update locally, instead of using the _update Elasticsearch API.

We could, in theory, do that only when we're in managed upgrade mode, but to avoid unnecessary divergence in the code, we will just do it for both modes.

questions

• update counter ?
• automatic retries ?
• conflict handling ?
• preflight checks in update operations no longer 'required' given we fetch the document for updates

[rudolf]

We should consider applying our schema validation (#118969) to the update method now that we have the full saved object

[pgayvallet]

Yeah. FWIW we need to port the schema feature to model versions for that, as atm the model version interface doesn't have it - I created #152994

[TinaHeiligers]

Tentatively assigned this to me, as discussed during planning.

[TinaHeiligers]

It looks like #152994 was handled in https://github.com/elastic/kibana/pull/156788/files#top, correct?

[TinaHeiligers]

I had to review the code again to see exactly how update's implemented.
My initial thoughts on step 1:

Fetch the document:

Update makes at least one round trip to ES in the preflight checks for alias conflict and we already have the full document early on:

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

Lines 111 to 130 in 505b4e8

	public async preflightCheckNamespaces({
	type,
	id,
	namespace,
	initialNamespaces,
	}: PreflightCheckNamespacesParams): Promise<PreflightCheckNamespacesResult> {
	if (!this.registry.isMultiNamespace(type)) {
	throw new Error(`Cannot make preflight get request for non-multi-namespace type '${type}'.`);
	}
	const { body, statusCode, headers } = await this.client.get<SavedObjectsRawDocSource>(
	{
	id: this.serializer.generateRawId(undefined, type, id),
	index: this.getIndexForType(type),
	},
	{
	ignore: [404],
	meta: true,
	}
	);

While these preflight checks don't return the full doc, if we change that and work with the doc from there, we won't need to make another round trip to fetch it.

The only conditions I can think of where we won't have an existing doc from the preflight checks is if there's an error (we throw early anyway and don't need to do anything more), or the doc doesn't exist and upsert is used. Sure, we'll need to create a new doc, since one doesn't exist in the namespace we're working in but that's ok, right?

I've still got to work through the rest.
As for the model migration (v1 -> v2 & v2(updatedDoc) -> v1), the document migrator handles that, correct?

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

Lines 111 to 120 in 505b4e8

	const migrated = migrator.migrateDocument({
	id,
	type,
	...(savedObjectNamespace && { namespace: savedObjectNamespace }),
	...(savedObjectNamespaces && { namespaces: savedObjectNamespaces }),
	attributes: {
	...(await encryptionHelper.optionallyEncryptAttributes(type, id, namespace, upsert)),
	},
	updated_at: time,
	});

where did we land with the v2 -> v1 conversion? Drop extra optional fields?
I'm not too familiar with this part.

@pgayvallet thoughts?

[pgayvallet]

While these preflight checks don't return the full doc, if we change that and work with the doc from there, we won't need to make another round trip to fetch it.

I agree, I think that's the trick here. Just fetch the document (or documents for bulk update) regardless of it's namespaceType at the beginning of the operation and then apply the preflight logic if required, then continue from there.

Sure, we'll need to create a new doc, since one doesn't exist in the namespace we're working in but that's ok, right?

Yeah, no doc + upsert => no problem I think.

As for the model migration (v1 -> v2 & v2(updatedDoc) -> v1), the document migrator handles that, correct?

Yes, this logic will be delegated to the Kibana/Document migrator.

where did we land with the v2 -> v1 conversion? Drop extra optional fields?

Yes, atm it's just based on the version schema. But this should be an implementation detail for what you do there, the SOR would just ask the migrator to convert the document to the correct version

[TinaHeiligers]

preflight checks in update operations no longer 'required' given we fetch the document for updates

Whether we need the check depends on how we're handling retrieving the doc but yes, barring edge cases, these shouldn't be needed.

I have a couple of edge cases in mind regarding race conditions between sharing and updating the same doc in the new shared space:

Scenario:

• some user, say, Bob, shares a SO to a space it wasn't originally shared to and edits the doc in the new space
• Some other user, say, James, is still busy editing and saving the original doc.

Given version control is concurrent though, the chances of that causing issues are minimal. The worst case is there's a conflict in the doc and the changes wouldn't stick.

[TinaHeiligers]

@pgayvallet
Update on progress:
I'm having a hard time refactoring and implementing version compatibility, given that we now have to implement the update or upsert behavior client-side.

Adding the new permutations has exploded the conditional code control flow, along with fighting with types (not wanting to force the issue with casting too much).

The upsert-as-create path is relatively simple, it's the pure update that's got me ATM 😵‍💫
At this stage, the work is at risk of not making the deadline and I'll need to pair for a fresh 👁️ .

[pgayvallet]

Adding the new permutations has exploded the conditional code control flow

Anything in particular causing this, or just the overall added complexity of the client-update operation?

[TinaHeiligers]

Anything in particular causing this, or just the overall added complexity of the client-update operation?

It's the overall complexity of implementing what ES would otherwise handle. I'm busy with a diagram to get all the variations nailed down and will share it with the team when done.

FWIW, an initial implementation failed miserably on Kibana startup with Fleet package registration 😖 so that didn't work!
I'm looking at extracting the decision tree code into smaller utils to ensure we have no uncaught errors.

[TinaHeiligers]

Progress update:

• update refactored, working on updating tests
• bulkUpdate still TODO

[TinaHeiligers]

Progress update:

• update api integration tests passing locally for:
  • test/api_integration/apis/saved_objects/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
• src/core/server/integration_tests/saved_objects/routes/update.test.ts passing locally
• update unit tests (mostly) refactored & passing: packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts, todos: ~ half of the client calls.
• Kibana runs against es snapshot

Remaining open questions:

• implement retries
• handle conflicts
• update counter

All of #bulk_delete

[TinaHeiligers]

Progress update:
The saved object related api integration test failed again, these are fixed.

Not done and can probably be with follow-up PRs:

• validation on creating a saved object
• refactor for better dev ex

The remaining open questions are still open. We need to decide if/how to handle those at some point too.
Neither create or index accept retrying, meaning another client-side implementation. A straightforward approach could be to only retry the calls to elasticsearch. Not sure it would be enough though.

[pgayvallet]

opened a dedicated issue to track the work on bulkUpdate: #165434

[TinaHeiligers]

Closed by #165434