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.

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

Remove metadata customs that can break serialization #30945

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.NamedDiff;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -187,9 +188,37 @@ public long getNumberOfTasksOnNode(String nodeId, String taskName) {
task -> taskName.equals(task.taskName) && nodeId.equals(task.assignment.executorNode)).count();
}

private static final Version MINIMAL_SUPPORTED_VERSION;

static {
/*
* Prior to version 6.3.0 of Elasticsearch, persistent tasks were in X-Pack and we could assume that every node in the cluster and
* all connected clients also had the X-Pack plugin. The only constraint against sending persistent tasks custom metadata in
* response to a cluster state request was that the version of the client was at least 5.4.0. When we migrated the persistent task
* code to the server codebase, we introduced the possibility that we could communicating with a client on a version between 5.4.0
* (inclusive) and 6.3.0 (exclusive). However, such a client could not understand persistent task metadata. As such, we not set the
* minimal supported version on persistent tasks custom metadata to 6.3.0 as that is the first version that we are certain any
* client of that version can understand persistent tasks custom metdata. However, this is a breaking change from previous versions
* where for persistent tasks custom metadata we assumed that any client would have X-Pack installed. To reinstate the previous
* behavior (e.g., for debugging) we provide the following undocumented system property. This system property could be removed at
* any time.
*/
final String property = System.getProperty("es.persistent_tasks.custom_metadata_minimum_version", "6.3.0");
final Version minimalSupportedVersion = Version.fromString(property);
if (minimalSupportedVersion.before(Version.V_5_4_0)) {
throw new IllegalArgumentException(
"es.persistent_tasks.custom_metadata_minimum_version must be after [5.4.0] but was [" + property + "]");
} else if (minimalSupportedVersion.after(Version.CURRENT)) {
throw new IllegalArgumentException(
"es.persistent_tasks.custom_metadata_minimum_version must be before [" + Version.CURRENT.toString()
+ "] but was [" + property + "]");
}
MINIMAL_SUPPORTED_VERSION = minimalSupportedVersion;
}

@Override
public Version getMinimalSupportedVersion() {
return Version.V_5_4_0;
return MINIMAL_SUPPORTED_VERSION;
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure that's OK? this is also used when publishing a cluster state to nodes. What happens when people upgrade from 6.2 + xpack to 6.3 + xpack in a rolling fashion?

Copy link
Contributor

Choose a reason for hiding this comment

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

The effect of losing the persistent tasks in a 6.2 -> 6.3 upgrade would be that running ML jobs and datafeeds would be killed and would have to be manually restarted.

It's currently documented best practice to stop ML jobs during upgrades. We are trying to make things more robust so this advice can be removed, but because the advice exists today I don't think it would be a complete disaster if ML jobs got killed during a rolling upgrade to 6.3 if it wasn't followed. (For upgrades beyond 6.3 this wouldn't be acceptable, as ML will be supported in Cloud and they'll be relying on ML jobs staying open during rolling upgrades from 6.3 to higher versions.)

We could also potentially document the es.persistent_tasks.custom_metadata_minimum_version system property and tell people who want to leave ML jobs running during a rolling upgrade to 6.3 that they need to set it.

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,27 @@ public String getWriteableName() {
return MLMetadataField.TYPE;
}

private static final EnumSet<MetaData.XContentContext> CONTEXT;

static {
/*
* We generally can not expose ML custom metadata because we might be communicating with a client that does not understand such
* custom metadata. This can happen, for example, when a transport client without the X-Pack plugin is connected to a 6.3.0 cluster
* running the default distribution. To avoid sending such a client a metadata custom that it can not understand, we do not return
* ML custom metadata in response to cluster state requests. However, this a breaking change from previous versions where we assumed
* that any client would have X-Pack installed. To reinstate the previous behavior (e.g., for debugging) we provide the following
* undocumented system property. This system property could be removed at any time.
*/
if (Boolean.parseBoolean(System.getProperty("es.xpack.ml.api_metadata_context", "false"))) {
CONTEXT = MetaData.ALL_CONTEXTS;
} else {
CONTEXT = EnumSet.of(MetaData.XContentContext.GATEWAY, MetaData.XContentContext.SNAPSHOT);
}
}

@Override
public EnumSet<MetaData.XContentContext> context() {
return MetaData.ALL_CONTEXTS;
return CONTEXT;
}

@Override
Expand Down