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 affinity settings in viz and jaeger chart #11464

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

naing2victor
Copy link
Contributor

Add nodeAffinity for deployment template to linkerd-viz and linkerd-jaeger charts

Fixes #10680

@naing2victor naing2victor requested a review from a team as a code owner October 6, 2023 20:07
@naing2victor naing2victor changed the title Add nodeAffinity in viz and jaeger chart (#10680) Add nodeAffinity in viz and jaeger chart Oct 6, 2023
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

Thanks, @naing2victor!

I noticed that you're setting the component and label values which are used by podAntiAffinity, not nodeAffinity. What do you think of adding

# -- Enables Pod Anti Affinity logic to balance the placement of replicas
# across hosts and zones for High Availability.
# Enable this only when you have multiple replicas of components.
enablePodAntiAffinity: false

to the values.yaml since we're setting those component and label values anyway. That makes it consistent with the other extensions as well.

@naing2victor
Copy link
Contributor Author

@adleong Thank you for your feedback. I can see that linkerd-viz chart is already implemented with podAntiAffinity. We'll only need to apply for linkerd-jaeger chart. Adding enablePodAntiAffinity may also need to variabilize the replicas for each component. May I know how would you suggest for the value usage.

Would it be better if we separate the variable by the component. For example,

replicas: {{.Values.jaegerInjector.replicas}}
replicas: {{.Values.collector.replicas}}
replicas: {{.Values.jaeger.replicas}}

Or just using 1 value for all the components.

replicas: {{.Values.replicas}}

@adleong
Copy link
Member

adleong commented Oct 20, 2023

I'd recommend having a separate replicas value for each component (which is what we do in linkerd-viz). However, the exception to that is that increasing the number of replicas of the jaeger component could be problematic since this is a stateful trace backend; so we may want to leave that one hardcoded at 1 replica.

- variablize replica value for each component except `jager` component
- add update strategy
- update docs
Signed-off-by: Naing Naing Htun <[email protected]>
@naing2victor
Copy link
Contributor Author

I've added enablePodAntiAffinity and make replicas values to get from values file.

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

If you try rendering the templates with enablePodAntiAffinity=true you'll see there's a problem. I believe the issue is not passing $tree into the linkerd.affinity include.

jaeger/charts/linkerd-jaeger/values.yaml Outdated Show resolved Hide resolved
jaeger/charts/linkerd-jaeger/values.yaml Outdated Show resolved Hide resolved
- issue is not passing `$tree` into the `linkerd.affinity` include
- move `jaegerInjector` under `webhook`
- Capitalize comments

Signed-off-by: Naing Naing Htun <[email protected]>
Signed-off-by: Alex Leong <[email protected]>
@alpeb alpeb changed the title Add nodeAffinity in viz and jaeger chart Add affinity settings in viz and jaeger chart Nov 17, 2023
@alpeb alpeb merged commit fd54697 into linkerd:main Nov 17, 2023
33 checks passed
@hawkw hawkw mentioned this pull request Nov 22, 2023
hawkw added a commit that referenced this pull request Nov 22, 2023
## edge-23.11.4

This edge release introduces support for the native sidecar containers
entering beta support in Kubernetes 1.29. This improves the startup and
shutdown ordering for the proxy relative to other containers, fixing the
long-standing shutdown issue with injected `Job`s. Furthermore, traffic
from other `initContainer`s can now be proxied by Linkerd.

In addition, this edge release includes Helm chart improvements, and
improvements to the multicluster extension.

* Added a new `config.alpha.linkerd.io/proxy-enable-native-sidecar`
  annotation and `Proxy.NativeSidecar` Helm option that causes the proxy
  container to run as an init-container (thanks @teejaded!) (#11465;
  fixes #11461)
* Fixed broken affinity rules for the multicluster `service-mirror` when
  running in HA mode (#11609; fixes #11603)
* Added a new check to `linkerd check` that ensures all extension
  namespaces are configured properly (#11629; fixes #11509)
* Updated the Prometheus Docker image used by the `linkerd-viz`
  extension to v2.48.0, resolving a number of CVEs in older Prometheus
  versions (#11633)
* Added `nodeAffinity` to `deployment` templates in the `linkerd-viz`
  and `linkerd-jaeger` Helm charts (thanks @naing2victor!) (#11464;
  fixes #10680)
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.

linkerd-viz and linkerd-jaeger charts do not support nodeAffinity
3 participants