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

Update demo_ingress to expect a reads service name instead of managing services #1634

Merged
merged 4 commits into from
Sep 30, 2024

Conversation

sjahl
Copy link
Contributor

@sjahl sjahl commented Sep 26, 2024

This addresses an issue that Phil ran into earlier this week after we rolled out reads using the new deployment methodology. Spinning up demos depended on being able to ask the k8s API what the current reads deployment was, and then generated a new Service object for you. Often times this just ended up being a new Service object called "gnomad-reads-demo-foobar" that pointed at the current production deployment pods, which didn't feel particularly useful.

I thought about this for a bit, and decided that managing the extra service object here was probably unnecessary? With these assumptions:

  • If you don't care which reads service you get and just want to point at prod, there's already a Service you can wire up the ingress to
  • If you do care which reads deployment your demo uses, you probably already spun one up and have a Service to point at it?

With those in mind, I've updated this script to just accept a reads Service name, and it uses that to configure the ingress instead of creating a bunch of extra Services. If you don't specify a service name, it just points /reads on your demo to the active prod service.

How do folks feel about that process, and are those assumptions I've made correct?

…s, instead of managing all services as part of the ingress deploy
@sjahl sjahl self-assigned this Sep 26, 2024
@sjahl
Copy link
Contributor Author

sjahl commented Sep 26, 2024

Closes #1342

@phildarnowsky-broad
Copy link
Contributor

@sjahl sounds like a sensible way to do it to me

@sjahl
Copy link
Contributor Author

sjahl commented Sep 30, 2024

Note, I've tacked the new demo/spot pool changes into this PR, since it felt quite related.

We have a new CLI flag in deployctl deployments create. You can now pass the --demo flag to set your deployment to use the new auto-scaling demo-pool with spot nodes.

@sjahl sjahl merged commit 732d049 into main Sep 30, 2024
4 checks passed
@sjahl sjahl deleted the fix-reads-deployment-for-demos branch September 30, 2024 20:56
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.

Allow for demo deployments to optionally include the reads deployment
2 participants