Use merge policy instead of fallbacks for KubeResourceConfig #309
Conversation
This file contains 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
|
|
|
azorej
commented
Mar 18, 2024
| @@ -38,6 +39,7 @@ airbyte: | |||
| cpu-request: 42 | |||
| micronauttest-source: | |||
| cpu-limit: 5 | |||
| cpu-request: <EMPTY> # Disable inheritance, force empty value | |||
There was a problem hiding this comment.
Be careful: that's a breaking change
| @@ -104,6 +104,7 @@ airbyte: | |||
| kube-job-configs: | |||
| default: | |||
| annotations: ${JOB_KUBE_ANNOTATIONS:} | |||
| labels: ${JOB_KUBE_LABELS:} | |||
Comment on lines
+10
to
+11
| cpu-request: default cpu request | ||
| memory-request: default memory request |
There was a problem hiding this comment.
Previous test cases weren't consistent
| check: | ||
| annotations: ${CHECK_JOB_KUBE_ANNOTATIONS:check annotations} | ||
| annotations: ${CHECK_JOB_KUBE_ANNOTATIONS:check_annotation_key=check_annotation_value} |
There was a problem hiding this comment.
We didn't check annotation overrides earlier
| assertNull(specKubeResourceConfig.getCpuLimit()); | ||
| assertNull(specKubeResourceConfig.getCpuRequest()); | ||
| assertEquals("", specKubeResourceConfig.getCpuLimit()); | ||
| assertEquals("", specKubeResourceConfig.getCpuRequest()); |
There was a problem hiding this comment.
We consider nulls as empty strings for KubeResourceConfig now
Comment on lines
-372
to
378
| private ResourceRequirements getResourceRequirementsFrom(final KubeResourceConfig kubeResourceConfig, final KubeResourceConfig defaultConfig) { | ||
| private ResourceRequirements getResourceRequirementsFrom(final KubeResourceConfig kubeResourceConfig) { | ||
| return new ResourceRequirements() | ||
| .withCpuLimit(useDefaultIfEmpty(kubeResourceConfig.getCpuLimit(), defaultConfig.getCpuLimit())) | ||
| .withCpuRequest(useDefaultIfEmpty(kubeResourceConfig.getCpuRequest(), defaultConfig.getCpuRequest())) | ||
| .withMemoryLimit(useDefaultIfEmpty(kubeResourceConfig.getMemoryLimit(), defaultConfig.getMemoryLimit())) | ||
| .withMemoryRequest(useDefaultIfEmpty(kubeResourceConfig.getMemoryRequest(), defaultConfig.getMemoryRequest())); | ||
| } | ||
|
|
||
| private static String useDefaultIfEmpty(final String value, final String defaultValue) { | ||
| return (value == null || value.isBlank()) ? defaultValue : value; | ||
| .withCpuLimit(kubeResourceConfig.getCpuLimit()) | ||
| .withCpuRequest(kubeResourceConfig.getCpuRequest()) | ||
| .withMemoryLimit(kubeResourceConfig.getMemoryLimit()) | ||
| .withMemoryRequest(kubeResourceConfig.getMemoryRequest()); | ||
| } |
There was a problem hiding this comment.
Remove custom fallbacks since we don't need them anymore
Comment on lines
-234
to
-242
| // if annotations are not defined for this specific resource, then fallback to the default | ||
| // resource's annotations | ||
| final Map<String, String> annotations; | ||
| if (Strings.isNullOrEmpty(kubeResourceConfig.getAnnotations())) { | ||
| annotations = splitKVPairsFromEnvString(workerConfigsDefaults.defaultKubeResourceConfig.getAnnotations()); | ||
| } else { | ||
| annotations = splitKVPairsFromEnvString(kubeResourceConfig.getAnnotations()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove custom fallbacks since we don't need them anymore
azorej
force-pushed
the
feature/ISSUE-33068_worker_config_default_fallbacks
branch
from
b092c44
to
147f247
Compare
|
Can ALL the resource settings be exposed please? |
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.
What
Fix for airbytehq/airbyte#33068
But the problem is more complex.
Logic in
io.airbyte.commons.workers.config.WorkerConfigsProvideris opaque.The main strategy of resolving
KubeResourceConfigis to fallback to the default for the whole config: if we cannot find the exact key, we will choose the closest default section.However, there are some exceptions: annotations and
ResourceRequirements(cpu/memory limits/requests). If any of these are empty, we will attempt to update them using the default config.In addition, the current fallback logic does not work well for Helm OSS installations.
We parameterize config values via environment variables.
And there is no way to remove a section from the
application.ymlwithout using a custom image.So one must setup all variables
JOB_KUBE_LABELS,CHECK_JOB_KUBE_LABELS,DISCOVER_JOB_KUBE_LABELS,REPLICATION_JOB_KUBE_LABELS,SPEC_JOB_KUBE_LABELSin case they want to use common k8s labels (for example,azure.workload.identity/use).The last issue I have is that I am not sure how to set a
nullvalue for a field in aKubeResourceConfigusing environment variables.How
I suggest to introduce two breaking changes:
KubeResourceConfig. It's more intuitive and powerful.<EMPTY>to differentiate between empty and null fields.The merge strategy will use the same order as previously: variant, type, subtype.
But it will try to merge configs based on
COALESCE(value, default_value)for every field inKubeResourceConfigfor every key part.With this we don't need custom logic for annotations and
ResourceRequirements.<EMPTY>literal is needed to override defaults with empty values.Recommended reading order
airbyte-commons-with-dependencies/src/test/resources/application-config-test.yamlairbyte-commons-with-dependencies/src/main/java/io/airbyte/commons/workers/config/WorkerConfigsProvider.javaairbyte-commons-with-dependencies/src/main/java/io/airbyte/commons/workers/config/KubeResourceConfig.javaCan this PR be safely reverted / rolled back?
If you know that your PR is backwards-compatible and can be simply reverted or rolled back, check the YES box.
Otherwise if your PR has a breaking change, like a database migration for example, check the NO box.
If unsure, leave it blank.
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.