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

Add ability to configure replicas for services #852

Closed
wants to merge 10 commits into from

Conversation

martinjt
Copy link
Member

@martinjt martinjt commented Aug 9, 2023

This PR adds 2 new settings to the values.yaml

  • default.replicas
    This will configure how many replicas of each of the services should be create
  • components.<service>.replicaOverride
    This will override the specific service's replicas from the default.

In the supplied values.yaml the default.replicas value is set to 1 so that it doesn't change for existing users. In addition there are specific services that I didn't think should be automatically scaled based on this setting, for these I've added a replicasOverride to their section to keep it at 1, users can obviously override these as they see fit. These are:

  • Redis
  • Postgres
  • Kafka
  • Load generator

I'm sure I need to update some change logs, but I'm not sure where, so any guidance would be appreciated.

@martinjt martinjt requested a review from puckpuck as a code owner August 9, 2023 13:13
@martinjt martinjt requested a review from a team August 9, 2023 13:13
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@martinjt looks great. Please update the README with the new values. Also, bump the chart's minor version and run make generate-examples CHARTS=opentelmetry-demo

@martinjt
Copy link
Member Author

Thanks @TylerHelmuth are there any other stateful services that I missed in the replicasOverride?

@TylerHelmuth
Copy link
Member

I'd like @puckpuck opinion, buy maybe featureflagService and frontendProxy.

@martinjt
Copy link
Member Author

The Feature Flag service has a postgres database, so I figured that was ok, and I'd assumed the proxy wasn't stateful.

@puckpuck
Copy link
Contributor

What use case would someone set default.replicas to not be 1? Should this even be specified as a default to start with? I think it makes sense to want to scale up specific components, and perhaps this should be just specified at the components level only.

@martinjt
Copy link
Member Author

The idea is to scale up all the services at once, and make that easier for the scenario of just "we want more load". which makes it easier than adding an entry for each of them. Also when/if new components are added, they'll immediately get the replicas.

@martinjt
Copy link
Member Author

martinjt commented Aug 14, 2023 via email

@TylerHelmuth
Copy link
Member

@martinjt we don't need the script in this PR. If you want to discuss examples in an issue thats fine.

For handling merge conflicts between chart.yaml and the examples I find the best solution is the accept the incoming chart.yaml change, modify the version appropriately, and run make generate-examples CHARTS=opentelemetry-demo. This will overwrite all the example merge conflicts and make them accurate.

@martinjt
Copy link
Member Author

I know we don't need it, it was a mistake, I was discussing that script in slack. I was asking whether you'd like me to PR the removal or whether it would be quicker for you to remove it as its not part of this PR.

@martinjt
Copy link
Member Author

No idea what's going on with that failure, it looks fine, I ran the generate-examples and committed them. Any ideas @puckpuck ?

@@ -4,10 +4,10 @@ kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels:
helm.sh/chart: grafana-6.58.8
helm.sh/chart: grafana-6.52.8
Copy link
Member

Choose a reason for hiding this comment

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

@martinjt it looks like you need to update your helm dependencies and then regenerate the examples. This is happening bc we bumped the helm chart dependencies in a previous merge. I believe all you need to do is run helm repo update and then regenerate the examples, but you might have to run helm dependency build ./charts/opentelemetry-demo.

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 6, 2023
@martinjt martinjt requested a review from Allex1 as a code owner September 7, 2023 17:12
@TylerHelmuth
Copy link
Member

@martinjt looks like you've had a bad merge maybe? Please undo the changes to ./charts/opentelemetry-collector and ./charts/opentelemetry-operator. When re-rendering the demo examples use make generate-examples CHARTS=opentelemetry-demo so that only the demo examples are rendered.

@github-actions github-actions bot removed the Stale label Sep 8, 2023
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 22, 2023
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants