Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.protobuf.ByteString;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.EnumSet;
Expand Down Expand Up @@ -175,6 +177,29 @@ public void setIpAddress(String ip) {
this.ipAddress = StringWithByteString.valueOf(ip);
}

/**
* Resolves and validates the IP address of the datanode based on its hostname.
* If the resolved IP address differs from the current IP address,
* it updates the IP address to the newly resolved value and logs a warning.
*/
public void validateDatanodeIpAddress() {
if (getIpAddress() == null) {
return;
}

try {
InetAddress inetAddress = InetAddress.getByName(getHostName());

if (!inetAddress.getHostAddress().equals(getIpAddress())) {
LOG.warn("Using resolved IP address '{}' as it differs from the persisted IP address '{}' for datanode '{}'",
inetAddress.getHostAddress(), getIpAddress(), getHostName());
setIpAddress(inetAddress.getHostAddress());
}
} catch (UnknownHostException e) {
LOG.warn("Failed to validate IP address for the datanode '{}'", getHostName(), e);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Let's use local variables to make it a bit easier to follow, and ensure consistency of values.
  • Changing IP address is normal, I think it can be logged at info level.
  • Use this instead of getHostName() in the messages, since DatanodeDetails#toString shows UUID, IP and hostname. (UUID is especially useful in tests where multiple datanodes may be running on the same host.)

public String toString() {
return id + "(" + hostName + "/" + ipAddress + ")";

    final String oldIP = getIpAddress();
    final String hostname = getHostName();
    if (StringUtils.isBlank(hostname)) {
      return;
    }

    try {
      final String newIP = InetAddress.getByName(hostname).getHostAddress();

      if (StringUtils.isNotBlank(newIP) && !newIP.equals(oldIP)) {
        LOG.info("Updating IP address of datanode {} to {}", this, newIP);
        setIpAddress(newIP);
      }
    } catch (UnknownHostException e) {
      LOG.warn("Could not resolve IP address of datanode {}", this, e);
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed review @adoroszlai. I have applied your suggestions.

}

/**
* Returns IP address of DataNode.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ public static synchronized void writeDatanodeDetailsTo(

/**
* Read {@link DatanodeDetails} from a local ID file.
* Use {@link DatanodeDetails#validateDatanodeIpAddress()} to ensure that the IP address matches with the hostname
*
* @param path ID file local path
* @return {@link DatanodeDetails}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,14 @@
import static org.apache.hadoop.ozone.container.ContainerTestHelper.getDummyCommandRequestProto;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.when;

import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
import java.net.InetAddress;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
Expand All @@ -45,6 +49,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.mockito.MockedStatic;

/**
* Test for {@link ContainerUtils}.
Expand Down Expand Up @@ -91,34 +96,50 @@ public void testTarName() throws IOException {
public void testDatanodeIDPersistent(@TempDir File tempDir) throws Exception {
// Generate IDs for testing
DatanodeDetails id1 = randomDatanodeDetails();
id1.setPort(DatanodeDetails.newStandalonePort(1));
assertWriteRead(tempDir, id1);

// Add certificate serial id.
id1.setCertSerialId("" + RandomUtils.nextLong());
assertWriteRead(tempDir, id1);

// Read should return an empty value if file doesn't exist
File nonExistFile = new File(tempDir, "non_exist.id");
assertThrows(IOException.class,
() -> ContainerUtils.readDatanodeDetailsFrom(nonExistFile));

// Read should fail if the file is malformed
File malformedFile = new File(tempDir, "malformed.id");
createMalformedIDFile(malformedFile);
assertThrows(IOException.class,
() -> ContainerUtils.readDatanodeDetailsFrom(malformedFile));

// Test upgrade scenario - protobuf file instead of yaml
File protoFile = new File(tempDir, "valid-proto.id");
try (OutputStream out = Files.newOutputStream(protoFile.toPath())) {
HddsProtos.DatanodeDetailsProto proto = id1.getProtoBufMessage();
proto.writeTo(out);
try (MockedStatic<InetAddress> mockedStaticInetAddress = mockStatic(InetAddress.class)) {
InetAddress mockedInetAddress = mock(InetAddress.class);
mockedStaticInetAddress.when(() -> InetAddress.getByName(id1.getHostName()))
.thenReturn(mockedInetAddress);

// If persisted ip address is different from resolved ip address,
// DatanodeDetails should return the persisted ip address.
// Upon validation of the ip address, DatanodeDetails should return the resolved ip address.
when(mockedInetAddress.getHostAddress())
.thenReturn("127.0.0.1");
assertWriteReadWithChangedIpAddress(tempDir, id1);

when(mockedInetAddress.getHostAddress())
.thenReturn(id1.getIpAddress());

id1.setPort(DatanodeDetails.newStandalonePort(1));
assertWriteRead(tempDir, id1);

// Add certificate serial id.
id1.setCertSerialId("" + RandomUtils.nextLong());
assertWriteRead(tempDir, id1);

// Read should return an empty value if file doesn't exist
File nonExistFile = new File(tempDir, "non_exist.id");
assertThrows(IOException.class,
() -> ContainerUtils.readDatanodeDetailsFrom(nonExistFile));

// Read should fail if the file is malformed
File malformedFile = new File(tempDir, "malformed.id");
createMalformedIDFile(malformedFile);
assertThrows(IOException.class,
() -> ContainerUtils.readDatanodeDetailsFrom(malformedFile));

// Test upgrade scenario - protobuf file instead of yaml
File protoFile = new File(tempDir, "valid-proto.id");
try (OutputStream out = Files.newOutputStream(protoFile.toPath())) {
HddsProtos.DatanodeDetailsProto proto = id1.getProtoBufMessage();
proto.writeTo(out);
}
assertDetailsEquals(id1, ContainerUtils.readDatanodeDetailsFrom(protoFile));

id1.setInitialVersion(1);
assertWriteRead(tempDir, id1);
}
assertDetailsEquals(id1, ContainerUtils.readDatanodeDetailsFrom(protoFile));

id1.setInitialVersion(1);
assertWriteRead(tempDir, id1);
}

private void assertWriteRead(@TempDir File tempDir,
Expand All @@ -133,6 +154,17 @@ private void assertWriteRead(@TempDir File tempDir,
assertEquals(details.getCurrentVersion(), read.getCurrentVersion());
}

private void assertWriteReadWithChangedIpAddress(@TempDir File tempDir,
DatanodeDetails details) throws IOException {
// Write a single ID to the file and read it out
File file = new File(tempDir, "valid-values.id");
ContainerUtils.writeDatanodeDetailsTo(details, file, conf);
DatanodeDetails read = ContainerUtils.readDatanodeDetailsFrom(file);
assertEquals(details.getIpAddress(), read.getIpAddress());
read.validateDatanodeIpAddress();
assertEquals("127.0.0.1", read.getIpAddress());
}

private void createMalformedIDFile(File malformedFile)
throws IOException {
DatanodeDetails id = randomDatanodeDetails();
Expand All @@ -149,5 +181,6 @@ private static void assertDetailsEquals(DatanodeDetails expected,
assertEquals(expected.getCertSerialId(), actual.getCertSerialId());
assertEquals(expected.getProtoBufMessage(), actual.getProtoBufMessage());
assertEquals(expected.getInitialVersion(), actual.getInitialVersion());
assertEquals(expected.getIpAddress(), actual.getIpAddress());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ x-om:
services:
dn1:
<<: *datanode
hostname: dn1
networks:
net:
ipv4_address: 10.9.0.11
Expand All @@ -57,6 +58,7 @@ services:
- ../..:${OZONE_DIR}
dn2:
<<: *datanode
hostname: dn2
networks:
net:
ipv4_address: 10.9.0.12
Expand All @@ -65,6 +67,7 @@ services:
- ../..:${OZONE_DIR}
dn3:
<<: *datanode
hostname: dn3
networks:
net:
ipv4_address: 10.9.0.13
Expand Down