-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
Fix registry unsupported pipeline update #96497
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,21 +112,10 @@ public void testTrainedModelDeployment() throws Exception { | |
request.addParameter("wait_for_status", "yellow"); | ||
request.addParameter("timeout", "70s"); | ||
})); | ||
|
||
// Workaround for an upgrade test failure where an ingest | ||
// pipeline config cannot be parsed by older nodes: | ||
// https://github.com/elastic/elasticsearch/issues/95766 | ||
// | ||
// In version 8.3.1 ml stopped parsing the full ingest | ||
// pipeline configuration so will avoid this problem. | ||
// TODO remove this check once https://github.com/elastic/elasticsearch/issues/95766 | ||
// is resolved | ||
if (UPGRADE_FROM_VERSION.onOrAfter(Version.V_8_3_1)) { | ||
waitForDeploymentStarted(modelId); | ||
// attempt inference on new and old nodes multiple times | ||
for (int i = 0; i < 10; i++) { | ||
assertInfer(modelId); | ||
} | ||
waitForDeploymentStarted(modelId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davidkyle Reverted your workaround. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @afoucret. I added the |
||
// attempt inference on new and old nodes multiple times | ||
for (int i = 0; i < 10; i++) { | ||
assertInfer(modelId); | ||
} | ||
} | ||
case UPGRADED -> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,19 +74,8 @@ public void testTrainedModelInference() throws Exception { | |
request.addParameter("timeout", "70s"); | ||
})); | ||
List<String> modelIds = getTrainedModels(); | ||
|
||
// Workaround for an upgrade test failure where an ingest | ||
// pipeline config cannot be parsed by older nodes: | ||
// https://github.com/elastic/elasticsearch/issues/95766 | ||
// | ||
// In version 8.3.1 ml stopped parsing the full ingest | ||
// pipeline configuration so will avoid this problem. | ||
// TODO remove this check once https://github.com/elastic/elasticsearch/issues/95766 | ||
// is resolved | ||
if (UPGRADE_FROM_VERSION.onOrAfter(Version.V_8_3_1)) { | ||
// Test that stats are serializable and can be gathered | ||
getTrainedModelStats(); | ||
} | ||
// Test that stats are serializable and can be gathered | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davidkyle Reverted your workaround. |
||
getTrainedModelStats(); | ||
// Verify that the pipelines still work and inference is possible | ||
testInfer(modelIds); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyalkoren Now checking all nodes are at least at v 8.9.0, so it does not fails because of the ignore_missing_pipeline parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you preventing with this?
AFAIK, once the local implementation of
requiresMasterNode()
returnstrue
, as in this case, this ensures that the upgrade will occur only on the elected master. I believe that during rolling upgrades this ensures that this happens only after all non-master nodes are already upgraded.Since the usage of
ignore_missing_pipeline
was introduced in #95971, which was added to 8.9.0 and not back-ported, I am not sure whether this is required.BTW,
AnalyticsTemplateRegistry
also requires master node, so double-check if this is required in the original case as well.@jbaiera @dakrone please confirm or enlighten me if I got it wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no guarantee that the non-master nodes will be upgraded first during a rolling upgrade. This is a sensible precaution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would nodes apply settings coming from a master of a higher version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dakrone just explained that I indeed got it wrong and there is no actual enforced guarantee as to the order of upgrade in code. This should normally not occur if the upgrade is done according to documentation (master node last), but such verification does make sense