Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add indices pipeline settings check when deleting a pipeline #77013

Merged
merged 10 commits into from
Oct 12, 2021
Merged

add indices pipeline settings check when deleting a pipeline #77013

merged 10 commits into from
Oct 12, 2021

Conversation

weizijun
Copy link
Contributor

When deleting a pipeline, check whether the pipeline is actually deleted in the indices settings.
If the indices have the deleting pipeline, fail the delete request.

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.0.0 labels Aug 30, 2021
@nik9000 nik9000 added the :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP label Sep 2, 2021
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Sep 2, 2021
@elasticmachine
Copy link
Collaborator

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

@danhermann danhermann self-requested a review September 16, 2021 15:50
@danhermann
Copy link
Contributor

@elasticmachine ok to test

@danhermann
Copy link
Contributor

@elasticmachine update branch

@danhermann
Copy link
Contributor

@elasticmachine run elasticsearch-ci/part-1

@danhermann
Copy link
Contributor

@weizijun, thank you for this contribution. It's a helpful improvement and we'd like to get it merged. There are a couple failing tests:

org.elasticsearch.ingest.common.IngestCommonClientYamlTestSuiteIT.test {yaml=ingest/240_required_pipeline/Test index with final pipeline}
org.elasticsearch.ingest.common.IngestCommonClientYamlTestSuiteIT.test {yaml=ingest/240_required_pipeline/Test final pipeline when target index is changed}
org.elasticsearch.ingest.common.IngestCommonClientYamlTestSuiteIT.test {yaml=ingest/200_default_pipeline/Test index with default pipeline}

Each of those tests is attempting to delete an ingest pipeline that is either default or final on an index. The fix for those will be to delete the index before deleting the ingest pipeline. If you can make those changes, we'll get this reviewed and merged.

@weizijun
Copy link
Contributor Author

@weizijun, thank you for this contribution. It's a helpful improvement and we'd like to get it merged. There are a couple failing tests:

org.elasticsearch.ingest.common.IngestCommonClientYamlTestSuiteIT.test {yaml=ingest/240_required_pipeline/Test index with final pipeline}
org.elasticsearch.ingest.common.IngestCommonClientYamlTestSuiteIT.test {yaml=ingest/240_required_pipeline/Test final pipeline when target index is changed}
org.elasticsearch.ingest.common.IngestCommonClientYamlTestSuiteIT.test {yaml=ingest/200_default_pipeline/Test index with default pipeline}

Each of those tests is attempting to delete an ingest pipeline that is either default or final on an index. The fix for those will be to delete the index before deleting the ingest pipeline. If you can make those changes, we'll get this reviewed and merged.

ok, I will fix, and passed ./gradlew check

@weizijun
Copy link
Contributor Author

@danhermann I have fixed the failed tests. but the elasticsearch-ci/part-1 is blocked

@weizijun
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-1

@weizijun
Copy link
Contributor Author

@elasticmachine ok to test

@weizijun
Copy link
Contributor Author

@elasticmachine update branch

@weizijun
Copy link
Contributor Author

@elasticmachine update branch

@weizijun
Copy link
Contributor Author

hi, @danhermann, all tests passed. can you help to review this PR?

@danhermann
Copy link
Contributor

@weizijun, apologies for the delay on this one. I will review it and help you to get it merged. One suggestion for improvement that I have is to update the error message to include the number of indices from which the pipeline must first be removed. Perhaps something like this:

pipeline [my-pipeline] cannot be deleted because it is the default pipeline for <x> indices including [<index1>,<index2>] and the required pipeline for <y> indices including [<index3>, <index4>, <index5>]

That way a user is informed immediately about the number of places from which the ingest pipeline must be removed before it can be deleted rather than removing it from the first index, attempting to delete it again, finding that it cannot be deleted because it's used on another index, etc. The "including" list of indices might include just the first 3 indices encountered in case it's used on a large number of indices.

@weizijun
improve
5bf270b
@weizijun
Merge branch 'upstream/master' into improve-delete-pipeline-check
23a38a2 
* upstream/master: (250 commits)
  [Transform] HLRC cleanups (elastic#78909)
  [ML] Make ML indices hidden when the node becomes master (elastic#77416)
  Introduce a Few Settings Singleton Instances (elastic#78897)
  Simplify TestCluster extraJar configuration (elastic#78837)
  Add @OverRide annotations to methods in EnrichPlugin class (elastic#76873)
  Add v7 restCompat for invalidating API key with the id field (elastic#78664)
  EQL: Refine repeatable queries (elastic#78895)
  Fix DataTierTests package and add a validation test (elastic#78880)
  Fix split package org.elasticsearch.common.xcontent (elastic#78831)
  Store DataTier Preference directly on IndexMetadata (elastic#78668)
  [DOCS] Fixes typo in calendar API example (elastic#78867)
  Improve Node Shutdown Observability (elastic#78727)
  Convert encrypted snapshot license object to LicensedFeature (elastic#78731)
  Revert "Make nodePaths() singular (elastic#72514)" (elastic#78801)
  Fix incorrect generic type in PolicyStepsRegistry (elastic#78628)
  [DOCS] Fixes ML get calendars API (elastic#78808)
  Implement GET API for System Feature Upgrades (elastic#78642)
  [TEST] More MetadataStateFormat tests (elastic#78577)
  Add support for rest compatibility headers to the HLRC (elastic#78490)
  Un-ignoring tests after backporting fix (elastic#78830)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/ingest/IngestService.java
#	server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java
@danhermann
Copy link
Contributor

@elasticmachine run elasticsearch-ci/bwc

@danhermann
Copy link
Contributor

@elasticmachine update branch

@danhermann
Copy link
Contributor

@elasticmachine run elasticsearch-ci/part-2

@weizijun
Copy link
Contributor Author

@elasticmachine update branch

@danhermann
Copy link
Contributor

@elasticmachine run elasticsearch-ci/part-2

danhermann
danhermann approved these changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@danhermann danhermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weizijun, the one remaining test failure is a known flaky test, so this looks good. I'll get it merged and backported for the appropriate release. Thanks for building this feature!

@danhermann danhermann merged commit 59ad8f9 into elastic:master Oct 12, 2021
@danhermann danhermann mentioned this pull request Oct 12, 2021
Merged
@weizijun
Copy link
Contributor Author

@weizijun, the one remaining test failure is a known flaky test, so this looks good. I'll get it merged and backported for the appropriate release. Thanks for building this feature!

@danhermann thank you for review this PR!

danhermann pushed a commit to danhermann/elasticsearch that referenced this pull request Oct 18, 2021
@weizijun @danhermann
Add indices pipeline settings check when deleting a pipeline (elastic…
6925633 
…#77013)
@danhermann danhermann mentioned this pull request Oct 18, 2021
Merged
danhermann added a commit that referenced this pull request Oct 18, 2021
@danhermann
Add indices pipeline settings check when deleting a pipeline (#77013) (…
60e8292 
…#79353)
@danhermann danhermann mentioned this pull request Oct 18, 2021
Merged
@danhermann danhermann mentioned this pull request Oct 27, 2021
Merged
danhermann added a commit that referenced this pull request Oct 27, 2021
@danhermann
Reenable REST compat tests after backport of #77013
4e09186
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danhermann danhermann danhermann approved these changes

Assignees
No one assigned
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v7.16.0 v8.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@weizijun @elasticmachine @danhermann @dakrone @nik9000 @jakelandis @elasticsearchmachine