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

socat gets more resources #19953

Merged
merged 4 commits into from
Dec 6, 2022
Merged

socat gets more resources #19953

merged 4 commits into from
Dec 6, 2022

Conversation

evantahler
Copy link
Contributor

@evantahler evantahler commented Nov 30, 2022

We currently only allocate 0.2 CPU shares to the socat processes, but we've observed that we are hitting that limit:

unnamed

Rather than remove socat and enforce the same pod affinity for all the pods in the sync, lets start by simply throwing money at the problem!

Future work would be to adjust this dynamically as needed at runtime, or for only those syncs with "fast enough" sources

@evantahler evantahler temporarily deployed to more-secrets November 30, 2022 23:16 Inactive
@evantahler evantahler marked this pull request as ready for review November 30, 2022 23:17
@evantahler
Copy link
Contributor Author

@davinchia can you confirm that this file is what is used by Cloud, and we don't overwrite these values?

@evantahler
Copy link
Contributor Author

@supertopher are there any worries about exhausting our cluster resources?

@evantahler evantahler temporarily deployed to more-secrets December 1, 2022 22:23 Inactive
@evantahler
Copy link
Contributor Author

I think this is the pattern to add ENV configs? I also think we shouldn't add this to the OSS helm charts for now, sort of like how CONTROL_PLANE_AUTH_ENDPOINT isn't there either.

@davinchia once this is merged, I have no idea how to try our a change in the cloud repo...

@evantahler evantahler temporarily deployed to more-secrets December 1, 2022 22:55 Inactive
@davinchia
Copy link
Contributor

@evantahler yes pattern looks right and yes we shouldn't add to OSS Helm charts for now as we don't really want to expose this to OSS users yet.

After merge we have to:

  1. Follow https://internal-docs.airbyte.io/Things-To-Know/Upgrading-OSS-Versions to update Cloud.
  2. Update the new values here: https://github.com/airbytehq/airbyte-cloud/blob/master/tools/helm-values-builder/templates/gcp_helm_values.yaml.j2#L223. See https://internal-docs.airbyte.io/How-Tos/Building-Helm-values-files#helm-values-development to understand what is happening.
  3. Follow https://internal-docs.airbyte.io/Deploying/Deploying-to-Cloud-using-CICD/One-Click-Deploy-Github-Actions#the-deployment-process to deploy the branch to a dev env for testing. On this step, you might have to manually increase the node pool size of the environment you are testing on.
  4. Merge to Cloud master with Auto CD to prod.

I'm PTO after today till Dec 18th so please reach out to @pmossman or @git-phu for help!

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.

Evan, I'm going to approve. Feel free to merge after addressing the comments.

@evantahler evantahler temporarily deployed to more-secrets December 6, 2022 22:03 Inactive
@evantahler evantahler temporarily deployed to more-secrets December 6, 2022 22:04 Inactive
@evantahler evantahler merged commit 4697835 into master Dec 6, 2022
@evantahler evantahler deleted the evan/more-powerful-socat branch December 6, 2022 23:29
@evantahler
Copy link
Contributor Author

Merged with a red test - that's a connector base format error, will be solved by #20163

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.

3 participants