Conversation
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved moderate test reliability issue and additional test coverage gaps remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Makes the HDFS NFS Gateway bind address configurable via nfs.server.bind.host, preserving 0.0.0.0 as the default.
Changes:
- Adds the bind-host configuration key and default.
- Propagates the configured host through NFS and mountd RPC servers.
- Adds configuration and binding tests.
File summaries
| File | Description |
|---|---|
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml |
Defines the default bind-host configuration. |
hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestNfsBindConfiguration.java |
Tests bind-host configuration. |
hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java |
Passes the configured host to NFS RPC. |
hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java |
Passes the configured host to mountd RPC. |
hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/conf/NfsConfigKeys.java |
Defines bind-host constants. |
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/nfs/nfs3/Nfs3Base.java |
Binds the NFS TCP server to the configured host. |
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/mount/MountdBase.java |
Binds mountd TCP and UDP servers to the configured host. |
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/oncrpc/TestSimpleServerBind.java |
Tests server binding behavior. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/oncrpc/SimpleUdpServer.java |
Supports configurable UDP binding. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/oncrpc/SimpleTcpServer.java |
Supports configurable TCP binding. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java |
Stores and exposes the RPC bind host. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The review identified one moderate test-reliability issue and three nit-level issues to fix before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/oncrpc/TestSimpleServerBind.java:64
- The body is not indented, which violates the repository's configured
Indentationcheck and leaves this new test inconsistent with the surrounding Java code. Indent the return statement.
return 0;
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/oncrpc/TestSimpleServerBind.java:65
- This helper is named
randomPort, but it always returns 0, which asks the OS for an ephemeral port rather than selecting a random port. Rename it (and its call sites) to make the test's collision-avoidance intent clear.
private static int randomPort() {
return 0;
}
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate compatibility and restart-state issues remain.
Review details
Suppressed comments (6)
Previously missed (4) — in code that hasn't changed since the last review.
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/mount/MountdBase.java:104
registeredis sticky across restarts: afterstart(true); stop();, a laterstart(false); stop()still attempts to unregister both ports even though the second run was not registered. Make this reflect the current invocation (for example, assignregistered = register) or clear it during cleanup.
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/nfs/nfs3/Nfs3Base.java:56registeredis sticky across restarts: afterstart(true); stop();, a laterstart(false); stop()still enters the unregister branch for a port that was never registered in the second run. Make this reflect the current invocation (for example, assignregistered = register) or clear it during cleanup.
hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java:92- The added tests cover configuration lookup and the generic base-class forwarding separately, but none constructs
RpcProgramMountdfrom anNfsConfigurationcontaining a non-default bind host. A regression in this new key-to-constructor wiring could therefore pass all of the added tests. Add a corresponding mountd integration assertion for the configured bind host or bound socket address.
hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java:182 - The added tests cover configuration lookup and the generic base-class forwarding separately, but none constructs
RpcProgramNfs3from anNfsConfigurationcontaining a non-default bind host. A regression in this new key-to-constructor wiring (for example, using the wrong key or default here) would therefore pass all of the added tests. Add an integration assertion that sets the key before constructing NFS and verifiesgetBindHost()or the bound socket address.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/oncrpc/SimpleTcpServer.java:61
- Before this compatibility constructor,
run()bound withnew InetSocketAddress(port), which uses the JVM's any-local address. Passing the hard-coded0.0.0.0changes that to an IPv4-only wildcard when IPv6 is enabled, so the no-configuration path can stop accepting IPv6 connections despite the stated no-functionality-change goal. Keep the legacy constructor on the original any-local path and use the explicit host only for the new overload.
this(port, "0.0.0.0", program, workercount);
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/oncrpc/SimpleUdpServer.java:56
- This compatibility constructor has the same IPv4-only default regression as the TCP server: the old
new InetSocketAddress(port)could use the JVM's any-local address, while this path now forces0.0.0.0. On a JVM configured for IPv6, the default UDP service may no longer accept IPv6 clients. Preserve the old any-local behavior for the legacy constructor and reserve the explicit host for the new overload.
this(port, "0.0.0.0", program, workerCount);
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the default address-family behavior and ensure NFS/mountd test services are stopped reliably.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/conf/NfsConfigKeys.java:31
- The old NFS path passed a null bind host to the legacy server constructors, which used
new InetSocketAddress(port)and preserved the JVM's wildcard address family. Making the default literal0.0.0.0forces the NFS and mountd sockets to IPv4; on IPv6-only or IPv6-preferred hosts this can fail to bind or lose IPv6 reachability, contrary to the stated default/no-functionality-change contract. Preserve the legacy wildcard for the default while still honoring an explicitly configured host.
public static final String DFS_NFS_SERVER_BIND_HOST_DEFAULT = "0.0.0.0";
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Ensure Nfs3 service starts and stops correctly in tests. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate lifecycle, test-isolation, and binding-default issues remain.
Review details
Suppressed comments (7)
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/mount/MountdBase.java:129
- With
register=false, this guard leavesudpBoundPortstale after the UDP server is shut down. If the sameMountdBaseis restarted and startup fails before assigning a new port,stop()can later unregister the old port; make clearing the bound-port field unconditional while keeping the unregister call conditional.
if (registered && udpBoundPort > 0) {
rpcProgram.unregister(PortmapMapping.TRANSPORT_UDP, udpBoundPort);
udpBoundPort = 0;
}
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/mount/MountdBase.java:133
- With
register=false, this guard leavestcpBoundPortstale after the TCP server is shut down. If the sameMountdBaseis restarted and startup fails before assigning a new port,stop()can later unregister the old port; make clearing the bound-port field unconditional while keeping the unregister call conditional.
if (registered && tcpBoundPort > 0) {
rpcProgram.unregister(PortmapMapping.TRANSPORT_TCP, tcpBoundPort);
tcpBoundPort = 0;
}
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/nfs/nfs3/Nfs3Base.java:94
- When
start(false)is used, this condition skips the body entirely, sonfsBoundPortis not cleared even though the TCP server is shut down below. A later restart that fails before assigning a new port can then makestop()unregister this stale port; keep the reset unconditional and guard only the unregister call withregistered.
if (registered && nfsBoundPort > 0) {
rpcProgram.unregister(PortmapMapping.TRANSPORT_TCP, nfsBoundPort);
nfsBoundPort = 0;
}
hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/TestMountd.java:110
- This test starts both the mountd and NFS Netty servers with
register=falsebut never stops them, so every successful or failed run leaves listening sockets and event-loop threads behind. Wrap the start/assertion block in atry/finallyand callnfs3.stop()as the preceding test does.
nfs3.startServiceInternal(false);
RpcProgramMountd mountd = (RpcProgramMountd) nfs3.getMountd().getRpcProgram();
assertEquals("127.0.0.1", mountd.getBindHost(),
"nfs.server.bind.host must be forwarded to RpcProgramMountd");
}
hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/TestMountd.java:80
- This NFS3 startup also launches
Nfs3HttpServer, but only the RPC ports are made ephemeral here; the HTTP server therefore uses its fixed default port 50079. Parallel NFS tests can collide on that port (andstartDaemonsonly logs the bind failure), so make the HTTP/HTTPS test addresses ephemeral as well.
config.set(NfsConfigKeys.DFS_NFS_SERVER_BIND_HOST_KEY, "127.0.0.1");
hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/TestMountd.java:101
- This second NFS3 startup also launches
Nfs3HttpServer, but only the RPC ports are made ephemeral here; the HTTP server therefore uses its fixed default port 50079. Parallel NFS tests can collide on that port (andstartDaemonsonly logs the bind failure), so make the HTTP/HTTPS test addresses ephemeral as well.
config.set(NfsConfigKeys.DFS_NFS_SERVER_BIND_HOST_KEY, "127.0.0.1");
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml:6780
- Using
0.0.0.0here changes the old wildcard behavior on JVMs that select an IPv6 wildcard: the previousnew InetSocketAddress(port)path could bind to::, while this configured path forces an IPv4 socket. That can make IPv6 clients unreachable by default, contrary to the stated no-functionality-change/default-preservation goal; preserve the old wildcard behavior when unset or explicitly document the IPv4-only change.
<value>0.0.0.0</value>
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
In some cases, one may want to bind the NFS server process to localhost or a single interface in a multiple interface host. At the moment the bind address is hard coded to 0.0.0.0. This change allows it to be configurable with a new config key, defaulting to the existing value.
nfs.server.bind.host
Aside from the configuration, there is not intended to be any functionality change.
https://issues.apache.org/jira/browse/HDFS-17981
How was this patch tested?
New unit tests and existing tests.