-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-24947] Support hostNetwork for native K8s integration on session mode #18119
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 7fcbf27 (Wed Dec 15 12:16:33 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
@flinkbot run azure |
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.
@spoon-lz Thanks for creating this PR. I think it could work in your internal use case. But we need to refine the PR in a good shape before merging. I left some comments and please have a look.
...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesClusterDescriptor.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/flink/kubernetes/entrypoint/KubernetesSessionClusterEntrypoint.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/flink/kubernetes/entrypoint/KubernetesSessionClusterEntrypoint.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/kubeclient/FlinkKubeClient.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/flink/kubernetes/kubeclient/factory/KubernetesTaskManagerFactory.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/Constants.java
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesUtils.java
Outdated
Show resolved
Hide resolved
...test/java/org/apache/flink/kubernetes/entrypoint/KubernetesSessionClusterEntrypointTest.java
Outdated
Show resolved
Hide resolved
...ernetes/src/test/java/org/apache/flink/kubernetes/kubeclient/Fabric8FlinkKubeClientTest.java
Outdated
Show resolved
Hide resolved
@wangyang0918 Thanks for helping review this PR, give me some time to modify them |
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.
@spoon-lz I appreciate that you updated this PR. It has looked really better to me.
I have left some comments about the implementations, especially about how to get the dynamic ports in KubernetesResourceManagerDriver
. Also the unit tests could be optimized.
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
...bernetes/src/main/java/org/apache/flink/kubernetes/entrypoint/KubernetesEntrypointUtils.java
Outdated
Show resolved
Hide resolved
...ernetes/src/test/java/org/apache/flink/kubernetes/kubeclient/Fabric8FlinkKubeClientTest.java
Outdated
Show resolved
Hide resolved
...-kubernetes/src/test/java/org/apache/flink/kubernetes/kubeclient/TestingFlinkKubeClient.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/runtime/entrypoint/component/DispatcherResourceManagerComponent.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/kubeclient/FlinkKubeClient.java
Outdated
Show resolved
Hide resolved
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.
@spoon-lz Thanks for updating this PR again. It looks almost good to me and I just left some minor comments. Once they are addressed, I will try to merge this PR. Please make sure that this PR is also verified in minikube or a real K8s cluster.
Moreover, could you please squash the commits and refine the commit message. Maybe they could looks like following.
- [hotfix][runtime] Extract a common method to parse rest port from web interface URL
- [FLINK-24947][k8s] Support host network for native K8s integration
...-kubernetes/src/main/java/org/apache/flink/kubernetes/kubeclient/Fabric8FlinkKubeClient.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/kubeclient/FlinkKubeClient.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesUtils.java
Outdated
Show resolved
Hide resolved
...-kubernetes/src/test/java/org/apache/flink/kubernetes/kubeclient/TestingFlinkKubeClient.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/runtime/entrypoint/component/DispatcherResourceManagerComponent.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/entrypoint/ClusterEntrypoint.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/rest/RestServerEndpoint.java
Outdated
Show resolved
Hide resolved
...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/flink/kubernetes/kubeclient/decorators/FlinkConfMountDecorator.java
Outdated
Show resolved
Hide resolved
@spoon-lz I have quickly verified this PR works well in a minikube. Great work. |
686b492
to
393b6c0
Compare
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/ResourceManagerUtils.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Show resolved
Hide resolved
...runtime/src/test/java/org/apache/flink/runtime/resourcemanager/ResourceManagerUtilsTest.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesClusterDescriptor.java
Outdated
Show resolved
Hide resolved
2361d49
to
99653eb
Compare
@wangyang0918 I think the code is sorted out already |
flink-runtime/src/main/java/org/apache/flink/runtime/util/ResourceManagerUtils.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/util/ResourceManagerUtilsTest.java
Outdated
Show resolved
Hide resolved
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.
Thanks for updating this PR. I will address my last minor comments when merging.
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/KubernetesResourceManagerDriver.java
Outdated
Show resolved
Hide resolved
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/kubeclient/FlinkKubeClient.java
Outdated
Show resolved
Hide resolved
99653eb
to
750404a
Compare
750404a
to
c15632c
Compare
cc @spoon-lz If you have no other comments, I will merge this PR once the azure pipeline gives the pass. |
@wangyang0918 Thanks for your help, I think it's ok now |
@spoon-lz I believe that you forget the changes in |
c15632c
to
6a4a6c0
Compare
Force pushed because:
|
6a4a6c0
to
237027a
Compare
@wangyang0918 Thanks for helping me modify the code |
Merging this PR. |
What is the purpose of the change
Provide hostnetwork network mode for jobs running in native kubernetes mode to obtain better performance in unmanaged clusters(for session mode)
Brief change log
When a job is running in session mode, you can start the hostnetwork mode by configuring "kubernetes.hostnetwork.enabled=true". After it is turned on, the pod of JobManager and TaskManager will use the host network instead of the third-party CNI plug-in.
Verifying this change
The session and job are started when "kubernetes.hostnetwork.enabled=true" is configured. When the task is running normally, the JobManager is killed. After the JobManager is automatically started, the TaskManager will be correctly connected to the JobManager and can be connected to the session through flink-client
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation