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

Specify namespace when creating pod #19399

Merged
merged 9 commits into from
Nov 15, 2022
Merged

Specify namespace when creating pod #19399

merged 9 commits into from
Nov 15, 2022

Conversation

benmoriceau
Copy link
Contributor

What

specify the namespace (pool) when creating a new pod

@benmoriceau benmoriceau requested a review from git-phu November 14, 2022 22:41
jdpgrailsdev
jdpgrailsdev approved these changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@benmoriceau benmoriceau temporarily deployed to more-secrets November 14, 2022 22:42 Inactive
git-phu
git-phu reviewed Nov 14, 2022
View reviewed changes
Copy link
Contributor

@git-phu git-phu left a comment

Choose a reason for hiding this comment

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

I believe what we need is to specify node selectors when creating the orchestrator pod, which is different from specifying the kubernetes namespace

@git-phu
Copy link
Contributor

git-phu commented Nov 14, 2022

in KubePodProcess we are already setting tolerations and selectors here

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

Lines 510 to 512 in ebbe5a9

final Pod pod = podBuilder.withTolerations(buildPodTolerations(tolerations))
.withImagePullSecrets(new LocalObjectReference(imagePullSecret)) // An empty string turns this into a no-op setting.
.withNodeSelector(nodeSelectors)

So I believe what is happening is that in the container orchestrator app we are setting the tolerations and nodeSelectors properly when calling the KubePodProcess constructor, but when creating the actual orchestrator pods (which I believe should happen in the worker app) we are not setting these configs.

@benmoriceau benmoriceau changed the title Specify namespace when creating pof Specify namespace when creating pod Nov 15, 2022
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker labels Nov 15, 2022
@benmoriceau benmoriceau temporarily deployed to more-secrets November 15, 2022 00:39 Inactive
git-phu
git-phu approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@git-phu git-phu left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

I think we should support pod tolerations as well for completeness, but for our own cluster setup we don't need it so I'll approve this PR as is so we can get it deployed.

We should add pod toleration support though, if not in this PR then in a follow up.

...yte-commons-worker/src/main/java/io/airbyte/workers/process/AsyncOrchestratorPodProcess.java
@@ -352,11 +353,13 @@ public void create(final Map<String, String> allLabels,
.withContainers(mainContainer)
.withInitContainers(initContainer)
.withVolumes(volumes)
.withNodeSelector(nodeSelectors)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should include tolerations as well like in KubePodProcess

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

Line 510 in ebbe5a9

final Pod pod = podBuilder.withTolerations(buildPodTolerations(tolerations))

However, our current node pools don't actually use taints, so we don't need tolerations for this to work for our own cluster. In general though, we should include it (maybe in a separate followup PR. buildPodTolerations looks like a private method though it should be easy to extract out of KubePodProcess.)

pmossman
pmossman approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@pmossman pmossman left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!

...yte-commons-worker/src/main/java/io/airbyte/workers/process/AsyncOrchestratorPodProcess.java Outdated
.endSpec()
.build();

// should only create after the kubernetes API creates the pod
final var createdPod = kubernetesClient.pods()

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra newline

@benmoriceau
Copy link
Contributor Author

looks good, thanks!

I think we should support pod tolerations as well for completeness, but for our own cluster setup we don't need it so I'll approve this PR as is so we can get it deployed.

We should add pod toleration support though, if not in this PR then in a follow up.

Ok, I'll do it in a follow up.

@benmoriceau benmoriceau temporarily deployed to more-secrets November 15, 2022 01:35 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets November 15, 2022 02:10 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets November 15, 2022 16:46 Inactive
@benmoriceau
Copy link
Contributor Author

the build seems to fail because of: https://airbytehq-team.slack.com/archives/C046KQF5GBT/p1668531356052829

@benmoriceau benmoriceau temporarily deployed to more-secrets November 15, 2022 21:37 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets November 15, 2022 22:20 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets November 15, 2022 23:09 Inactive
@benmoriceau benmoriceau merged commit a586537 into master Nov 15, 2022
@benmoriceau benmoriceau deleted the bmoric/specify-namespace-when-creating-pod branch November 15, 2022 23:52
@github-actions github-actions bot mentioned this pull request Nov 17, 2022
Merged
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Nov 18, 2022
Merged
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
@benmoriceau @akashkulk
Specify namespace when creating pod (#19399)
02b05f8 
* Specify namespace when creating pof

* PR comments

* rm new line

* Fix micronaut injection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmossman pmossman pmossman approved these changes

@git-phu git-phu git-phu approved these changes

@jdpgrailsdev jdpgrailsdev jdpgrailsdev approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@benmoriceau @git-phu @jdpgrailsdev @pmossman @octavia-squidington-iv