-
Notifications
You must be signed in to change notification settings - Fork 358
CDAP-21264 : Respect custom CPU and memory multipliers for Task Workers #16196
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -490,6 +490,14 @@ | |||||||||||||||||
| */ | ||||||||||||||||||
| public static final String PROGRAM_SUBMISSION_MASTER_ENV_ENABLED = "program.submission.master.environment.enabled"; | ||||||||||||||||||
| } | ||||||||||||||||||
| /** | ||||||||||||||||||
| * Kubernetes constants. | ||||||||||||||||||
| * These same as defined in KubeTwillPreparer | ||||||||||||||||||
| */ | ||||||||||||||||||
|
Comment on lines
+493
to
+496
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the grammatical typo in the Javadoc comment and update it to reflect that these are the centralized constants used by
Suggested change
|
||||||||||||||||||
| public static final class Kube { | ||||||||||||||||||
|
Check warning on line 497 in cdap-common/src/main/java/io/cdap/cdap/common/conf/Constants.java
|
||||||||||||||||||
| public static final String CPU_MULTIPLIER = "master.environment.k8s.container.cpu.multiplier"; | ||||||||||||||||||
| public static final String MEMORY_MULTIPLIER = "master.environment.k8s.container.memory.multiplier"; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Task worker. | ||||||||||||||||||
|
|
@@ -610,6 +618,8 @@ | |||||||||||||||||
| public static final String CONTAINER_MEMORY_MB = "artifact.localizer.container.memory.mb"; | ||||||||||||||||||
| public static final String CONTAINER_CORES = "artifact.localizer.container.num.cores"; | ||||||||||||||||||
| public static final String CONTAINER_JVM_OPTS = "artifact.localizer.container.jvm.opts"; | ||||||||||||||||||
| public static final String CONTAINER_CPU_MULTIPLIER = "artifact.localizer.container.cpu.multiplier"; | ||||||||||||||||||
| public static final String CONTAINER_MEMORY_MULTIPLIER = "artifact.localizer.container.memory.multiplier"; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Artifact localizer http handler configuration. | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -215,6 +215,8 @@ | |||||||||||||||||||||||||
| private StringBuilder globalJvmOptions; | ||||||||||||||||||||||||||
| private final V1EmptyDirVolumeSource workDirVolumeSource; | ||||||||||||||||||||||||||
| private boolean shouldLocalizeConfigurationAsConfigmap; | ||||||||||||||||||||||||||
| private String systemCpuMultiplier; | ||||||||||||||||||||||||||
| private String systemMemoryMultiplier; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| KubeTwillPreparer(MasterEnvironmentContext masterEnvContext, ApiClient apiClient, | ||||||||||||||||||||||||||
| String kubeNamespace, | ||||||||||||||||||||||||||
|
|
@@ -436,6 +438,12 @@ | |||||||||||||||||||||||||
| if (config.containsKey(MasterOptionConstants.RUNTIME_NAMESPACE)) { | ||||||||||||||||||||||||||
| cdapRuntimeNamespace = config.get(MasterOptionConstants.RUNTIME_NAMESPACE); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| if (config.containsKey(CPU_MULTIPLIER)) { | ||||||||||||||||||||||||||
| systemCpuMultiplier = config.get(CPU_MULTIPLIER); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| if (config.containsKey(MEMORY_MULTIPLIER)) { | ||||||||||||||||||||||||||
| systemMemoryMultiplier = config.get(MEMORY_MULTIPLIER); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+441
to
+446
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using the locally defined
Suggested change
|
||||||||||||||||||||||||||
| for (String runnableName : runnables) { | ||||||||||||||||||||||||||
| withEnv(runnableName, config); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
@@ -640,7 +648,7 @@ | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
|
Check warning on line 651 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| public TwillController start(long timeout, TimeUnit timeoutUnit) { | ||||||||||||||||||||||||||
| validateSpecification(); | ||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||
|
|
@@ -1006,10 +1014,10 @@ | |||||||||||||||||||||||||
| if (memory == null) { | ||||||||||||||||||||||||||
| throw new IllegalArgumentException("No memory settings in the given resource requirements"); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| int memoryMB = (int) (memory.getNumber().longValue() >> 20); | ||||||||||||||||||||||||||
|
Check warning on line 1017 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Map<String, String> cConf = masterEnvContext.getConfigurations(); | ||||||||||||||||||||||||||
| int reservedMemoryMB = Integer.parseInt(cConf.get(Configs.Keys.JAVA_RESERVED_MEMORY_MB)); | ||||||||||||||||||||||||||
|
Check warning on line 1020 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| double minHeapRatio = Double.parseDouble(cConf.get(Configs.Keys.HEAP_RESERVED_MIN_RATIO)); | ||||||||||||||||||||||||||
| return org.apache.twill.internal.utils.Resources.computeMaxHeapSize(memoryMB, reservedMemoryMB, | ||||||||||||||||||||||||||
| minHeapRatio); | ||||||||||||||||||||||||||
|
|
@@ -1201,7 +1209,7 @@ | |||||||||||||||||||||||||
| RuntimeSpecification mainRuntimeSpec = getMainRuntimeSpecification(runtimeSpecs); | ||||||||||||||||||||||||||
| String runnableName = mainRuntimeSpec.getName(); | ||||||||||||||||||||||||||
| final V1ResourceRequirements initContainerResourceRequirements = | ||||||||||||||||||||||||||
| createResourceRequirements(mainRuntimeSpec.getResourceSpecification()); | ||||||||||||||||||||||||||
| createResourceRequirements(mainRuntimeSpec.getName(), mainRuntimeSpec.getResourceSpecification()); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Setup the container environment. Inherit everything from the current pod except workload identity env vars. | ||||||||||||||||||||||||||
| Map<String, String> initContainerEnvirons = podInfo.getContainerEnvironments().stream() | ||||||||||||||||||||||||||
|
|
@@ -1330,13 +1338,13 @@ | |||||||||||||||||||||||||
| containers.add(createContainer(mainRuntimeSpec.getName(), podInfo.getContainerImage(), | ||||||||||||||||||||||||||
| podInfo.getImagePullPolicy(), | ||||||||||||||||||||||||||
| workDir, | ||||||||||||||||||||||||||
| createResourceRequirements(mainRuntimeSpec.getResourceSpecification()), | ||||||||||||||||||||||||||
| createResourceRequirements(mainRuntimeSpec.getName(), mainRuntimeSpec.getResourceSpecification()), | ||||||||||||||||||||||||||
| mounts, environs, KubeTwillLauncher.class, | ||||||||||||||||||||||||||
| Stream.concat(Stream.of(mainRuntimeSpec.getName()), args.stream()) | ||||||||||||||||||||||||||
| .toArray(String[]::new))); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| for (String name : this.dependentRunnableNames) { | ||||||||||||||||||||||||||
| RuntimeSpecification spec = runtimeSpecs.get(name); | ||||||||||||||||||||||||||
|
Check warning on line 1347 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| // Add all environments for the runnable | ||||||||||||||||||||||||||
| environs.putAll(environments.get(name)); | ||||||||||||||||||||||||||
| // Add JVM options to environment. | ||||||||||||||||||||||||||
|
|
@@ -1356,7 +1364,7 @@ | |||||||||||||||||||||||||
| mounts = addSecreteVolMountIfNeeded(spec, volumeMounts); | ||||||||||||||||||||||||||
| containers.add( | ||||||||||||||||||||||||||
| createContainer(name, podInfo.getContainerImage(), podInfo.getImagePullPolicy(), workDir, | ||||||||||||||||||||||||||
| createResourceRequirements(spec.getResourceSpecification()), | ||||||||||||||||||||||||||
| createResourceRequirements(name, spec.getResourceSpecification()), | ||||||||||||||||||||||||||
| mounts, envs, KubeTwillLauncher.class, | ||||||||||||||||||||||||||
| Stream.concat(Stream.of(name), args.stream()).toArray(String[]::new))); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
@@ -1391,7 +1399,7 @@ | |||||||||||||||||||||||||
| // Set the process memory is through the JAVA_HEAPMAX variable. | ||||||||||||||||||||||||||
| environs.put("JAVA_HEAPMAX", | ||||||||||||||||||||||||||
| String.format("-Xmx%dm", computeMaxHeapSize(resourceRequirements))); | ||||||||||||||||||||||||||
| List<V1EnvVar> containerEnvironments = environs.entrySet().stream() | ||||||||||||||||||||||||||
|
Check warning on line 1402 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| .map(e -> new V1EnvVar().name(e.getKey()).value(e.getValue())) | ||||||||||||||||||||||||||
| .collect(Collectors.toList()); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -1416,7 +1424,7 @@ | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (containerProbes.containsKey(name)){ | ||||||||||||||||||||||||||
|
Check warning on line 1427 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| containerProbes.get(name).forEach((probeName, probeObj) -> { | ||||||||||||||||||||||||||
| switch (probeName) { | ||||||||||||||||||||||||||
| case LIVENESS: | ||||||||||||||||||||||||||
|
|
@@ -1452,11 +1460,23 @@ | |||||||||||||||||||||||||
| * the namespace has a resource quota, the objects must also specify resource limits. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| @VisibleForTesting | ||||||||||||||||||||||||||
| V1ResourceRequirements createResourceRequirements(ResourceSpecification resourceSpec) { | ||||||||||||||||||||||||||
| V1ResourceRequirements createResourceRequirements(String runnableName, ResourceSpecification resourceSpec) { | ||||||||||||||||||||||||||
| Map<String, String> cConf = masterEnvContext.getConfigurations(); | ||||||||||||||||||||||||||
| float cpuMultiplier = Float.parseFloat(cConf.getOrDefault(CPU_MULTIPLIER, DEFAULT_MULTIPLIER)); | ||||||||||||||||||||||||||
| float memoryMultiplier = Float.parseFloat( | ||||||||||||||||||||||||||
| cConf.getOrDefault(MEMORY_MULTIPLIER, DEFAULT_MULTIPLIER)); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| String runnableCpu = null; | ||||||||||||||||||||||||||
| String runnableMem = null; | ||||||||||||||||||||||||||
| if (runnableName != null && runnableConfigs.containsKey(runnableName)) { | ||||||||||||||||||||||||||
| Map<String, String> configMap = runnableConfigs.get(runnableName); | ||||||||||||||||||||||||||
| runnableCpu = configMap.get(CPU_MULTIPLIER); | ||||||||||||||||||||||||||
| runnableMem = configMap.get(MEMORY_MULTIPLIER); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| float cpuMultiplier = Float.parseFloat(runnableCpu != null ? runnableCpu : | ||||||||||||||||||||||||||
| (systemCpuMultiplier != null ? systemCpuMultiplier : | ||||||||||||||||||||||||||
| cConf.getOrDefault(CPU_MULTIPLIER, DEFAULT_MULTIPLIER))); | ||||||||||||||||||||||||||
|
Check warning on line 1476 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| float memoryMultiplier = Float.parseFloat(runnableMem != null ? runnableMem : | ||||||||||||||||||||||||||
| (systemMemoryMultiplier != null ? systemMemoryMultiplier : | ||||||||||||||||||||||||||
| cConf.getOrDefault(MEMORY_MULTIPLIER, DEFAULT_MULTIPLIER))); | ||||||||||||||||||||||||||
|
Check warning on line 1479 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| V1ResourceRequirementsBuilder requirementsBuilder = new V1ResourceRequirementsBuilder(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -1626,9 +1646,9 @@ | |||||||||||||||||||||||||
| * Get {@link Map} of properties prefixed with the string provided as an input. | ||||||||||||||||||||||||||
| * Property names in the mapping are trimmed to remove the prefix. | ||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||
| * @param originalMap | ||||||||||||||||||||||||||
|
Check warning on line 1649 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| * @param prefix | ||||||||||||||||||||||||||
|
Check warning on line 1650 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| * @return | ||||||||||||||||||||||||||
|
Check warning on line 1651 in cdap-kubernetes/src/main/java/io/cdap/cdap/k8s/runtime/KubeTwillPreparer.java
|
||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| public static Map<String, String> filterAndRemoveKeyPrefix(Map<String, String> originalMap, | ||||||||||||||||||||||||||
| String prefix) { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
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.
To avoid redundant lookups in
cConf(which can be expensive as it resolves configuration variables), store the retrieved multiplier values in local variables instead of callingcConf.get()multiple times for the same key.