airbyte-platform icon indicating copy to clipboard operation
airbyte-platform copied to clipboard

Use merge policy instead of fallbacks for KubeResourceConfig

Open azorej opened this issue 1 year ago • 3 comments

What

Fix for https://github.com/airbytehq/airbyte/issues/33068

But the problem is more complex. Logic in io.airbyte.commons.workers.config.WorkerConfigsProvider is opaque. The main strategy of resolving KubeResourceConfig is 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.yml without 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_LABELS in 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 null value for a field in a KubeResourceConfig using environment variables.

How

I suggest to introduce two breaking changes:

  1. Use a merge strategy to resolve KubeResourceConfig. It's more intuitive and powerful.
  2. Use the literal <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 in KubeResourceConfig for 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

  1. airbyte-commons-with-dependencies/src/test/resources/application-config-test.yaml
  2. airbyte-commons-with-dependencies/src/main/java/io/airbyte/commons/workers/config/WorkerConfigsProvider.java
  3. airbyte-commons-with-dependencies/src/main/java/io/airbyte/commons/workers/config/KubeResourceConfig.java
  4. all other files

Can 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.

  • [x] YES 💚
  • [ ] NO ❌

🚨 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.

azorej avatar Mar 17 '24 01:03 azorej

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 17 '24 01:03 CLAassistant

Can ALL the resource settings be exposed please?

mhemken-vts avatar May 07 '24 19:05 mhemken-vts