More clean up. #3586

davinchia · 2021-05-25T13:24:04Z

What

The first 6 points of #3464.

Will remove the debugging statements before merging in.

How

Various clean up and fixes. All pretty simple/straightforward.

The only interesting thing about this PR is the kube pod shutdown. For whatever reason, the OkHttpPool isn't respecting the evictAll call and 1 idle thread remains. So instead of shutting down immediately, the worker pod shuts down after 5 mins when the idle thread id reaped. There isn't an easy way to modify the pool's idle reap configuration now. I do not think this issue is blocking since it's relatively benign, so I vote we create a ticket and come back to this once we do an e2e test.

See https://github.com/airbytehq/airbyte/pull/3586/files#diff-313805107b9eb92e35dfa2bb96aa906ccfe42bf27fe113f7098821303dacc476R93 for info.

Recommended reading order

KubePodProcess.java (meat of change is here)
KubePodProcessTest.java
the rest

…getting the right entrypoint.

davinchia · 2021-05-25T13:25:12Z

airbyte-workers/build.gradle

@@ -8,6 +8,15 @@ plugins {

 application {
    mainClass = 'io.airbyte.workers.process.KubeProcessBuilderFactoryPOC'
+    applicationDefaultJvmArgs = ['-Xmx500m',


For remote debugging. Will remove before merging into branch. Leaving this here in case someone wants to take a look at what I saw.

port forwarding the port to localhost 6000 and attaching visual vm to the 6000 will confirm okhttp is the thread leak.

davinchia · 2021-05-25T13:26:37Z

airbyte-workers/src/main/java/io/airbyte/workers/process/KubeProcessBuilderFactoryPOC.java

-  private static final KubernetesClient KUBE_CLIENT = new DefaultKubernetesClient();
-
-  // todo: this should really be cached
-  public static String getCommandFromImage(String imageName) throws IOException {


I moved all these functions to KubePodProcess as there was a hidden cyclic dependency. The process builder factory called the kube pod process class that called the factory back for these static functions when creating the process.

davinchia · 2021-05-25T13:26:51Z

airbyte-workers/src/main/java/io/airbyte/workers/process/KubeProcessBuilderFactoryPOC.java

    LOGGER.info("Done!");

-    System.exit(0); // todo: handle executors so we don't need to kill the JVM
+    // Debug Statements, to remove both merging.


These will be removed before merge.

davinchia · 2021-05-25T13:29:08Z

airbyte-workers/build.gradle

@@ -30,7 +39,7 @@ dependencies {
    implementation project(':airbyte-protocol:models')

    testImplementation 'org.mockito:mockito-inline:2.13.0'
-    testImplementation 'org.testcontainers:testcontainers:1.14.3'
+    testImplementation 'org.testcontainers:testcontainers:1.15.3'


I was running into intermittent test errors without this.

jrhizor

Looks good.

I don't see make a processbuilderfactory that uses KubePodProcess which is checked on the feature branch?

I'm going to address my own comments and merge this in today. I'll leave the debugging comments though.

airbyte-workers/src/main/java/io/airbyte/workers/process/KubePodProcess.java

* Use CDK to generate source that can be configured to emit a certain number of records and always works. * Checkpoint: socat works from inside the docker container. * Override the entry point. * Clean up and add ReadMe. * Clean up socat. * Checkpoint: connect to Kube cluster and list all the pods. * Checkpoint: Sync worker pod is able to send output to the destination pod. * Checkpoint: Sync worker creates Dest pod if none existed previously. It also waits for the pod to be ready before doing anything else. Sync worker will also remove the pod on termination. * update readme * Checkpoint: Dest pod does nott restart after finishing. Comment out delete command in Sync worker. * working towards named pipes * named pipes working * update readme * WIP named pipe / socat sidecar kube port forwarding (#3518) * nearly working sources * update * stdin example * move all kube testing yamls into the airbyte-workers directories. sort the airbyte-workers resource folder; place all the poc yamls together. * Format. * Put back the original KubeProcessBuilderFactory. * Fix slight errors. * Checkpoint: Worker pod knows its own IP. Successfully starts and writes to Dest pod after refactor. * remove unused file and update readme * Dest pod loops back into worker pod. However, the right messages do not seem to be passing in. * Switch back to worker ip. * SWEET VICTORY!. * wrap kube pod in process (#3540) also clean up kubernetes deploys. * More clean up. (#3586) The first 6 points of #3464. The only interesting thing about this PR is the kube pod shutdown. For whatever reason, the OkHttpPool isn't respecting the evictAll call and 1 idle thread remains. So instead of shutting down immediately, the worker pod shuts down after 5 mins when the idle thread id reaped. There isn't an easy way to modify the pool's idle reap configuration now. I do not think this issue is blocking since it's relatively benign, so I vote we create a ticket and come back to this once we do an e2e test. * Implements redirecting standard error as well. (#3623) * Clean up before next implementation. * kube process launching (#3790) * processes must handle file mounting * remove comment * default to base entrypoint * use process builder factory / select stdin / use a pool of ports * fix up * add super hacky copying example * Checkpoint: Works end to end! * Checkpoint: Use API to make sure init container is ready instead of blind sleep. Propagate exception in DefaultCheckConnectionWorker. * Refactor KubePodProcess. Checked to make sure everything still works. * Format. * Clean up code. Begin putting this into variables and breaking up long constructor function. * Add comments to explain what is happening. * fix normalization test * increase timeout for initcontainer Co-authored-by: Davin Chia <[email protected]> * facepalm moment * clean up kube poc pr (#3834) * clean up * remove source-always-works * create separate commons-docker * fix test * enable kube e2e tests (#3866) * enable kube e2e tests * use more generally accepted env definition * use new runners * use its own runner and install minikube differently * update name * use kubectl alias * use link instead of alias that doesn't propagate * start minikube * use driver=none * go back to using action * mess with versions * revert runner * install socat * print logs after run * also try re-runnining tasks * always wait for file transfer * use ports * increase wait timeout for kube * use different localhost ips and bump normalization to include an entrypoint * proposed fix * all working locally * revert temporary changes * revert normalization image change that's happening in a separate pr * readability * final comment * Working Kube Cancel. (#3983) * Port over the basic changes. * Add logic to return proper exit code in the event of termination. Add comments to explain why. * revert envs change and merge master to fix kube acceptance tests (#4012) * use older env format * fix build Co-authored-by: jrhizor <[email protected]> Co-authored-by: Jared Rhizor <[email protected]>

davinchia added 8 commits May 25, 2021 16:20

Checkpoint: create getCommandFromImage test.

5517a10

Update test containers.

fb41ce1

Use fabric api for getting entrypoint. Add tests to make sure we are …

6dd0c4a

…getting the right entrypoint.

Remove kubectl install dependency.

cfed967

Clean up getPodId and add tests.

582df9d

Make sure all client.pod calls include namespace.

9aae5da

Add comments to explain what is happening.

2072dd0

Format.

a24afdb

davinchia requested review from jrhizor and cgardens May 25, 2021 13:24

davinchia commented May 25, 2021

View reviewed changes

Remove unused test dockerfile.

193f994

davinchia commented May 25, 2021

View reviewed changes

davinchia marked this pull request as ready for review May 25, 2021 13:34

auto-assign bot requested review from ChristopheDuong and sherifnada May 25, 2021 13:34

davinchia removed request for sherifnada and ChristopheDuong May 25, 2021 13:39

jrhizor approved these changes May 25, 2021

View reviewed changes

davinchia added 4 commits May 26, 2021 11:34

Make namespace a parameter.

052ff55

Respond to PR feedback.

061d2fa

Better kube pod process shutdown.

5c1b329

Clean up and reintroduce system.exit to end the sync worker.

c775afa

davinchia merged commit d605c74 into davinchia/kube-queueing-poc May 26, 2021

davinchia deleted the davinchia/kube-api-cleanup branch May 26, 2021 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More clean up. #3586

More clean up. #3586

davinchia commented May 25, 2021 •

edited

Loading

davinchia May 25, 2021

davinchia May 25, 2021

davinchia May 25, 2021

davinchia May 25, 2021

jrhizor left a comment

More clean up. #3586

More clean up. #3586

Conversation

davinchia commented May 25, 2021 • edited Loading

What

How

Recommended reading order

davinchia May 25, 2021

Choose a reason for hiding this comment

davinchia May 25, 2021

Choose a reason for hiding this comment

davinchia May 25, 2021

Choose a reason for hiding this comment

davinchia May 25, 2021

Choose a reason for hiding this comment

jrhizor left a comment

Choose a reason for hiding this comment

davinchia commented May 25, 2021 •

edited

Loading