Fix bwc issue with RemoteDiscoveryNodes

[cwperks]

Description

I saw this bwc error on another PR and dug a bit further into the issue. This issue arises on mixed cluster tests with 3.6.0 and 2.19.5 when using remote cluster state. I see a code path on the remote cluster state path that codes Version.CURRENT as the version for the output stream which disregards the actual version of a target node that would consume the state.

I was able to add minimumCompatibleVersion() to get the lowest compatible node version to serialize any structures to ensure no bwc issues.

Opening this up in Draft for feedback, it may also be possible to actually get the lowest version of a node in the cluster and serialize accordingly.

» ERROR][o.o.g.r.RemoteClusterStateService] [v2.19.5-remote-2] Failure in downloading full cluster state. 
»  org.opensearch.gateway.remote.RemoteStateTransferException: Exception during reading cluster state from remote
»  	at org.opensearch.gateway.remote.RemoteClusterStateService.readClusterStateInParallel(RemoteClusterStateService.java:1391)
»  	at org.opensearch.gateway.remote.RemoteClusterStateService.getClusterStateForManifest(RemoteClusterStateService.java:1497)
»  	at org.opensearch.cluster.coordination.PublicationTransportHandler.handleIncomingRemotePublishRequest(PublicationTransportHandler.java:269)
»  	at org.opensearch.cluster.coordination.PublicationTransportHandler.lambda$new$1(PublicationTransportHandler.java:136)
»  	at org.opensearch.wlm.WorkloadManagementTransportInterceptor$RequestHandler.messageReceived(WorkloadManagementTransportInterceptor.java:63)
»  	at org.opensearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:108)
»  	at org.opensearch.transport.NativeMessageHandler$RequestHandler.doRun(NativeMessageHandler.java:487)
»  	at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:1014)
»  	at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:52)
»  	at java.****/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
»  	at java.****/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
»  	at java.****/java.lang.Thread.run(Thread.java:1583)
»  	Suppressed: org.opensearch.gateway.remote.RemoteStateTransferException: Download failed for nodes
»  		at org.opensearch.gateway.remote.RemoteClusterStateAttributesManager.lambda$getWrappedReadListener$3(RemoteClusterStateAttributesManager.java:103)
»  		at org.opensearch.core.action.ActionListener$1.onFailure(ActionListener.java:90)
»  		at org.opensearch.common.remote.RemoteWriteableEntityBlobStore.lambda$readAsync$0(RemoteWriteableEntityBlobStore.java:87)
»  		at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:955)
»  		... 3 more
»  	Caused by: java.lang.IllegalStateException: unexpected byte [0x1d]
»  		at org.opensearch.core.common.io.stream.StreamInput.readBoolean(StreamInput.java:596)
»  		at org.opensearch.core.common.io.stream.StreamInput.readBoolean(StreamInput.java:586)
»  		at org.opensearch.cluster.node.DiscoveryNode.<init>(DiscoveryNode.java:344)
»  		at org.opensearch.cluster.node.DiscoveryNodes.readFrom(DiscoveryNodes.java:777)
»  		at org.opensearch.gateway.remote.model.RemoteDiscoveryNodes.lambda$static$0(RemoteDiscoveryNodes.java:37)
»  		at org.opensearch.repositories.blobstore.ChecksumWritableBlobStoreFormat.deserialize(ChecksumWritableBlobStoreFormat.java:105)
»  		at org.opensearch.gateway.remote.model.RemoteDiscoveryNodes.deserialize(RemoteDiscoveryNodes.java:101)
»  		at org.opensearch.gateway.remote.model.RemoteDiscoveryNodes.deserialize(RemoteDiscoveryNodes.java:32)
»  		at org.opensearch.common.remote.RemoteWriteableEntityBlobStore.read(RemoteWriteableEntityBlobStore.java:77)
»  		at org.opensearch.common.remote.RemoteWriteableEntityBlobStore.lambda$readAsync$0(RemoteWriteableEntityBlobStore.java:85)
»  		... 4 more
»   ↑ repeated 44 times ↑

Related Issues

