[#879][#925] Docker network Pool overlaps with other one on this address space #1211
Conversation
| @@ -140,6 +181,62 @@ private static Network createDockerNetwork() { | |||
| .build()); | |||
| } | |||
|
|
|||
| public static boolean ipRangesOverlap(String cidr1, String cidr2) throws Exception { | |||
| long[] net1 = cidrToRange(cidr1); | |||
There was a problem hiding this comment.
What the meaning of cidr?
There was a problem hiding this comment.
There was a problem hiding this comment.
OK, can you include the link in the Java doc, others may also be confused by it.
There was a problem hiding this comment.
I added the comment about CIDR in the codes.
Another, This is only Integration Test, I think not need write in the Java doc.
There was a problem hiding this comment.
I added the comment about CIDR in the codes.
That's exactly what I mean here.
...n-test/src/test/java/com/datastrato/gravitino/integration/test/container/ContainerSuite.java
Outdated
Show resolved
Hide resolved
...n-test/src/test/java/com/datastrato/gravitino/integration/test/container/ContainerSuite.java
Show resolved
Hide resolved
|
Do we need backport this pr to branch 0.3? |
xunliu
added
need backport
Yes, I think need. |
|
I'm OK with this PR except comment here https://github.com/datastrato/gravitino/pull/1211/files#r1432213859 |
|
@jerryshao Please help me review this PR, Thanks. |
| @@ -189,7 +189,7 @@ public SELF withEnvVars(Map<String, String> envVars) { | |||
| } | |||
|
|
|||
| public SELF withNetwork(Network network) { | |||
| this.network = Optional.of(network); | |||
| this.network = network != null ? Optional.of(network) : Optional.empty(); | |||
There was a problem hiding this comment.
Please change to Optional.ofNullable().
| @@ -38,7 +46,10 @@ private ContainerSuite() { | |||
| Info info = dockerClient.infoCmd().exec(); | |||
| LOG.info("Docker info: {}", info); | |||
|
|
|||
| network = createDockerNetwork(); | |||
| if (System.getenv("NEED_CREATE_DOCKER_NETWORK") != null | |||
| && Objects.equals(System.getenv("NEED_CREATE_DOCKER_NETWORK"), "true")) { | |||
There was a problem hiding this comment.
I would suggest to use string ignore case compare.
| if (System.getenv("NEED_CREATE_DOCKER_NETWORK") != null | ||
| && Objects.equals(System.getenv("NEED_CREATE_DOCKER_NETWORK"), "true")) { | ||
| String needCreateDockerNetwork = System.getenv("NEED_CREATE_DOCKER_NETWORK"); | ||
| if (needCreateDockerNetwork != null && needCreateDockerNetwork.equalsIgnoreCase("true")) { |
There was a problem hiding this comment.
Use "true".equals(xxx) instead and we can remove needCreateDockerNetwork != null
There was a problem hiding this comment.
I think it's ok for me. We can improve it later on. @xunliu do you want to fix it?
…ess space (#1211) ### What changes were proposed in this pull request? 1. Added Docker network overlap detection 2. Reduced IP allocation ranges for lower conflict probability, Modified to `10.20.30.0/28` only allocate IP ranger `10.20.30.1` ~ `10.20.30.14` 3. Docker network only created on MacOS and default Docker server environment ### Why are the changes needed? Fix: #879, #925 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? CI
What changes were proposed in this pull request?
10.20.30.0/28only allocate IP ranger10.20.30.1~10.20.30.14Why are the changes needed?
Fix: #879, #925
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
CI