-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use a separate node pool to run custom connector jobs #19770
Conversation
...ommons-worker/src/main/java/io/airbyte/workers/normalization/DefaultNormalizationRunner.java
Show resolved
Hide resolved
airbyte-commons-worker/src/main/java/io/airbyte/workers/general/DbtTransformationRunner.java
Show resolved
Hide resolved
@pmossman when you get a chance can you take another look at this PR? thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiaohansong I left a couple more comments but overall this looks good to me! I'll approve the PR assuming we don't disagree about any of my comments, let me know if you want to sync about any of them!
// If using isolated pool, check workerConfigs has isolated pool set. If not set, fall back to use | ||
// regular node pool. | ||
final var nodeSelectors = | ||
usesIsolatedPool ? workerConfigs.getWorkerIsolatedKubeNodeSelectors().orElse(workerConfigs.getworkerKubeNodeSelectors()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I wonder if we should just thrown an exception if usesIsolatedPool
is true but the getWorkerIsolatedKubeNodeSelectors
Optional is empty. I think it would be safer for us to fail loudly if we didn't configure the isolated pool properly, rather than accidentally letting custom connectors run in our main pool. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call! fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realized this is not true - usesIsolatedPool here is actually passed by callers (isCustomConnector flag). Thus for OSS users who do not run them in a separate pool, usesIsolatedPool might still be true because they are custom connectors. I should rename this to isCustomConnectors instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good call, OSS users should be able to use custom connectors without an isolated pool indeed
// custom connectors run in an isolated node pool from airbyte-supported connectors | ||
// to reduce the blast radius of any problems with custom connector code. | ||
final var nodeSelectors = | ||
isCustomConnector ? workerConfigs.getWorkerIsolatedKubeNodeSelectors().orElse(workerConfigs.getworkerKubeNodeSelectors()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment here, I think it's more dangerous to fall back on our primary node selectors, and perhaps we should throw an exception instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this check in WorkerConfigurationBeanFactory.java
instead - because 1) workerConfigs does not carry useIsolatedPool
flag, 2) should throw this error as early as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
airbyte-server/src/main/java/io/airbyte/server/handlers/SourceDefinitionsHandler.java
Outdated
Show resolved
Hide resolved
airbyte-server/src/test/java/io/airbyte/server/handlers/DestinationDefinitionsHandlerTest.java
Outdated
Show resolved
Hide resolved
airbyte-server/src/test/java/io/airbyte/server/handlers/SourceDefinitionsHandlerTest.java
Show resolved
Hide resolved
...c/main/java/io/airbyte/workers/temporal/scheduling/activities/GenerateInputActivityImpl.java
Show resolved
Hide resolved
airbyte-workers/src/main/java/io/airbyte/workers/config/WorkerConfigurationBeanFactory.java
Outdated
Show resolved
Hide resolved
Connector base build has been failing for unrelated reasons. Submitting this. |
What
#19684
Custom connectors - user can upload their own docker image as the source or destination of the sync. Such sourceDefinition or destinationDefinition will be classified as custom connectors. Since they are not vetted by airbyte, we want to run it in a separate node pool for safety reasons to avoid failing jobs running with airbyte vetted connectors.
The goal of the PR is to:
How
Passing the value in launchConfig - launcher will read launchConfig and launch a kube process with correct node selector.
For jobs spawned by Orchestrator, we also need to pass down the env variable to containers.