Optimistic concurrency control for updating ingest pipelines #78551

danhermann · 2021-10-01T10:16:50Z

This commit adds optional optimistic concurrency control to the API for updating ingest pipelines. This is achieved through the addition of an if_version query parameter similar to the if_seq_no and if_primary_term parameters used for optimistic concurrency control in Elasticsearch. When this parameter is specified, the pipeline update succeeds only if there is an existing pipeline with a version that matches the specified version.

PUT /_ingest/pipeline/my_pipeline?if_version=5
{
  "version": 9,
  "processors": [
    ...
  ]
}

In the example above, the update succeeds only if "my_pipeline" exists with a version of 5. Any version can be specified in the body of the update so long as it is not the existing version. If no version is specified in the body of the update, the version is automatically set to <currentVersion> + 1. Pipelines that do not have a version attribute with a numeric value cannot be updated with the if_version parameter because checking for a null or missing version is not a meaningful guard against concurrent updates.

Note that any pipeline updates that do not include an if_version parameter will be processed unconditionally. Note also that there are no optimizations possible for no-op pipeline updates when the if_version parameter is specified because such an update will always change, at the very least, the version attribute of the pipeline and will therefore never be a no-op update.

Fixes #77031

elasticmachine · 2021-10-01T10:16:53Z

Pinging @elastic/es-data-management (Team:Data Management)

server/src/main/java/org/elasticsearch/ingest/IngestService.java

danhermann · 2021-10-01T10:58:47Z

@elasticmachine update branch

dakrone

Just leaving a drive-by comment, but why allow setting the version to null explicitly? If we enforced it being a number in the request then we don't need the version integer and the versionedUpdate boolean flag, we can just use the presence of the number as an indication?

dakrone · 2021-10-01T15:57:23Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

