Skip to content
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

enable kube e2e tests #3866

Merged
merged 27 commits into from
Jun 9, 2021

Conversation

jrhizor
Copy link
Contributor

@jrhizor jrhizor commented Jun 3, 2021

Reverts the testing component of #2315

My hope is that these are more stable after our updates + the new EC2 test runners.

Keeping as a draft until these are passing + go through a few rounds.

@jrhizor
Copy link
Contributor Author

jrhizor commented Jun 3, 2021

actually I still need to update this to use the new runner

@jrhizor
Copy link
Contributor Author

jrhizor commented Jun 4, 2021

I think this is all working now except for normalization which was missing an AIRBYTE_ENTRYPOINT. I don't think it'll build until we push that image. Testing locally.

@jrhizor
Copy link
Contributor Author

jrhizor commented Jun 4, 2021

These are the two interesting commits so far:
8f4d826
928762a

@jrhizor
Copy link
Contributor Author

jrhizor commented Jun 4, 2021

After building and tagging the normalization image, I'm running CORE_ONLY=true ./gradlew --no-daemon build --scan --rerun-tasks -x test && ./tools/bin/acceptance_test_kube.sh (and removing the port-foward process between iterations).

Currently getting:

io.airbyte.workers.WorkerException: Address already in use
	at io.airbyte.workers.process.KubeProcessFactory.create(KubeProcessFactory.java:88) ~[io.airbyte-airbyte-workers-0.24.6-alpha.jar:?]
	at io.airbyte.workers.process.ProcessFactory.create(ProcessFactory.java:68) ~[io.airbyte-airbyte-workers-0.24.6-alpha.jar:?]
	at io.airbyte.workers.process.AirbyteIntegrationLauncher.read(AirbyteIntegrationLauncher.java:126) ~[io.airbyte-airbyte-workers-0.24.6-alpha.jar:?]
	at io.airbyte.workers.protocols.airbyte.DefaultAirbyteSource.start(DefaultAirbyteSource.java:82) ~[io.airbyte-airbyte-workers-0.24.6-alpha.jar:?]
	at io.airbyte.workers.DefaultReplicationWorker.run(DefaultReplicationWorker.java:121) [io.airbyte-airbyte-workers-0.24.6-alpha.jar:?]
	at io.airbyte.workers.DefaultReplicationWorker.run(DefaultReplicationWorker.java:51) [io.airbyte-airbyte-workers-0.24.6-alpha.jar:?]
	at io.airbyte.workers.temporal.TemporalAttemptExecution.lambda$getWorkerThread$2(TemporalAttemptExecution.java:147) [io.airbyte-airbyte-workers-0.24.6-alpha.jar:?]
	at java.lang.Thread.run(Thread.java:832) [?:?]

I think that means that we aren't releasing the ports correctly / it doesn't coincide with server shut down.

The underlying failure appears to be:

2021-06-04 22:54:11 ERROR i.a.c.i.LineGobbler(voidCall):69 - {job_id=1, job_log_filename=logs.log, job_root=/workspace/1/1} - Exception in thread "main" java.sql.SQLException: Cannot create PoolableConnectionFactory (Connection to host.docker.internal:55076 refused. Check that the hostname and port are correct and that the postmaster is accepting TCP/IP connections.)

It isn't clear how previous interactions would be able to interact with the destination db and then it wouldn't be able to after that.

@jrhizor jrhizor requested a review from davinchia June 7, 2021 17:10
@jrhizor jrhizor mentioned this pull request Jun 7, 2021
@@ -14,7 +14,13 @@ kubectl apply -k kube/overlays/dev
kubectl wait --for=condition=Available deployment/airbyte-server --timeout=300s || (kubectl describe pods && exit 1)
kubectl wait --for=condition=Available deployment/airbyte-scheduler --timeout=300s || (kubectl describe pods && exit 1)

sleep 20s
sleep 120s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment here to explain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a nit

copyFilesToKubeConfigVolume(client, podName, namespace, files);
}

LOGGER.info("Copying files...");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment on why this is the case

@@ -75,6 +75,14 @@ public Process create(String jobId,

return new KubePodProcess(
kubeClient,
port -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var portReleaser = port -> {
  if (!ports.contains(port)) {
              ports.add(port);
              LOGGER.info("Port consumer releasing: " + port);
            } else {
              LOGGER.info("Port consumer skipping releasing: " + port);
            }
}

new KubePodProcess(
  kubeClient,
  portReleaser,
.....);

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Feel free to merge after comments and nits.

@jrhizor jrhizor marked this pull request as ready for review June 9, 2021 02:10
@auto-assign auto-assign bot requested review from davinchia and sherifnada June 9, 2021 02:10
@jrhizor jrhizor merged commit 03d9ed9 into davinchia/kube-queueing-poc Jun 9, 2021
@jrhizor jrhizor deleted the jrhizor/re-enable-kube-tests branch June 9, 2021 02:11
jrhizor added a commit that referenced this pull request Jun 10, 2021
* 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 <davinchia@gmail.com>

* 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 <me@jaredrhizor.com>
Co-authored-by: Jared Rhizor <jared@dataline.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants