HDDS-16067. Intermittent timeout in testNodeWithOpenPipelineCanBeDecommissionedAndRecommissioned - #10939
HDDS-16067. Intermittent timeout in testNodeWithOpenPipelineCanBeDecommissionedAndRecommissioned#10939echonesis wants to merge 2 commits into
Conversation
…mmissionedAndRecommissioned
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for working on this! @echonesis
starting datanodes with the latest layout version;
waiting only for the restarted datanode to become healthy instead of the entire cluster;
using the refreshed DatanodeDetails after restart.
I looked into this a bit more and tested the changes separately with flaky-test-check on my fork:
- Control: failure reproduced
- Wait-only: same NPE reproduced
- Layout-only: 50 completed iterations passed + another 2 split
Note: Not sure why but flaky-test-check hung test forks are never killed; a split can burn a 6 hour runner. (I'll investigate it later)
However, based on the results, I believe the root cause appears to be the JVM-wide static VersionedDatanodeFeatures.versionManager. Background clusters can overwrite it with layout version 0 while the restarted DN loads containers, causing it to select an uninitialized legacy table and fail to register.
Since this is not an upgrade test, pinning DNs to maxLayoutVersion() looks like the right fix, while the wait-only change does not address the failure. Could you please double-check my findings? I left a few comments. Thanks!
| waitForDnToReachHealthState(nm, newDn, HEALTHY); | ||
| waitForDnToReachOpState(nm, newDn, IN_SERVICE); | ||
| waitForDnToReachPersistedOpState(dn, IN_SERVICE); | ||
| waitForDnToReachPersistedOpState(newDn, IN_SERVICE); |
There was a problem hiding this comment.
Could you help clarify why this change is needed here? It is in a different test method, so I may be missing how it relates to HDDS-16067.
There was a problem hiding this comment.
This change was added based on an earlier full-class flaky-test-check: before the combined fix, 6 of 10 splits failed, while the full class passed after adding the layout pin and refreshing post-restart DatanodeDetails references, including this one. At that point, I suspected this assertion could also be observing a stale pre-restart instance.
However, those changes were not tested independently. Your layout-only run keeps the original line here and passes the completed full-class iterations, so I agree that this change is not necessary for HDDS-16067. I’ll revert it to keep the PR scoped to the isolated root cause.
| waitForDnToReachPersistedOpState(toDecommission, IN_SERVICE); | ||
| getDNHostAndPort(restarted))); | ||
| waitForDnToReachOpState(nm, restarted, IN_SERVICE); | ||
| waitForDnToReachPersistedOpState(restarted, IN_SERVICE); |
There was a problem hiding this comment.
I tested this without the layout pin above, and the test still failed with the same containerCreateInfoTable NPE in this wait-only run.
Do you know if there is another CI failure that still occurs with the layout fix alone? If not, I’m wondering whether this wait change is needed for this PR, especially since it reduces the timeout from 120 to 30 seconds.
There was a problem hiding this comment.
Thanks for isolating the changes. I’m not aware of another CI failure that still occurs with the layout fix alone. My earlier full-class flaky-test-check compared the wait-only change with the combined patch, so it did not establish that this wait change provided any additional benefit.
I agree that reducing the timeout from 120 to 30 seconds could introduce a separate failure on slower runners. I’ve restored the original restartHddsDatanode(dnIndex, true) flow and removed the refreshed DatanodeDetails changes, leaving only the layout-version fix.
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the patch! @echonesis LGTM +1.
The PR description still mentions the reverted changes, could you update it?
Thanks for catching this. I’ve updated the PR description to remove the reverted changes. |
What changes were proposed in this pull request?
This PR stabilizes
TestDecommissionAndMaintenanceby starting datanodes with the latest layout version.This avoids intermittent metadata initialization timeouts without increasing the existing timeout.
Generated-by: Codex (GPT-5)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16067
How was this patch tested?
Local Test
GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/30869467058
Flaky-test-check: https://github.com/echonesis/ozone/actions/runs/30869665597