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.

Sign up for GitHub

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

Jump to bottom

Use Micronaut auto-detected environment for docker vs kubernetes choices #17388

Merged
merged 20 commits into from
Oct 4, 2022
Merged

Use Micronaut auto-detected environment for docker vs kubernetes choices #17388

merged 20 commits into from
Oct 4, 2022

Conversation

jdpgrailsdev
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev commented Sep 29, 2022 •
edited
Loading

What

  • Leverage Micronaut auto-detected environments for conditional choices between Docker and Kubernetes

How

  • Remove the custom WORKER_ENVIRONMENT environment variable from airbyte-workers
  • Use the Micronaut auto-detected environment to know when running in k8s
  • Remove the needless passing of WorkerConfigs around just to do a check in WorkerUtils

Closes #16700

Recommended reading order

  1. airbyte-workers/src/main/resources/application.yml
  2. airbyte-workers/src/main/java/io/airbyte/workers/ApplicationInitializer.java
  3. airbyte-workers/src/main/java/io/airbyte/workers/config/ApplicationBeanFactory.java
  4. airbyte-workers/src/main/java/io/airbyte/workers/config/ProcessFactoryBeanFactory.java
  5. airbyte-workers/src/main/java/io/airbyte/workers/config/WorkerConfigurationBeanFactory.java
  6. charts/airbyte-worker/templates/deployment.yaml
  7. docker-compose.yaml
  8. kube/resources/worker.yaml
  9. airbyte-workers/src/main/java/io/airbyte/workers/WorkerUtils.java
  10. All other changes are reactions to removing WorkerConfigs as a parameter to WorkerUtils

I have a corresponding branch ready to go in cloud. This does not have the same risk of the previous PR that changed the environment names for mutil-cloud parameters. This PR simply removes referencing an existing environment variable WORKER_ENVIRONMENT in favor of the Microanut auto-detected environments.

Tests

  • All unit tests pass/builds locally without failure
  • Tested locally via docker-compose. Runs as expected in Docker mode
  • Tested via Cloud in dev environment. Runs as expected in Kubernetes mode

@github-actions github-actions bot added area/platform issues related to the platform area/worker Related to worker kubernetes labels Sep 29, 2022
@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-sftp
  • destination-pubsub
  • destination-jdbc
  • source-tidb
  • destination-mssql-strict-encrypt
  • destination-snowflake
  • destination-clickhouse-strict-encrypt
  • destination-bigquery-denormalized
  • destination-mongodb-strict-encrypt
  • destination-redshift
  • source-cockroachdb-strict-encrypt
  • destination-mariadb-columnstore
  • source-alloydb
  • source-mysql-strict-encrypt
  • source-clickhouse
  • source-postgres
  • source-mongodb-v2
  • source-db2
  • destination-elasticsearch
  • destination-tidb
  • source-oracle
  • destination-mqtt
  • destination-databricks
  • destination-kafka
  • source-mssql
  • destination-meilisearch
  • destination-pulsar
  • destination-postgres-strict-encrypt
  • destination-gcs
  • destination-postgres
  • destination-elasticsearch-strict-encrypt
  • source-snowflake
  • destination-mysql
  • destination-e2e-test
  • destination-bigquery
  • destination-csv
  • destination-s3
  • destination-kinesis
  • destination-clickhouse
  • destination-dev-null
  • source-mssql-strict-encrypt
  • source-mysql
  • source-cockroachdb
  • source-postgres-strict-encrypt
  • destination-aws-datalake
  • destination-keen
  • source-scaffold-java-jdbc
  • source-redshift
  • destination-azure-blob-storage
  • source-oracle-strict-encrypt
  • source-jdbc
  • destination-scylla
  • destination-oracle
  • source-e2e-test-cloud
  • destination-mongodb
  • destination-redis
  • destination-oracle-strict-encrypt
  • destination-rockset
  • source-alloydb-strict-encrypt
  • source-mongodb-strict-encrypt
  • destination-mssql
  • destination-mysql-strict-encrypt
  • source-elasticsearch
  • source-e2e-test
  • destination-r2
  • source-clickhouse-strict-encrypt
  • source-kafka
  • source-db2-strict-encrypt
  • destination-local-json
  • source-bigquery
  • destination-cassandra
  • destination-dynamodb

