HDDS-15641. SCM should send write pipeline version with DatanodeDetails on block allocation - #10878
HDDS-15641. SCM should send write pipeline version with DatanodeDetails on block allocation#10878dombizita wants to merge 4 commits into
Conversation
errose28
left a comment
There was a problem hiding this comment.
Thanks for working on this @dombizita. I realized while reviewing that the current implementation has a compatibility issue, but luckily our versioning framework is equipped to handle it. It's a tricky case but let me know if this explanation makes sense.
Right now we are setting currentVersion in the DatanodeDetails to the Datanode's apparent version. Before ZDU is finalized, DNs will report their apparent version from the HDDSLayoutFeature enum. Clients, however, will be comparing this agains the HDDSVersion enum, creating a type mismatch which may cause them to incorrectly interpret the DN server's available functionality.
The ZDU version is the merge point of the HDDSLayoutFeature and HDDSVersion enums. Once ZDU is finalized, we know all apparent versions have type HDDSVersion. Only HDDSVersions are compatible with clients and able to be shared outside the cluster.
The fix is to only set DatanodeDetails#currentVersion to apparentVersion after ZDU is finalized. At this point the server knows all apparent versions will have type HDDSVersion and are compatible to share with clients.
When ZDU is pre-finalized, we must set DatanodeDetails#currentVersion to the last HDDSVersion before ZDU, regardless of our actual software version which may be later than that. This can be thought of as "pre-finalizing" the HDDSVersion to match the cluster's currently supported capabilities. We cannot keep sending the latest software version because it may cause the latest clients to try to use unfinalized features.
The latest clients who support forwarding write pipeline version to DNs can skip the forwarding when they get a DN currentVersion less than ZDU. This means the cluster is not yet finalized for ZDU and no write pipeline versioning is in place. Even if clients forwarded the version in this case the server would ignore it due to its lower apparent version.
We can use this PR to fix the HDDS write path. The OM and HDDS read paths have this problem as well, which will be fixed in HDDS-16044.
| * features until every datanode handling their writes has finalized, so the | ||
| * minimum apparent version across the pipeline is used. | ||
| */ | ||
| private int computeClusterWriteVersion(Pipeline pipeline) throws NodeNotFoundException { |
There was a problem hiding this comment.
| private int computeClusterWriteVersion(Pipeline pipeline) throws NodeNotFoundException { | |
| private int computePipelineWriteVersion(Pipeline pipeline) throws NodeNotFoundException { |
| * version. Member order is preserved, keeping {@code memberOrders} and | ||
| * {@code memberReplicaIndexes} indices valid. | ||
| */ | ||
| private static HddsProtos.Pipeline withWriteVersion( |
There was a problem hiding this comment.
This is doing an extra copy of the pipeline object. We can add the apparent pipeline version as a ComponentVersion parameter to Pipeline#getProtobufMessage. It would then be passed through to DatanodeDetails#toProtoBuilder as a new parameter as well to set the apparent version of the node. We may need to add overloads for these methods when they are used elsewhere that use the DatanodeDetail's existing version when a new one is not specified.
| return scm.getScmNodeManager() | ||
| .getLowestApparentVersion(pipeline.getNodes().toArray(new DatanodeDetails[0])) | ||
| .serialize(); |
There was a problem hiding this comment.
We can return ComponentVersion here and save serialization for the last step before proto conversion.
| @Test | ||
| void finalizedClusterForwardsRealVersion() throws Exception { | ||
| // Default setup: SCM and all datanodes at ZDU. | ||
| assertAllMembersHaveVersion(HDDSVersion.ZDU.serialize(), |
There was a problem hiding this comment.
ZDU won't be the finalized version in future releases. SOFTWARE_VERSION would need to be used for that.
| // Datanodes still run ZDU software but have not finalized past | ||
| // STREAM_BLOCK_SUPPORT, so their apparent version is the older one. | ||
| for (DatanodeDetails dn : nodes) { | ||
| setDatanodeApparentVersion(dn, HDDSVersion.STREAM_BLOCK_SUPPORT); |
There was a problem hiding this comment.
Technically we should use HDDSLayoutFeature for the earlier versions, since that's what Datanodes will be reporting to SCM when pre-finalized for ZDU from an older version. We should update the tests for clarity, but it won't matter in practice because nothing within the datanode will be reading this version field until a write pipeline incompatibility is potentially introduced after ZDU.
| /** The version each source datanode reports as its own software version. */ | ||
| private static final int DN_SOFTWARE_VERSION = | ||
| HDDSVersion.ZDU.serialize(); |
There was a problem hiding this comment.
Datanodes report their apparent and software versions, but the functionality under test is only concerned with apparent version. It looks like this is aiming to set the default apparent version reported to show nodes as finalized, which should be SOFTWARE_VERSION.
| assertAllMembersHaveVersion(HDDSVersion.STREAM_BLOCK_SUPPORT.serialize(), | ||
| allocateAndGetMembers()); |
There was a problem hiding this comment.
nit. We don't need to wrap the lines this much.
| try { | ||
| pipelineProto = withWriteVersion( | ||
| pipeline.getProtobufMessage(clientVersion, Name.IO_PORTS), | ||
| computeClusterWriteVersion(pipeline)); |
There was a problem hiding this comment.
We shouldn't have a pipeline with zero DNs, but just in case we should validate this and return a checked exception if it happens. Otherwise will hit the unchecked IllegalArgumentException when finding the minimum version.
| throw new IllegalStateException("Datanode not found in NodeManager " | ||
| + "while computing the write version for pipeline " | ||
| + pipeline.getId() + " during block allocation. Should not happen", e); |
There was a problem hiding this comment.
In this case I think we should convert to ScmException with result code NO_SUCH_DATANODE to cleanly fail the RPC call.
What changes were proposed in this pull request?
During a zero-downtime upgrade (ZDU), a write pipeline can contain a mix of finalized and unfinalized datanodes. An unfinalized datanode already runs the newer software but continues to report the older apparent version until it finalizes. If a client enables a newer write-path feature against such a pipeline, the unfinalized datanode cannot handle it.
This change makes SCM send, on block allocation, a write pipeline version that reflects what every datanode in the pipeline can actually handle, so clients only enable features supported by all of them.
I needed to adjust
TestBlockDataStreamOutput, so both the DN (so it matches the expected currentVersion) and SCM (just the test was passing without this, but running the whole test class was failing, because during the datanodes over the lifetime of the test class got finalized via SCM) apparent versions are set to the same version.These changes are based on #10570, please check the last 4 commits for this change set.
Used Claude Opus 4.8 for planning and coding, reviewed by me.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15641
How was this patch tested?
CI on my fork: https://github.com/dombizita/ozone/actions/runs/30096185706 (with newly added unit tests)