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

[DPE-1033/DPE-1188] Add docker authenticated pulls & Drop SigNoz self hosted #54

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

BryanFauble
Copy link
Contributor

@BryanFauble BryanFauble commented Dec 26, 2024

Problem:

  1. Docker has some strict rate limits for pulling images when not logged in.
  2. When a node goes down (Spot instance) or a cheaper on-demand node is spun up and workloads moved over many images need to be pulled in a short amount of time.
  3. Self hosted SigNoz had a number of issues with the clickhouse database management

Solution:

  1. Authenticating docker pulls to docker hub starting with SigNoz.
  2. Drop SigNoz from the K8s deployment

Testing:

  1. Changes looked good in the sandbox cluster!
  2. Will be tearing down SigNoz in all environments after merge

@spacelift-int-sagebionetworks spacelift-int-sagebionetworks bot temporarily deployed to spacelift/dpe-dev-kubernetes-deployments December 26, 2024 19:19 Inactive
@BryanFauble BryanFauble marked this pull request as ready for review December 26, 2024 20:41
@BryanFauble BryanFauble requested a review from a team as a code owner December 26, 2024 20:41
@BryanFauble BryanFauble changed the title [DPE-1033] Add docker authenticated pulls [DPE-1033/DPE-1188] Add docker authenticated pulls & Drop SigNoz self hosted Dec 26, 2024
Copy link
Contributor

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 LGTM!

Thanks for doing this - we do have a sage organization which we could potentially add this individual. Once added, this individual could have less restrictions with Docker pulls, but maybe we can figure that out later.

I'd like someone else on @Sage-Bionetworks-Workflows/dpe to do a final review.

Another critical note: be sure that Signoz cloud can be used before we drop this and then have to resurrect all of this. Alberto did approve months ago but nothing is final until we get that subscription.

@BryanFauble BryanFauble requested a review from a team December 30, 2024 16:33
Copy link
Contributor

@BWMac BWMac left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question

@@ -2354,7 +2354,7 @@ redis:
# Auth secret for a private registry
# This is used if pulling airflow images from a private registry
registry:
secretName: ~
secretName: docker-cfg
Copy link
Contributor

Choose a reason for hiding this comment

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

How would it work if we ever were to pull an image from another registry such as quay.io/would we ever need to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we were to use another registry we would update the resource "kubernetes_secret" "docker-cfg" to include the authentication for the other registry. Or, as this S/O covers - The alternative is to create separate secrets for each registry we might want to use: https://stackoverflow.com/a/59717512

Further docs: https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/#using-images-from-multiple-registries

Using images from multiple registries

A pod can have multiple containers, each container image can be from a different registry. You can use multiple imagePullSecrets with one pod, and each can contain multiple credentials.

The image pull will be attempted using each credential that matches the registry. If no credentials match the registry, the image pull will be attempted without authorization or using custom runtime specific configuration.

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