HDDS-5916. Datanodes stuck in leader election in Kubernetes #3186
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| /** | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with this | ||
| * work for additional information regarding copyright ownership. The ASF | ||
| * licenses this file to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * <p> | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * <p> | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| package org.apache.hadoop.hdds.scm.node; | ||
|
|
||
| import org.apache.hadoop.hdds.protocol.DatanodeDetails; | ||
| import org.apache.hadoop.hdds.protocol.proto.HddsProtos; | ||
| import org.apache.hadoop.hdds.scm.ha.SCMService; | ||
| import org.apache.hadoop.hdds.scm.ha.SCMServiceManager; | ||
| import org.apache.hadoop.hdds.scm.node.states.NodeNotFoundException; | ||
| import org.apache.hadoop.hdds.scm.pipeline.PipelineManager; | ||
| import org.apache.hadoop.hdds.server.events.EventHandler; | ||
| import org.apache.hadoop.hdds.server.events.EventPublisher; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Handles datanode ip or hostname change event. | ||
| */ | ||
| public class NodeIpOrHostnameUpdateHandler | ||
| implements EventHandler<DatanodeDetails> { | ||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(NodeIpOrHostnameUpdateHandler.class); | ||
|
|
||
| private final PipelineManager pipelineManager; | ||
| private final NodeDecommissionManager decommissionManager; | ||
| private final SCMServiceManager serviceManager; | ||
|
|
||
| public NodeIpOrHostnameUpdateHandler(PipelineManager pipelineManager, | ||
| NodeDecommissionManager decommissionManager, | ||
| SCMServiceManager serviceManager) { | ||
| this.pipelineManager = pipelineManager; | ||
| this.decommissionManager = decommissionManager; | ||
| this.serviceManager = serviceManager; | ||
| } | ||
|
|
||
| @Override | ||
| public void onMessage(DatanodeDetails datanodeDetails, | ||
| EventPublisher publisher) { | ||
| try { | ||
| pipelineManager.closeStalePipelines(datanodeDetails); | ||
| serviceManager.notifyEventTriggered(SCMService.Event | ||
| .NODE_IP_OR_HOSTNAME_UPDATE_HANDLER_TRIGGERED); | ||
|
|
||
| if (datanodeDetails.getPersistedOpState() | ||
| != HddsProtos.NodeOperationalState.IN_SERVICE) { | ||
| decommissionManager.continueAdminForNode(datanodeDetails); | ||
|
Contributor
There was a problem hiding this comment. It might be better to depend on the guarantees of
Contributor
There was a problem hiding this comment. continueAdminForNode implements the logic for when the |
||
| } | ||
| } catch (NodeNotFoundException e) { | ||
| // Should not happen, as the node has just registered to call this event | ||
| // handler. | ||
| LOG.warn( | ||
|
|
||
| "NodeNotFound when updating the node Ip or host name to the " + | ||
| "decommissionManager", | ||
| e); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -359,33 +359,34 @@ public RegisteredCommand register( | |
| .build(); | ||
| } | ||
|
|
||
| InetAddress dnAddress = Server.getRemoteIp(); | ||
| if (dnAddress != null) { | ||
| // Mostly called inside an RPC, update ip | ||
| datanodeDetails.setHostName(dnAddress.getHostName()); | ||
| datanodeDetails.setIpAddress(dnAddress.getHostAddress()); | ||
| } | ||
|
|
||
| String dnsName; | ||
| String networkLocation; | ||
| datanodeDetails.setNetworkName(datanodeDetails.getUuidString()); | ||
| if (useHostname) { | ||
| dnsName = datanodeDetails.getHostName(); | ||
| } else { | ||
| dnsName = datanodeDetails.getIpAddress(); | ||
| } | ||
| networkLocation = nodeResolve(dnsName); | ||
| if (networkLocation != null) { | ||
| datanodeDetails.setNetworkLocation(networkLocation); | ||
| } | ||
|
|
||
| if (!isNodeRegistered(datanodeDetails)) { | ||
|
Contributor
Author
There was a problem hiding this comment. I delete this line, because these days, when I tested it, I found sometimes dnAddress.getHostName() returns IP instead of hostName, which makes the datanode restarting not work. Please let me know if it is OK to delete this line. @GeorgeJahad @adoroszlai
Contributor
There was a problem hiding this comment. @sokui @adoroszlai I'm nervous about removing the call to setHostName(). I just took around and it seems to get used in many places. I've included some below: Why does restart not work when it returns the IP string instead of the host string?
Contributor
Author
There was a problem hiding this comment. Hi @GeorgeJahad , To not change the old code path, I added the if condition: when
This is the whole story. That's why now I keep the old code path same, but if
Contributor
There was a problem hiding this comment.
If you are sure this is true, then I'm fine with the change.
I'm confused about this statement. The old code path is when "(!isNodeRegistered(datanodeDetails))" is true, isn't it? not when "(!useHostname)" is true? what am I missing?
Contributor
There was a problem hiding this comment. Old codepath means current
Contributor
Author
There was a problem hiding this comment. Sorry for the confusion. The old path is the current master. So I made the following change: from To
Contributor
Author
There was a problem hiding this comment. What I was saying is that even we delete this line |
||
| try { | ||
| clusterMap.add(datanodeDetails); | ||
| nodeStateManager.addNode(datanodeDetails, layoutInfo); | ||
| // Check that datanode in nodeStateManager has topology parent set | ||
| DatanodeDetails dn = nodeStateManager.getNode(datanodeDetails); | ||
| Preconditions.checkState(dn.getParent() != null); | ||
| addEntryToDnsToUuidMap(dnsName, datanodeDetails.getUuidString()); | ||
| // Updating Node Report, as registration is successful | ||
| processNodeReport(datanodeDetails, nodeReport); | ||
| LOG.info("Registered Data node : {}", datanodeDetails); | ||
|
|
@@ -399,6 +400,44 @@ public RegisteredCommand register( | |
| LOG.error("Cannot find datanode {} from nodeStateManager", | ||
| datanodeDetails.toString()); | ||
| } | ||
| } else { | ||
| // Update datanode if it is registered but the ip or hostname changes | ||
| try { | ||
| final DatanodeInfo datanodeInfo = | ||
| nodeStateManager.getNode(datanodeDetails); | ||
| if (!datanodeInfo.getIpAddress().equals(datanodeDetails.getIpAddress()) | ||
| || !datanodeInfo.getHostName() | ||
| .equals(datanodeDetails.getHostName())) { | ||
| LOG.info("Updating data node {} from {} to {}", | ||
| datanodeDetails.getUuidString(), | ||
| datanodeInfo, | ||
| datanodeDetails); | ||
| if (clusterMap.contains(datanodeInfo)) { | ||
|
Contributor
There was a problem hiding this comment. It might be better to implement |
||
| clusterMap.remove(datanodeInfo); | ||
| } | ||
| clusterMap.add(datanodeDetails); | ||
|
|
||
| String oldDnsName; | ||
| if (useHostname) { | ||
| oldDnsName = datanodeInfo.getHostName(); | ||
| } else { | ||
| oldDnsName = datanodeInfo.getIpAddress(); | ||
| } | ||
| removeEntryFromDnsToUuidMap(oldDnsName); | ||
| addEntryToDnsToUuidMap(dnsName, datanodeDetails.getUuidString()); | ||
|
Contributor
There was a problem hiding this comment. Same here better to implement a new method |
||
|
|
||
| nodeStateManager.updateNode(datanodeDetails, layoutInfo); | ||
| DatanodeDetails dn = nodeStateManager.getNode(datanodeDetails); | ||
| Preconditions.checkState(dn.getParent() != null); | ||
| processNodeReport(datanodeDetails, nodeReport); | ||
| LOG.info("Updated Datanode to: {}", dn); | ||
| scmNodeEventPublisher | ||
| .fireEvent(SCMEvents.NODE_IP_OR_HOSTNAME_UPDATE, dn); | ||
| } | ||
| } catch (NodeNotFoundException e) { | ||
| LOG.error("Cannot find datanode {} from nodeStateManager", | ||
| datanodeDetails); | ||
| } | ||
| } | ||
|
|
||
| return RegisteredCommand.newBuilder().setErrorCode(ErrorCode.success) | ||
|
|
@@ -415,11 +454,9 @@ public RegisteredCommand register( | |
| * @param dnsName String representing the hostname or IP of the node | ||
| * @param uuid String representing the UUID of the registered node. | ||
| */ | ||
| @SuppressFBWarnings(value = "AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION") | ||
| private synchronized void addEntryToDnsToUuidMap( | ||
| String dnsName, String uuid) { | ||
| Set<String> dnList = dnsToUuidMap.get(dnsName); | ||
| if (dnList == null) { | ||
| dnList = ConcurrentHashMap.newKeySet(); | ||
|
|
@@ -428,6 +465,19 @@ private synchronized void addEntryTodnsToUuidMap( | |
| dnList.add(uuid); | ||
| } | ||
|
|
||
| private synchronized void removeEntryFromDnsToUuidMap(String dnsName) { | ||
| if (!dnsToUuidMap.containsKey(dnsName)) { | ||
| return; | ||
| } | ||
| Set<String> dnSet = dnsToUuidMap.get(dnsName); | ||
| if (dnSet.contains(dnsName)) { | ||
| dnSet.remove(dnsName); | ||
| } | ||
| if (dnSet.isEmpty()) { | ||
| dnsToUuidMap.remove(dnsName); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Send heartbeat to indicate the datanode is alive and doing well. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well print the actual value calculated in the debug log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure