airbyte-workers: add support for kubernetes pod annotations #10753
Conversation
This reverts commit 2ac74f3.
This will be handled in a separate PR
github-actions
bot
added
area/documentation
| * @param input string | ||
| * @return map containing kv pairs | ||
| */ | ||
| public Optional<Map<String, String>> splitKVPairsFromEnvString(String input) { |
| 6. `JOB_KUBE_BUSYBOX_IMAGE` - Define the Job pod busybox image. | ||
| 7. `JOB_KUBE_CURL_IMAGE` - Define the Job pod curl image pull. | ||
| 8. `JOB_KUBE_NAMESPACE` - Define the Kubernetes namespace Job pods are created in. | ||
| 3. `JOB_KUBE_ANNOTATIONS` - Define one or more Job pod annotations. Each kv-pair is separated by a `,`. |
There was a problem hiding this comment.
Could you add an example of kv-pair? For me it's not clear the user should add key1=value1 instead of key1:value1.
| @@ -288,6 +288,26 @@ | |||
| */ | |||
| Optional<Map<String, String>> getDiscoverJobKubeNodeSelectors(); | |||
|
|
|||
| /** | |||
| * Define one or more Job pod annotations. Each kv-pair is separated by a `,`. | |||
There was a problem hiding this comment.
Can you include in the comment that this annotation is used as a fallback in case other annotations aren't set, as well as, used as the sync job pod annotation.
| 7. `JOB_KUBE_CURL_IMAGE` - Define the Job pod curl image pull. | ||
| 8. `JOB_KUBE_NAMESPACE` - Define the Kubernetes namespace Job pods are created in. | ||
| 2. `JOB_KUBE_NODE_SELECTORS` - Define one or more Job pod node selectors. Each k=v pair is separated by a `,`. For example: `key1=value1,key2=value2` | ||
| 3. `JOB_KUBE_ANNOTATIONS` - Define one or more Job pod annotations. Each k=v pair is separated by a `,`. For example: `key1=value1,key2=value2` |
There was a problem hiding this comment.
can we update the comment here to match my earlier comment on reflecting that this the default annotation fallback and the annotation configuration for sync jobs.
| 8. `JOB_KUBE_NAMESPACE` - Define the Kubernetes namespace Job pods are created in. | ||
| 2. `JOB_KUBE_NODE_SELECTORS` - Define one or more Job pod node selectors. Each k=v pair is separated by a `,`. For example: `key1=value1,key2=value2` | ||
| 3. `JOB_KUBE_ANNOTATIONS` - Define one or more Job pod annotations. Each k=v pair is separated by a `,`. For example: `key1=value1,key2=value2` | ||
| 4. `JOB_KUBE_MAIN_CONTAINER_IMAGE_PULL_POLICY` - Define the Job pod connector image pull policy. |
There was a problem hiding this comment.
can we also add the other job specific annotation env vars? it was an oversight on our part to leave out the job specific node selectors.
| @@ -483,6 +484,7 @@ public KubePodProcess(final boolean isOrchestrator, | |||
| .withNewMetadata() | |||
| .withName(podName) | |||
| .withLabels(labels) | |||
| .withAnnotations(annotations.orElse(null)) | |||
There was a problem hiding this comment.
what happens if this is an empty map or null? if the effect is the same, it'll be slightly cleaner to drop the optional and have splitKVPairsFromEnvString. Sorry, I know you are following an already existing pattern - however I think it complicates things. What do you think?
There was a problem hiding this comment.
Hi @davinchia, the optional has been added in this PR: #10200 , but I also don't really see its benefit.
splitKVPairsFromEnvString (previously getNodeSelectorsFromEnvString) is now used for both the annotations and the node selectors.
So, you would suggest that splitKVPairsFromEnvString returns a simple Map<String, String> instead of Optional<Map<String, String>> and adapt its usage for the annotations and node selectors, correct?
There was a problem hiding this comment.
That was actually discussed in this comment: #10200 (comment)
There was a problem hiding this comment.
yeap! returning a map would simplify the entire code execution chain.
|
@tbcdns thank you! Pretty close. I left some comments. Did you get a chance to do a sanity test on the node annotations? |
|
@davinchia , @marcosmarxm I fixed your comments, could you please review it? |
|
I ended up testing this locally and confirmed it works. I think there is a format issue. I don't have push to the fork so I will fix this after. |
What
This PR adds the possibility to define pod annotations to the pods created by the workers.
Pod annotations can be required in different situations, such as configuring which IP pool to use when using some network plugins.
The original PR was here: #9874 we decided to split it into 3 different PRs.
How
Pod node selectors are already available (using the
JOB_KUBE_NODE_SELECTORSenvironment variable), pod annotations can be configured in the same way, using theJOB_KUBE_ANNOTATIONSenvironment variable)🚨 User Impact 🚨
No user impact