HDDS-14812. Support datanode topology in MiniOzoneCluster - #10598
HDDS-14812. Support datanode topology in MiniOzoneCluster#10598hevinhsu wants to merge 11 commits into
Conversation
peterxcli
left a comment
peterxcli
left a comment
There was a problem hiding this comment.
I remember you said this is the behaviour cloning of existing topology test cases but in unit test, could you also add them in description? Thanks!
|
Thanks @peterxcli for the reminder! I've updated the PR description. Originally I intended to port the unit test cases directly, but the unit test logic relies on mocked components that don't translate to an integration test context, so I instead validated that rack topology works correctly at a high level across the three test classes. Let me know if you'd like more test scenarios covered or if the tests should be included in this PR. |
|
Thanks @hevinhsu for the patch, I'll review this soon.
@hevinhsu I think it's good to include the rack placement test and we can add a test for each of the pipeline and container placement policy. For the TestScmHAFinalizationWithRacks and TestSecretKeySnapshotWithRack I don't think they are affected by the the topology so might not be needed. |
|
Hi @ivandika3, thanks for your suggestion. I've added Based on my understanding, the rack-aware placement policies are covered as follows:
If I misunderstood the placement policies or missed another rack-aware policy that should be covered, please let me know. |
| * DNSToSwitchMapping.class); | ||
| * }</pre> | ||
| */ | ||
| public class FixedHostMapping extends CachedDNSToSwitchMapping { |
There was a problem hiding this comment.
Can I check what is the difference between this and the Hadoop StaticMapping used in Hadoop?
There was a problem hiding this comment.
The main difference is that StaticMapping extends AbstractDNSToSwitchMapping, whereas our implementation extends CachedDNSToSwitchMapping.
Because of this, StaticMapping is wrapped by CachedDNSToSwitchMapping in SCM (code).
As a result, calls to resolve() first go through CachedDNSToSwitchMapping#resolve(), which normalizes the input hostnames using NetUtils.normalizeHostNames() (code). This performs a native DNS lookup, so a hostname such as host1.com is resolved to its IP address before the rack mapping is applied. As a result, the predefined hostname-to-rack mapping used by the test is no longer matched correctly.
There was a problem hiding this comment.
Thanks for the info. Yes, unlike HDFS seems Ozone will try to wrap it in CachedDNSToSwitchMapping which was introduced in HDDS-1663. Personally, I don't think we need to always wrap to to CachedDNSToSwitchMapping due to the issue you mentioned, that way we can simply use StaticMapping. However, let's leave it be for now.
Although I saw some StaticMapping usage in integration tests like in TestFailureHandlingByClient, not sure if this has similar issues.
conf.setClass(NET_TOPOLOGY_NODE_SWITCH_MAPPING_IMPL_KEY,
StaticMapping.class, DNSToSwitchMapping.class);
StaticMapping.addNodeToRack(NetUtils.normalizeHostNames(
Collections.singleton(HddsUtils.getHostName(conf))).get(0),
"/rack1");
cc: @ChenSammi
There was a problem hiding this comment.
In my understanding, TestFailureHandlingByClient does not use a user-defined host, so this shouldn't be an issue.
I'm curious though — TestFailureHandlingByClient doesn't seem to verify racks. I tried removing conf.setClass(NET_TOPOLOGY_NODE_SWITCH_MAPPING_IMPL_KEY, StaticMapping.class, DNSToSwitchMapping.class); and all tests still passed. So I'm wondering why we need to configure StaticMapping here at all.
| * DNSToSwitchMapping.class); | ||
| * }</pre> | ||
| */ | ||
| public class FixedHostMapping extends CachedDNSToSwitchMapping { |
There was a problem hiding this comment.
Thanks for the info. Yes, unlike HDFS seems Ozone will try to wrap it in CachedDNSToSwitchMapping which was introduced in HDDS-1663. Personally, I don't think we need to always wrap to to CachedDNSToSwitchMapping due to the issue you mentioned, that way we can simply use StaticMapping. However, let's leave it be for now.
Although I saw some StaticMapping usage in integration tests like in TestFailureHandlingByClient, not sure if this has similar issues.
conf.setClass(NET_TOPOLOGY_NODE_SWITCH_MAPPING_IMPL_KEY,
StaticMapping.class, DNSToSwitchMapping.class);
StaticMapping.addNodeToRack(NetUtils.normalizeHostNames(
Collections.singleton(HddsUtils.getHostName(conf))).get(0),
"/rack1");
cc: @ChenSammi
| } | ||
|
|
||
| // Bypass InetAddress.getName() resolution for custom hostnames by starting DN via YAML. | ||
| confDatanodeViaYaml(dnConf); |
There was a problem hiding this comment.
Could you help explain this? This seems quite to be another workaround.
There was a problem hiding this comment.
This looks like the same problem as StaticMapping#resolve.
During Datanode startup, DatanodeDetails attempts to set its ipAddress via validateDatanodeIpAddress(). If there is no existing DatanodeDetails (i.e., not imported from a yaml file), the ipAddress may initially be null. When a custom hostname is set in the configuration, resolving it via InetAddress.getByName() may throw an UnknownHostException. Although the exception is caught and logged, the ipAddress remains null (since it was never successfully set). This can prevent the Datanode from successfully connecting to and registering with SCM, which in turn causes MiniOzoneCluster.waitForClusterToBeReady to time out.
There was a problem hiding this comment.
This can prevent the Datanode from successfully connecting to and registering with SCM, which in turn causes MiniOzoneCluster.waitForClusterToBeReady to time out
This seems to be a latent bug waiting to be triggered. Could you raise a ticket for this? If we know that the datanode is not going to be registered to the SCM, we should not let it start at all (we can simply throw the exception and let the DN crash before starting). Otherwise, we will have a situation where the datanode looks online, but it is not registered which is very hard to debug. cc: @ptlrs
To avoid this workaround, how about we just set setHostname to 127.0.0.1 or DNS.getDefaultHost? Will it work?
There was a problem hiding this comment.
To avoid this workaround, how about we just set
setHostnameto 127.0.0.1 orDNS.getDefaultHost? Will it work?
Thanks for the suggestion! I think it should work for avoiding this workaround.
I'm wondering if it would affect the rack mapping, since it is based on the hostname. If all Datanodes use 127.0.0.1 or DNS.getDefaultHost(), they would all share the same hostname and could no longer be mapped to different racks. It would also mean that the values provided via setHosts() are no longer reflected in the registered DatanodeDetails.
There was a problem hiding this comment.
This seems to be a latent bug waiting to be triggered. Could you raise a ticket for this?
I did some more investigation into this.
When the configured hostname cannot be resolved, validateDatanodeIpAddress() catches the UnknownHostException and only logs it, leaving DatanodeDetails.ipAddress as null while allowing the datanode to continue startup.
During the subsequent communication with SCM, the datanode attempts to construct a protobuf message from DatanodeDetails, but DatanodeDetails#getProtoBufMessage() fails because the required ipAddress field is missing, preventing the registration request from being sent to SCM.
The datanode remains in the RUNNING state, and subsequent registration attempts continue to fail for the same reason. As a result, the datanode remains running but cannot register with SCM, and MiniOzoneCluster.waitForClusterToBeReady() eventually times out.
This seems to match the latent issue you mentioned, so I'll file a follow-up ticket with the updated investigation results.
There was a problem hiding this comment.
Hi @ivandika3, I've raised a follow-up JIRA ticket: https://issues.apache.org/jira/browse/HDDS-16007
PTAL when you have time. Thanks!
|
@ivandika3 No worries at all! Thanks for the review. I've addressed your comments, PTAL when you have time! Also, this implementation ended up being a bit awkward because of the host resolution workarounds. If you have any suggestions for a cleaner approach, I'd really appreciate your thoughts. |
What changes were proposed in this pull request?
Support rack and host topology awareness in
MiniOzoneClusterandMiniOzoneHAClusterImpl. This enables integration tests to properly exercise topology-sensitive pipeline placement.Key changes:
FixedHostMapping, aCachedDNSToSwitchMappingimplementation that resolves hostnames to rack locations via a static map, bypassing DNS lookups to handle synthetic or unresolvable test hostnames.DatanodeDetailsvia the DN yaml file mechanism to bypassvalidateDatanodeIpAddress()duringDatanodeServicestartup, which performs a native DNS lookup and would fail for synthetic hostnames.PipelinePlacementPolicyobserves the full DN pool and produces rack-diverse pipelines.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14812
How was this patch tested?
CI was run on the changes in this PR: MiniOzoneCluster topology tests
Additionally, three integration test classes covering rack-aware scenarios were added and verified in a personal fork (not included in this PR). The tests are structured around three topology configurations: racks+hosts, racks only, and hosts only.
TestRackAwarePlacement— verifies that rack and hostname topology configuredvia
setRacks()/setHosts()is correctly propagated to SCM'sNodeManagerthrough the real cluster startup path, covering three topology configurations
(racks + hosts, racks only, hosts only):
testDatanodesHaveCorrectRack: verifies each datanode'snetworkLocationmatchesthe configured rack
testRatisPipelineSpansMultipleRacks: verifies all open RATIS THREE pipelinesspan ≥ 2 racks
testDatanodesAllInDefaultRack(hosts only): verifies all nodes fall back toNetworkTopology.DEFAULT_RACKwhen no rack is configuredtestDatanodesHaveCorrectHostname: verifies SCM-registered hostnames matchsetHosts()TestScmHAFinalizationWithRacks— verifies that SCM HA upgrade finalization succeeds under different topology configurations. Modelled afterTestScmHAFinalization, withsetRacks()/setHosts()added to the cluster builder:testFinalizationWithLeaderChange*: verifies finalization completes correctly after a leader change mid-finalizationtestFinalizationWithRestart*: verifies finalization resumes correctly after all SCMs are restartedtestSnapshotFinalization*: verifies a lagging SCM correctly catches up via snapshot installationTestSecretKeySnapshotWithRack— verifies that symmetric secret keys are correctly synchronized from leader to follower during snapshot installation under different topology configurations. Modelled afterTestSecretKeySnapshot, withsetRacks()/setHosts()added to the cluster builder:testInstallSnapshot*: verifies the follower receives and applies the leader's secret keys correctly after snapshot installationTests were validated with green CI.