@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets September 29, 2022 13:09 Inactive
@jdpgrailsdev
Copy link
Contributor Author

I just ran the integration tests for the destination-local-json connector, per the auto-generated message above and it all passes. I think the messages are because I had to update three classes (one in standard-destination-test and two in standard-source-test), which have no changes in actual logic.

@jdpgrailsdev
Copy link
Contributor Author

One Kube-based acceptance test failed, which most likely means we have a configuration issue somewhere. However, since the output of each service is unreachable, I can't see what the issue is. If someone could help me with running the Kube-based tests locally, that would help figure out where the issue is.

@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets September 29, 2022 13:46 Inactive
@jdpgrailsdev
Copy link
Contributor Author

I was able to find the issue locally: another environment variable that is set to an empty string occasionally, leading to Micronaut thinking that the property is missing completely when using @Value. Changed to use @Property to handle that case.

@airbytehq airbytehq deleted a comment from github-actions bot Sep 29, 2022
@airbytehq airbytehq deleted a comment from github-actions bot Sep 29, 2022
@airbytehq airbytehq deleted a comment from github-actions bot Sep 29, 2022
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets September 29, 2022 14:01 Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets September 29, 2022 14:07 Inactive
@airbytehq airbytehq deleted a comment from github-actions bot Sep 29, 2022
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets September 29, 2022 16:38 Inactive
@airbytehq airbytehq deleted a comment from github-actions bot Sep 29, 2022
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets October 3, 2022 20:06 Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets October 3, 2022 20:36 Inactive
@gosusnp gosusnp temporarily deployed to more-secrets October 4, 2022 01:29 Inactive
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets October 4, 2022 14:58 Inactive
@airbytehq airbytehq deleted a comment from github-actions bot Oct 4, 2022
@airbytehq airbytehq deleted a comment from github-actions bot Oct 4, 2022
@airbytehq airbytehq deleted a comment from github-actions bot Oct 4, 2022
@airbytehq airbytehq deleted a comment from github-actions bot Oct 4, 2022
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets October 4, 2022 15:26 Inactive
@airbytehq airbytehq deleted a comment from github-actions bot Oct 4, 2022
@airbytehq airbytehq deleted a comment from github-actions bot Oct 4, 2022
@airbytehq airbytehq deleted a comment from github-actions bot Oct 4, 2022
@jdpgrailsdev jdpgrailsdev temporarily deployed to more-secrets October 4, 2022 17:37 Inactive
@jdpgrailsdev jdpgrailsdev merged commit 759a27e into master Oct 4, 2022
@jdpgrailsdev jdpgrailsdev deleted the jonathan/workers-runtime-environment branch October 4, 2022 19:14
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Oct 5, 2022
Merged
@sh4sh
Copy link
Contributor

sh4sh commented Oct 5, 2022

When deploying with kube/overlays/stable the WORKER_ENVIRONMENT env block has to be re-added or the worker pod doesn't start. Helm is fine though!

More info and logs in this ticket: #17595

@sh4sh sh4sh mentioned this pull request Oct 5, 2022
Closed
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@jdpgrailsdev @gosusnp @jhammarstedt
Use Micronaut auto-detected environment for docker vs kubernetes choi…
90210db 
…ces (airbytehq#17388)

* Use Micronaut auto-detected environment for docker vs kubernetes choices

* Handle set, but blank env var

* Formatting

* Revert change

* Add default value

* Explicitly set WORKER_ENVIRONMENT for container orchestrator

* Make public

* Add sleep to allow Temporal cache to populate

Co-authored-by: Jimmy Ma <gosusnp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colesnodgrass colesnodgrass colesnodgrass approved these changes

@gosusnp gosusnp gosusnp approved these changes

@pmossman pmossman pmossman approved these changes

@benmoriceau benmoriceau Awaiting requested review from benmoriceau

@alovew alovew Awaiting requested review from alovew

Assignees
No one assigned
Labels
area/platform issues related to the platform area/worker Related to worker kubernetes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Micronaut Environment to control ProcessFactory conditional bean selection
5 participants
@jdpgrailsdev @sh4sh @colesnodgrass @gosusnp @pmossman