Fixes #20792 (comment)

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b47e678)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Core infrastructure: version-aware serialization in blob store format and remote entity base classes Relevant files: server/src/main/java/org/opensearch/repositories/blobstore/ChecksumWritableBlobStoreFormat.java server/src/main/java/org/opensearch/common/remote/RemoteWriteableBlobEntity.java server/src/main/java/org/opensearch/common/remote/AbstractClusterMetadataWriteableBlobEntity.java Sub-PR theme: Remote model classes: propagate minNodeVersion to all cluster state remote objects Relevant files: server/src/main/java/org/opensearch/gateway/remote/model/RemoteDiscoveryNodes.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteClusterBlocks.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteHashesOfConsistentSettings.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteCoordinationMetadata.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteTransientSettingsMetadata.java server/src/main/java/org/opensearch/gateway/remote/model/RemotePersistentSettingsMetadata.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteTemplatesMetadata.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteCustomMetadata.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteClusterStateCustoms.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteIndexMetadata.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteClusterMetadataManifest.java server/src/main/java/org/opensearch/gateway/remote/model/RemoteGlobalMetadata.java Sub-PR theme: Routing table and cluster state service: wire minNodeVersion through write actions Relevant files: server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java server/src/main/java/org/opensearch/cluster/routing/remote/NoopRemoteRoutingTableService.java server/src/main/java/org/opensearch/cluster/routing/remote/InternalRemoteRoutingTableService.java server/src/main/java/org/opensearch/gateway/remote/routingtable/RemoteIndexRoutingTable.java server/src/main/java/org/opensearch/gateway/remote/routingtable/RemoteRoutingTableDiff.java server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java server/src/main/java/org/opensearch/gateway/remote/RemoteManifestManager.java

⚡ Recommended focus areas for review Hardcoded Version The manifest is written with Version.CURRENT hardcoded rather than using minNodeVersion. This is inconsistent with the fix applied elsewhere and may still cause BWC issues when the manifest is read by older nodes. uploadManifest, clusterUUID, compressor, namedXContentRegistry, Version.CURRENT ); Hardcoded Version RemoteGlobalMetadata uses Version.CURRENT hardcoded in its constructor rather than accepting a minNodeVersion parameter. This class may also be involved in the remote cluster state path and could still cause BWC issues. super(clusterUUID, compressor, namedXContentRegistry, Version.CURRENT); minNodeVersion Scope minNodeVersion is derived from clusterState.nodes().getMinNodeVersion() at the start of writeMetadataInParallel. If the cluster state's node list is incomplete or stale (e.g., during a rolling upgrade where not all nodes have joined yet), the minimum version could be incorrect, potentially causing serialization with a version that is too high or too low. final Version minNodeVersion = clusterState.nodes().getMinNodeVersion(); Null Reader in Test In mockBlobContainerForGlobalMetadata, a ChecksumWritableBlobStoreFormat is created with a null reader. If deserialization is ever triggered in this test path, it will throw a NullPointerException. This should be validated or documented as intentional. ChecksumWritableBlobStoreFormat<Metadata.Custom> customMetadataFormat = new ChecksumWritableBlobStoreFormat<>( "custom", null, Version.CURRENT );

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b47e678

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Manifest written with hardcoded current version The manifest is being written with Version.CURRENT hardcoded, but the manifest writing should also use the cluster's minimum node version to ensure backward compatibility during rolling upgrades. Using Version.CURRENT here means the manifest may be serialized in a format that older nodes in the cluster cannot read. server/src/main/java/org/opensearch/gateway/remote/RemoteManifestManager.java [150-156] RemoteClusterMetadataManifest remoteClusterMetadataManifest = new RemoteClusterMetadataManifest( uploadManifest, clusterUUID, compressor, namedXContentRegistry, - Version.CURRENT + uploadManifest.getMinNodeVersion() != null ? uploadManifest.getMinNodeVersion() : Version.CURRENT ); Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about using Version.CURRENT for manifest writing during rolling upgrades. However, the ClusterMetadataManifest may not have a getMinNodeVersion() method, and the improved code assumes this API exists without verification from the diff. The concern is legitimate but the proposed fix may not compile.	Low
	Missing null guard for version during serialization When serializing, the stream version is now set to opensearchVersion (the minimum node version). However, if opensearchVersion is null, this will throw a NullPointerException. A null check or a fallback to Version.CURRENT should be added to guard against this. server/src/main/java/org/opensearch/repositories/blobstore/ChecksumWritableBlobStoreFormat.java [90-91] -stream.setVersion(opensearchVersion); +stream.setVersion(opensearchVersion != null ? opensearchVersion : Version.CURRENT); writer.write(stream, obj); Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that opensearchVersion could be null and cause an NPE. Looking at the PR, minNodeVersion is passed from clusterState.nodes().getMinNodeVersion() which should not be null in practice, but the defensive null check is a reasonable safety measure for the serialization path.	Low
General	Null version may cause NPE in downstream serialization The old no-arg namedXContentRegistry constructor previously passed null as the registry, which is preserved here. However, the old constructor also didn't accept a minNodeVersion, so callers that previously used the two-argument form now must provide a version. Ensure that all callers that previously used the old two-argument constructor (without namedXContentRegistry) are updated to pass a valid minNodeVersion rather than null, to avoid NPE in ChecksumWritableBlobStoreFormat. server/src/main/java/org/opensearch/common/remote/AbstractClusterMetadataWriteableBlobEntity.java [35-38] public AbstractClusterMetadataWriteableBlobEntity(final String clusterUUID, final Compressor compressor, final Version minNodeVersion) { - super(clusterUUID, compressor, minNodeVersion); + super(clusterUUID, compressor, minNodeVersion != null ? minNodeVersion : Version.CURRENT); this.namedXContentRegistry = null; } Suggestion importance[1-10]: 4 __ Why: The suggestion is valid in that callers must now provide a non-null minNodeVersion, but the PR consistently passes Version.CURRENT or minNodeVersion from cluster state, making null unlikely. The defensive fallback is a minor improvement but not critical given the PR's usage patterns.	Low

---

Previous suggestions

Suggestions up to commit 43d8165

Category	Suggestion	Impact
General	Avoid redundant double-setting of minNodeVersion The constructor calls super(clusterUUID, compressor, null) which sets minNodeVersion to Version.CURRENT in the parent, and then immediately calls setMinNodeVersion(minNodeVersion) to override it. This is redundant and fragile. Instead, use the parent constructor that accepts minNodeVersion directly: super(clusterUUID, compressor, null, minNodeVersion) (if available) or refactor to avoid the double-set pattern. server/src/main/java/org/opensearch/gateway/remote/model/RemoteClusterBlocks.java [41-53] public RemoteClusterBlocks( final ClusterBlocks clusterBlocks, long stateVersion, String clusterUUID, final Compressor compressor, final Version minNodeVersion ) { - super(clusterUUID, compressor, null); - setMinNodeVersion(minNodeVersion); + super(clusterUUID, compressor, minNodeVersion); this.clusterBlocks = clusterBlocks; this.stateVersion = stateVersion; this.clusterBlocksFormat = new ChecksumWritableBlobStoreFormat<>("blocks", ClusterBlocks::readFrom, minNodeVersion); } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that super(clusterUUID, compressor, null) sets minNodeVersion to Version.CURRENT and then setMinNodeVersion(minNodeVersion) overrides it. However, the improved_code calls super(clusterUUID, compressor, minNodeVersion) which skips the namedXContentRegistry=null parameter, so the parent constructor signature doesn't match AbstractClusterMetadataWriteableBlobEntity(clusterUUID, compressor, namedXContentRegistry, minNodeVersion). The fix is valid in spirit but the improved code doesn't correctly use the available constructor.	Low
	Document default version assumption in constructor The no-arg constructor defaults minNodeVersion to Version.CURRENT, but this is used for deserialization paths where the version should come from the manifest, not the current node. Defaulting to Version.CURRENT could cause incorrect serialization format assumptions during reads on older data. Consider using null or a sentinel value and requiring callers to explicitly set the version, or at minimum document this behavior clearly. server/src/main/java/org/opensearch/common/remote/RemoteWriteableBlobEntity.java [31-35] public RemoteWriteableBlobEntity(final String clusterUUID, final Compressor compressor) { this.clusterUUID = clusterUUID; this.compressor = compressor; + // minNodeVersion defaults to CURRENT; callers performing reads should set the appropriate version this.minNodeVersion = Version.CURRENT; } Suggestion importance[1-10]: 2 __ Why: The suggestion only adds a comment to document existing behavior without changing any logic. The improved_code is functionally identical to the existing_code, making this a documentation-only change with minimal impact.	Low
Possible issue	Validate version is non-null before serialization When serializing, stream.setVersion(opensearchVersion) is set to the minimum node version of the cluster. If opensearchVersion is an older version, this could cause serialization to omit fields or use a format incompatible with the current node reading the data back. Ensure that opensearchVersion is validated to be non-null and within an acceptable range before being used for serialization to prevent data corruption. server/src/main/java/org/opensearch/repositories/blobstore/ChecksumWritableBlobStoreFormat.java [90-91] +Objects.requireNonNull(opensearchVersion, "opensearchVersion must not be null for serialization"); stream.setVersion(opensearchVersion); writer.write(stream, obj); Suggestion importance[1-10]: 3 __ Why: Adding a null check for opensearchVersion is a reasonable defensive measure, but the constructor already requires a non-null Version parameter and all callers in the PR pass explicit non-null values, making this check low-priority. This is a minor defensive improvement.	Low

Suggestions up to commit 0875f94

Category	Suggestion	Impact
General	Use constructor injection instead of setter for version The constructor calls super(clusterUUID, compressor, null) which uses the base constructor without minNodeVersion, then calls setMinNodeVersion(minNodeVersion) as a workaround. Since AbstractClusterMetadataWriteableBlobEntity now has a constructor that accepts minNodeVersion, you should use it directly to avoid redundant setter calls and ensure consistency. The same pattern applies to RemoteClusterStateCustoms, RemoteCustomMetadata, RemoteDiscoveryNodes, RemoteHashesOfConsistentSettings, RemoteIndexRoutingTable, and RemoteRoutingTableDiff. server/src/main/java/org/opensearch/gateway/remote/model/RemoteClusterBlocks.java [41-53] public RemoteClusterBlocks( final ClusterBlocks clusterBlocks, long stateVersion, String clusterUUID, final Compressor compressor, final Version minNodeVersion ) { - super(clusterUUID, compressor, null); - setMinNodeVersion(minNodeVersion); + super(clusterUUID, compressor, null, minNodeVersion); this.clusterBlocks = clusterBlocks; this.stateVersion = stateVersion; this.clusterBlocksFormat = new ChecksumWritableBlobStoreFormat<>("blocks", ClusterBlocks::readFrom, minNodeVersion); } Suggestion importance[1-10]: 5 __ Why: The suggestion is valid - using super(clusterUUID, compressor, null, minNodeVersion) would be cleaner than calling super(clusterUUID, compressor, null) followed by setMinNodeVersion(minNodeVersion). However, the AbstractClusterMetadataWriteableBlobEntity constructor with minNodeVersion passes null for namedXContentRegistry, which matches the current call pattern. This is a minor code quality improvement.	Low
	Delegate to primary constructor to reduce duplication The no-arg version of the constructor defaults minNodeVersion to Version.CURRENT, which may silently use the wrong version when callers forget to pass minNodeVersion. This could cause BWC issues to go undetected. Consider delegating to the two-arg constructor to keep initialization consistent and reduce duplication. server/src/main/java/org/opensearch/common/remote/RemoteWriteableBlobEntity.java [31-35] public RemoteWriteableBlobEntity(final String clusterUUID, final Compressor compressor) { - this.clusterUUID = clusterUUID; - this.compressor = compressor; - this.minNodeVersion = Version.CURRENT; + this(clusterUUID, compressor, Version.CURRENT); } Suggestion importance[1-10]: 4 __ Why: The suggestion to use constructor delegation (this(clusterUUID, compressor, Version.CURRENT)) is a valid refactoring that reduces code duplication. The improved_code accurately reflects the change, though the impact is minor as both approaches produce the same result.	Low
	Verify version-gated fields in serialized Writeable types Setting the stream version to opensearchVersion (i.e., minNodeVersion) during serialization means the written format will be compatible with the minimum node version. Ensure that all Writeable types serialized through this format correctly check StreamOutput.getVersion() before writing version-specific fields, otherwise newer fields may be silently dropped when minNodeVersion is an older version. server/src/main/java/org/opensearch/repositories/blobstore/ChecksumWritableBlobStoreFormat.java [90-91] +stream.setVersion(opensearchVersion); +writer.write(stream, obj); - Suggestion importance[1-10]: 2 __ Why: This suggestion asks the user to verify behavior rather than proposing a concrete code change - the existing_code and improved_code are identical. While the concern about BWC compatibility is valid, it's more of a review comment than an actionable code suggestion.	Low

Suggestions up to commit 3c11e50

Category	Suggestion	Impact
Possible issue	Use configured version instead of minimum compatibility version for writing Setting the stream version to minimumCompatibilityVersion() when writing blobs means data will always be serialized at the minimum compatible version, potentially losing fields or features introduced in newer versions. The stream version during write should reflect the actual node version (Version.CURRENT) or the configured opensearchVersion field, not the minimum compatibility version. Consider using this.opensearchVersion instead to respect the version passed during construction. server/src/main/java/org/opensearch/repositories/blobstore/ChecksumWritableBlobStoreFormat.java [95] -stream.setVersion(Version.CURRENT.minimumCompatibilityVersion()); +stream.setVersion(opensearchVersion); Suggestion importance[1-10]: 7 __ Why: Using this.opensearchVersion instead of Version.CURRENT.minimumCompatibilityVersion() would make the write version consistent with the version passed during construction, avoiding potential data loss for features in newer versions. This is a meaningful improvement in consistency and correctness.	Medium
	Avoid unnecessary version downgrade in default constructor Using minimumCompatibilityVersion() as the default version may cause issues when writing blobs, as it downgrades the serialization version unnecessarily for nodes running the current version. The default constructor is likely called when no specific version context is available, and should default to Version.CURRENT to ensure full feature compatibility. Only when dealing with cross-version BWC scenarios should a lower version be used. server/src/main/java/org/opensearch/repositories/blobstore/ChecksumWritableBlobStoreFormat.java [62] -this(codec, reader, Version.CURRENT.minimumCompatibilityVersion()); +this(codec, reader, Version.CURRENT); Suggestion importance[1-10]: 6 __ Why: The PR intentionally changes the default from Version.CURRENT to Version.CURRENT.minimumCompatibilityVersion(), but this suggestion raises a valid concern that downgrading the serialization version in the default constructor could lose newer features. However, this may be an intentional BWC change, so the score reflects uncertainty about intent.	Low

Suggestions up to commit 4535908

Category	Suggestion	Impact
General	Avoid always downgrading stream version unnecessarily Using minimumCompatibilityVersion() for all writes means the stream will always be serialized at the minimum compatibility version, even when all nodes in the cluster support the current version. This could cause unnecessary data loss of newer serialization features. The version should ideally be negotiated based on the actual target node's version or at least conditionally set based on cluster state, rather than always defaulting to the minimum compatibility version. server/src/main/java/org/opensearch/repositories/blobstore/ChecksumWritableBlobStoreFormat.java [95] -stream.setVersion(Version.CURRENT.minimumCompatibilityVersion()); +// Use the minimum compatibility version only when needed for BWC, otherwise use CURRENT +stream.setVersion(Version.CURRENT); Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about always using minimumCompatibilityVersion() instead of Version.CURRENT, which could unnecessarily limit serialization features. However, the PR's intent appears to be improving backward compatibility for repository formats, and the 'improved_code' essentially reverts the PR change back to the original code, which may not be the right solution either.	Low

[github-actions]

❌ Gradle check result for 4535908: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 3c11e50

[github-actions]

❌ Gradle check result for 3c11e50: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 0875f94

[github-actions]

❌ Gradle check result for 0875f94: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 43d8165

[github-actions]

❌ Gradle check result for 43d8165: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit b47e678

[github-actions]

❌ Gradle check result for b47e678: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?