Re: [PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
surendralilhore commented on PR #23768: URL: https://github.com/apache/flink/pull/23768#issuecomment-1826843398 There is one more Jira to fix this issue in better way : [FLINK-33548](https://issues.apache.org/jira/browse/FLINK-33548) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
surendralilhore closed pull request #23768: [FLINK-33609] Take into account the resource limit specified in the pod template. URL: https://github.com/apache/flink/pull/23768 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
surendralilhore commented on PR #23768: URL: https://github.com/apache/flink/pull/23768#issuecomment-1824329775 Thanks @gyfora for review. I will send the discussion mail in community. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
gyfora commented on PR #23768: URL: https://github.com/apache/flink/pull/23768#issuecomment-1823921369 Since this will considerably change the resource handling I think we need to have at least a quick discussion on this in the dev list. Furthermore the logic should be consistent for the resource request / limit, not only apply to one of them. Please start a discussion on the dev list. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
gyfora commented on code in PR #23768:
URL: https://github.com/apache/flink/pull/23768#discussion_r1402993805
##
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesUtils.java:
##
@@ -367,10 +367,24 @@ public static ResourceRequirements
getResourceRequirements(
Map externalResources,
Map externalResourceConfigKeys) {
final Quantity cpuQuantity = new Quantity(String.valueOf(cpu));
-final Quantity cpuLimitQuantity = new Quantity(String.valueOf(cpu *
cpuLimitFactor));
+final Quantity cpuLimitQuantity =
+new Quantity(
+String.valueOf(
+getLimit(
+cpu,
+cpuLimitFactor,
+resourceRequirements,
+Constants.RESOURCE_NAME_CPU)));
final Quantity memQuantity = new Quantity(mem +
Constants.RESOURCE_UNIT_MB);
Review Comment:
It seems to be very inconsistent/wrong to take the value for limit from the
podTemplate and not for the quantity.
I think we should make a decision and do it either for both or neither.
##
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesUtils.java:
##
@@ -399,6 +413,40 @@ public static ResourceRequirements getResourceRequirements(
return resourceRequirementsBuilder.build();
}
+/**
+ * Calculates the final limit value for a resource based on the new value,
limit factor, and
+ * existing resource requirements.
+ *
+ * @param newValue The new value for the resource.
+ * @param limitFactor The limit factor for the resource.
+ * @param resourceRequirements Existing resource requirements.
+ * @param resourceName The name of the resource.
+ * @return The final limit value for the resource.
+ */
+private static double getLimit(
+double newValue,
+double limitFactor,
+ResourceRequirements resourceRequirements,
+String resourceName) {
+Map limits = resourceRequirements.getLimits();
+double limit = newValue * limitFactor;
+if (limits != null) {
+Quantity quantity = limits.get(resourceName);
+if (quantity != null) {
+try {
+return Math.max(Double.parseDouble(quantity.getAmount()),
limit);
Review Comment:
We should always log on info level that we are using the limit from the
podTemplate instead of the config to avoid confusion
##
flink-kubernetes/src/test/java/org/apache/flink/kubernetes/utils/KubernetesUtilsTest.java:
##
@@ -143,6 +146,21 @@ void testLoadPodFromTemplateAndCheckInitContainer() {
.isEqualTo(KubernetesPodTemplateTestUtils.createInitContainer());
}
+@Test
+void testPodTemplateResourceLimitUtilisation() {
+final FlinkPod flinkPod =
+KubernetesUtils.loadPodFromTemplateFile(
+flinkKubeClient,
+
KubernetesPodTemplateTestUtils.getPodTemplateFileWithResourceLimit(),
+
KubernetesPodTemplateTestUtils.TESTING_MAIN_CONTAINER_NAME);
Review Comment:
We need to have tests for the different scenarios (covered in the logic):
- limits specified but no memory in them
- incorrect format in memory limit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
surendralilhore commented on PR #23768: URL: https://github.com/apache/flink/pull/23768#issuecomment-1821341423 @gyfora Please can you help me to review. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
flinkbot commented on PR #23768: URL: https://github.com/apache/flink/pull/23768#issuecomment-1821007113 ## CI report: * bbf51c6e8a359305f3e0d67cf0ece9bc7240070f UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-33609] Take into account the resource limit specified in the pod template. [flink]
surendralilhore opened a new pull request, #23768: URL: https://github.com/apache/flink/pull/23768 Flink is currently not considering the pod template resource limits and is only utilizing the limit obtained from the configured or default limit factor. Flink should consider both the value obtained from the limit factor and the pod template resource limits. It should take the maximum value of the pod template resource limits and the value obtained from the limit factor calculation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