+        if (request.isVersionedUpdate()) {
+            var currentPipeline = currentIngestMetadata != null ? currentIngestMetadata.getPipelines().get(request.getId()) : null;
+            if (currentPipeline == null) {
+                throw new IllegalStateException(String.format(


I think this should be more like an IllegalArgumentException, since the required parameter is illegal if there is no pre-existing pipeline?

Actually, we do have an exception for something like this, a VersionConflictEngineException, which will return back to the user RestStatus.CONFLICT (a 409), what do you think about using that?

That would be great, especially if it resolves the problem with the HTTP error code, but VersionConflictingEngineException requires a shard ID and some other index-related values that don't apply to ingest pipelines.

@dakrone, I changed the instances of IllegalStateException to IllegalArgumentException in 19d60f5 which I agree better characterizes the problem and is also reported as an HTTP 400 error to the client. Let me know if you think there's a better approach for that.

danhermann · 2021-10-01T18:00:00Z

Just leaving a drive-by comment, but why allow setting the version to null explicitly? If we enforced it being a number in the request then we don't need the version integer and the versionedUpdate boolean flag, we can just use the presence of the number as an indication?

I didn't do that because we currently allow the version to be set to null so there could be existing pipelines with that value. That has the unfortunate effect of requiring the ability to distinguish between three states -- no version specified, a version of null explicitly specified, or an integer-valued version specified.

elasticmachine · 2021-10-04T21:10:46Z

Pinging @elastic/clients-team (Team:Clients)

sethmlarson

Couple of comments:

rest-api-spec/src/main/resources/rest-api-spec/api/ingest.put_pipeline.json

.../ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_versioned_update.yml

sethmlarson · 2021-10-04T21:55:22Z

Sorry for the useless comments, didn't get that "null" had a special meaning here. That value might require some special handling within the Elasticsearch specification? cc @delvedor

N/A

danhermann · 2021-10-04T21:57:22Z

No worries. It is an unusual situation and we're still discussing the best way of handling it. If there's a particular way or particular considerations that would be best for clients, that would be helpful information, too.

danhermann · 2021-10-07T20:29:20Z

@elasticmachine update branch

danhermann · 2021-10-08T13:01:16Z

@elasticmachine run elasticsearch-ci/part-2

danhermann · 2021-10-08T14:12:09Z

Sorry for the useless comments, didn't get that "null" had a special meaning here. That value might require some special handling within the Elasticsearch specification? cc @delvedor

@sethmlarson, we've gone back and changed this feature to work only with pipelines that have a numeric version so we no longer have to accommodate both integer values and null in the if_version parameter.

sethmlarson · 2021-10-08T14:14:04Z

@danhermann Excellent, thanks for the update! That'll be a lot simpler to represent in the specification.

danhermann · 2021-10-12T12:58:33Z

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

server/src/main/java/org/elasticsearch/action/ingest/PutPipelineRequest.java

dakrone · 2021-10-14T15:03:05Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

+            var pipelineConfig = XContentHelper.convertToMap(request.getSource(), false, request.getXContentType()).v2();
+            final Integer specifiedVersion = (Integer) pipelineConfig.get("version");
+            if (pipelineConfig.containsKey("version") && Objects.equals(specifiedVersion, currentVersion)) {
+                throw new IllegalArgumentException(String.format(
+                    Locale.ROOT,
+                    "cannot update pipeline [%s] with the same version [%s]",
+                    request.getId(),
+                    request.getVersion()
+                ));
+            }


Why do we pull out a version from the pipeline configuration when we know that one has been specified in the request already? (the if (request.getVersion() != null) check above)

Seems like if the request specifies the version, it should be part of the request validation that request.getVersion() matches the pipelineConfig.get("version")

Just to clarify, it needs to check that request.getVersion() does not match pipelineConfig.get("version"). I like that check where it is because it defers a heavier check due to the XContentHelper.convertToMap call to the latest possible point, keeps all the OCC-related checks in the same place, and keeps no-op pipeline updates as very lightweight.

And just in case it's not clear, one of those versions is the value of the if_version parameter and the other is the value of the version attribute in the pipeline definition. The latter doesn't have to be specified, but if specified, must be different from the former.

Ahh okay, I see my confusion now, thanks for clarifying this!

danhermann · 2021-10-14T16:09:50Z

@elasticmachine update branch

dakrone

LGTM, thanks Dan!

dakrone · 2021-10-14T16:13:02Z

server/src/main/java/org/elasticsearch/ingest/IngestService.java

+            var pipelineConfig = XContentHelper.convertToMap(request.getSource(), false, request.getXContentType()).v2();
+            final Integer specifiedVersion = (Integer) pipelineConfig.get("version");
+            if (pipelineConfig.containsKey("version") && Objects.equals(specifiedVersion, currentVersion)) {
+                throw new IllegalArgumentException(String.format(
+                    Locale.ROOT,
+                    "cannot update pipeline [%s] with the same version [%s]",
+                    request.getId(),
+                    request.getVersion()
+                ));
+            }


Ahh okay, I see my confusion now, thanks for clarifying this!

danhermann · 2021-10-15T13:41:54Z

Thanks for the review, @dakrone!

danhermann · 2021-10-15T18:08:03Z

Pinging @elastic/kibana-stack-management since this adds a new option to the pipeline API

…#78551)

* upstream/master: (34 commits) Add extensionName() to security extension (elastic#79329) More robust and consistent allowAll indicesAccessControl (elastic#79415) Fix circuit breaker leak in MultiTerms aggregation (elastic#79362) guard geoline aggregation from parents aggegator that emit empty buckets (elastic#79129) Vector tiles: increase the size of the envelope used to clip geometries (elastic#79030) Revert "[ML] Add queue_capacity setting to start deployment API (elastic#79369)" (elastic#79374) Convert token service license object to LicensedFeature (elastic#79284) [TEST] Fix ShardPathTests for MDP (elastic#79393) Fix fleet search API with no checkpints (elastic#79400) Reduce BWC version for transient settings (elastic#79396) EQL: Rename a test class for eclipse (elastic#79254) Use search_coordination threadpool in field caps (elastic#79378) Use query param instead of a system property for opting in for new cluster health response code (elastic#79351) Add new kNN search endpoint (elastic#79013) Disable BWC tests Convert auditing license object to LicensedFeature (elastic#79280) Update BWC versions after backport of elastic#78551 Enable InstantiatingObjectParser to pass context as a first argument (elastic#79206) Move xcontent filtering tests (elastic#79298) Update links to Fleet/Agent docs (elastic#79303) ...

OCC for updating ingest pipelines

4f47288

danhermann added >enhancement :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v8.0.0 v7.16.0 labels Oct 1, 2021

elasticmachine added the Team:Data Management Meta label for data/management team label Oct 1, 2021

danhermann commented Oct 1, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/ingest/IngestService.java Outdated Show resolved Hide resolved

fix test

8c81989

Merge branch 'master' into 77031_occ_for_ingest_pipelines

ac8289a

dakrone reviewed Oct 1, 2021

View reviewed changes

change IllegalStateException to IllegalArgumentException

19d60f5

sethmlarson added the Team:Clients Meta label for clients team label Oct 4, 2021

sethmlarson previously requested changes Oct 4, 2021

View reviewed changes

rest-api-spec/src/main/resources/rest-api-spec/api/ingest.put_pipeline.json Outdated Show resolved Hide resolved

.../ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/290_versioned_update.yml Outdated Show resolved Hide resolved

elasticmachine and others added 3 commits October 8, 2021 07:29

Merge branch 'master' into 77031_occ_for_ingest_pipelines

82cc67c

require numeric version for OCC update

fd15765

fix test

1ce0ab9

danhermann and others added 3 commits October 12, 2021 07:33

Merge branch 'master' into 77031_occ_for_ingest_pipelines

a10cb40

fix merge

cd71d7a

more merge fixes

dad598b

dakrone reviewed Oct 14, 2021

View reviewed changes

All members of request class are final

43f72e0

Merge branch 'master' into 77031_occ_for_ingest_pipelines

c9e4163

dakrone approved these changes Oct 14, 2021

View reviewed changes

danhermann merged commit 77d8d5b into elastic:master Oct 15, 2021

danhermann deleted the 77031_occ_for_ingest_pipelines branch October 15, 2021 13:41

danhermann added the backport pending label Oct 15, 2021

danhermann added a commit to danhermann/elasticsearch that referenced this pull request Oct 18, 2021

Optimistic concurrency control for updating ingest pipelines (elastic…

f22f631

…#78551)

danhermann mentioned this pull request Oct 18, 2021

[7.x] Optimistic concurrency control for updating ingest pipelines #79364

Merged

danhermann removed the backport pending label Oct 18, 2021

danhermann mentioned this pull request Oct 18, 2021

Update BWC versions after backport of #78551 #79373

Merged

danhermann added a commit that referenced this pull request Oct 18, 2021

Update BWC versions after backport of #78551

906e163

danhermann mentioned this pull request Oct 21, 2021

[DOCS] if_version parameter for OCC on pipeline updates #79640

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

danhermann mentioned this pull request Nov 11, 2021

[7.16] [DOCS] if_version parameter for OCC on pipeline updates (#79640) #80663

Merged

sethmlarson mentioned this pull request May 9, 2022

Add 'if_version' parameter to 'ingest.put_pipeline' API elastic/elasticsearch-specification#1689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimistic concurrency control for updating ingest pipelines #78551

Optimistic concurrency control for updating ingest pipelines #78551

danhermann commented Oct 1, 2021 •

edited

Loading

elasticmachine commented Oct 1, 2021

danhermann commented Oct 1, 2021

dakrone left a comment

dakrone Oct 1, 2021

dakrone Oct 1, 2021

danhermann Oct 1, 2021

danhermann Oct 4, 2021

danhermann commented Oct 1, 2021

elasticmachine commented Oct 4, 2021

sethmlarson left a comment

sethmlarson commented Oct 4, 2021

danhermann commented Oct 4, 2021

danhermann commented Oct 7, 2021

danhermann commented Oct 8, 2021

danhermann commented Oct 8, 2021

sethmlarson commented Oct 8, 2021

danhermann commented Oct 12, 2021

dakrone Oct 14, 2021

danhermann Oct 14, 2021

danhermann Oct 14, 2021

dakrone Oct 14, 2021

danhermann commented Oct 14, 2021

dakrone left a comment

dakrone Oct 14, 2021

danhermann commented Oct 15, 2021

danhermann commented Oct 15, 2021

Optimistic concurrency control for updating ingest pipelines #78551

Optimistic concurrency control for updating ingest pipelines #78551

Conversation

danhermann commented Oct 1, 2021 • edited Loading

elasticmachine commented Oct 1, 2021

danhermann commented Oct 1, 2021

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhermann commented Oct 1, 2021

elasticmachine commented Oct 4, 2021

sethmlarson left a comment

Choose a reason for hiding this comment

sethmlarson commented Oct 4, 2021

danhermann commented Oct 4, 2021

danhermann commented Oct 7, 2021

danhermann commented Oct 8, 2021

danhermann commented Oct 8, 2021

sethmlarson commented Oct 8, 2021

danhermann commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhermann commented Oct 14, 2021

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhermann commented Oct 15, 2021

danhermann commented Oct 15, 2021

danhermann commented Oct 1, 2021 •

edited

Loading