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

Implement GET API for System Feature Upgrades #78642

Merged
merged 15 commits into from
Oct 7, 2021
Merged

Implement GET API for System Feature Upgrades #78642

merged 15 commits into from
Oct 7, 2021

Conversation

williamrandolph
Copy link
Contributor

Here is an implementation for the GET API for system feature upgrades. It is pretty simple. When called, we iterate over all features, resolve all indices, and fetch the Version for each index from cluster state index metadata. We then determine the earliest index creation version for each feature to determine whether the features need to be upgraded or not.

I've also added a simple BWC test for rolling upgrades to make sure that we are in fact returning the correct index creation versions.

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 4, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

gwbrown
gwbrown reviewed Oct 4, 2021
View reviewed changes
Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking good, especially the production code! Other than a super minor nit, my only comments were on the tests.

...org/elasticsearch/action/admin/cluster/migration/TransportGetFeatureUpgradeStatusAction.java Outdated
.orElse(Version.CURRENT)
.before(Version.V_7_0_0);

listener.onResponse(new GetFeatureUpgradeStatusResponse(features, isUpgradeNeeded ? "UPGRADE_NEEDED" : "NO_UPGRADE_NEEDED"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can you move "UPGRADE_NEEDED" and "NO_UPGRADE_NEEDED" into a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I should have done that in the API stubs PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used an enum, but let me know if you think a pair of string constants would be better.

...stem-indices/src/javaRestTest/java/org/elasticsearch/system/indices/FeatureUpgradeApiIT.java Outdated Show resolved Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/migration/FeatureUpgradeIT.java Outdated Show resolved Hide resolved
...org/elasticsearch/action/admin/cluster/migration/TransportGetFeatureUpgradeStatusAction.java Outdated Show resolved Hide resolved
qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/FeatureUpgradeIT.java Outdated Show resolved Hide resolved
qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/FeatureUpgradeIT.java Outdated Show resolved Hide resolved
qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/FeatureUpgradeIT.java Show resolved Hide resolved
@sabarasaba
Copy link
Member

Great work @williamrandolph! I have a few questions regarding upgrade_status that I couldn't figure out from looking at the code:

  1. If upgrading and index fails, will the upgrade_status of the feature change to something like ERROR? If so, is there going to be a way to have more details about why it failed?
  2. What are all the possible values upgrade_status can have? So far I've noted 'UPGRADE_NEEDED' | 'NO_UPGRADE_NEEDED' | 'IN_PROGRESS' | 'ERROR' but not sure about the last two

@williamrandolph
Copy link
Contributor Author

@sabarasaba All of the failure handling is being implemented for the POST API, so if we add an ERROR response it will come in a future PR. I will need to talk to @gwbrown about what sort of error reporting we can do in this API. The same goes for IN PROGRESS I will tag you on PRs that affect this behavior.

gwbrown
gwbrown approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

LGTM!

@williamrandolph williamrandolph merged commit 0a28c7c into elastic:master Oct 7, 2021
williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Oct 7, 2021
@williamrandolph
Implement GET API for System Feature Upgrades (elastic#78642)
ccfec46 
* Implement and test get feature upgrade status API
* Add integration test for feature upgrade endpoint
* Use constant enum for statuses
* Add unit tests for transport class methods
@williamrandolph williamrandolph mentioned this pull request Oct 7, 2021
Merged
wjp719 added a commit to wjp719/elasticsearch that referenced this pull request Oct 9, 2021
@wjp719
Merge branch 'master' into feature/data_stream_support_routing
af2a5f0 
* master:
  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)
  Add node REPLACE shutdown implementation (elastic#76247)
  Wrap VersionPropertiesLoader in a BuildService to decouple build logic projects (elastic#78704)
  Adjust /_cat/templates not to request all metadata (elastic#78829)
  [DOCS] Fixes ML get scheduled events API (elastic#78809)
  Enable exit on out of memory error (elastic#71542)

# Conflicts:
#	server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Oct 11, 2021
@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
williamrandolph added a commit that referenced this pull request Oct 12, 2021
@williamrandolph
Implement GET API for System Feature Upgrades (#78642) (#78860)
1b5827d 
* Implement GET API for System Feature Upgrades (#78642)

* Implement and test get feature upgrade status API
* Add integration test for feature upgrade endpoint
* Use constant enum for statuses
* Add unit tests for transport class methods
* Fix bwc tests for 7.x
@williamrandolph williamrandolph mentioned this pull request Oct 15, 2021
Open
@williamrandolph williamrandolph deleted the get-upgrade-status-api branch May 23, 2022 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gwbrown gwbrown gwbrown approved these changes

@sabarasaba sabarasaba Awaiting requested review from sabarasaba

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v7.16.0 v8.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@williamrandolph @elasticmachine @sabarasaba @gwbrown @jakelandis